Skip to content

Conversation

@zandre-eng
Copy link
Contributor

Product Description

The following columns have been renamed in the delivery stats admin report:

  • Average paid to Top FLWs -> Average Earned by Top FLWs
  • NM Amount Paid -> Intervention Funding Deployed
  • NM Other Amount Paid -> Organization Funding Deployed
  • Paid Users -> Monthly Activated Connect Users
  • Total Paid Users -> Activated Connect Users

The following calculation updates were also made:

  • Average Earned by Top FLWs: This column now calculates the average earned by top FLWs, previously from calculating the paid amount.
  • Intervention Funding Deployed: This column now calculates the LLO amount earned, previously from calculating the paid amount.

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

  • Local testing.
  • Unit tests.
  • Admin-only report, does not affect real end-users.

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

  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 18, 2025

Walkthrough

This PR modifies the reporting infrastructure to redefine user engagement and funding metrics. The function get_eligible_user_counts_cumulative is renamed to get_activated_connect_user_counts_cumulative. In the reporting schema, columns nm_amount_paid and nm_other_amount_paid are replaced with intervention_funding_deployed and organization_funding_deployed, while avg_top_paid_flws is renamed to avg_top_earned_flws. A new helper function _calculate_avg_top_earned_flws is introduced to compute average earnings per delivery type. Data aggregation logic is updated to calculate funding deployed with invoice service delivery filters, and test cases are adjusted to reflect the new column names and updated aggregation values.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

  • Multiple interconnected files with heterogeneous changes (renaming functions, removing/adding columns, modifying aggregation logic)
  • New helper function with non-trivial aggregation logic requiring verification
  • Data schema changes across tables and helpers require cross-file consistency checks
  • Test value adjustments suggest business logic changes in calculations that need validation
  • Payment and funding calculations with conditional filters need careful verification

Areas requiring extra attention:

  • New aggregation logic for intervention_funding_deployed (with invoice__service_delivery=True filter) and its calculation accuracy
  • Implementation of _calculate_avg_top_earned_flws helper function and how it integrates with the data flow
  • Verification that expected values in tests reflect the new aggregation patterns correctly (e.g., reduced flw_amount_paid values)
  • Consistency of the semantic shift from "eligible/paid" users to "activated connect" users across all data population paths

Possibly related PRs

  • Rename KPIs #835 — Modifies verbose name labels for KPI columns in AdminReportTable, whereas this PR performs deeper structural changes including column removal, renaming, and addition.

Suggested reviewers

  • calellowitz
  • Charl1996
  • sravfeyn
  • ajeety4

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Delivery Stats KPI Updates' is directly related to the changeset, which involves renaming and updating KPI columns in the delivery stats admin report.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, detailing column renames, calculation updates, technical rationale, and testing performed.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ze/kpi-updates

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between b0e9a03 and d0fb921.

📒 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_cumulative to get_activated_connect_user_counts_cumulative aligns 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_paid from 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_deployed from Payment records where invoice__service_delivery=False. Note that this aggregates paid amounts (from Payment model), whereas intervention_funding_deployed aggregates 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_counts to reflect the function rename, maintaining consistency throughout the codebase.


234-234: LGTM: Activated connect users field added.

The activated_connect_users field 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 PaymentInvoice and links it to CompletedWork, which is required for the new intervention_funding_deployed calculation that filters by invoice__service_delivery.


86-99: LGTM: Payment and invoice setup correctly reflects new calculation logic.

The test creates:

  1. FLW payments (line 86) for the flw_amount_paid calculation
  2. Invoice payments with service_delivery=True (line 87)
  3. Additional CompletedWork with service_delivery=False invoice (lines 89-98) for the organization_funding_deployed calculation

This 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_users replacing the old field name
  • flw_amount_paid with the correct expected value (2250)
  • New intervention_funding_deployed and organization_funding_deployed fields
  • avg_top_earned_flws correctly 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 = 500
  • avg_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_users with verbose name "Activated Connect Users" (cumulative total)
  • users renamed 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_deployed and organization_funding_deployed replace the previous nm_amount_paid and nm_other_amount_paid columns, providing clearer semantics for the different types of funding.


23-23: LGTM: Average earned column updated.

The column is renamed from avg_top_paid_flws to avg_top_earned_flws to reflect that it now tracks earned amounts rather than paid amounts, aligning with the PR objectives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants