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.

Sign up for GitHub

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

Jump to bottom

Status page: Display total number of accounts #84

Merged
merged 1 commit into from
Sep 8, 2021
Merged

Status page: Display total number of accounts #84

merged 1 commit into from
Sep 8, 2021

Conversation

AnnikaCodes
Copy link
Contributor

@AnnikaCodes AnnikaCodes commented Sep 4, 2021 •
edited
Loading

Displays the total number of accounts on the status page

This provides transparency about how many people use Pantry (and by extension, gives users an idea of how much time and money goes into maintaining it).

  • Adds a static getTotalNumber() method to the Account class
    • We use the SCAN pattern account:*-*-*-*-[a-z0-9][a-z0-9][a-z0-9][a-z0-9][a-z0-9][a-z0-9][a-z0-9][a-z0-9][a-z0-9][a-z0-9][a-z0-9][a-z0-9] to match 'account:' followed by a UUID, with nothing after it.
  • Adds a totalAccount field to the ISystemStatus interface.
  • Adds a scan() method to the data store service, which allows use of the Redis command SCAN.
  • Updates the system controller unit tests to test the above.
  • Displays the total number of accounts on the status page.
    • Adds some formatting logic to the frontend to make it look better.

Partially resolves #59

@imRohan
Copy link
Owner

imRohan commented Sep 5, 2021

@AnnikaCodes Awesome work, ill get to reviewing this shortly. I'm curious why the Github Workflow hasn't run properly yet. Can you push an empty commit git commit --amend --no-edit to see if it re-triggers all the checks?

@imRohan imRohan self-requested a review September 5, 2021 14:04
@imRohan imRohan added the enhancement New feature or request label Sep 5, 2021
@imRohan imRohan linked an issue Sep 5, 2021 that may be closed by this pull request
Add Transparency #59
Closed
@AnnikaCodes
Copy link
Contributor Author

AnnikaCodes commented Sep 5, 2021 •
edited
Loading

Do you need to approve it? When using GitHub Actions on other repositories, I've had to manually approve the CI run for first-time contributors.

@imRohan
Copy link
Owner

imRohan commented Sep 6, 2021 •
edited
Loading

Do you need to approve it? When using GitHub Actions on other repositories, I've had to manually approve the CI run for first-time contributors.

I merged in the latest changes from master which lets the CI action run on pull requests. Could you please squash all of the commits into one please? Thanks!

@AnnikaCodes
Copy link
Contributor Author

Sure, I'll squash it and also fix the linter errors while I'm at it.

@AnnikaCodes
Copy link
Contributor Author

There's still one linter failure locally, but it's because tslint crashes. This bug was reported (palantir/tslint#4949), but hasn't been fixed because tslint is deprecated. You might want to switch to typescript-eslint or another linting tool.

imRohan
imRohan requested changes Sep 7, 2021
View reviewed changes
Copy link
Owner

@imRohan imRohan left a comment

Choose a reason for hiding this comment

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

Looks great, this is a welcomed addition for sure!

I've left a few comments, mostly small. In addition to the requested changes, could you please update the PR message as per the newly added pull request template? link

src/models/account.ts Outdated Show resolved Hide resolved
src/controllers/system.ts Outdated Show resolved Hide resolved
src/models/account.ts Outdated Show resolved Hide resolved
src/services/dataStore.ts Outdated Show resolved Hide resolved
src/services/dataStore.ts Show resolved Hide resolved
@imRohan imRohan mentioned this pull request Sep 7, 2021
Closed
@AnnikaCodes
Status page: Display total number of accounts
8a54e38 
As described in #59, this publicized the current number of active accounts/pantries.
I don't know how big Pantry is or how much traffic you get; it might make sense to cache the total number of accounts, although I'm not familiar enough with Redis to know how to mark that cache as stale when Redis marks a key as expired (though it's probably not a huge deal if old accounts are included in the tally for a bit?).

Anyway, this seems like an intriguing project and I look forward to helping out more!
@imRohan imRohan self-requested a review September 8, 2021 00:46
@imRohan imRohan added Ready for Review Once you've squashed your commits, the PR is now ready for review! and removed enhancement New feature or request labels Sep 8, 2021
@imRohan imRohan merged commit 4a064be into imRohan:master Sep 8, 2021
@imRohan
Copy link
Owner

imRohan commented Sep 9, 2021

@AnnikaCodes Just a heads up but this is now live on production 🎉

@AnnikaCodes
Copy link
Contributor Author

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imRohan imRohan Awaiting requested review from imRohan

Assignees

@AnnikaCodes AnnikaCodes

Labels
Ready for Review Once you've squashed your commits, the PR is now ready for review!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Transparency
2 participants
@AnnikaCodes @imRohan