-
Notifications
You must be signed in to change notification settings - Fork 4
Delivery Stats KPI Updates #900
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis PR modifies the reporting infrastructure to redefine user engagement and funding metrics. The function Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
commcare_connect/reports/helpers.py (1)
243-266: Consider using float division for average calculation.The function computes the average earnings for top FLWs using integer division (line 264:
// top_five_percent_flw_count). This truncates the result and loses precision. Consider using regular division (/) to preserve decimal precision, especially since payment amounts are tracked in USD and typically have decimal values.🔎 Suggested fix
- avg_top_earned_flws = ( - sum(sorted(sum_total_users.values(), reverse=True)[:top_five_percent_flw_count]) - // top_five_percent_flw_count - ) + avg_top_earned_flws = ( + sum(sorted(sum_total_users.values(), reverse=True)[:top_five_percent_flw_count]) + / top_five_percent_flw_count + )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
commcare_connect/reports/helpers.py(5 hunks)commcare_connect/reports/tables.py(1 hunks)commcare_connect/reports/tests/test_reports.py(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-11T08:11:43.757Z
Learnt from: sravfeyn
Repo: dimagi/commcare-connect PR: 804
File: commcare_connect/opportunity/views.py:909-910
Timestamp: 2025-11-11T08:11:43.757Z
Learning: In commcare_connect/opportunity/views.py, when calling update_payment_accrued, it's acceptable and more efficient to pass a QuerySet (e.g., visits.values_list("user_id", flat=True).distinct()) rather than converting to a list, as the function only uses it in a filter(user__in=users) context where Django natively supports QuerySets.
Applied to files:
commcare_connect/reports/tests/test_reports.py
📚 Learning: 2025-11-12T14:56:27.468Z
Learnt from: Charl1996
Repo: dimagi/commcare-connect PR: 795
File: commcare_connect/users/user_credentials.py:43-59
Timestamp: 2025-11-12T14:56:27.468Z
Learning: In commcare_connect/users/user_credentials.py, the credential calculation for delivery credentials uses Count("opportunity_access__user_id") to count the number of CompletedWork instances per user, not Sum("saved_approved_count"). This is intentional—credentials are earned based on the number of CompletedWork records, not the total number of deliveries within those records.
Applied to files:
commcare_connect/reports/helpers.py
🧬 Code graph analysis (2)
commcare_connect/reports/tests/test_reports.py (2)
commcare_connect/opportunity/tests/factories.py (4)
PaymentInvoiceFactory(257-264)UserVisitFactory(130-153)PaymentFactory(248-254)CompletedWorkFactory(120-127)commcare_connect/opportunity/models.py (2)
VisitValidationStatus(436-442)CompletedWorkStatus(512-517)
commcare_connect/reports/tables.py (1)
commcare_connect/opportunity/tables.py (1)
SumColumn(106-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
🔇 Additional comments (17)
commcare_connect/reports/helpers.py (7)
44-44: LGTM: Function renamed to reflect "activated connect users" semantics.The rename from
get_eligible_user_counts_cumulativetoget_activated_connect_user_counts_cumulativealigns with the PR objective to clarify that these are activated users (those with approved completed work) rather than just "eligible" users.
101-104: LGTM: Field names updated to reflect new semantics.The defaultdict now includes the renamed fields (
intervention_funding_deployed,organization_funding_deployed,avg_top_earned_flws) that align with the PR objectives.
180-189: LGTM: FLW payment aggregation updated.The aggregation now computes
flw_amount_paidfrom Payment records filtered by opportunity-level constraints, which correctly reflects payments to FLWs.
191-204: LGTM: Organization funding deployment aggregation added.The aggregation computes
organization_funding_deployedfrom Payment records whereinvoice__service_delivery=False. Note that this aggregates paid amounts (from Payment model), whereasintervention_funding_deployedaggregates earned amounts (from CompletedWork model). This distinction appears intentional based on the PR objectives.
206-206: LGTM: Average top earned FLWs calculation extracted to helper.The inline calculation has been extracted to a separate helper function for better maintainability and readability.
209-209: LGTM: Updated to use activated connect user counts.The variable has been renamed to
total_activated_connect_user_countsto reflect the function rename, maintaining consistency throughout the codebase.
234-234: LGTM: Activated connect users field added.The
activated_connect_usersfield is now populated in the report data, replacing the previous field name to align with the new semantics.commcare_connect/reports/tests/test_reports.py (7)
69-79: LGTM: Invoice setup added to test data.The test now creates a
PaymentInvoiceand links it toCompletedWork, which is required for the newintervention_funding_deployedcalculation that filters byinvoice__service_delivery.
86-99: LGTM: Payment and invoice setup correctly reflects new calculation logic.The test creates:
- FLW payments (line 86) for the
flw_amount_paidcalculation- Invoice payments with
service_delivery=True(line 87)- Additional
CompletedWorkwithservice_delivery=Falseinvoice (lines 89-98) for theorganization_funding_deployedcalculationThis setup correctly validates the new earned vs. paid semantics and the service_delivery filter logic.
114-125: LGTM: Assertions updated to reflect new field names and calculations.The assertions correctly validate:
activated_connect_usersreplacing the old field nameflw_amount_paidwith the correct expected value (2250)- New
intervention_funding_deployedandorganization_funding_deployedfieldsavg_top_earned_flwscorrectly computed as 900 (top earner among 10 users)
203-221: LGTM: Delivery type test setup updated with invoice wiring.The test setup for delivery type filtering correctly creates invoices and links them to completed work, matching the pattern in the main test.
236-248: LGTM: Delivery type test assertions updated.The assertions correctly validate the new field names and expected values for the filtered delivery type scenario:
flw_amount_paid= 500 (5 users)intervention_funding_deployed= 500avg_top_earned_flws= 400 (top earner among 5 users)
263-293: LGTM: Currency filter test setup updated with invoice wiring.The test setup for currency filtering correctly creates invoices for both service delivery and organization funding scenarios.
311-315: LGTM: Currency filter test assertions updated.The assertions correctly validate the new field names and expected values, matching the main test expectations for 10 users.
commcare_connect/reports/tables.py (3)
11-12: LGTM: Table columns updated to reflect new user metrics.The table now includes:
activated_connect_userswith verbose name "Activated Connect Users" (cumulative total)usersrenamed to "Monthly Activated Connect Users" (monthly count)This aligns with the PR objective to clarify the distinction between monthly and cumulative user counts.
20-21: LGTM: Funding deployment columns added.The new columns
intervention_funding_deployedandorganization_funding_deployedreplace the previousnm_amount_paidandnm_other_amount_paidcolumns, providing clearer semantics for the different types of funding.
23-23: LGTM: Average earned column updated.The column is renamed from
avg_top_paid_flwstoavg_top_earned_flwsto reflect that it now tracks earned amounts rather than paid amounts, aligning with the PR objectives.
Product Description
The following columns have been renamed in the delivery stats admin report:
The following calculation updates were also made:
Technical Summary
Link to ticket here.
This PR renames a few columns for the delivery stats admin report. Additionally, some updates were made to the calculations of two columns to reflect the earned amount instead of the paid amount. Variable names were updated along with the column name changes to accurately reflect what the variable represents.
Safety Assurance
Safety story
Automated test coverage
Unit tests have been updated to validate that the calculation and column rename changes are working as expected.
QA Plan
No QA planned.
Labels & Review