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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENG-6122] Added new attributes to the mirage factories #2339

Merged

Conversation

bp-cos
Copy link
Contributor

@bp-cos bp-cos commented Sep 20, 2024

Purpose

Added new attributes to the mirage factories

Summary of Changes

Added attributes to the institution-summary-metric and instution mirage files.

Copy link
Contributor

@futa-ikeda futa-ikeda left a comment

Choose a reason for hiding this comment

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

Looks good to me. A couple nice-to-haves, but not necessary if you just want to merge it

Comment on lines 25 to 26
storageByteCount() {
return faker.random.number({ min: 100, max: 1000 });
Copy link
Contributor

@futa-ikeda futa-ikeda Sep 20, 2024

Choose a reason for hiding this comment

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

Nit: Could be worthwhile to bump the max on this one a little higher

Comment on lines 52 to 63
summaryMetrics.update({
publicProjectCount,
privateProjectCount,
userCount,
publicRegistrationCount: 1250,
preprintCount: 11250,
embargoedRegistrationCount: 456,
storageByteCount: 47382032,
publicFileCount: 87,
monthlyLoggedInUserCount: 24563,
monthlyActiveUserCount:456,
});
Copy link
Contributor

@futa-ikeda futa-ikeda Sep 20, 2024

Choose a reason for hiding this comment

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

It seems like the logic that was here before this PR was trying to be smart and infer publicProjectCount, privateProjectCount and userCount from the 15 institution-users created on line 36 to make the data look a bit more "realistic". While I think that was likely a good idea when we showed the user numbers and summary numbers on the same page before, I don't think this entire summaryMetrics.update is necessary anymore.

Nit: Would you mind removing this update block and lines 42-48 (const userCount through userMetrics.forEach)?

@bp-cos bp-cos merged commit ac605ac into CenterForOpenScience:feature/insti-dash-improv Sep 20, 2024
9 checks passed
@bp-cos bp-cos deleted the feature/eng-6122 branch September 20, 2024 19:24
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.

2 participants