Token and Work History Parsing tests + some validation changes

[shubham1g5]

Product Description

https://dimagi.atlassian.net/browse/CCCT-1892

Continuation of #3402

Network Parsers tests for ConnectTokenResponseParser.kt and RetrieveWorkHistoryResponseParser.kt

Technical Summary

Parsing tests + more stricter validations on token parser

Labels and Review

• Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
• Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
• Risk label is set correctly
• The set of people pinged as reviewers is appropriate for the level of risk of the change

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request refactors the ConnectTokenResponseParser to use specific constant imports instead of generic imports, and adds validation for token content (ensuring it is not empty or the string "null") and expiration time (ensuring it is non-negative). Two new comprehensive test classes are introduced: ConnectTokenResponseParserTest covers valid token responses and error scenarios, while RetrieveWorkHistoryResponseParserTest tests work history response parsing with extensive coverage of valid responses, empty credentials, and malformed input cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• ConnectTokenResponseParser.kt: Review the new token and expiration validation logic, particularly the emptiness check and "null" string comparison, and confirm the non-negative boundary check for expiration is correct.
• ConnectTokenResponseParserTest.kt: Verify test coverage is comprehensive, including all error cases and expiration time calculations with appropriate tolerances.
• RetrieveWorkHistoryResponseParserTest.kt: Pay close attention to the mock setup using mockStatic, assertion helpers for credential matching, and database storage verification logic to ensure mocks are correctly configured.

Suggested reviewers

• OrangeAndGreen
• conroy-ricketts

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 3.57% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check	⚠️ Warning	The pull request description is incomplete and does not follow the required template structure.	Complete the PR description by adding: (1) Product Description with visible user-facing effects; (2) Safety Assurance section with safety story, automated test coverage details, and QA plan; (3) Complete all checklist items for labels, QA notes, and release notes.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: adding parsing tests and validation improvements to token and work history parsers.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch parsingTests

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (6)

app/unit-tests/src/org/commcare/connect/network/connectId/parser/RetrieveWorkHistoryResponseParserTest.kt (3)

5-12: Remove unused imports.

The following imports are not used in this file:

• CommCareApplication (line 5)
• assertNotNull (line 11)
• assertTrue (line 12)

 import android.content.Context
 import androidx.test.ext.junit.runners.AndroidJUnit4
-import org.commcare.CommCareApplication
 import org.commcare.CommCareTestApplication
 import org.commcare.android.database.connect.models.PersonalIdWorkHistory
 import org.commcare.connect.database.ConnectAppDatabaseUtil
 import org.json.JSONException
 import org.junit.Assert.assertEquals
-import org.junit.Assert.assertNotNull
-import org.junit.Assert.assertTrue
 import org.junit.Before

---

28-35: Remove unnecessary MockitoAnnotations.openMocks() call.

There are no @Mock or @Captor annotated fields in this class, so the MockitoAnnotations.openMocks(this) call is unnecessary.

Also, consider moving context initialization into the @Before method to ensure Robolectric has fully prepared the test application:

-    private var context: Context = CommCareTestApplication.instance()
+    private lateinit var context: Context

     private lateinit var parser: RetrieveWorkHistoryResponseParser<List<PersonalIdWorkHistory>>

     @Before
     fun setup() {
-        MockitoAnnotations.openMocks(this)
+        context = CommCareTestApplication.instance()
         parser = RetrieveWorkHistoryResponseParser(context)
     }

---

201-266: Good exception handling coverage.

The tests thoroughly cover JSON-level error scenarios.

Consider adding tests for:

1. Malformed individual credentials - e.g., a credential missing required fields like uuid or app_id
2. Non-200 HTTP status codes - to verify error response handling

These are optional enhancements for more comprehensive coverage.

app/src/org/commcare/connect/network/connectId/parser/ConnectTokenResponseParser.kt (1)

4-5: Token/expiry validation and constant usage look solid; only tiny optional cleanups.

Using CONNECT_KEY_TOKEN/CONNECT_KEY_EXPIRES directly plus the check guards gives clear, predictable failures for bad responses, and the optInt(..., 0) + seconds >= 0 constraint is a good balance between flexibility and safety. If you want to tighten things further, you could very lightly:

• Prefer token.isNotEmpty() (or token.isNotBlank() if whitespace-only tokens should also be rejected) for readability.
• Construct expiration in one expression, e.g. val expiration = Date(System.currentTimeMillis() + seconds.toLong() * 1000).

These are purely stylistic; current logic is correct and testable as‑is.

Also applies to: 25-32

app/unit-tests/src/org/commcare/connect/network/connectId/parser/ConnectTokenResponseParserTest.kt (2)

20-93: Well-factored test harness; expiration assertions are reasonable with minor optional polish.

