Skip to content
New issue

Have a question about this project?Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of serviceand privacy statement.We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V5/homepage #20924

Closed
wants to merge 19 commits into from
Closed

V5/homepage #20924

wants to merge 19 commits into from

Conversation

Mcastres
Copy link
Contributor

@Mcastres Mcastres commented Aug 1, 2024

Hello there 👋

I submitted a commit on this PR. Please find below what it contains:

CE Users

Project statistics

  • Entries count
  • Assets count
  • CT count
  • Component count
  • Locales count
  • Admins count
  • Webhooks count
  • API tokens count

Collection Type overview

  • Dropdown button to select an existing Collection-Type
    • Entries count
    • Pie Chart of entries count broken down by status
    • Locales count
    • Pie Chart of entries count broken down by locales
    • Stacked Area Chart of entries count created in time broken down by locales
    • Lists
      • Top 10 contributors (createBy/updateBy)
      • 10 Latest Draft entries
      • 10 Latest Published entries

EE Users

Collection Type overview

  • Stacked Area Chart of entries count created in time broken down by review workflow stages

Assigned Entries

  • Assigned Entries count
  • Pie Chart of assigned entries count broken down by review workflow stage
  • Stacked Area Chart of assigned entries count created in time broken down by review workflow stage
  • Lists of assigned entries broken down by review workflow stage (tabs)

Advanced features

  • Upcoming release(s) count
  • Number of days before the next release
  • Overview of audit logs (last 10 actions)

What's missing

While I'm proposing a dashboard foundation idea with this commit, some aspects are missing and will need discussions to move forward:

  • When do we display the dashboard? Should it become the default homepage? Should it appear when there is the first Collection Type created?@Aurelsicoko@alexandrebodin
  • Permissions. This is more a technical question but based on what I proposed, how can I actually make sure that users can't see the collection-type data that they don't have permission to see?
  • I'm executing a single query using the Query Engine API. From what@derrickmehaffysaid, it's not a good practice, but based on what I proposed, is there a better way to fetch the data and not making too much API requests using the Document Service API?
  • Global Design feedback@lucasboilly
  • Maybe there is some interesting data we can display in the dashboard that I'm missing

Thanks for your time!

This is how it looks like for an Enterprise User (Stacked Area Charts entries are all from the same day, so no area lol)
screen

Copy link

vercel bot commented Aug 1, 2024

The latest updates on your projects.Learn more aboutVercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
contributor-docs ❌ Failed (Inspect) Aug 6, 2024 6:22pm

@derrickmehaffy
Copy link
Member

@derrickmehaffy derrickmehaffy self-requested a review August 3, 2024 19:58
@derrickmehaffy
Copy link
Member

Looks like experimental failed to build because the yarn.lock is out of date, can you update it@Mcastres

@Mcastres
Copy link
Contributor Author

Mcastres commented Aug 6, 2024

What yarn.lock?

@derrickmehaffy
Copy link
Member

What yarn.lock?

Take a look at the failure log from the experimental build:https://github /strapi/strapi/actions/runs/10230554919/job/28305494036

yarn.lock looks like it needs to be updated as when the GH actions is installing modules it's updating the yarn.lock and our automations don't allow that.

@derrickmehaffy
Copy link
Member

building a new experimental:https://github /strapi/strapi/actions/runs/10271879316

@derrickmehaffy
Copy link
Member

exp version:0.0.0-experimental.1bca8e0e074de8b0775bcddc7656fbc9e9f1393b

})}
</Typography>
<Grid.Root gap={6} marginTop={4} marginBottom={8}>
{upcomingReleasesCount && (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Don't use a number as a conditional or you will just get a 0 and react will render a zero.

image

@derrickmehaffy
Copy link
Member

image


export async function getContentTypeStatistics(contentType: UID.CollectionType, userId: number): Promise<ContentTypeStatistics> {
// Fetch entries with associated user data that would always be there and just enough for the dashboard
const [entries, initialCount] = await strapi.db.query(contentType).findWithCount({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

While the documentService doesn't have a findWidthCount we can still use findMany and count here since we aren't saving any resources on using findWithCount. That function makes two DB calls on it's own:

findWithCount(params={}){
returnPromise.all([
db.entityManager.findMany(uid,params),
db.entityManager.count(uid,params),
]);
},

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Or we sweet talk the engineers into adding a findWithCount function the document service

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

on top of this I would add that we might want to exclude drafts or published versions here, or a pair of them might count as 2 entries, using the doc service would handle that automatically

@SalahAdDin
Copy link
Contributor

Technically looks amazing, but aesthetically ugly, pretty ugly.


export async function getContentTypeStatistics(contentType: UID.CollectionType, userId: number): Promise<ContentTypeStatistics> {
// Fetch entries with associated user data that would always be there and just enough for the dashboard
const [entries, initialCount] = await strapi.db.query(contentType).findWithCount({
Copy link
Collaborator

@Boegie19 Boegie19 Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

rbac rules should be checked here to ensure we are not leaking any protected field information (Names could be protected with RBAC rules and this would not respect that) + permissions should be check to see if I am allowed to know that the content-type exists

In its current state an user with 0 permissions and just a jwt token can see firstnames of everyone plus can check any content type likeadmin::usersto see how many users exist and when they where created.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

+1, normally we do add those set of permissions in the controller handler, we should check if user has permissions to read the content type by its uid.

an example of how you would build such permission check on a controller:

const{userAbility}=ctx.state;
if(userAbility.cannot('plugin::content-manager.explorer.read',uid){
returnctx.forbidden()
}

@Marc-Roig
Copy link
Contributor

Marc-Roig commented Aug 7, 2024

Finally!!! A useful homepage ❤️
I am surprised that you managed to pull that many info in that little amount of queries:) And about the amount of queries necessary, I would still use the document service findMany & count, and do a Promise.all to load both at the same time.

I have been testing your branch and I wanted to share some comments and feedback!, probably all of this could be iterated:

  1. Empty content types are not properly handled
    image

I believe we could show a brief warning of what is supposed to be there, or show empty graphs.

  1. Loading transition
    While it works fabulous on small databases, I believe we could improve the loading transition. Specially for projects with a big amount of data.

Loading the home page for the first time, if there is a delay loading the data ( I just added a delay withawait new Promise((resolve) => setTimeout(resolve, 5000));), you see a single loader
image

I would expect the "Project statistics" to be loaded quickly, and the "Collection Types Overview" to be slower, that way we could have a quicker first load:)

And when you select a different collection type, there is no loader:
https:// loom /share/e4b44e4e618f4b2abff70ca55e1e896c

  1. Performance
    While I normally prefer doing less queries, I believe this is a scenario where I would do multiple. Instead of loading all entries into memory (which could lead to millions of rows transfered into memory) I would rather do a single query per statistic and load them concurrently.

Probably a bit tricky for some of them, but this might be the most performant way for heavy dbs.

And another option would be to cache the statistics server side.

@Marc-Roig Marc-Roig removed their request for review September 3, 2024 07:31
@remidej remidej removed their request for review September 9, 2024 15:04
@alexandrebodin alexandrebodin deleted the branch v5/main September 18, 2024 15:32
@SalahAdDin
Copy link
Contributor

@alexandrebodinWhy did you close this?

@alexandrebodin
Copy link
Member

@SalahAdDinIt got closed automatically because we removed the base branch. Right now this work is on hold and will be taken over by a team in the upcoming month. They will resubmit the PR when ready 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants