Handled locked account error on api failure #3227
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen,
pm-dimagi and
shubham1g5
📝 WalkthroughWalkthroughThis change introduces enhanced handling for locked account scenarios in the PersonalID feature. A new string resource, Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Server
User->>App: Initiate PersonalID configuration
App->>Server: Send configuration request
Server-->>App: Responds with HTTP 403 + error_code ("LOCKED_ACCOUNT")
App->>App: Parse error_code from response
alt error_code == "LOCKED_ACCOUNT"
App->>App: Trigger ACCOUNT_LOCKED_ERROR handling
App->>User: Display "account locked" message (localized)
App->>Analytics: Log START_CONFIGURATION_LOCKED_ACCOUNT_FAILURE
else error_code != "LOCKED_ACCOUNT"
App->>App: Handle as generic FORBIDDEN_ERROR
App->>User: Display generic forbidden message
end
Suggested reviewers
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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/res/values/strings.xml (1)
601-601: String key added correctly; punctuation style minorKey/value pair looks fine and matches naming convention used for other PersonalID errors.
Optional: many user-facing sentences in this file end with a period. Consider appending “.” to keep punctuation consistent, but not blocking.app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java (1)
178-182: Constant follows existing pattern – keep section sortedConstant name and value are consistent with neighbouring keys.
If you ever rely on alphabetical scans/merge conflict reduction, consider keeping this small block sorted (INTEGRITY_CHECK, INTEGRITY_DEVICE, LOCKED_ACCOUNT) – not required for correctness.app/res/values-pt/strings.xml (1)
449-449: Translation added; minor localisation nitTranslation reads well. Two tiny points you may want to double-check:
- Terminology: most of the file uses Brazilian Portuguese (“contato”) rather than European (“contacto”); keep consistency if that’s intentional.
- Add a final period to match nearby sentences.
Not blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/res/values-fr/strings.xml(1 hunks)app/res/values-pt/strings.xml(1 hunks)app/res/values-sw/strings.xml(1 hunks)app/res/values-ti/strings.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/connect/network/base/BaseApiCallback.kt(1 hunks)app/src/org/commcare/connect/network/base/BaseApiHandler.kt(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(1 hunks)app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:95-131
Timestamp: 2025-02-04T21:38:11.970Z
Learning: Biometric authentication security improvements were considered but skipped in PR #2949 as per user's request. The implementation remained with basic biometric authentication without additional security layers.
Learnt from: shubham1g5
PR: dimagi/commcare-android#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
PR: dimagi/commcare-android#3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
app/res/values/strings.xml (2)
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3070
File: app/res/values/strings.xml:73-73
Timestamp: 2025-05-07T06:50:52.518Z
Learning: The empty `remote_form_payload_url` string resource in strings.xml is intentional legacy code and should be preserved as-is.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
app/src/org/commcare/connect/network/base/BaseApiHandler.kt (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/connect/network/PersonalIdApiHandler.java:51-56
Timestamp: 2025-06-13T15:53:12.951Z
Learning: For `PersonalIdApiHandler`, the team’s convention is to propagate `JSONException` as an unchecked `RuntimeException` so the app crashes, signalling a contract/implementation bug rather than attempting a graceful retry.
app/res/values-pt/strings.xml (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
app/res/values-ti/strings.xml (2)
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3070
File: app/res/values/strings.xml:73-73
Timestamp: 2025-05-07T06:50:52.518Z
Learning: The empty `remote_form_payload_url` string resource in strings.xml is intentional legacy code and should be preserved as-is.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
app/res/values-fr/strings.xml (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
app/src/org/commcare/connect/network/base/BaseApiCallback.kt (3)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/connect/network/PersonalIdApiHandler.java:51-56
Timestamp: 2025-06-13T15:53:12.951Z
Learning: For `PersonalIdApiHandler`, the team’s convention is to propagate `JSONException` as an unchecked `RuntimeException` so the app crashes, signalling a contract/implementation bug rather than attempting a graceful retry.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-01-21T18:20:43.883Z
Learning: In the Connect ID feature, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information (owner_name and phone_number) and secondary phone verification across all authentication flows.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
app/res/values-sw/strings.xml (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/res/values-ti/strings.xml:350-350
Timestamp: 2025-05-22T14:32:53.133Z
Learning: PersonalID and Connect features haven't been translated to Spanish, Lithuanian, or Norwegian yet, so users with those language settings see the English strings by default.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (10)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:62-64
Timestamp: 2025-06-04T19:17:21.213Z
Learning: In ConnectUnlockFragment.java, the user prefers to let getArguments() potentially throw NullPointerException rather than adding null checks, as the arguments are required for proper navigation flow and their absence indicates a programming error that should fail fast.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/fragments/connect/ConnectPaymentSetupFragment.java:61-66
Timestamp: 2025-01-21T17:29:58.014Z
Learning: In the CommCare Android app, for non-critical convenience features like phone number auto-population, exceptions should be logged but fail silently when there's a manual fallback available. This approach prevents app crashes while maintaining the ability to debug issues through logs.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.
Learnt from: shubham1g5
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:173-247
Timestamp: 2025-03-10T08:16:29.416Z
Learning: In the ConnectIdPasswordVerificationFragment, password comparisons should use MessageDigest.isEqual() rather than equals() to prevent timing attacks, and empty password validation should be implemented before verification attempts.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:235-236
Timestamp: 2025-02-04T21:29:29.594Z
Learning: The empty performPasswordUnlock method in ConnectIdBiometricConfigFragment is intentionally left empty and should not be flagged in reviews.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/connect/network/PersonalIdApiHandler.java:51-56
Timestamp: 2025-06-13T15:53:12.951Z
Learning: For `PersonalIdApiHandler`, the team’s convention is to propagate `JSONException` as an unchecked `RuntimeException` so the app crashes, signalling a contract/implementation bug rather than attempting a graceful retry.
Learnt from: shubham1g5
PR: dimagi/commcare-android#3116
File: app/src/org/commcare/fragments/connectId/PersonalIdPhotoCaptureFragment.java:0-0
Timestamp: 2025-05-19T15:36:34.511Z
Learning: In the CommCare Android codebase, a "fail fast" approach is preferred for internal encoding/decoding operations where both sides are controlled by the application. This helps surface bugs immediately rather than masking them with graceful error handling.
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2927
File: app/src/org/commcare/activities/connect/HQUserInviteActivity.java:0-0
Timestamp: 2025-02-04T15:29:12.993Z
Learning: In HQUserInviteActivity, parameters from Intent data (domain, inviteCode, username, etc.) are required and the code is designed to fail immediately if they're missing, rather than adding null checks. This helps catch integration issues early.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (6)
app/src/org/commcare/connect/network/base/BaseApiHandler.kt (1)
30-31: LGTM! Enum addition is correctly implemented.The new
ACCOUNT_LOCKED_ERRORenum value is properly positioned and follows the existing naming convention. TheshouldAllowRetry()method correctly excludes this error code, which is appropriate since locked accounts should not automatically retry.app/res/values-sw/strings.xml (1)
449-449: LGTM! Localized string resource correctly added.The new
personalid_configuration_locked_accountstring resource provides an appropriate Swahili message for locked account scenarios, guiding users to contact support. The addition is consistent with the broader localization strategy for the PersonalID feature.app/res/values-ti/strings.xml (1)
435-435: LGTM! Tigrinya localization correctly implemented.The new
personalid_configuration_locked_accountstring resource provides a clear Tigrinya message for locked account scenarios, directing users to contact support. This addition maintains consistency with the localization strategy across language files for the PersonalID feature.app/res/values-fr/strings.xml (1)
443-443: LGTM: French localization added correctly.The French translation for the locked account message is grammatically correct and appropriately conveys the error state to users.
app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
32-50: LGTM: Robust error handling implementation.The enhanced 403 error handling properly:
- Parses JSON response body to detect specific "LOCKED_ACCOUNT" error codes
- Uses proper resource management with the
useblock- Handles parsing failures gracefully with exception logging
- Falls back to existing behavior when error code doesn't match
- Uses case-insensitive comparison to handle server response variations
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
361-385: LGTM: Improved error handling with specific locked account case.The enhanced error handling properly:
- Adds null check to prevent potential NPE
- Uses switch statement for better maintainability
- Provides specific handling for
ACCOUNT_LOCKED_ERRORwith appropriate analytics and user messaging- Preserves existing
FORBIDDEN_ERRORbehavior- Ensures all other errors are handled by the default case
| if (errorResponse != null){ | ||
| try { | ||
| errorResponse.use { | ||
| val json = JSONObject(String(StreamsUtil.inputStreamToByteArray(it), Charsets.UTF_8)) | ||
| if (json.has("error_code")){ | ||
| val errorCode = json.optString("error_code") | ||
| if (errorCode.equals("LOCKED_ACCOUNT", ignoreCase = true)) { | ||
| baseApiHandler.onFailure( | ||
| PersonalIdOrConnectApiErrorCodes.ACCOUNT_LOCKED_ERROR, | ||
| null | ||
| ) | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
@jaypanchal-13 we want to do this for all response codes and not just 403.
There was a problem hiding this comment.
@shubham1g5 checked with charl on this. He informed this locked account will only come with 403 error code and not with any other
There was a problem hiding this comment.
that's true but we want to have support for error_code for all response codes on mobile.
|
@jaypanchal-13 When user tries for 3 failed attempt for back up code, we need to lock it there also so need implementation at |
@Jignesh-dimagi for backup code @OrangeAndGreen has already completed that |
Yes correct when response is |
|
But you have not handled that failure code in |
|
That is strange!, One API is sending error_code in |
|
jaypanchal-13
changed the title
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
can we also add the new string for hindi and spanish.
| Logger.exception("Unhandled API error code: " + errorCode, t); | ||
| return activity.getString(R.string.recovery_network_unknown); |
There was a problem hiding this comment.
think what we want to do here is this -
if (t is not null) -> throw new RuntimeException(t);
else ->
Logger.exception("Unhandled API error code", new RuntimeException("Unhandled API error code: " + errorCode));
return activity.getString(R.string.recovery_network_unknown);
as we should still throw any unhandled exceptions from here.
There was a problem hiding this comment.
@shubham1g5 We will any way get exception in log why to crash app for it and give poor experience to user? Do you want to change it?
There was a problem hiding this comment.
The philosophy here is user is much more likely to report a crash to us rather than a silent failure. We prefer to crash hard for any unknown exceptions we didn't anticipate to happen in the code due to external factors. Here is a doc on this approach that gives more details on what we want to catch and what not.
|
|
||
| if (responseCode == 429 || responseCode == 503) { | ||
| baseApiHandler.onFailure( | ||
| 429, 503 -> baseApiHandler.onFailure( |
There was a problem hiding this comment.
unrelated to this PR, but think we want to treat all 50x as server errors instead
There was a problem hiding this comment.
@shubham1g5 Yes correct but I think before managing all 50X for server errors, backend sholud make sure that in this case 503 is rate limit exceed. Should be modified from backend to avoid any crash regarding this in future. Should I go ahead and manage it from our side?
There was a problem hiding this comment.
I can confirm that backend sends 429 for rate limiting only and not 503 . This was more of an error on mobile implementation earlier to treat 503 as rate limiting
|
|
||
| else -> baseApiHandler.onFailure( | ||
| PersonalIdOrConnectApiErrorCodes.UNKNOWN_ERROR, | ||
| Exception("Response $responseCode") |
There was a problem hiding this comment.
- We should ensure that we are handling all possible http response codes in this method above if we are going to throw anything else as unknown error with an exception
UNKNOWN_ERRORdoesn't log the associated exception inPersonalIdApiErrorHandler.handle, so there is no real value in addingException("Response $responseCode")here but we can just log the unknown response code here directly asLogger.exception("Unknown http response code ", new Exception("Encountered response code " + responseCode + " for url " url))
…count-error-handle
For unknow error thrown exception with url Added common server error for 50X error codes
…count-error-handle # Conflicts: # app/res/values-es/strings.xml
Product Description
Ticket-> https://dimagi.atlassian.net/browse/CCCT-1396
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review