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

Add platform statistics #487

Merged
merged 10 commits into from
Apr 24, 2023
Merged

Conversation

Nnachevvv
Copy link
Member

@Nnachevvv Nnachevvv commented Apr 18, 2023

Part of podkrepi-bg/frontend#1412, which will close this issue podkrepi-bg/frontend#1391

Motivation and context

In order to gather the statistics for the new Homepage section we would need a way to get the sum of donated money and number of the donors.

Testing

Steps to test

GET request to:
http://localhost:3040/api/v1/donation/users-donated
http://localhost:3040/api/v1/vault/money

New endpoints

This change opens two new endpoints needed for homepage statistics:

  • vault/money - gather money from all vaults
  • donations/users-donated - get number of donated users + anonymous users

@github-actions
Copy link

github-actions bot commented Apr 18, 2023

✅ Tests will run for this PR. Once they succeed it can be merged.

@Nnachevvv Nnachevvv marked this pull request as ready for review April 18, 2023 13:03
Comment on lines 97 to 99
@Get('users-donated')
@Public()
async usersDonated() {
Copy link
Contributor

@igoychev igoychev Apr 21, 2023

Choose a reason for hiding this comment

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

[naming] in this way the endpoint will be /api/v1/donation/user-donations, however it actually returns donors-count, so let's name it like this: /api/v1/donation/donors-count

@Nnachevvv Nnachevvv requested a review from igoychev April 21, 2023 09:07
@@ -642,6 +642,20 @@ export class DonationsService {
return user.id
}

async getDonatedUsersCount() {
const donatedUsersCount = await this.prisma.donation.groupBy({
Copy link
Member

@sashko9807 sashko9807 Apr 21, 2023

Choose a reason for hiding this comment

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

Can't quite get the idea to group the donations by personId. Is there a reason not to use the prisma count() API, to get the total amount of succeeded donations?

Edit: Saw that I commented on an outdated file for which I apologize, but since the logic has not changed, the question remains.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, After a discussion with @igoychev, we decided that it would be better to group the donations by "billingName".

Regarding your question - we should have a way to count how many donations has "billingName" as null value, since we count them as different donors. Unfortunately, this is currently not possible with count()+distinct functionality provided by prisma, at least I couldn't find out how to do that. Therefore we had a two ways to implement this - having two queries or the way which I had implemented it. I'm open to any suggestions how we can improve that.

Copy link
Member

@sashko9807 sashko9807 Apr 21, 2023

Choose a reason for hiding this comment

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

Thanks for the explanation, by reading the front-end related issue, I was left with the impression that total amount of donations was supposed to be shown, rather than the total amount of users who have donated.

Aside from using raw SQL query to avoid prisma's limitation, the only improvement I could really think of is to order the records by null first(if possible). That way array.find, becomes O(1) in best-case scenario.

Also not sure if it is necessary to cover the unlikely usecase of anonymousDonations being undefined(there are no anonymous donations). In such scenarios the donorsCount will always be off by one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I have implemented it and now donations with billingName = "null" will appear always first , and in this way query is running in O(1).

So regarding the front-end issue , there is a small change. Instead of the number of active contributors, we decided to show the number of donors. So we will have both - total amount of donations and number of different donors.

Copy link
Contributor

@igoychev igoychev 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!

@igoychev igoychev added the run tests Allows running the tests workflows for forked repos label Apr 23, 2023
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Apr 23, 2023
@igoychev igoychev merged commit baa6dcc into podkrepi-bg:master Apr 24, 2023
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.

3 participants