CI-418 Error Message For Opportunities Failing To Refresh #3446
Conversation
Added code to handle an alternative error message for a 403 forbidden error.
📝 WalkthroughWalkthroughMaps PersonalId API FORBIDDEN error to a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
🧰 Additional context used🧠 Learnings (12)📚 Learning: 2025-05-08T11:08:18.530ZApplied to files:
📚 Learning: 2025-07-29T14:11:36.386ZApplied to files:
📚 Learning: 2025-06-06T19:54:26.428ZApplied to files:
📚 Learning: 2025-07-29T14:10:58.243ZApplied to files:
📚 Learning: 2025-05-09T10:57:41.073ZApplied to files:
📚 Learning: 2025-06-20T15:51:14.157ZApplied to files:
📚 Learning: 2025-05-22T14:28:35.959ZApplied to files:
📚 Learning: 2025-04-18T20:13:29.655ZApplied to files:
📚 Learning: 2025-06-04T19:17:21.213ZApplied to files:
📚 Learning: 2025-03-10T08:16:59.436ZApplied to files:
📚 Learning: 2025-05-22T14:32:53.133ZApplied to files:
📚 Learning: 2025-06-13T15:53:12.951ZApplied to files:
🔇 Additional comments (11)
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: 0
🧹 Nitpick comments (3)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (3)
35-35: Align enum type usage for API error codesYou now import
BaseApiHandlerand referenceBaseApiHandler.PersonalIdOrConnectApiErrorCodesinhandleApiErrorCode, whileonFailurestill uses the barePersonalIdOrConnectApiErrorCodestype. Please double‑check that:
- Both refer to the same enum, and
- You have an explicit import for whichever nested type owns
PersonalIdOrConnectApiErrorCodes,so there’s no ambiguity or accidental duplication. It may be clearer to standardize on a single fully qualified owner + import and use the same simple type name everywhere.
83-88: Centralizing error handling looks good; consider skipping empty messagesRouting
onFailurethroughhandleApiErrorCodeis a nice cleanup and makes the 403 override straightforward. One small improvement:PersonalIdApiErrorHandler.handlecan intentionally return an empty string (e.g., when it has already handled the UX), but this path will still show a blank Toast.Consider only showing the Toast when there is a non‑empty message, e.g.:
-String errorMessage = handleApiErrorCode(errorCode, t); -Toast.makeText(requireContext(), errorMessage, Toast.LENGTH_LONG).show(); +String errorMessage = handleApiErrorCode(errorCode, t); +if (!errorMessage.isEmpty()) { + Toast.makeText(requireContext(), errorMessage, Toast.LENGTH_LONG).show(); +}This keeps behavior cleaner while preserving existing error handling.
102-119: Finalize 403 mapping and reconcile with existing FORBIDDEN_ERROR handlingThe new
handleApiErrorCodehelper cleanly centralizes the “403 for opportunities refresh” behavior, but right now the FORBIDDEN case still returns the genericrecovery_network_server_errorwith a TODO. That doesn’t yet encode the product‑specific copy described in the PR, and it also overrides the existingFORBIDDEN_ERRORmessage defined inPersonalIdApiErrorHandler.Before merging, it would be good to:
- Introduce and wire up the finalized string resource that product wants for this scenario (instead of the generic server‑error string), and
- Confirm that this divergence from the global
FORBIDDEN_ERRORmessage is intentional for this fragment.Optionally, while the underlying 403 cause is still being investigated, consider logging
tin this branch for easier debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(3 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 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: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.
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: pm-dimagi
Repo: dimagi/commcare-android PR: 2956
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java:58-60
Timestamp: 2025-02-19T15:15:01.935Z
Learning: Error handling for message retrieval in ConnectMessageChannelListFragment's retrieveMessages callback is not required as per user preference.
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.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:08:55.466Z
Learning: In ConnectUnlockFragment.java and similar Connect-related code, the team has agreed that JSONExceptions should crash the app by throwing RuntimeException (fail-fast approach for data contract violations), while IOExceptions should be logged and allow processing to continue (graceful handling for network/stream issues). This intentional inconsistency in error handling reflects different treatment for different types of failures.
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.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:10:55.131Z
Learning: In ConnectUnlockFragment.java, opportunityId values are expected to always contain valid integer strings. Integer.parseInt() should be allowed to throw NumberFormatException if invalid data is encountered, following the fail-fast philosophy used throughout the Connect feature for data contract violations.
📚 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
📚 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: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-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/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-07-29T14:10:55.131Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:10:55.131Z
Learning: In ConnectUnlockFragment.java, opportunityId values are expected to always contain valid integer strings. Integer.parseInt() should be allowed to throw NumberFormatException if invalid data is encountered, following the fail-fast philosophy used throughout the Connect feature for data contract violations.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-02-19T15:15:01.935Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2956
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java:58-60
Timestamp: 2025-02-19T15:15:01.935Z
Learning: Error handling for message retrieval in ConnectMessageChannelListFragment's retrieveMessages callback is not required as per user preference.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-07-29T14:08:55.466Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:08:55.466Z
Learning: In ConnectUnlockFragment.java and similar Connect-related code, the team has agreed that JSONExceptions should crash the app by throwing RuntimeException (fail-fast approach for data contract violations), while IOExceptions should be logged and allow processing to continue (graceful handling for network/stream issues). This intentional inconsistency in error handling reflects different treatment for different types of failures.
Applied to files:
app/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/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-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-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/fragments/connect/ConnectJobsListsFragment.java
🧬 Code graph analysis (1)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (1)
app/src/org/commcare/connect/network/connectId/PersonalIdApiErrorHandler.java (1)
PersonalIdApiErrorHandler(26-72)
⏰ 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
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Think the problem we want to solve here is the default error handling in here. Given this method is used to determine all errors it should adopt more generalised messaging and any classes that want more specific error messaging should instead overide this error handling in their onFailure implementations.
Here I think the right approach is -
-
Change
FORBIDDEN_ERRORmessage to say something generalised -Access denied by server, Please contact customer support if the problem persists -
Use the current error message
Unfortunately, this device did not pass the security checks required to sign up for PersonalID. Please try again on a different device.only inonFailureimplementation for Personal ID Phone page (first page in sign up process) as that's the only place we are doing security checks.
What are your thoughts on the contentions Kim raised in Slack? I don't want to completely dismiss input from someone with Product UX experience:
I see there's been more chatter in the ticket comments about new findings by Charl. Are we closer to pinpointing the exact root cause of the issue? Is it actually just an expired token that caused it? It seems like the error code may have been totally wrong from server |
|
@conroy-ricketts The errror message to say That is I am not seeing this error change as enehcing messaging but more like fixing a bug that was un-intentionally introduced in code base. To resolve this, we need to find a generalised default error message for
Even if the error code is incorrect from server, we still need to correct default 403 error messaging on the app as there can be valid scenarious in which server might reply with a 403 in future and showing the current error message would be wrong in that case. |
|
@shubham1g5 Agreed, that makes sense to me! I'll update this and re-request your review when I pull this PR out of draft |
Reverted changes made to the ConnectJobsListsFragment.
Added new string resource for general 403 forbidden error message.
Added missing translations for recovery_network_token_unavailable.
Added missing translations for recovery_network_unauthorized.
Added missing translations for recovery_network_token_unavailable that I missed in an earlier commit.
Added missing translations for recovery_network_server_error.
Added missing translations for recovery_network_unknown.
Added strings including non-English translations for network_forbidden_error.
Escaped a character in the French translation for recovery_network_server_error.
conroy-ricketts
requested review from
Jignesh-dimagi,
OrangeAndGreen,
jaypanchal-13 and
shubham1g5
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Thanks for filling in the missing string translations!
| return context.getString(R.string.personalid_configuration_locked_account); | ||
| case FORBIDDEN_ERROR: | ||
| return context.getString(R.string.personalid_configuration_process_failed_subtitle); | ||
| return context.getString(R.string.network_forbidden_error); |
There was a problem hiding this comment.
Not related to this PR but I think we should name this class PersonalIdOrConnectApiErrorHandler from PersonalIdApiErrorHandler
There was a problem hiding this comment.
@also, do we know when network_forbidden_error is thrown to user?
There was a problem hiding this comment.
for a
403response code
Yeah so 403 is when token is invalid or expired or it can happen in other cases too?
There was a problem hiding this comment.
401 is what should happen for the case you described, 403 is more like you are banned by server even after having a successful auth (like failing integrity check for example or if server notices anything suspicious about request) . Another example can be user trying to request data that user doesn't have permission to , for exmaple requesting an opportunity data which belongs to a different org than what user belongs to. These are just hypothetical examples though and may or may not be applicable for different endpoints we have.
There was a problem hiding this comment.
Not related to this PR but I think we should name this class PersonalIdOrConnectApiErrorHandler from PersonalIdApiErrorHandler
I actually think this is a good idea! I'm going to push a quick commit for this @Jignesh-dimagi
There was a problem hiding this comment.
Oooh nevermind, the changelist is pretty big. I'll do this in a new PR
CI-418
(this is for a mobile action item)
Product Description
I updated the user-facing error message in the event of a 403 forbidden error when tapping the refresh button for the opportunities list (and for all 403 forbidden errors for that matter).
Technical Summary
I noticed that we are already handling the forbidden error case with the correct message for the
PersonalIdPhoneFragmenthere, so there is no need to make any specific changes for that error message in Personal ID after generalizing the default error message for all 403 forbidden errors.Also, when I was looking at our other error messages, I noticed that we had some missing string translations. So I went ahead and added them.
Safety Assurance
Safety story
I verified this code will show an updated error message when the user taps on the refresh button and the API call fails with a 403 forbidden error by forcing it in the code.
QA Plan
For anyone that is able, we should verify that the non-English translations are correct.