Issue solved for App crashes on entering wrong backup code #3418
Conversation
📝 WalkthroughWalkthroughThe changes refactor the backup code response parsing logic in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (4)
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java (3)
24-24: Fix typo in comment.The comment contains a typo: "responed" should be "responds".
- if (json.has("attempts_left")) { // whenever wrong code is entered, server responed with attempts_left + if (json.has("attempts_left")) { // whenever wrong code is entered, server responds with attempts_left
26-26: Fix typo in comment.Same typo as above: "responed" should be "responds".
- } else if (json.has("error_code")) { // whenever account is locked, server responed with error_code + } else if (json.has("error_code")) { // whenever account is locked, server responds with error_code
37-38: Consider simplifying error message wording.The phrase "amongst" is less commonly used than "among" in modern English. Consider simplifying for better readability.
- throw new IllegalStateException( - "Any of the fields amongst username, db_key or password cannot be empty"); + throw new IllegalStateException( + "Fields username, db_key, and password cannot be empty");app/unit-tests/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParserTest.kt (1)
49-86: Consider adding edge case tests for ambiguous responses.While the current tests cover the three main branches, consider adding tests for edge cases where multiple indicators are present (e.g., both
attempts_leftandusername). This would document the priority order and ensure it's intentional.Example test case:
@Test fun testParsePrioritizesAttemptsLeftWhenMultipleIndicatorsPresent() { // Arrange - Response contains both attempts_left and credentials val json = JSONObject().apply { put("attempts_left", 2) put("username", "test-user") put("db_key", "test-key") put("password", "test-pass") } // Act parser.parse(json, sessionData) // Assert - Only attempts_left should be processed assertEquals(2, sessionData.attemptsLeft) assertEquals(null, sessionData.personalId) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java(2 hunks)app/unit-tests/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParserTest.kt(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-01-21T18:20:43.883Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java
📚 Learning: 2025-02-04T21:22:56.537Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 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.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java
📚 Learning: 2025-06-13T15:53:12.951Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java
🧬 Code graph analysis (1)
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java (1)
app/src/org/commcare/utils/JsonExtensions.kt (1)
optStringSafe(7-10)
⏰ 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 (4)
app/unit-tests/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParserTest.kt (3)
28-47: LGTM!The test correctly validates the success case, verifying all credentials and the invited_user flag are properly set.
69-86: Test logic is sound, but verify default assumption.The test correctly validates the locked account scenario. However, similar to the previous test, verify that
invitedUserdefaults to false (same verification as above).
49-67: Default value assumption is correct.The
invitedUserfield inPersonalIdSessionDatais initialized with a default value offalse, confirming the test's assertion is valid. The test properly verifies that when parsing a wrong backup code response (failure case),invitedUserremains at its default value while onlyattemptsLeftis populated.app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java (1)
24-46: No changes required—server contract is validated by comprehensive tests.The test file (
app/unit-tests/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParserTest.kt) confirms the if-else structure's assumptions. Tests explicitly validate three mutually exclusive response scenarios: attempts_left-only (wrong code), error_code-only (locked account), and credentials-only (success). No test combines multiple indicators, confirming the server contract truly sends them exclusively. The parser's priority order and behavior are correct and well-tested.
| sessionData.setOauthPassword(password); | ||
|
|
||
| if (json.has("attempts_left")) { | ||
| if (json.has("attempts_left")) { // whenever wrong code is entered, server responed with attempts_left |
There was a problem hiding this comment.
Nit: typo on responed vs responds
Technical Summary
https://dimagi.atlassian.net/browse/QA-8226
Server is sending 200 response whenever back up is wrong with attempt number, so application cannot expect username, dbkey and password in that response. Handled server response separately for attempt (wrong back up code), error code (locked account) and valid response.
Labels and Review