CCCT-2888 Handle Optional Daily Limits For Backward Compatibility

[conroy-ricketts]

CCCT-2888

Product Description

With no daily limit, the app shows only today's visit count, without daily-limit bars, warnings or "per day" text.

Here are before (left side) and after (right side) screenshots of what the screens look like with null values:

Technical Summary

Null limits are stored as a -1 sentinel in the existing columns, avoiding a DB migration. The API bump to 2.0 is what tells the server it may send nulls (CCCT-2765).

Safety Assurance

Safety story

What gives confidence

• I checked each affected screen on a device with null limits forced locally.
• Opportunities with limits keep their behavior, covered by existing tests.

Risks to review

• API 2.0 applies to every Connect endpoint, not just opportunities.

Automated test coverage

Unit and Robolectric tests cover null parsing, skipped warnings and blocking, and the count-only UI.

🤖 Generated with Claude Code

[conroy-ricketts]

Suggested Review Order

Read commit by commit; each is self-contained with its tests:

1. Parsing null daily limits (sentinel + JSON helper)
2. Skipping daily-limit warnings and work blocking when there is no limit
3. UI: dashboard, home job tile, intro and learn-complete screens
4. Connect API bump to 2.0
5. Release and QA notes

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 213854cc-1042-41d3-9be2-7f68a0a375e5

📥 Commits

Reviewing files that changed from the base of the PR and between 4cf86c6 and 83a7b33.

📒 Files selected for processing (21)

• RELEASES.md
• app/res/layout/item_progress_job_summary_visit.xml
• app/src/org/commcare/activities/StandardHomeActivityUIController.java
• app/src/org/commcare/adapters/ConnectProgressJobSummaryAdapter.java
• app/src/org/commcare/android/database/connect/models/ConnectDeliveryPaymentSummaryInfo.java
• app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java
• app/src/org/commcare/android/database/connect/models/ConnectPaymentUnitRecord.java
• app/src/org/commcare/connect/network/connect/ConnectNetworkClient.kt
• app/src/org/commcare/fragments/connect/ConnectDeliveryDashboardFragment.kt
• app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.kt
• app/src/org/commcare/utils/JsonExtensions.kt
• app/src/org/commcare/views/connect/ConnectLearnCompleteView.kt
• app/src/org/commcare/views/connect/ConnectProgressCard.kt
• app/unit-tests/src/org/commcare/android/database/connect/models/ConnectJobRecordCardMessageTest.kt
• app/unit-tests/src/org/commcare/android/database/connect/models/ConnectJobRecordWorkBlockedTest.kt
• app/unit-tests/src/org/commcare/connect/network/connect/parser/ConnectOpportunitiesParserTest.kt
• app/unit-tests/src/org/commcare/fragments/connect/ConnectDeliveryDashboardFragmentTest.kt
• app/unit-tests/src/org/commcare/fragments/connect/ConnectJobIntroFragmentTest.kt
• app/unit-tests/src/org/commcare/utils/JsonExtensionsTest.kt
• app/unit-tests/src/org/commcare/views/connect/ConnectLearnCompleteViewTest.kt
• app/unit-tests/src/org/commcare/views/connect/ConnectProgressCardTest.kt

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Connect treats null daily limits as absent limits for jobs and payment units. Daily-limit warnings and blocked-work checks use guarded comparisons. Home and Connect screens display visit counts without daily-limit indicators when no limit exists. The change also updates the Connect API version to 2.0 and adds tests and release notes for the behavior.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ConnectOpportunitiesParser
  participant ConnectJobRecord
  participant ConnectDeliveryDashboardFragment
  participant ConnectProgressCard
  ConnectOpportunitiesParser->>ConnectJobRecord: Parse job with absent daily limit
  ConnectJobRecord->>ConnectDeliveryDashboardFragment: Provide job and limit state
  ConnectDeliveryDashboardFragment->>ConnectProgressCard: Provide visit count and no maximum
  ConnectProgressCard->>ConnectDeliveryDashboardFragment: Render count without progress bar

Loading

Suggested reviewers: jignesh-dimagi, shubham1g5

Merge Risk: ⚪ Minimal · up to 83a7b

The change consistently represents uncapped opportunities and removes daily-limit indicators while preserving other limits. No concrete merge-blocking issue is established; normal checks and server compatibility validation remain appropriate.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 83a7b

The no-limit behavior retains overall visit caps and existing authentication. The main uncertainty is rollout compatibility: API version 2.0 also applies to progress, job claims and payment confirmations, and compatibility with the real server has not yet been demonstrated.

Retained concerns

• Medium · architecture · inferred: The opportunity compatibility change advances negotiation to API 2.0 for learning progress, delivery progress, learning initiation, job claims and payment confirmations as well. A partially compatible server rollout could therefore disrupt synchronization or state-changing operations beyond opportunities. Matching server contracts remain unverified; no failure or authorization bypass was observed.

Security review details

Security Blast Radius

• observed — The reviewed exposure is the authenticated Connect client and its local opportunity state. The shared version change reaches job-UUID progress and claim operations and payment-UUID confirmations. Server-side tenant isolation and authorization under version 2.0 remain outside the supplied evidence.

Trust Boundaries and Controls

• observed — Server-supplied opportunity JSON determines local limits, while request identity comes through the existing SSO authorization path. The reviewed change does not replace that credential path or introduce an unauthenticated request route; no attacker-controlled route to overriding server policy was established.

Resilience and Maintainability Implications

• observed — No-limit handling preserves local suspension, finished-job and total-cap blocking. Mixed payment units remain checked individually, so an unlimited daily unit can still exhaust its total allocation.

Hardening Proposals

• proposed — Establish the version-2 contract for all six endpoints, including explicit-null versus omitted daily fields, and define recovery for incompatible refresh responses and rollback with retained sentinel records. These are compatibility and failure-containment proposals, not verified security defects.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 63 functions across 19 files. (2 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly identifies the change: handling optional daily limits for backward compatibility.
Description check	✅ Passed	The description covers user-facing behavior, technical rationale, safety, and automated test coverage. It omits the template’s Labels and Review checklist, but the description is otherwise mostly comp…

Full details: Docstring Coverage

Explanation

Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 63 functions across 19 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

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.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 37.16%. Comparing base (19ad76c) to head (0a0dfe3).
⚠️ Report is 29 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3938      +/-   ##
============================================
+ Coverage     36.85%   37.16%   +0.30%     
- Complexity     6748     6829      +81     
============================================
  Files          1032     1034       +2     
  Lines         60861    61093     +232     
  Branches       7399     7471      +72     
============================================
+ Hits          22432    22705     +273     
+ Misses        35828    35749      -79     
- Partials       2601     2639      +38     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[OrangeAndGreen]

I see this constant defined in several places but thinking it should be a single source of truth since changing it in one place alone would break functionality