Incorrect Accrued Payment Calculation Due to exclude Usage on CompletedWork

[hemant10yadav]

Technical Summary

While debugging an interrupt issue, I identified a bug in how accrued payments are calculated.

When a visit is initially marked as rejected, the corresponding CompletedWork record is also marked as rejected. However, if the visit is later approved through the visit import workflow, the CompletedWork record is not updated accordingly. Because the queryset excludes rejected records.

This results in inconsistencies between the visit status and the CompletedWork status, leading to inaccurate payment accruals. The bulk approval/reject UI flow does not apply this exclusion logic, which further introduces inconsistent behavior between the two flows.

To ensure consistency across both flows and to correctly include updated records in the accrual calculation, I have removed the exclusion condition from the queryset.

Safety Assurance

Safety story

I’ve removed the exclude filter, as the payment accrual method already handles these scenarios correctly. Given its existing logic and safeguards, I’m confident this change is safe and maintains the intended behaviour.

Automated test coverage

Already exisit

QA Plan

No QA

Labels & Review

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

[coderabbitai]

Walkthrough

A filter condition in the bulk_update_payment_accrued function that previously excluded CompletedWork objects with "rejected" status has been removed. The query now selects all completed works with their related payment_unit records without filtering out rejected statuses. This expands the set of CompletedWork objects passed to update_status. The control flow logic and database locking mechanisms remain unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• The change is isolated to a single function in one file
• The edit is straightforward (removal of a filter condition)
• However, consider the following during review:
  • Behavioral impact: Verify that including rejected CompletedWork objects in payment accrual calculations is intentional, as this changes which records are processed
  • Historical context: Understand why the rejection filter existed originally and confirm its removal is appropriate
  • Downstream effects: Check if processing rejected work items has any unintended consequences on payment calculations or reporting
  • Test coverage: Ensure adequate testing covers scenarios with rejected work items now being included in bulk updates

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically references the main bug fix—removing an incorrect exclude filter that was causing inaccurate accrued payment calculations on CompletedWork records.
Description check	✅ Passed	The description provides detailed context about the bug, explains the inconsistency between flows, and justifies the fix, directly relating to the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ 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 hy/fix-import-visit-pay

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b76493f and 76f8ca1.

📒 Files selected for processing (1)

• commcare_connect/opportunity/tasks.py (1 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

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.

📚 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/opportunity/tasks.py

⏰ 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 (1)

commcare_connect/opportunity/tasks.py (1)

426-433: I'll analyze this review comment by examining the update_status function and the related code to verify the concerns about the removed .exclude() filter and the inconsistency with bulk_approve_completed_work.
<function_calls>


#!/bin/bash

First, let's examine the update_status function implementation

rg -n "def update_status" --type=py -A 30 commcare_connect/opportunity/utils/completed_work.py


</function_calls>

Waiting for script execution results to analyze the update_status function behavior and verify the original review comment's concerns.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[calellowitz]

Can you hold off on merging this until I get back next week? I want to make sure we aren't overriding manually rejected completed works. I am also curious how this came up. Once a delivery is rejected we wouldn't expect it to be approved later, that is meant to be a terminal status.

[hemant10yadav]

Can you hold off on merging this until I get back next week? I want to make sure we aren't overriding manually rejected completed works. I am also curious how this came up. Once a delivery is rejected we wouldn't expect it to be approved later, that is meant to be a terminal status.

I was not able to figure out the exact reason behind this but adding the ticket where all this happened.