Preserve Over Limit Status

[zandre-eng]

Product Description

Service-delivery submissions that exceed the per-worker Max Service Deliveries cap will now be correctly rejected as Over Limit. Before this fix, the cap was silently ignored on opportunities where the duplicate verification flag was off, allowing workers to submit beyond their configured limit. There is no new UI or workflow — this restores the intended cap-enforcement behaviour on the backend.

Technical Summary

Link to ticket here

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

The cap-check in process_deliver_unit() correctly sets user_visit.status = over_limit when a worker exceeds their OpportunityClaimLimit.max_visits, but clean_form_submission() ran immediately afterwards and unconditionally reset user_visit.status back to pending whenever OpportunityVerificationFlags.duplicate was False. With auto_approve_visits=True the auto-approve branch then flipped that pending straight to approved, silently bypassing the cap. The function's docstring describes the intended behaviour as "resetting duplicate status when the duplicate flag is disabled" — the implementation was just broader than the intent. The fix scopes the reset to only act when the prior status is duplicate, leaving over_limit and trial intact.

• Code change: one-line scope tightening in commcare_connect/form_receiver/processor.py:280 — else: → elif user_visit.status == VisitValidationStatus.duplicate:. No model, schema, or API changes.
• No backfill in this PR: the fix prevents future over-cap acceptances; existing over-cap rows in production are remediated separately via the script in this PR.

Safety Assurance

Safety story

• Blast radius is small and well-bounded. The change only narrows a status mutation that previously fired in two cases (status == duplicate and status == anything else) so that it now fires in only one (status == duplicate). Any code path that relied on the broader behaviour would have been overwriting over_limit or trial — neither of which is desirable. All existing tests in form_receiver/ and opportunity/ continue to pass (529 passed).
• Existing data impact: zero direct impact. Already-saved UserVisit rows are not modified by this PR. Over-cap rows that pre-date the fix continue to show their current statuses until the separate remediation script is run.
• Risk of regression on the opposite case (duplicate flag on): the if opportunity_flags.duplicate: branch is unchanged; behaviour for that path is identical to before. Specifically covered by the existing test_receiver_duplicate and test_receiver_duplicate_concurrent_submissions tests.

Automated test coverage

New regression test in commcare_connect/form_receiver/tests/test_receiver_integration.py:

• test_over_limit_status_preserved_when_duplicate_flag_disabled — sets verification_flags={"duplicate": False} on the opportunity fixture, submits a form against a payment unit configured with daily_max_per_user=0 to force the cap to trigger on the very first submission, and asserts the resulting UserVisit.status == over_limit. Pre-fix this test fails with assert 'approved' == 'over_limit', exactly mirroring the prod symptom.

Existing coverage that continues to validate the surrounding behaviour:

• test_receiver_deliver_form_daily_visits_reached, test_receiver_deliver_form_max_visits_reached, test_receiver_deliver_form_end_date_reached — cap-enforcement happy paths (with default duplicate=True).
• test_receiver_duplicate, test_receiver_duplicate_concurrent_submissions — duplicate-flagging path, ensuring the touched conditional still preserves duplicate detection.
• Auto-approve, suspended-user, flagged-form, and trial-status tests — confirm no other status-transition regressed.

QA Plan

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

• Identify or create a non-managed opportunity with OpportunityVerificationFlags.duplicate = False and auto_approve_visits = True.
• Set a payment unit with a small max_total (e.g. 2) to make the cap easy to hit.
• As an enrolled mobile worker, submit deliveries up to and one beyond the configured max_total.
• Verify the over-cap submission's UserVisit.status is over_limit (not approved) on the worker-deliveries view in the org dashboard.
• Verify the corresponding CompletedWork.status is over_limit.
• Verify the worker's payment_accrued does not include the over-cap visit.
• Repeat against an opportunity with duplicate = True to confirm the duplicate-detection path is unaffected: submit two visits with the same entity_id; the second should be flagged as duplicate, not over_limit or pending.

Labels & Review

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

[coderabbitai]

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: f17a48fc-7d7a-4f1e-b3ab-0d20d28d8da2

📥 Commits

Reviewing files that changed from the base of the PR and between 1f57678 and 75a7ba4.

📒 Files selected for processing (1)

• commcare_connect/form_receiver/processor.py

🚧 Files skipped from review as they are similar to previous changes (1)

• commcare_connect/form_receiver/processor.py

---

Walkthrough

The change tightens cleanup logic in clean_form_submission: status normalization for user_visit is now applied only when the current user_visit.status is duplicate. The code raises the duplicate flag and/or sets status to pending only in the nested condition that checks both user_visit.status == "duplicate" and opportunity_flags.duplicate. If duplicate verification is disabled, other statuses (e.g., over_limit) are no longer overwritten. An integration test was added to ensure an over_limit status set by cap checks is preserved when duplicate verification is disabled.

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 25.00% 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 'Preserve Over Limit Status' clearly and concisely describes the main change: fixing a bug where over_limit status was being incorrectly overwritten when the duplicate verification flag was disabled.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, providing product context, technical details, safety analysis, test coverage, and QA plans all aligned with the code changes.
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/preserve-over-limit-status

---

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

_{Review rate limit: 0/1 reviews remaining, refill in 60 minutes.}

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

[mkangia]

this would sound silly but I think this would be easier to understand if you just separate this completely out as

if user_visit.status == VisitValidationStatus.duplicate and (not opportunity_flags.duplicate):
    user_visit.status = VisitValidationStatus.pending

I say so because the elif conditions seems to be not related to the if one at all.

[zandre-eng]

That's fair, and I can see it appearing a bit odd for other devs reviewing.

I've refactored the conditional in 1f57678.

[mkangia]

I guess a better thing would be to check for the duplicate flag requirement even further up when the duplicate is actually set but that could be a much bigger change

[zandre-eng]

I guess a better thing would be to check for the duplicate flag requirement even further up when the duplicate is actually set but that could be a much bigger change

@mkangia I think that would logically make more sense, but I'm hesitant of doing a bigger refactor and potentially introducing a new bug. So my thinking was to do a minimal bug fix now, and a bigger logical refactoring can safely be done at a later point.

[calellowitz]

i think this would be slightly clearer as
elif user_visit.status == VisitValidationStatus.duplicate: or even

else:
    if user_visit.status == VisitValidationStatus.duplicate:

but repeating the flag check makes it quite verbose and hard to tell that it is the opposite path as the block above

[zandre-eng]

That's a fair point. I did a force push to refactor this conditional according to Sravan's suggestion.

Addressed in 75a7ba4.

[mkangia]

aha,
@calellowitz
you suggested what Zandre originally wrote and I asked for a change here

You and I see code clarity a bit differently. In my head, it was NOT an "opposite path"

[sravfeyn]

Just throwing another option

    if user_visit.status == VisitValidationStatus.duplicate:
        if opportunity_flags.duplicate:
            flags.append(["duplicate", "A beneficiary with the same identifier already exists"])
        else:
            user_visit.status = VisitValidationStatus.pending

[zandre-eng]

This is a nice approach, very clean. Will update.

[zandre-eng]

I did a force push to incorporate the above suggestion in the second commit.

Addressed in 75a7ba4.

[sravfeyn]

Nice find, LGTM except for the nit comments.