Refactor PersonalIdApiErrorHandler #3451
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…onnect feature in its naming and documentation.
conroy-ricketts
requested review from
Jignesh-dimagi,
OrangeAndGreen,
jaypanchal-13 and
shubham1g5
📝 WalkthroughWalkthroughThis PR consolidates Personal ID and Connect API error handling by renaming and relocating Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 (1)
app/src/org/commcare/fragments/personalId/PersonalIdNameFragment.java (1)
20-20: Consider normalizing the error code type used innavigateFailure
onFailureuses the unqualifiedPersonalIdOrConnectApiErrorCodestype, whilenavigateFailurestill declaresPersonalIdApiHandler.PersonalIdOrConnectApiErrorCodes. If these now alias the sameBaseApiHandler.PersonalIdOrConnectApiErrorCodes, this is fine but a bit confusing.You might want to update
navigateFailureto use the same imported enum type thatonFailureuses, to make the relationship withPersonalIdOrConnectApiErrorHandler.handle(...)more obvious and avoid future type drift.If you want to double‑check, please verify that both references resolve to the same underlying enum/interface (now owned by
BaseApiHandler) and that there are no lingering references to the old nested enum inPersonalIdApiHandler.Also applies to: 84-89, 97-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/src/org/commcare/activities/CommCareSetupActivity.java(2 hunks)app/src/org/commcare/activities/connect/PersonalIdWorkHistoryActivity.kt(2 hunks)app/src/org/commcare/connect/ConnectJobHelper.kt(2 hunks)app/src/org/commcare/connect/network/PersonalIdOrConnectApiErrorHandler.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdNameFragment.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java(2 hunks)app/src/org/commcare/utils/FirebaseAuthService.java(0 hunks)app/src/org/commcare/utils/PersonalIDAuthService.kt(0 hunks)app/src/org/commcare/utils/PushNotificationApiHelper.kt(2 hunks)
💤 Files with no reviewable changes (2)
- app/src/org/commcare/utils/PersonalIDAuthService.kt
- app/src/org/commcare/utils/FirebaseAuthService.java
🧰 Additional context used
🧠 Learnings (24)
📓 Common learnings
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/network/ApiPersonalId.java:0-0
Timestamp: 2025-07-29T14:56:40.681Z
Learning: User OrangeAndGreen prefers to defer ApiPersonalId refactoring (specifically InputStream resource management improvements) to separate PRs rather than mixing them with Connect feature work, even when the code is already in master.
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/connect/MessageManager.java:0-0
Timestamp: 2025-07-29T14:54:47.940Z
Learning: User OrangeAndGreen organizes messaging-related changes into separate PRs rather than mixing them with other Connect work, which aligns with their preference for handling code issues in separate PRs when they are not directly related to the main changes.
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.
📚 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/ConnectJobIntroFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.javaapp/src/org/commcare/activities/CommCareSetupActivity.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.javaapp/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/ConnectJobIntroFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdNameFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaapp/src/org/commcare/activities/connect/PersonalIdWorkHistoryActivity.ktapp/src/org/commcare/activities/CommCareSetupActivity.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.javaapp/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/ConnectJobIntroFragment.javaapp/src/org/commcare/activities/CommCareSetupActivity.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.javaapp/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/ConnectJobIntroFragment.javaapp/src/org/commcare/connect/ConnectJobHelper.ktapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaapp/src/org/commcare/activities/connect/PersonalIdWorkHistoryActivity.ktapp/src/org/commcare/activities/CommCareSetupActivity.javaapp/src/org/commcare/utils/PushNotificationApiHelper.ktapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.javaapp/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/ConnectJobIntroFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdNameFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaapp/src/org/commcare/activities/CommCareSetupActivity.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.javaapp/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/ConnectJobIntroFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaapp/src/org/commcare/activities/CommCareSetupActivity.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.javaapp/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/ConnectJobIntroFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdNameFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.javaapp/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/ConnectJobIntroFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.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/ConnectJobIntroFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.javaapp/src/org/commcare/connect/ConnectJobHelper.ktapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaapp/src/org/commcare/activities/CommCareSetupActivity.javaapp/src/org/commcare/utils/PushNotificationApiHelper.ktapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-06-20T15:51:14.157Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/models/connect/ConnectLoginJobListModel.java:79-92
Timestamp: 2025-06-20T15:51:14.157Z
Learning: The ConnectLoginJobListModel class in app/src/org/commcare/models/connect/ConnectLoginJobListModel.java does not need to implement Parcelable interface as it is not passed between Android activities or fragments.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.javaapp/src/org/commcare/activities/CommCareSetupActivity.javaapp/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/ConnectJobIntroFragment.javaapp/src/org/commcare/activities/CommCareSetupActivity.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 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/fragments/connect/ConnectJobIntroFragment.javaapp/src/org/commcare/activities/CommCareSetupActivity.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.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/fragments/connect/ConnectJobIntroFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdNameFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaapp/src/org/commcare/utils/PushNotificationApiHelper.ktapp/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.javaapp/src/org/commcare/connect/network/PersonalIdOrConnectApiErrorHandler.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/personalId/PersonalIdBackupCodeFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.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/fragments/personalId/PersonalIdBackupCodeFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-06-04T19:17:21.213Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdNameFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java
📚 Learning: 2025-05-09T10:57:41.073Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 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.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdNameFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaapp/src/org/commcare/activities/connect/PersonalIdWorkHistoryActivity.ktapp/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java
📚 Learning: 2025-04-21T15:02:17.492Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 3042
File: app/src/org/commcare/fragments/BreadcrumbBarViewModel.java:50-55
Timestamp: 2025-04-21T15:02:17.492Z
Learning: ViewModels should not store View or Activity references as this can cause memory leaks. Unlike Fragments with setRetainInstance(true), ViewModels don't have automatic view detachment mechanisms. When migrating from Fragments to ViewModels, view references should be replaced with data-only state in the ViewModel.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdNameFragment.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.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 the Connect error handling flow of CommCare Android, error messages are shown once and then automatically cleared because the underlying error record gets deleted after being displayed. This is why there's no need for explicit methods to hide these messages.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.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/fragments/personalId/PersonalIdPhoneVerificationFragment.java
📚 Learning: 2025-04-22T15:48:29.346Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3037
File: app/src/org/commcare/connect/ConnectIDManager.java:233-243
Timestamp: 2025-04-22T15:48:29.346Z
Learning: Never instantiate Android Activity classes directly with 'new'. Activities should only be created through the Android framework using Intents.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📚 Learning: 2025-04-22T15:48:29.346Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3037
File: app/src/org/commcare/connect/ConnectIDManager.java:233-243
Timestamp: 2025-04-22T15:48:29.346Z
Learning: Never instantiate Android Activity classes directly with 'new' as it bypasses the Android component lifecycle. Activities should only be created by the system through Intents. Non-static instance fields don't need manual resetting as they'll be initialized with their default values when a new activity instance is properly created.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.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/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
🧬 Code graph analysis (9)
app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java (1)
app/src/org/commcare/connect/network/PersonalIdOrConnectApiErrorHandler.java (1)
PersonalIdOrConnectApiErrorHandler(25-74)
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (1)
app/src/org/commcare/connect/network/PersonalIdOrConnectApiErrorHandler.java (1)
PersonalIdOrConnectApiErrorHandler(25-74)
app/src/org/commcare/fragments/personalId/PersonalIdNameFragment.java (1)
app/src/org/commcare/connect/network/PersonalIdOrConnectApiErrorHandler.java (1)
PersonalIdOrConnectApiErrorHandler(25-74)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
app/src/org/commcare/connect/network/PersonalIdOrConnectApiErrorHandler.java (1)
PersonalIdOrConnectApiErrorHandler(25-74)
app/src/org/commcare/activities/CommCareSetupActivity.java (2)
app/src/org/commcare/connect/network/PersonalIdOrConnectApiErrorHandler.java (1)
PersonalIdOrConnectApiErrorHandler(25-74)app/src/org/commcare/utils/PersonalIDAuthService.kt (1)
activity(8-42)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (2)
app/src/org/commcare/connect/network/PersonalIdOrConnectApiErrorHandler.java (1)
PersonalIdOrConnectApiErrorHandler(25-74)app/src/org/commcare/utils/PersonalIDAuthService.kt (1)
activity(8-42)
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java (1)
app/src/org/commcare/connect/network/PersonalIdOrConnectApiErrorHandler.java (1)
PersonalIdOrConnectApiErrorHandler(25-74)
app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java (1)
app/src/org/commcare/connect/network/PersonalIdOrConnectApiErrorHandler.java (1)
PersonalIdOrConnectApiErrorHandler(25-74)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (1)
app/src/org/commcare/connect/network/PersonalIdOrConnectApiErrorHandler.java (1)
PersonalIdOrConnectApiErrorHandler(25-74)
🔇 Additional comments (12)
app/src/org/commcare/connect/network/PersonalIdOrConnectApiErrorHandler.java (1)
1-74: LGTM! Clean refactoring to generalize the error handler.The rename from
PersonalIdApiErrorHandlertoPersonalIdOrConnectApiErrorHandlerand the package relocation fromorg.commcare.connect.network.connectIdtoorg.commcare.connect.networkaccurately reflect that this handler serves both Personal ID and Connect features. The error code type is now properly sourced fromBaseApiHandler.PersonalIdOrConnectApiErrorCodes, and the parameter type change fromActivitytoContextprovides better flexibility.app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (1)
36-36: LGTM! Import and usage correctly updated.The import and method call have been mechanically updated to use
PersonalIdOrConnectApiErrorHandlerconsistently with the refactoring.Also applies to: 83-83
app/src/org/commcare/utils/PushNotificationApiHelper.kt (1)
27-27: LGTM! Kotlin file correctly updated.The import and error handler invocation have been properly updated to use the renamed
PersonalIdOrConnectApiErrorHandler.Also applies to: 101-101
app/src/org/commcare/activities/connect/PersonalIdWorkHistoryActivity.kt (1)
12-12: LGTM! Activity correctly updated.The import and error handling call have been properly updated to use
PersonalIdOrConnectApiErrorHandler.Also applies to: 76-76
app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java (1)
27-27: LGTM! Fragment correctly updated.The import and error handler usage have been properly updated to use the renamed
PersonalIdOrConnectApiErrorHandler.Also applies to: 109-109
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
43-43: LGTM! Fragment correctly updated.The import and error handler invocation have been properly updated to use
PersonalIdOrConnectApiErrorHandler.Also applies to: 514-514
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (1)
29-29: LGTM! Fragment correctly updated.The import and error handling call have been properly updated to use the renamed
PersonalIdOrConnectApiErrorHandler.Also applies to: 144-144
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
34-34: LGTM! Activity correctly updated.The import and error handler usage have been properly updated to use
PersonalIdOrConnectApiErrorHandler. This completes the consistent refactoring across all affected files in the PR.Also applies to: 1012-1012
app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java (1)
23-23: Wiring to shared PersonalIdOrConnectApiErrorHandler looks correctThe updated import and onFailure implementation correctly delegate Connect job intro errors to
PersonalIdOrConnectApiErrorHandler.handle(...)with anActivitycontext, matching the new shared handler’s contract and the existing fail‑fast error‑handling approach for Connect. Based on learnings, this keeps behavior consistent without introducing new branches.Also applies to: 116-120
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (1)
24-24: Shared error handler integration preserves existing recovery flowSwitching to
PersonalIdOrConnectApiErrorHandler.handle(...)here cleanly reuses the shared handler while keeping the existing flow (handleCommonSignupFailurescheck, then field error display and retry enablement) intact. No functional change beyond the handler relocation.Also applies to: 205-214
app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java (1)
24-24: Connect delivery error handling correctly migrated to shared handlerThe refactor to use
PersonalIdOrConnectApiErrorHandler.handle(...)in the non‑BAD_REQUEST_ERRORbranch keeps the existing UX (custom message forBAD_REQUEST_ERROR, shared handler for everything else) and aligns this fragment with the other Connect flows using the centralized handler.Also applies to: 102-112
app/src/org/commcare/connect/ConnectJobHelper.kt (1)
14-14: Shared error handler usage for payment failures looks goodSwitching the payment confirmation failure path to
PersonalIdOrConnectApiErrorHandler.handle(context, errorCode, t)cleanly reuses the centralized error mapping while preserving the existing toast + analytics + callback behavior.Also applies to: 155-167
shubham1g5
approved these changes
Dec 4, 2025
OrangeAndGreen
approved these changes
Dec 4, 2025
conroy-ricketts
deleted the
no-ticket/rename-personal-id-error-handler
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Technical Summary
There is no ticket for these changes. This is a quick, simple refactor based on this feedback which I thought was a good idea.
For context, the idea is that we want
PersonalIdApiErrorHandlerto be more ambiguous in naming to include the Connect feature.Note that I also moved
PersonalIdApiErrorHandlerout of theconnectIdpackage because it is not just for Personal ID.Also, for some reason, doing the refactor caused Android Studio to automatically optimize imports in various different classes.
Safety Assurance
Safety story
I did a quick general smoke test of both Personal ID and Connect to make sure things are generally still working properly, but these changes should have no impact on either the user or the app's functionality.
QA Plan
QA is not needed for this as it is a simple refactor.