Tests for Personal ID Signup Parsers + More Validations on mandatory fields #3402
Conversation
shubham1g5
requested review from
Jignesh-dimagi,
OrangeAndGreen and
conroy-ricketts
📝 WalkthroughWalkthroughThe PR enhances input validation in two response parser classes and introduces comprehensive unit test coverage for five parser implementations. The validation enhancements extract username, db_key, and password into local variables, perform null checks using Objects.requireNonNull, validate for non-empty strings, and throw IllegalStateException when validation fails. Five new test classes are added to cover successful parsing scenarios, missing or null field error cases, empty string validation, and edge cases including null inputs and large data handling. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes Key areas requiring attention:
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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: 1
🧹 Nitpick comments (3)
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java (1)
23-37: Improve null-check diagnosticsLine 27 currently throws a bare
NullPointerExceptionwhen the API omits"username", leaving crash logs with no hint about which field violated the contract. Please add explicit messages to eachObjects.requireNonNullso on-call engineers get immediate context.- Objects.requireNonNull(username); - Objects.requireNonNull(dbKey); - Objects.requireNonNull(password); + Objects.requireNonNull(username, "Missing username from confirm-backup-code response"); + Objects.requireNonNull(dbKey, "Missing db_key from confirm-backup-code response"); + Objects.requireNonNull(password, "Missing password from confirm-backup-code response");app/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParser.java (1)
23-37: Surface the offending field when validation failsLine 27 also emits an anonymous
NullPointerExceptionif the server omits"username". Please mirror the explicit messages suggested for the other parser so the crash report immediately names the missing field.- Objects.requireNonNull(username); - Objects.requireNonNull(dbKey); - Objects.requireNonNull(password); + Objects.requireNonNull(username, "Missing username from complete-profile response"); + Objects.requireNonNull(dbKey, "Missing db_key from complete-profile response"); + Objects.requireNonNull(password, "Missing password from complete-profile response");app/unit-tests/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParserTest.java (1)
124-158: Update the empty-string commentLine 126 says “Empty strings should be accepted,” but the test explicitly asserts
IllegalStateException. Please tweak the comment so it matches the expected behaviour and avoids confusing future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParser.java(1 hunks)app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java(2 hunks)app/unit-tests/src/org/commcare/connect/network/connectId/parser/AddOrVerifyNameParserTest.java(1 hunks)app/unit-tests/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParserTest.java(1 hunks)app/unit-tests/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParserTest.java(1 hunks)app/unit-tests/src/org/commcare/connect/network/connectId/parser/ReportIntegrityResponseParserTest.java(1 hunks)app/unit-tests/src/org/commcare/connect/network/connectId/parser/StartConfigurationResponseParserTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/unit-tests/src/org/commcare/connect/network/connectId/parser/StartConfigurationResponseParserTest.javaapp/unit-tests/src/org/commcare/connect/network/connectId/parser/AddOrVerifyNameParserTest.javaapp/unit-tests/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParserTest.javaapp/unit-tests/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParserTest.javaapp/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.javaapp/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParser.java
📚 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/unit-tests/src/org/commcare/connect/network/connectId/parser/StartConfigurationResponseParserTest.javaapp/unit-tests/src/org/commcare/connect/network/connectId/parser/AddOrVerifyNameParserTest.javaapp/unit-tests/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParserTest.javaapp/unit-tests/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParserTest.javaapp/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.javaapp/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParser.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.javaapp/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParser.java
📚 Learning: 2025-03-10T08:16:29.416Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 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.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParser.java
🧬 Code graph analysis (7)
app/unit-tests/src/org/commcare/connect/network/connectId/parser/StartConfigurationResponseParserTest.java (2)
app/unit-tests/src/org/commcare/CommCareTestApplication.java (1)
CommCareTestApplication(69-359)app/unit-tests/src/org/commcare/connect/network/connectId/parser/AddOrVerifyNameParserTest.java (1)
Config(19-146)
app/unit-tests/src/org/commcare/connect/network/connectId/parser/AddOrVerifyNameParserTest.java (3)
app/unit-tests/src/org/commcare/CommCareTestApplication.java (1)
CommCareTestApplication(69-359)app/unit-tests/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParserTest.java (1)
Config(19-155)app/unit-tests/src/org/commcare/connect/network/connectId/parser/StartConfigurationResponseParserTest.java (1)
Config(19-112)
app/unit-tests/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParserTest.java (2)
app/unit-tests/src/org/commcare/CommCareTestApplication.java (1)
CommCareTestApplication(69-359)app/unit-tests/src/org/commcare/connect/network/connectId/parser/AddOrVerifyNameParserTest.java (1)
Config(19-146)
app/unit-tests/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParserTest.java (1)
app/unit-tests/src/org/commcare/CommCareTestApplication.java (1)
CommCareTestApplication(69-359)
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java (1)
app/src/org/commcare/utils/JsonExtensions.kt (1)
optStringSafe(7-10)
app/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParser.java (1)
app/src/org/commcare/utils/JsonExtensions.kt (1)
optStringSafe(7-10)
app/unit-tests/src/org/commcare/connect/network/connectId/parser/ReportIntegrityResponseParserTest.java (1)
app/unit-tests/src/org/commcare/CommCareTestApplication.java (1)
CommCareTestApplication(69-359)
⏰ 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/ReportIntegrityResponseParserTest.java (1)
33-48: Happy-path coverage looks solidLine 44 drives the success branch with a realistic payload and verifies the boolean outcome, which is exactly the regression guard we needed for this parser.
app/unit-tests/src/org/commcare/connect/network/connectId/parser/StartConfigurationResponseParserTest.java (1)
32-111: Great coverage for parser defaults and error pathsLines 63-110 validate both the happy path and the failure scenarios (default values, empty strings, null arguments), giving high confidence in the parser’s behaviour. Nicely done.
app/unit-tests/src/org/commcare/connect/network/connectId/parser/AddOrVerifyNameParserTest.java (1)
1-146: LGTM! Comprehensive test coverage for AddOrVerifyNameParser.The test class provides excellent coverage of the parser's behavior across multiple scenarios:
- Valid response parsing with all fields
- Default value handling when fields are missing
- Partial data scenarios
- Edge cases including empty strings, null values, and large photo data
- Defensive null checks for JSON and sessionData
The test structure follows the established patterns from other parser tests in the PR and properly validates the optional nature of the
account_existsandphotofields.app/unit-tests/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParserTest.java (1)
1-155: Excellent test coverage for CompleteProfileResponseParser validation.The test class thoroughly validates the new mandatory field validation logic introduced in the PR:
- Successful parsing with all required fields present
- Proper null checking for missing or null fields (NullPointerException)
- Empty string validation (IllegalStateException)
- Comprehensive coverage of all three mandatory fields: username, db_key, and password
The test structure follows the established patterns from other parser tests and aligns well with the PR objectives to add validations for mandatory fields.
|
|
||
| @Test(expected = IllegalStateException.class) | ||
| public void testParseWithEmptyUsername() throws JSONException { | ||
| // Arrange - Empty strings should be accepted (not null) |
There was a problem hiding this comment.
Fix misleading comments in empty string validation tests.
The comments state "Empty strings should be accepted (not null)" but the tests correctly expect IllegalStateException, which means empty strings are not accepted by the parser. These comments are contradictory and could confuse future maintainers.
Apply this diff to fix the comments:
- // Arrange - Empty strings should be accepted (not null)
+ // Arrange - Empty strings should be rejected with IllegalStateExceptionApply the same fix at lines 134 and 146.
Also applies to: 134-134, 146-146
🤖 Prompt for AI Agents
In
app/unit-tests/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParserTest.java
around lines 122, 134 and 146, the inline comments currently read "Empty strings
should be accepted (not null)" which contradicts the test assertions that expect
IllegalStateException; update those comments to accurately state that empty
strings are not accepted and the parser should throw IllegalStateException
(e.g., "Empty strings are not accepted; expect IllegalStateException") so the
comments match the test expectations.
cc2ad8e to
3d4dc78
Compare
shubham1g5
force-pushed
the
parsingTests
branch
from
3d4dc78 to
58b10de
Compare
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
These unit tests are great! I think that the comments are fantastic
app/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParser.java
Show resolved
Hide resolved
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java
Show resolved
Hide resolved
| /** | ||
| * Unit tests for AddOrVerifyNameParser | ||
| */ |
There was a problem hiding this comment.
Optional (nit): This comment seems a bit redundant due to both the file's name and the folder it's in
app/unit-tests/src/org/commcare/connect/network/connectId/parser/AddOrVerifyNameParserTest.kt
Show resolved
Hide resolved
| /** | ||
| * Unit tests for CompleteProfileResponseParser | ||
| */ |
There was a problem hiding this comment.
Optional (nit): This comment seems a bit redundant due to both the file's name and the folder it's in
...tests/src/org/commcare/connect/network/connectId/parser/ReportIntegrityResponseParserTest.kt
Show resolved
Hide resolved
| // Assert | ||
| assertEquals("biometric", sessionData.requiredLock) | ||
| assertEquals("partial-token", sessionData.token) | ||
| assertFalse(sessionData.demoUser ?: true) // optBoolean returns false when not set |
There was a problem hiding this comment.
Optional: In the comment, it may be worth mentioning that demoUser is nullable here too like on line 71 and that's the reason for ?: true
...ts/src/org/commcare/connect/network/connectId/parser/StartConfigurationResponseParserTest.kt
Show resolved
Hide resolved
...ts/src/org/commcare/connect/network/connectId/parser/StartConfigurationResponseParserTest.kt
Show resolved
Hide resolved
| // Assert - Should only set known fields, ignore extra ones | ||
| assertEquals("pin", sessionData.requiredLock) | ||
| assertEquals("test-token", sessionData.token) | ||
| assertFalse(sessionData.demoUser ?: true) // optBoolean returns false when not set |
There was a problem hiding this comment.
Optional: Same as earlier, in the comment, it may be worth mentioning that demoUser is nullable here too
| // Assert | ||
| assertEquals(true, sessionData.accountExists) | ||
| assertEquals( | ||
| "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk+M9QDwADhgGAWjR9awAAAABJRU5ErkJggg==", |
There was a problem hiding this comment.
Nit: Is there a reason not to store this as a const string so it isn't copy-pasted here and above?
| Objects.requireNonNull(username); | ||
| Objects.requireNonNull(dbKey); | ||
| Objects.requireNonNull(password); |
There was a problem hiding this comment.
Do we want to add the text here as reason i.e. Objects.requireNonNull(username,"username cannot be null");
There was a problem hiding this comment.
think that will be redundant in simple cases like this where it's quite evident that the username is null which is why there is an NPE here.
| Objects.requireNonNull(dbKey); | ||
| Objects.requireNonNull(password); | ||
| if (username.isEmpty() || dbKey.isEmpty() || password.isEmpty()) { | ||
| throw new IllegalStateException( |
There was a problem hiding this comment.
PersonalIdApiHandler doesn't handle IllegalStateException so application might crash here.
There was a problem hiding this comment.
Yes, we do want to crash in these scenarios as it's critical info for app to process a successful sign-in
| Objects.requireNonNull(username); | ||
| Objects.requireNonNull(dbKey); | ||
| Objects.requireNonNull(password); |
There was a problem hiding this comment.
| Objects.requireNonNull(dbKey); | ||
| Objects.requireNonNull(password); | ||
| if (username.isEmpty() || dbKey.isEmpty() || password.isEmpty()) { | ||
| throw new IllegalStateException( |
There was a problem hiding this comment.
| } | ||
|
|
||
| @Test | ||
| fun testParseWithLargePhotoData() { |
There was a problem hiding this comment.
Not sure if this test is different from other test except the string length
There was a problem hiding this comment.
that is right, the purpose of this test is to do a explicit check on very large photo data which is a real possibility with base 64 photo data.
|
@shubham1g5 Please handle the kotlin lint failure |
Product Description
https://dimagi.atlassian.net/browse/CCCT-1871
Unit tests on Network Parsers + Some more validations on mandatory fields inside Parsers.
Labels and Review