Correct "total donated" amount in user's monthly report #1563

marcogbarcellos · 2018-12-03T13:50:33Z

This PR properly filters orders to return orders only when their subscriptions are active.

references opencollective/opencollective#1504
fixes opencollective/opencollective#1505

Betree

Edit: Sorry @marcogbarcellos I forgot something in the previous query, please see the comment below.

Betree · 2018-12-03T20:18:07Z

cron/monthly/user-report.js

+    include: [
+      {
+        model: models.Subscription,
+        where: {


Adding a where clause to the include automatically makes it required. I think we may miss orders that don't have a subscription here.

Hum, I'll better use just a filter then. thanks for the good catch.

coveralls · 2018-12-04T03:38:39Z

Coverage remained the same at 69.59% when pulling 33dfb89 on fix-1504/orders-user-report into 909e79a on master.

Betree · 2018-12-04T09:20:12Z

--Removed--

znarf · 2018-12-04T11:01:19Z

cron/monthly/user-report.js

+      [Op.or]: {
+        FromCollectiveId,
+        UsingVirtualCardFromCollectiveId: FromCollectiveId,
+      },


No need to support Virtual Card here at the moment.

Yes sorry @marcogbarcellos, I didn't realized this was user report and user cannot emit virtualcards, my first comment was misleading.

znarf · 2018-12-04T11:09:04Z

cron/monthly/user-report.js

  const collectivesWithOrders = [];
  collectives.map(collective => {
+    // It's only possible to have one order with subscription active Per Collective
+    const collectiveOrder = ordersByCollectiveId[collective.id] && ordersByCollectiveId[collective.id][0];


What happens if collectiveOrder is null?

@marcogbarcellos ?

znarf · 2018-12-04T11:10:36Z

cron/monthly/user-report.js

-    ordersByCollectiveId[o.CollectiveId] = o;
-  });
+  // group orders(by collective) that either don't have subscription or have active subscription
+  const ordersByCollectiveId = groupBy(orders.filter(o => !o.Subscription || o.Subscription.isActive), 'CollectiveId');


It might be possible that we have collectives with multiple donations each month (multiple single time donation, mix of subscription + single time, and even multiple subscriptions).

How do we handle that?

This script was considering only subscription orders. At first I thought it was intentional but It makes senses to cover all orders regardless. I'll update the code to reflect it.

@marcogbarcellos Was the user-report limited to subscriptions until now? If that's the case, where was that enforced?

Sorry, i explained wrong. I meant it was only getting the latest order of each collective which would ONLY make sense if it was considering subscriptions only(afaik a contributor is supposed to have only one subscription per collective). Actually still wouldn't because the subscription deactivation were being ignored as well. The following part of master branch's user-report is wrong because it's basically iterating over the Order.findAll result and if there is more than one order of the same collective it would consider only the latest one:
https://github.com/opencollective/opencollective-api/blob/master/cron/monthly/user-report.js#L204-L207

marcogbarcellos · 2018-12-05T08:08:42Z

all good to merge here @znarf @Betree ?

marcogbarcellos · 2018-12-05T09:07:36Z

cron/monthly/user-report.js

-    else return 1;
+    // It's only possible to have one order with subscription active Per Collective
+    const collectiveOrders = ordersByCollectiveId[collective.id] && ordersByCollectiveId[collective.id];
+    collectiveOrders &&


@znarf that commented you've just marked me is outdated. I'm just pushing to collectivesWithOrders after validatingcollectiveOrders.

znarf · 2018-12-05T09:20:12Z

cron/monthly/user-report.js

-    if (get(a, 'order.totalAmount') > get(b, 'order.totalAmount')) return -1;
-    else return 1;
+    // It's only possible to have one order with subscription active Per Collective
+    const collectiveOrders = ordersByCollectiveId[collective.id] && ordersByCollectiveId[collective.id];


Doesn't looks good.

znarf · 2018-12-05T09:21:29Z

cron/monthly/user-report.js

-    else return 1;
+    // It's only possible to have one order with subscription active Per Collective
+    const collectiveOrders = ordersByCollectiveId[collective.id] && ordersByCollectiveId[collective.id];
+    collectiveOrders &&


In that case, avoid the && syntax, if will be easier to read.

agreed and done. thanks.

znarf · 2018-12-05T09:49:11Z

@marcogbarcellos What about line 214 comment?

marcogbarcellos · 2018-12-05T09:53:27Z

@znarf the line 214 comment doesn't make sense anymore as we are getting all orders given a FromCollective and a Collective.

znarf · 2018-12-05T09:58:05Z

@marcogbarcellos Ok, my 2nd comment was made invalid by: https://github.com/opencollective/opencollective-api/compare/e461b0a7121364cbf8191262bab8652ee23cc8dc..a9b9bc88908119b5f679fc120f5946cb6d511a32

znarf · 2018-12-05T10:08:02Z

cron/monthly/user-report.js

@@ -451,6 +452,7 @@ const computeStats = async (collectives, currency = 'USD') => {
  stats.expensesBreakdownString = `${Object.keys(categories).length > 3 ? ', mostly in' : ' in'} ${formatArrayToString(
    ar,
  )}`;
+  console.log(`>>> Stats: ${JSON.stringify(stats, null, 2)}`);


Remove console.log ?

I believe this console.log may be really helpful to check the logs after running these scripts. in this case it'll show the consolidated stats per user. I think it worths keeping this console.log as we already have other logs in this report as well. But let me know if you think it's too much.

Make sense. Do you know how to retrieve logs from a cron task?

afaik the same way we can log the app but instead of using --app we'd use the scheduler job id. example: heroku logs -d scheduler.5255

…n active

… don't have subscription OR have active subscriptions, consider UsingVirtualCardFromCollectiveId as well fix eslint

…veId case for now on Transactions query

…ubscription ones) of the contributors fix: better validation fix: better validation

marcogbarcellos requested a review from znarf December 3, 2018 13:51

znarf requested a review from Betree December 3, 2018 14:10

Betree requested changes Dec 3, 2018

View reviewed changes

marcogbarcellos force-pushed the fix-1504/orders-user-report branch from 8b48432 to 158a904 Compare December 4, 2018 01:37

znarf reviewed Dec 4, 2018

View reviewed changes

marcogbarcellos force-pushed the fix-1504/orders-user-report branch 2 times, most recently from 3a9266d to e461b0a Compare December 4, 2018 14:56

marcogbarcellos commented Dec 5, 2018

View reviewed changes

znarf reviewed Dec 5, 2018

View reviewed changes

marcogbarcellos force-pushed the fix-1504/orders-user-report branch 2 times, most recently from a9b9bc8 to 36b77fb Compare December 5, 2018 09:28

znarf approved these changes Dec 5, 2018

View reviewed changes

znarf reviewed Dec 5, 2018

View reviewed changes

marcogbarcellos added 4 commits December 5, 2018 11:14

fix(cron/user-report): filter orders to get the ones with subscriptio…

f049f04

…n active

fix(scripts/monthly/user-report): we should filter orders that either…

3372a02

… don't have subscription OR have active subscriptions, consider UsingVirtualCardFromCollectiveId as well fix eslint

fix(scron/monthly/user-report): ignoring UsingVirtualCardFromCollecti…

c340ac5

…veId case for now on Transactions query

fix(cron/monthly/user-report): we should sum up all orders(not only s…

33dfb89

…ubscription ones) of the contributors fix: better validation fix: better validation

marcogbarcellos force-pushed the fix-1504/orders-user-report branch from 36b77fb to 33dfb89 Compare December 5, 2018 16:14

marcogbarcellos merged commit 8b5044b into master Dec 5, 2018

marcogbarcellos deleted the fix-1504/orders-user-report branch December 5, 2018 16:15

snyk-bot mentioned this pull request Apr 27, 2021

[Snyk] Fix for 6 vulnerabilities Jankyboy/opencollective-api#38

Open

snyk-bot mentioned this pull request Aug 11, 2021

[Snyk] Upgrade redis from 3.0.2 to 3.1.2 Andrea-MariaDB-2/opencollective-api#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct "total donated" amount in user's monthly report #1563

Correct "total donated" amount in user's monthly report #1563

marcogbarcellos commented Dec 3, 2018

Betree left a comment •

edited

Loading

Betree Dec 3, 2018

marcogbarcellos Dec 3, 2018

coveralls commented Dec 4, 2018 •

edited

Loading

Betree commented Dec 4, 2018 •

edited

Loading

znarf Dec 4, 2018

Betree Dec 4, 2018

znarf Dec 4, 2018

znarf Dec 5, 2018

znarf Dec 4, 2018

marcogbarcellos Dec 4, 2018

znarf Dec 4, 2018

marcogbarcellos Dec 4, 2018 •

edited

Loading

marcogbarcellos commented Dec 5, 2018 •

edited

Loading

marcogbarcellos Dec 5, 2018

znarf Dec 5, 2018

znarf Dec 5, 2018

marcogbarcellos Dec 5, 2018

znarf commented Dec 5, 2018

marcogbarcellos commented Dec 5, 2018 •

edited

Loading

znarf commented Dec 5, 2018

znarf Dec 5, 2018

marcogbarcellos Dec 5, 2018

znarf Dec 5, 2018

marcogbarcellos Dec 5, 2018

Correct "total donated" amount in user's monthly report #1563

Correct "total donated" amount in user's monthly report #1563

Conversation

marcogbarcellos commented Dec 3, 2018

Betree left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 4, 2018 • edited Loading

Betree commented Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcogbarcellos Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

marcogbarcellos commented Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

znarf commented Dec 5, 2018

marcogbarcellos commented Dec 5, 2018 • edited Loading

znarf commented Dec 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Betree left a comment •

edited

Loading

coveralls commented Dec 4, 2018 •

edited

Loading

Betree commented Dec 4, 2018 •

edited

Loading

marcogbarcellos Dec 4, 2018 •

edited

Loading

marcogbarcellos commented Dec 5, 2018 •

edited

Loading

marcogbarcellos commented Dec 5, 2018 •

edited

Loading