CCCT-1774 Redirect User To Job Status Page For Completed Opportunity #3462
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Redirected the user to the job status page whenever an opportunity is completed.
conroy-ricketts
requested review from
Jignesh-dimagi,
OrangeAndGreen,
jaypanchal-13 and
shubham1g5
📝 WalkthroughWalkthroughThe changes add conditional delivery-completion handling to the Connect job launching flow. A new utility function Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/org/commcare/connect/ConnectAppUtils.kt (1)
128-134: Add KDoc to clarify function purpose and constraints.The function hardcodes "Deliver" for analytics (line 130) and navigates to the job status page rather than launching the app directly. To prevent misuse and clarify the intent, add documentation.
Apply this diff to add KDoc:
+/** + * Launches the delivery app for a completed opportunity by navigating to the job status page. + * This should only be called for delivery apps (not learning apps) when the opportunity is complete. + * + * @param activity The current activity context + * @param appId The delivery app identifier + * @param job The completed job record + */ fun launchAppForCompletedDelivery(activity: Activity, appId: String, job: ConnectJobRecord) { CommCareApplication.instance().closeUserSession() FirebaseAnalyticsUtil.reportCccAppLaunch("Deliver", appId) ConnectNavHelper.goToActiveInfoForJob(activity, job, true) - + activity.finish() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/org/commcare/connect/ConnectAppUtils.kt(2 hunks)app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:11:36.386Z
Learning: In ConnectJobsListsFragment.java error handling for JSON parsing, if the JSONObject obj is null when passed to handleCorruptJob(), the team prefers to let the code crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately rather than masking them.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Applied to files:
app/src/org/commcare/connect/ConnectAppUtils.ktapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-04-18T20:13:29.655Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.
Applied to files:
app/src/org/commcare/connect/ConnectAppUtils.ktapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-06-06T19:52:53.173Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java:163-180
Timestamp: 2025-06-06T19:52:53.173Z
Learning: In the CommCare Android Connect feature, database operations like ConnectJobUtils.upsertJob should be allowed to crash rather than being wrapped in try-catch blocks. The team prefers fail-fast behavior for database errors instead of graceful error handling.
Applied to files:
app/src/org/commcare/connect/ConnectAppUtils.kt
📚 Learning: 2025-06-06T20:15:21.134Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java:65-71
Timestamp: 2025-06-06T20:15:21.134Z
Learning: In the CommCare Android Connect module, job.getLearnAppInfo() and getLearnModules() should never return null according to the system design. The team prefers to let the code crash if these values are unexpectedly null rather than adding defensive null checks, following a fail-fast philosophy to catch bugs early rather than masking them.
Applied to files:
app/src/org/commcare/connect/ConnectAppUtils.ktapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-12-10T16:39:05.007Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3448
File: app/src/org/commcare/gis/EntityMapUtils.kt:214-234
Timestamp: 2025-12-10T16:39:05.007Z
Learning: In Kotlin files (here: app/src/org/commcare/gis/EntityMapUtils.kt), follow fail-fast guidance: treat conditions that indicate a programmer error or broken invariant as exceptions rather than defensive handling. If a condition represents a bug (e.g., internal API assumptions like array/list size mismatches), crash early to alert developers instead of attempting to recover. Use clear runtime exceptions (e.g., IllegalStateException, IllegalArgumentException) or assertions where appropriate, and avoid swallowing or masking bugs. Ensure invariants are checked and failures are surfaced with actionable messages for quick debugging.
Applied to files:
app/src/org/commcare/connect/ConnectAppUtils.kt
📚 Learning: 2025-05-22T14:28:35.959Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-07-29T14:11:36.386Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:11:36.386Z
Learning: In ConnectJobsListsFragment.java error handling for JSON parsing, if the JSONObject obj is null when passed to handleCorruptJob(), the team prefers to let the code crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately rather than masking them.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-05-22T14:26:41.341Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In SelectInstallModeFragment.java, the showConnectErrorMessage method intentionally omits null checks because it's called at a specific point in the startup flow where UI is guaranteed to be loaded. It's designed to crash if activity or view is null to make potential issues immediately visible rather than hiding them with defensive programming.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-06-06T19:54:26.428Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java:74-78
Timestamp: 2025-06-06T19:54:26.428Z
Learning: In ConnectDownloadingFragment.java and similar Connect-related code, the team prefers to let "should never happen" scenarios like null app records crash rather than add defensive null checks, following a fail-fast philosophy to catch programming errors during development.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-07-29T14:10:58.243Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java:0-0
Timestamp: 2025-07-29T14:10:58.243Z
Learning: In ConnectJobsListsFragment.java, the team intentionally uses different error handling strategies: JSONException should throw RuntimeException to crash the app (fail-fast for data contract violations), while IOException should be logged and allow graceful continuation (for network/stream issues). This follows the established Connect module pattern of treating different exception types differently based on their nature.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
🧬 Code graph analysis (1)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (2)
app/src/org/commcare/AppUtils.java (1)
AppUtils(34-209)app/src/org/commcare/connect/database/ConnectJobUtils.java (1)
ConnectJobUtils(25-383)
⏰ 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: Lint Code Base
🔇 Additional comments (2)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (2)
133-133: Add null-safety for getDeliveries().If
compositeJob.getDeliveries()returns null, this line will throw a NullPointerException. While the codebase follows a fail-fast philosophy,getDeliveries()returning null could be a valid state for a job that hasn't started deliveries yet.Apply this diff to add a null check:
-boolean deliveryComplete = compositeJob != null && - (compositeJob.isFinished() || compositeJob.getDeliveries().size() >= compositeJob.getMaxVisits()); +boolean deliveryComplete = compositeJob != null && + (compositeJob.isFinished() || + (compositeJob.getDeliveries() != null && compositeJob.getDeliveries().size() >= compositeJob.getMaxVisits()));⛔ Skipped due to learnings
Learnt from: OrangeAndGreen Repo: dimagi/commcare-android PR: 3043 File: app/src/org/commcare/connect/database/ConnectJobUtils.java:213-216 Timestamp: 2025-04-22T17:03:41.387Z Learning: In the ConnectJobDeliveryRecord class, the flags list is initialized as an empty list in the default constructor rather than left as null, following the "Null Object Pattern" as a best practice.
128-144: The composite job null-handling and delivery count logic is correct.The
getCompositeJob()method returns null when the jobId doesn't exist in the local database (line 40 of ConnectJobUtils.java). This can occur due to timing issues between API syncing and local persistence. The composite job retrieval correctly populates accurate delivery counts viapopulateJobs(), which fetches all related delivery records from storage.The null-handling in ConnectJobsListsFragment (lines 131-133) is appropriate: it treats a missing composite job as an incomplete delivery, allowing the app to launch with the original job record as a fallback. This is a reasonable defensive pattern that handles the edge case where a job exists in the UI list but hasn't been persisted to the local database yet.
No issues found with the implementation at lines 128-144.
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
Outdated
Show resolved
Hide resolved
Added check for learning apps.
shubham1g5
requested changes
Dec 10, 2025
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
Outdated
Show resolved
Hide resolved
Refactored code to simply navigate the user to the correct screen rather than restart the activity.
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
Outdated
Show resolved
Hide resolved
Contributor
Author
|
Marking this PR as a draft for now until the new action item is implemented based on discussions with the product team |
Contributor
Author
|
Note: These changes are blocked until this ticket is implemented. |
…nto task/CCCT-1774-redirect-user-to-job-status
Refactored check for completed delivery to use helper method.
Contributor
Author
|
This PR is ready for another round of review. #3473 unblocks this work, and these changes will not be merged in until #3473 is merged in. FYI @Jay13Panchal @Jignesh-dimagi @shubham1g5 @OrangeAndGreen |
…nto task/CCCT-1774-redirect-user-to-job-status
shubham1g5
requested changes
Dec 29, 2025
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
Outdated
Show resolved
Hide resolved
Removed check for app installed.
A bit of code cleanup after removing check for app installed.
OrangeAndGreen
approved these changes
Dec 29, 2025
shubham1g5
approved these changes
Dec 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CCCT-1774
Product Description
I redirected the user to the job status page whenever an opportunity is completed.
Before my changes:
Screen_Recording_20251210_155157_CommCare.Debug.mp4
After my changes:
Screen_Recording_20251210_155317_CommCare.Debug.mp4
EDIT this is what it looks like after @shubham1g5's suggestion!:
Screen_Recording_20251211_121345_CommCare.Debug.mp4
Technical Summary
At first I wanted to use the
ConnectJobRecordthat was already present inside theConnectJobsListsFragment.launchAppForJob()function to determine if an opportunity is complete, but I ran into a bug where the job did not have the correct number of deliveries set on it and thus the code was incorrectly determining that an opportunity was incomplete when it was actually complete. So that's the reason for me utilizingConnectJobUtilsto get the composite job. Let me know if there is a better way to achieve this!Safety Assurance
Safety story
I verified that, completed opportunities, as defined in the ticket, now redirect the user to the job status page instead of the home page. And, all non-complete opportunities are unaffected.
QA Plan
On the opportunities list page, tap through several different opportunities (both complete and non-complete) and ensure that you are directed to the correct page.