The TokenTestData model plus createTokenJson/parseTokenAndAssertBasics/assertExpirationTime/testTokenWithExpiration gives concise, readable coverage of the happy‑path permutations, and the 5s tolerance around Date() is pragmatic for CI. If you ever want to tighten these tests, you could:

• Store expectedDeltaSeconds directly in TokenTestData instead of expectedTimeDeltaMs and avoid the / 1000 in testTokenWithExpiration.
• Consider injecting a clock or wrapping Date() behind a helper to make time‑based tests fully deterministic.

Not required for this PR; current structure is clear and maintainable.

---

135-195: Exception coverage is comprehensive; only tiny nits.

The suite of negative tests (empty token, invalid JSON, missing token, "null" token, invalid/negative expires, empty response) maps cleanly onto the parser’s check and JSONException behaviors, and parseTokenExpectingException keeps them DRY. Two very small nits you may consider:

• In testParseWithInvalidExpiresInType, you might also assert that expiration is effectively “now” (0s delta) to document the optInt fallback behavior.
• In testParseWithNegativeExpiresIn, the token string "test_toen" looks like a typo; it doesn’t affect behavior but could be renamed for clarity.

Functionally these tests look good as‑is.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f2a9ac0 and aad128b.

📒 Files selected for processing (3)

• app/src/org/commcare/connect/network/connectId/parser/ConnectTokenResponseParser.kt (3 hunks)
• app/unit-tests/src/org/commcare/connect/network/connectId/parser/ConnectTokenResponseParserTest.kt (1 hunks)
• app/unit-tests/src/org/commcare/connect/network/connectId/parser/RetrieveWorkHistoryResponseParserTest.kt (1 hunks)

🧰 Additional context used

🧠 Learnings (8)

📓 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: 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.

📚 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/ConnectTokenResponseParserTest.kt
• app/unit-tests/src/org/commcare/connect/network/connectId/parser/RetrieveWorkHistoryResponseParserTest.kt
• app/src/org/commcare/connect/network/connectId/parser/ConnectTokenResponseParser.kt

📚 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/ConnectTokenResponseParser.kt

📚 Learning: 2025-01-21T17:28:09.007Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:66-71
Timestamp: 2025-01-21T17:28:09.007Z
Learning: The ConnectUserRecord class in CommCare Android uses Persisting annotations with sequential indices for field persistence, and MetaField annotations for fields that have corresponding META constants. Fields should be documented with Javadoc comments explaining their purpose, format requirements, and relationships with other fields.

Applied to files:

• app/src/org/commcare/connect/network/connectId/parser/ConnectTokenResponseParser.kt

📚 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/network/connectId/parser/ConnectTokenResponseParser.kt

📚 Learning: 2025-01-21T18:19:05.799Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:214-215
Timestamp: 2025-01-21T18:19:05.799Z
Learning: In ConnectIdPasswordVerificationFragment, when creating a ConnectUserRecord, it's acceptable for payment information (paymentName and paymentPhone) to be empty strings if the server response doesn't include payment info in the CONNECT_PAYMENT_INFO field.

Applied to files:

• app/src/org/commcare/connect/network/connectId/parser/ConnectTokenResponseParser.kt

📚 Learning: 2025-04-22T17:04:26.780Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3043
File: app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java:0-0
Timestamp: 2025-04-22T17:04:26.780Z
Learning: In the Connect API contract, the server doesn't include the fields `created`, `answered_consent`, and `key` in JSON responses for messaging channels, which is why the `fromJson` method in `ConnectMessagingChannelRecord` explicitly initializes these values rather than parsing them from JSON.

Applied to files:

• app/src/org/commcare/connect/network/connectId/parser/ConnectTokenResponseParser.kt

📚 Learning: 2025-01-26T14:32:09.266Z

Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2847
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecordV5.java:72-76
Timestamp: 2025-01-26T14:32:09.266Z
Learning: In ConnectUserRecordV5, initializing lastPasswordDate and connectTokenExpiration to new Date() in the constructor is intentional.

Applied to files:

• app/src/org/commcare/connect/network/connectId/parser/ConnectTokenResponseParser.kt

⏰ 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/unit-tests/src/org/commcare/connect/network/connectId/parser/RetrieveWorkHistoryResponseParserTest.kt (2)

38-89: Well-structured test helpers.

The ExpectedCredential data class and assertion helpers reduce duplication and make the tests more readable. The verifyDatabaseStorage function nicely encapsulates the mock verification logic.

---

91-199: Good coverage for valid response scenarios.

These tests effectively verify:

• Multi-credential parsing with all fields
• Database storage verification
• Empty credentials array handling

The use of mockStatic().use {} ensures proper resource cleanup.

[conroy-ricketts]

Looking great! I just had a few nitpick comments and minor things I noticed, and I also think it's a good idea to look over the nitpick comments from Coderabbit too