Fix date filters for recurringdonation stats #1872
Conversation
WalkthroughThe changes made in this pull request involve modifications to the handling of recurring donations within two files: Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/services/recurringDonationService.ts (1)
Inconsistent Date Field Usage
The date field in
TO_CHARfunctions is usingcreatedAtinstead ofupdatedAtin the following lines:
src/services/recurringDonationService.ts: .addSelect("TO_CHAR(recurringDonation.createdAt, 'YYYY/MM')", 'date')🔗 Analysis chain
Line range hint
532-636: Verify date field consistency across all statistics functionsLet's verify if there are any other instances where the date field needs to be updated from
createdAttoupdatedAtfor consistency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances where createdAt is used in TO_CHAR for date grouping rg -A 2 "TO_CHAR.*createdAt.*YYYY/MM" --type tsLength of output: 566
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/repositories/recurringDonationRepository.ts(1 hunks)src/services/recurringDonationService.ts(3 hunks)
🔇 Additional comments (4)
src/repositories/recurringDonationRepository.ts (2)
109-110: LGTM! The updatedAt timestamp update aligns with PR objectives.
The addition of updatedAt = NOW() ensures the timestamp is updated whenever the recurring donation's amounts are recalculated from stream donations, which is essential for accurate date-based filtering.
Line range hint 182-196: Consider updating getPendingRecurringDonationsIds to use updatedAt
This function still uses createdAt for filtering pending donations. For consistency with the PR's objective of using updatedAt for date filters, consider updating this as well.
Let's verify if this is the only instance where we might need to update the filter:
Consider this change:
return RecurringDonation.find({
where: {
status: RECURRING_DONATION_STATUS.PENDING,
- createdAt: MoreThan(date),
+ updatedAt: MoreThan(date),
},
select: ['id'],
});src/services/recurringDonationService.ts (2)
532-538: LGTM! Date filter change improves accuracy of streamed totals
The change from createdAt to updatedAt in the date filters will correctly capture the total USD value of streamed donations based on when they were last updated, aligning with the PR's objective.
630-636: LGTM! Consistent date filter update for per-token totals
The change from createdAt to updatedAt in the date filters maintains consistency with other statistics functions and ensures accurate per-token totals based on when donations were streamed.
CarlosQ96
force-pushed
the
hotfix_recurring_donation_totals
branch
from
1e00582
to
66f9699
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/services/recurringDonationService.ts (1)
Line range hint
667-693: Maintain consistency in date field usageWhile most functions have been updated to use
updatedAt, this function still usescreatedAtfor date filtering. This inconsistency could lead to mismatched results when comparing stats across different queries.Apply this diff to maintain consistency:
if (fromDate) { - query.andWhere('recurringDonation.createdAt >= :fromDate', { + query.andWhere('recurringDonation.updatedAt >= :fromDate', { fromDate: new Date(fromDate), }); } if (toDate) { - query.andWhere('recurringDonation.createdAt <= :toDate', { + query.andWhere('recurringDonation.updatedAt <= :toDate', { toDate: new Date(toDate), }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/repositories/recurringDonationRepository.ts(1 hunks)src/services/recurringDonationService.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/repositories/recurringDonationRepository.ts
🔇 Additional comments (2)
src/services/recurringDonationService.ts (2)
532-538: LGTM: Date filter changes align with PR objectives
The changes correctly update the date filters from createdAt to updatedAt in these functions, which aligns with the PR objective to fix recurring donation stats by using the timestamp that reflects updates from mini donations.
Also applies to: 578-584, 630-636
532-538: Verify cache key implementation
The cache keys for these functions include the date range parameters but don't distinguish between createdAt and updatedAt usage. This could lead to serving stale data during the transition.
Also applies to: 578-584, 630-636
Issue: Giveth/analytics-dashboard#42
Basically the filters in our queries used the createdAt however this would excluded the data expected from the streams in the stats. We should use the updatedAt parameter because the stream is updated on every new mini donation in our db, totals will make sense now and won't be excluded.
Summary by CodeRabbit
New Features
updatedAttimestamp for data queries.Bug Fixes
Refactor