Add contributions report query and allow filtering by order transaction charge date #11342
Conversation
gustavlrsn
left a comment
gustavlrsn
left a comment
There was a problem hiding this comment.
Looking good! Added a question about possible performance issues
| if (args.chargedDateFrom || args.chargedDateTo) { | ||
| where[Op.and].push( | ||
| sequelize.where(sequelize.literal(`COALESCE("Subscription"."lastChargedAt", "Order"."createdAt")`), { | ||
| [Op.gte]: args.chargedDateFrom, | ||
| }), | ||
| ); | ||
| } | ||
| if (args.chargedDateTo) { | ||
| where[Op.and].push( | ||
| sequelize.where(sequelize.literal(`COALESCE("Subscription"."lastChargedAt", "Order"."createdAt")`), { | ||
| [Op.lte]: args.chargedDateTo, | ||
| }), | ||
| sequelize.literal( | ||
| SequelizeUtils.formatNamedParameters( | ||
| `EXISTS ( | ||
| SELECT 1 FROM "Transactions" t | ||
| WHERE t."OrderId" = "Order"."id" AND | ||
| ${args.chargedDateFrom ? `COALESCE(t."clearedAt", t."createdAt") >= :chargedDateFrom AND` : ''} | ||
| ${args.chargedDateTo ? `COALESCE(t."clearedAt", t."createdAt") <= :chargedDateTo AND` : ''} | ||
| t."kind" in ('CONTRIBUTION', 'ADDED_FUNDS') AND | ||
| t."type" = 'CREDIT' AND | ||
| NOT t."isRefund" AND | ||
| t."RefundTransactionId" IS NULL AND | ||
| t."deletedAt" IS NULL | ||
| LIMIT 1 | ||
| )`, | ||
| { | ||
| chargedDateFrom: args.chargedDateFrom, | ||
| chargedDateTo: args.chargedDateTo, | ||
| }, | ||
| 'postgres', | ||
| ), | ||
| ), |
There was a problem hiding this comment.
What's the performance impact of this subquery?
If there is not already an index for this, maybe we could add one, or maybe use the existing condition when you only supply chargedDateFrom without chargedDateTo
There was a problem hiding this comment.
I tested the generated query on prod and the result is not great (~5s), I am trying something else I will update this PR with a better query strategy.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
server/graphql/v2/object/Host.ts
Outdated
| dateTo: moment(args.dateTo).utc().toISOString(), | ||
| dateFrom: moment(args.dateFrom).utc().toISOString(), |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| { | ||
| chargedDateFrom: args.chargedDateFrom, | ||
| chargedDateTo: args.chargedDateTo, | ||
| }, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
server/lib/sequelize-cte.ts
Outdated
| identifierField: leftKey, | ||
| } as unknown as Association<Model, Model>, | ||
| _pseudo: true, | ||
| required: true, |
There was a problem hiding this comment.
This should be optional, the filtering nature of this include should be kept within the main query and not implicitly defined by the helper function.
| ${args.chargedDateFrom ? `COALESCE(t."clearedAt", t."createdAt") >= :chargedDateFrom AND` : ''} | ||
| ${args.chargedDateTo ? `COALESCE(t."clearedAt", t."createdAt") <= :chargedDateTo AND` : ''} |
There was a problem hiding this comment.
You can use the helper ifStr.
| }, | ||
| models.Order, | ||
| 'id', | ||
| { required: true }, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
server/graphql/v2/object/Host.ts
Outdated
| dateTo: moment(args.dateTo).utc().toISOString(), | ||
| dateFrom: moment(args.dateFrom).utc().toISOString(), | ||
| }, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ${ifStr(args.chargedDateFrom, `COALESCE(t."clearedAt", t."createdAt") >= :chargedDateFrom AND`)} | ||
| ${ifStr(args.chargedDateTo, `COALESCE(t."clearedAt", t."createdAt") <= :chargedDateTo AND`)} | ||
| t."type" = 'CREDIT' AND | ||
| NOT t."isRefund" AND | ||
| t."RefundTransactionId" IS NULL AND | ||
| t."deletedAt" IS NULL | ||
| ${ifStr(host?.id, `AND t."HostCollectiveId" = :hostCollectiveId`)} |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| chargedDateFrom: args.chargedDateFrom, | ||
| chargedDateTo: args.chargedDateTo, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
3 participants
Related opencollective/opencollective#8477
Resolve opencollective/opencollective#8498
Resolve opencollective/opencollective#8500