Change recurringdonation status to exclude failed On STATS #1869
Conversation
CarlosQ96
changed the title
WalkthroughThe changes in this pull request primarily involve updates to the 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/services/recurringDonationService.ts(3 hunks)
🔇 Additional comments (3)
src/services/recurringDonationService.ts (3)
480-481: LGTM! Change is consistent with previous method.
The modification to exclude FAILED status is consistent with the changes in recurringDonationsCountPerDateRange(). The query structure maintains proper performance with WHERE clause before GROUP BY.
675-676: LGTM! Change is consistent with other count methods.
The modification to exclude FAILED status is consistent with other counting methods. The query maintains proper structure and performance.
431-432: LGTM! Verify handling of other donation statuses.
The change from filtering only ACTIVE to excluding FAILED status looks good. This will now include donations with other statuses (PENDING, ACTIVE, ENDED) in the count.
Let's verify the impact on other recurring donation statuses:
✅ Verification successful
Let me analyze the results to verify the handling of other donation statuses.
From the search results, I can see:
- All possible statuses are used in the codebase: ACTIVE, PENDING, ENDED, FAILED, VERIFIED
- Status transitions are well-defined:
- New donations start as PENDING
- PENDING can transition to ACTIVE
- PENDING can transition to FAILED
- ACTIVE can transition to ENDED
- The test files show proper handling of all statuses in counting scenarios
Let me verify one more aspect - the other similar count queries in the file to ensure consistency:
Change is consistent and correctly handles all donation statuses
The verification confirms that:
- All count queries in the service consistently exclude FAILED status
- Status transitions are well-defined in the codebase (PENDING → ACTIVE/FAILED, ACTIVE → ENDED)
- Test files demonstrate proper handling of all statuses (PENDING, ACTIVE, ENDED, FAILED, VERIFIED)
- The change aligns with the business logic by including all non-FAILED donations in counts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of RECURRING_DONATION_STATUS across the codebase
# Expected: Find all status transitions and status checks to ensure they're handled correctly
# Search for status assignments and transitions
rg -A 3 "status = RECURRING_DONATION_STATUS"
# Search for status checks in conditions
rg -A 3 "status === RECURRING_DONATION_STATUS"
Length of output: 8162
Script:
#!/bin/bash
# Check all count queries in recurringDonationService.ts
rg -A 3 "COUNT" src/services/recurringDonationService.ts
# Also check other WHERE clauses with status
rg -A 3 "where.*status" src/services/recurringDonationService.ts
Length of output: 1212
| .where('recurringDonation.status != :status', { | ||
| status: RECURRING_DONATION_STATUS.FAILED, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding status filter to USD total methods.
While the count methods now consistently exclude FAILED status, the related methods recurringDonationsStreamedCUsdTotal() and recurringDonationsStreamedCUsdTotalPerMonth() don't have this filter. Consider adding the same status filter to maintain consistency.
Example implementation:
const query = RecurringDonation.createQueryBuilder(
'recurringDonation',
).select('COALESCE(SUM(recurringDonation.totalUsdStreamed), 0)', 'total')
+ .where('recurringDonation.status != :status', {
+ status: RECURRING_DONATION_STATUS.FAILED,
+ });Committable suggestion skipped: line range outside the PR's diff.
Related to: https://github.com/orgs/Giveth/projects/9/views/1?sliceBy%5Bvalue%5D=CarlosQ96&pane=issue&itemId=85803313&issue=Giveth%7Canalytics-dashboard%7C42
Summary by CodeRabbit
New Features
Bug Fixes