Skip to content

Resolve Invoice Number Approved count Mismatches Actual Approved Visits#1155

Open
zandre-eng wants to merge 2 commits intomainfrom
ze/use-saved-approved-count-in-query
Open

Resolve Invoice Number Approved count Mismatches Actual Approved Visits#1155
zandre-eng wants to merge 2 commits intomainfrom
ze/use-saved-approved-count-in-query

Conversation

@zandre-eng
Copy link
Copy Markdown
Contributor

Product Description

The "Number approved" column in the Line Items table on the Review Service Delivery Invoice page now matches the Total Amount calculation. Previously, when a single CompletedWork represented multiple approved visits for the same entity, the count under-reported the actual number of approved visits, causing Number approved × Payment Unit Amount not to equal Total Amount. Network and Program Managers will now see arithmetically consistent line items.

Technical Summary

Link to ticket here

This is a release path 1 feature — Improvements to existing features & quick wins

get_invoice_items() aggregated line items per (payment_unit, month_approved) and computed total_amount_local = Sum("saved_payment_accrued") but record_count = Count("id"). Since saved_payment_accrued = saved_approved_count × payment_unit.amount per CompletedWork, the totals were correct but the count counted rows instead of visits. A CompletedWork is keyed by (opportunity_access, entity_id, payment_unit) and can carry saved_approved_count > 1, so the two aggregates diverged whenever any record had more than one approved visit.

  • Fix: Switched record_count from Count("id") to Sum("saved_approved_count") in completed_work.get_invoice_items. This makes the count algebraically consistent with the total: payment_unit.amount × Sum(saved_approved_count) == Sum(saved_payment_accrued).
  • No data migration needed: saved_approved_count and saved_payment_accrued are written together in CompletedWorkUpdater._update_payment, so existing rows already have the right values.

Safety Assurance

Safety story

Read-only query change on the invoice review page. No schema changes, no data writes, no migrations. The new aggregate uses an already-populated cached column (saved_approved_count) that is written alongside saved_payment_accrued in the same CompletedWorkUpdater._update_payment call, so the two are guaranteed to be in sync for any record that has gone through the recompute path. If a stale row existed with saved_payment_accrued > 0 but saved_approved_count = 0, the displayed count would be lower than before — but that condition implies an inconsistent record that would already mis-display elsewhere; spot-checking production data is straightforward (WHERE saved_payment_accrued > 0 AND saved_approved_count = 0).

The change is fully reversible — a one-line revert restores the prior behaviour.

Automated test coverage

  • TestUninvoicedVisitItems.test_number_approved_uses_saved_approved_count — regression test for the bug. Two CompletedWork rows in the same payment unit with saved_approved_count of 2 and 1 must produce number_approved = 3 and total_amount_local = 3 × payment_unit.amount. This test fails on the prior implementation (assert 2 == 3) and passes after the fix.
  • Updated existing TestUninvoicedVisitItems tests (test_items_without_prior_invoice, test_items_with_prior_invoice, and _create_completed_work) to set saved_approved_count alongside saved_payment_accrued, mirroring how the two fields are written together in production.

QA Plan

QA will not be performed for this change. Below is the testing plan for reference:

  • On a managed opportunity that has at least one CompletedWork with saved_approved_count > 1, open the Review Service Delivery Invoice page.
  • Verify the "Number approved" column for each line item equals Total Amount / Payment Unit Amount exactly.
  • Verify line items where saved_approved_count == 1 for every record (the common case) are unchanged.
  • Verify the "Total Amount" column is unchanged from before the fix.

Labels & Review

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 68f324be-fa33-47dd-bce4-a90a0a40de57

📥 Commits

Reviewing files that changed from the base of the PR and between 1843fbd and df313f3.

📒 Files selected for processing (2)
  • commcare_connect/opportunity/tests/test_completed_work.py
  • commcare_connect/opportunity/utils/completed_work.py

Walkthrough

This change updates the invoice item generation logic to aggregate approved work items using a saved_approved_count field instead of row counting. The get_invoice_items function in the utils module now sums CompletedWork.saved_approved_count to compute the approved count per payment unit. Correspondingly, the Count ORM import is removed as it is no longer needed. The test file is updated with explicit saved_approved_count values in existing tests and a new test validates proper aggregation of multiple approved entries sharing the same grouping.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly addresses the core fix: resolving the mismatch between the invoice's 'Number Approved' count and actual approved visits.
Description check ✅ Passed The description is comprehensive and directly relevant to the changeset, explaining the bug, the fix, technical details, safety assurance, test coverage, and QA plan.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ze/use-saved-approved-count-in-query

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
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.

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

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