Backmerge Commcare 2.60.0 #3372
Conversation
Hide Notifications behind a feature flag
…s we are using both data types in different places in code
Fixes crash on clicking View Job Status
…er in the workflow.
Job Status Fix
Remove messaging from app bar
Correct error parsing for sub code
…-issue Fix FCMMssageData serialization issue
Log exceptions when stop tracing
…ig page. Fixed issue where fingerprint option could be shown when device doesn't have a fingerprint scanner.
… errorBody for all apis
Fixed biometric config options when fingerprint not available
Added analytics event for PersonalID account lockout (on Backup Code …
…estionview Improve logging around scrolling in QuestionsView
|
@damagatchi retest this please |
📝 WalkthroughWalkthroughThis pull request introduces several interconnected changes across the Connect feature, network error handling, and Personal ID biometric verification flows. The manifest version is downgraded from 2.61 to 2.60.0. The messaging menu item is removed from the Connect menu. Localization strings are updated across multiple locales to standardize "PersonalID" capitalization and add separate unlock guidance messages for PIN and fingerprint methods. Network error handling is systematically refactored to expose error streams as strings through a new Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant API as Network Layer
participant Callback as IApiCallback
participant Handler as Error Handler
Client->>API: Make Request
API->>API: Receive Error Response
API->>API: Extract Error Body (InputStream → String)
API->>API: Parse Error Codes from String
API->>Callback: processFailure(responseCode, url, errorBody)
Callback->>Handler: Handle specific error code
Handler-->>Client: Error Resolution/Retry
sequenceDiagram
participant Fragment as PersonalIdBiometricConfigFragment
participant UI as UI Elements
Fragment->>Fragment: updateUiBasedOnMinSecurityRequired()
Fragment->>Fragment: Determine button visibility<br/>(fingerprint/PIN config status)
Fragment->>Fragment: Select message based on<br/>hardware availability
Fragment->>Fragment: Compute title based on<br/>active buttons
Fragment->>UI: Bind title, message, button states
UI-->>Fragment: UI Updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR contains heterogeneous changes distributed across network callbacks, UI fragments, localization resources, and database models. While the localization updates are repetitive (reducing cognitive load), the systematic refactoring of error handling signatures across multiple API callback implementations and activities requires careful verification that all call sites are updated consistently. The feature flag introduction and UI refactoring in Possibly related PRs
Suggested labels
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/org/commcare/services/FCMMessageData.java (1)
116-131: Add serialization versioning and migration code to prevent data loss when deserializing persisted FCMMessageData objects.The new
actionfield added towriteExternal(line 127) creates a breaking change.FCMMessageDatais persisted inSharedPreferencesviaHiddenPreferences.setPendingSyncRequest(), and old serialized data lacks theactionfield. When users upgrade and the app attempts to deserialize old data,readExternalat line 118 will read the wrong fields (attempting to deserializeusernameas theactionfield), causing data corruption or crashes ingetPendingSyncRequest()which has no error handling.Required fixes:
- Add a version field to
readExternal/writeExternal(written first, checked first during deserialization)- Implement conditional logic in
readExternalto handle both old (4-field) and new (5-field) formats- When reading old data, derive
actionfromactionTypeor set a default valueapp/src/org/commcare/connect/network/ConnectNetworkHelper.java (1)
143-146: Don’t log raw error bodies; fix typo and standardize logging.Use NetworkUtils.logFailedResponse for consistency and to avoid dumping raw payloads.
- } else if (response.errorBody() != null) { - String error = response.errorBody().string(); - Logger.log("Netowrk Error", error); - } + } else if (response.errorBody() != null) { + String error = response.errorBody().string(); + NetworkUtils.logFailedResponse(response.message(), responseCode, url, error); + }
♻️ Duplicate comments (3)
app/src/org/commcare/activities/InstallFromListActivity.java (1)
327-329: Same unusederrorStreamparameter pattern.Like PostRequestActivity.java, the
errorStreamparameter is unused. This appears to be a consistent pattern across the error handling refactor.app/src/org/commcare/activities/QueryRequestActivity.java (1)
176-178: UnusederrorStreamparameter (duplicate pattern).Same observation as PostRequestActivity.java and InstallFromListActivity.java.
app/src/org/commcare/network/GetAndParseActor.java (1)
83-85: UnusederrorStreamparameter (duplicate pattern).Consistent with the pattern in activity files.
🧹 Nitpick comments (10)
app/src/org/commcare/views/QuestionsView.java (1)
362-373: Helper method correctly builds diagnostic information.The null handling and inclusion of parent class information will be helpful for debugging.
Minor formatting: For consistency, consider spacing around the concatenation operator at line 369:
- focusedViewClassName += "/"+ focusedView.getParent().getClass(); + focusedViewClassName += "/" + focusedView.getParent().getClass();app/src/org/commcare/connect/database/JobStoreManager.java (1)
104-107: Good defensive state management.This guard correctly prevents job status from regressing during updates, which is essential for maintaining state machine integrity. The logic assumes higher status values represent more advanced states, which is a standard pattern for status enumerations.
Consider adding logging when status downgrades are prevented to aid debugging:
if(existingJob.getStatus() > job.getStatus()) { //Status should never go backwards + Logger.log(LogTypes.TYPE_MAINTENANCE, + String.format("Prevented status downgrade for job %d: %d -> %d", + job.getJobId(), existingJob.getStatus(), job.getStatus())); job.setStatus(existingJob.getStatus()); }app/src/org/commcare/services/FCMMessageData.java (1)
126-127: Consider serializing onlyactioninstead of both fields.Since
actionTypeis derived fromactionvia thegetActionTypemethod (lines 100-113), serializing both fields is redundant. You could reduce the serialized data size by only writingactionand reconstructingactionTypeduring deserialization.Apply this diff to remove the redundancy:
@Override public void writeExternal(DataOutputStream out) throws IOException { - ExtUtil.writeString(out, actionType.toString()); ExtUtil.writeString(out, action); ExtUtil.writeString(out, username); ExtUtil.writeString(out, domain); ExtUtil.writeNumeric(out, creationTime.getMillis()); } @Override public void readExternal(DataInputStream in, PrototypeFactory pf) throws IOException, DeserializationException { - actionType = ActionTypes.valueOf(ExtUtil.readString(in)); action = ExtUtil.readString(in); + actionType = getActionType(action); username = ExtUtil.readString(in); domain = ExtUtil.readString(in); creationTime = new DateTime(ExtUtil.readLong(in)); }app/src/org/commcare/connect/network/base/BaseApi.kt (1)
43-45: Consider including exception details in the error body.When stream reading fails, the error is logged but
processFailureis called with an empty error body. Consider passing exception details to help with debugging:} catch (e: IOException) { Logger.exception("Error reading response stream", e); - callback.processFailure(response.code(), endPoint, "") + callback.processFailure(response.code(), endPoint, "Error reading response: ${e.message}") }app/src/org/commcare/activities/connect/ConnectActivity.java (1)
145-147: Typo + clarity: rename local var and guard menu item lookup.- MenuItem notiificationsMenuItem = menu.findItem(R.id.action_bell); - notiificationsMenuItem.setVisible(PersonalIdFeatureFlagChecker.isFeatureEnabled(NOTIFICATIONS)); + MenuItem notificationsMenuItem = menu.findItem(R.id.action_bell); + if (notificationsMenuItem != null) { + notificationsMenuItem.setVisible(PersonalIdFeatureFlagChecker.isFeatureEnabled(NOTIFICATIONS)); + }app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
238-248: Fix method name typo and references for readability.- binding.notificationView.visibility = if (shouldShowNotiifcations()) View.VISIBLE else View.GONE + binding.notificationView.visibility = if (shouldShowNotifications()) View.VISIBLE else View.GONE @@ - private fun shouldShowNotiifcations(): Boolean { - return PersonalIdManager.getInstance().isloggedIn() && isFeatureEnabled(NOTIFICATIONS); + private fun shouldShowNotifications(): Boolean { + return PersonalIdManager.getInstance().isloggedIn() && isFeatureEnabled(NOTIFICATIONS) }app/src/org/commcare/connect/network/ConnectNetworkHelper.java (2)
359-371: Optional: log server/other errors via the same utility.For parity, consider:
- handler.processFailure(responseCode, url, ""); + NetworkUtils.logFailedResponse("", responseCode, url, ""); + handler.processFailure(responseCode, url, ""); @@ - handler.processFailure(responseCode, url, ""); + NetworkUtils.logFailedResponse("", responseCode, url, ""); + handler.processFailure(responseCode, url, "");
311-313: Pass full GET URL to the processor for accurate logging.Currently the base URL is passed; prefer getUrl.toString() so downstream logs show the full endpoint with query.
Example:
getTask.connect(getResponseProcessor(context, getUrl.toString(), authInfo instanceof AuthInfo.TokenAuth, background, handler));app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (1)
72-88: Minor: avoid writing empty failure codes to session.- try { - sessionData.setSessionFailureCode(errorCode); + try { + if (errorCode != null && !errorCode.isEmpty()) { + sessionData.setSessionFailureCode(errorCode); + }app/src/org/commcare/connect/network/NetworkUtils.kt (1)
66-66: Remove unnecessary null-safety checks on non-nullable parameters.The
endPointparameter is declared as non-nullableStringin both methods, so the elvis operators on lines 66 and 85 (endPoint ?: "...") are unnecessary and misleading.Apply this diff:
- errorMessage += " for url ${endPoint ?: "unknown url"}" + errorMessage += " for url $endPoint"And for line 85:
- errorMessage += " for url ${endPoint ?: "url not found"}" + errorMessage += " for url $endPoint"Also applies to: 85-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
app/AndroidManifest.xml(1 hunks)app/res/menu/menu_connect.xml(0 hunks)app/res/values-es/strings.xml(1 hunks)app/res/values-fr/strings.xml(1 hunks)app/res/values-hi/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/activities/InstallFromListActivity.java(1 hunks)app/src/org/commcare/activities/PostRequestActivity.java(1 hunks)app/src/org/commcare/activities/QueryRequestActivity.java(1 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(5 hunks)app/src/org/commcare/adapters/EntityStringFilterer.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java(1 hunks)app/src/org/commcare/connect/MessageManager.java(5 hunks)app/src/org/commcare/connect/database/JobStoreManager.java(1 hunks)app/src/org/commcare/connect/network/ConnectNetworkHelper.java(7 hunks)app/src/org/commcare/connect/network/IApiCallback.java(1 hunks)app/src/org/commcare/connect/network/NetworkUtils.kt(1 hunks)app/src/org/commcare/connect/network/base/BaseApi.kt(3 hunks)app/src/org/commcare/connect/network/base/BaseApiCallback.kt(2 hunks)app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java(4 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java(1 hunks)app/src/org/commcare/navdrawer/BaseDrawerController.kt(2 hunks)app/src/org/commcare/network/GetAndParseActor.java(1 hunks)app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt(2 hunks)app/src/org/commcare/services/FCMMessageData.java(1 hunks)app/src/org/commcare/tasks/DataPullTask.java(2 hunks)app/src/org/commcare/tasks/EntityLoaderTask.java(1 hunks)app/src/org/commcare/tasks/FormLoaderTask.java(1 hunks)app/src/org/commcare/tasks/ModernHttpTask.java(3 hunks)app/src/org/commcare/views/QuestionsView.java(2 hunks)
💤 Files with no reviewable changes (1)
- app/res/menu/menu_connect.xml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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: Jignesh-dimagi
PR: dimagi/commcare-android#0
File: :0-0
Timestamp: 2025-05-08T13:40:19.645Z
Learning: PR #3048 introduces a comprehensive messaging system in the Connect feature, implementing secure encryption using AES-GCM for message content, proper channel management with consent flows, and a well-designed UI separation between sent and received messages with real-time notification integration.
📚 Learning: 2025-05-22T14:32:53.133Z
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.
Applied to files:
app/res/values-es/strings.xml
📚 Learning: 2025-05-08T11:08:18.530Z
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.
Applied to files:
app/src/org/commcare/activities/connect/ConnectActivity.java
🧬 Code graph analysis (9)
app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt (1)
isFeatureEnabled(21-28)
app/src/org/commcare/connect/MessageManager.java (1)
app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
processFailure(20-53)
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (2)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(43-608)app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java (1)
AnalyticsParamValue(7-194)
app/src/org/commcare/connect/network/IApiCallback.java (1)
app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
processFailure(20-53)
app/src/org/commcare/activities/connect/ConnectActivity.java (3)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-49)app/src/org/commcare/connect/database/ConnectJobUtils.java (1)
ConnectJobUtils(25-383)app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt (1)
isFeatureEnabled(21-28)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (1)
app/src/org/commcare/utils/BiometricsHelper.java (1)
BiometricsHelper(33-258)
app/src/org/commcare/connect/network/base/BaseApi.kt (1)
app/src/org/commcare/connect/network/NetworkUtils.kt (1)
logFailedResponse(49-73)
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (2)
app/src/org/commcare/connect/network/NetworkUtils.kt (1)
getErrorCodes(35-47)app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
processFailure(20-53)
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (2)
app/src/org/commcare/connect/network/NetworkUtils.kt (2)
getErrorBody(15-26)logFailedResponse(49-73)app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
processFailure(20-53)
⏰ 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 (27)
app/AndroidManifest.xml (1)
5-5: Verify the version downgrade before merging.The
versionNameis being downgraded from "2.61" to "2.60.0" whileversionCoderemains at "106". This version downgrade is unusual and could introduce issues:
- App store systems may reject or incorrectly handle version downgrades.
- User upgrade paths may break if master is the active development branch.
- The mismatch between
versionCode(106) andversionName(2.60.0) could confuse version tracking.Confirm this is an intentional backmerge of hotfixes and ensure the version management strategy is documented. Consider whether
versionCodeshould also be adjusted for consistency with the 2.60 release timeline.app/src/org/commcare/adapters/EntityStringFilterer.java (1)
77-79: LGTM! Exception logging improves observability.This change properly surfaces tracing errors for debugging without crashing the filter operation. The logging approach is appropriate for performance monitoring code where failures should be visible but non-blocking.
app/src/org/commcare/tasks/EntityLoaderTask.java (1)
72-74: Good improvement to error visibility.Logging the exception when tracing fails to stop is a solid observability improvement. This helps diagnose potential Firebase performance monitoring issues without affecting the primary entity loading flow.
app/src/org/commcare/tasks/DataPullTask.java (2)
285-287: Good improvement to error visibility.Logging exceptions when stopping performance tracing helps surface monitoring issues without disrupting the sync flow.
335-337: Consistent error handling.This mirrors the pattern at lines 285-287, ensuring tracing stop failures are logged in both success and failure sync scenarios.
app/src/org/commcare/tasks/FormLoaderTask.java (1)
157-159: Good improvement to error visibility.Consistent with the error handling pattern applied to EntityLoaderTask and DataPullTask, this improves observability of Firebase performance monitoring issues during form loading.
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (1)
259-266: LGTM! Analytics tracking added for account lockout.The addition of
reportPersonalIdConfigurationFailureproperly tracks account lockout failures during the backup code recovery flow. The placement afterlogRecoveryResult(false)and before UI updates is appropriate, and the constantSTART_CONFIGURATION_LOCKED_ACCOUNT_FAILUREaccurately represents the failure scenario.app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java (1)
566-576: Verify the behavioral change aligns with business requirements.The logic now counts all deliveries dated today regardless of their status. This means users may be blocked from submitting additional deliveries if they've reached their daily maximum, even if some submissions are pending approval or were rejected.
This change makes the method consistent with
getDeliveryCountsPerPaymentUnit()(lines 587-602), which also doesn't filter by status.Please confirm this behavioral change is intentional. Should daily visit limits apply to all delivery attempts, or only to approved deliveries?
app/src/org/commcare/views/QuestionsView.java (2)
43-43: LGTM!Import is required for the
@NonNullannotation used in the new helper method below.
351-360: Good defensive logging for diagnosing resize failures.The try-catch pattern captures valuable context (dimensions and focused view) before the crash occurs. Note that since the exception is rethrown, the crash will still happen—this code adds diagnostic information to help identify the root cause.
app/res/values/strings.xml (1)
346-348: LGTM! Localization strings properly support conditional unlock method messaging.The capitalization fix to "PersonalID" ensures consistency, and the two new strings (
connect_verify_message_pinandconnect_verify_message_fingerprint) enable the UI to display context-appropriate guidance based on available unlock methods.app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (1)
133-174: Solid refactoring to support conditional unlock method UI.The two-phase approach (determine UI state → bind title/message) is well-designed:
- Lines 137-153 determine which unlock options are available and set appropriate messages
- Lines 161-170 compute the title based on visible buttons
- Lines 172-173 bind the final title and message
The logic correctly handles all combinations of fingerprint hardware presence, PIN requirements, and biometric requirements, using the new localized strings appropriately.
app/src/org/commcare/tasks/ModernHttpTask.java (3)
37-37: LGTM! Consistent field addition.The new
errorResponseDataStreamfield mirrors the existingresponseDataStreampattern and supports exposing error response bodies.
100-105: LGTM! Proper nullability handling.The
@Nullableannotation correctly indicates that this method returnsnullfor successful responses. Consumers of this API must handle the nullable return appropriately.
73-75: Fix extra whitespace before opening brace on line 73 and verify stream cleanup.The extra space in
} else {(line 73) should be corrected to} else {per Java style conventions.Regarding resource management: The
responseDataStreamanderrorResponseDataStreamare captured at lines 72 and 74, then passed toModernHttpRequester.processResponse()via aResponseStreamAccessor. The implementation ofprocessResponse()and how it manages these streams could not be located in the searchable source. Confirm that all code paths in the HTTP response processing chain properly close both success and error response streams to prevent resource leaks.app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt (2)
15-15: LGTM! Infrastructure for NOTIFICATIONS feature flag.The constant is properly defined and integrated into the feature flag check logic.
25-25: LGTM! Feature flag returns false (disabled by default).Appropriate default for a new feature being introduced via backmerge.
app/src/org/commcare/connect/network/IApiCallback.java (2)
12-12: LGTM! Error handling signature improved.The refactored signature moves to string-based error bodies and reorders parameters for better consistency (url before errorBody). This change is properly reflected across all implementations in the codebase.
16-16: LGTM! New callback method added.
processTokenRequestDeniedError()provides specific handling for token denial scenarios, improving error handling granularity.app/src/org/commcare/connect/MessageManager.java (1)
90-90: LGTM! All processFailure calls properly migrated.All five anonymous IApiCallback implementations consistently use the new signature
(responseCode, url, errorBody), aligning with the IApiCallback interface changes.Also applies to: 142-142, 194-194, 254-254, 308-308
app/src/org/commcare/connect/network/base/BaseApi.kt (1)
50-56: LGTM! Proper error handling with NetworkUtils.The refactoring correctly:
- Extracts error body using NetworkUtils
- Logs failure details consistently
- Ensures stream cleanup in the finally block
app/src/org/commcare/activities/PostRequestActivity.java (1)
182-196: Clarify thaterrorStreamis part of the external interface contract and review the suggestion for optional logging.The
errorStreamparameter is indeed unused in this method. However, it's part of theHttpResponseProcessorinterface signature from the externalorg.commcare.corelibrary, which all implementations must follow. The same parameter goes unused across all five implementations in the codebase (GetAndParseActor, QueryRequestActivity, PostRequestActivity, InstallFromListActivity, and ConnectNetworkHelper).If you want to leverage the error stream for enhanced debugging, the suggestion to extract and log diagnostic information is reasonable but optional. Otherwise, this can be left as-is since it's a required interface method signature.
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1)
329-348: Good: client error now extracts and forwards errorBody.Error stream parsing + structured logging + callback propagation look correct.
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (1)
62-67: Error-body routing looks correct.Parses errorBody once and delegates to specific handlers; falls back to base handling when unknown.
app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
20-53: API alignment confirmed—all implementations and call sites use the 3-arg form.Verification shows the
processFailuremethod signature is consistently implemented across all classes (BaseApiCallback, MessageManager, PersonalIdApiHandler) and all call sites pass exactly three arguments (responseCode, url, errorBody) in the correct positions. The interface contract in IApiCallback.java is properly defined and followed throughout the codebase.app/src/org/commcare/activities/connect/ConnectActivity.java (1)
100-109: Review comment is incorrect and contradicts the team's established fail-fast philosophy.The review suggests adding defensive try-catch and null-checks, but the retrieved learnings from prior Connect feature PRs explicitly state the team prefers to let code crash for data contract violations.
Learning 2 directly addresses this: "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"
The current code at line 113 already follows this philosophy with
Objects.requireNonNull(job), and PaymentAcknowledgeReceiver.java line 35 demonstrates the same pattern elsewhere:Integer.parseInt(opportunityId)without try-catch.The code as-is correctly enforces data contracts and allows failures to propagate immediately.
Likely an incorrect or invalid review comment.
app/src/org/commcare/connect/network/NetworkUtils.kt (1)
76-91: Good error categorization approach.The method appropriately categorizes network errors by throwable type and logs them consistently. The logging pattern on line 90 is correct (unlike line 72 in the previous method).
| <string name="connect_verify_message">Configura un método a continuación para desbloquear tanto tu dispositivo como tu cuenta de PersonalID</string> | ||
| <string name="connect_verify_message_pin">Configure un PIN o patrón a continuación para desbloquear su dispositivo y su cuenta PersonalID</string> | ||
| <string name="connect_verify_message_fingerprint">Configure su huella digital a continuación para desbloquear tanto su dispositivo como su cuenta PersonalID</string> |
There was a problem hiding this comment.
Align imperative tone in Spanish localization.
Most Spanish strings in this screen use the informal imperative (“Configura”), but Lines 213-214 switch to the formal “Configure,” breaking consistency. Please keep all three sentences in the same voice (e.g., “Configura…”).
- <string name="connect_verify_message_pin">Configure un PIN o patrón a continuación para desbloquear su dispositivo y su cuenta PersonalID</string>
- <string name="connect_verify_message_fingerprint">Configure su huella digital a continuación para desbloquear tanto su dispositivo como su cuenta PersonalID</string>
+ <string name="connect_verify_message_pin">Configura un PIN o patrón a continuación para desbloquear tu dispositivo y tu cuenta PersonalID</string>
+ <string name="connect_verify_message_fingerprint">Configura tu huella digital a continuación para desbloquear tanto tu dispositivo como tu cuenta PersonalID</string>🤖 Prompt for AI Agents
In app/res/values-es/strings.xml around lines 212 to 214, the three strings on
this screen mix informal ("Configura") and formal ("Configure") imperatives;
change the two formal occurrences to the informal form to match the rest of the
UI. Update the values for connect_verify_message_pin and
connect_verify_message_fingerprint to use "Configura" and keep the rest of the
phrasing identical (i.e., "Configura un PIN o patrón a continuación..." and
"Configura su huella digital a continuación...") so all three strings share the
same informal imperative tone.
| fun getErrorBody(stream: InputStream?): String { | ||
| try { | ||
| if (stream != null) { | ||
| val errorBytes = StreamsUtil.inputStreamToByteArray(stream) | ||
| return String(errorBytes, StandardCharsets.UTF_8) | ||
| } | ||
| } catch (e: Exception) { | ||
| Logger.exception("Error parsing error_code", e); | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Critical: Resource leak - stream is never closed.
The InputStream is read but never closed, which will leak file descriptors and potentially exhaust system resources over repeated failures.
Apply this diff to close the stream in a finally block:
@JvmStatic
fun getErrorBody(stream: InputStream?): String {
+ var errorBytes: ByteArray? = null
try {
if (stream != null) {
- val errorBytes = StreamsUtil.inputStreamToByteArray(stream)
+ errorBytes = StreamsUtil.inputStreamToByteArray(stream)
- return String(errorBytes, StandardCharsets.UTF_8)
}
} catch (e: Exception) {
Logger.exception("Error parsing error_code", e);
+ } finally {
+ StreamsUtil.closeStream(stream)
}
- return ""
+ return if (errorBytes != null) String(errorBytes, StandardCharsets.UTF_8) else ""
}🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/NetworkUtils.kt around lines 16 to 26,
the InputStream passed to getErrorBody is read but never closed causing a
resource leak; update the function to ensure the stream is closed after reading
— either wrap the read in Kotlin's use { } block on the InputStream or add a
try/finally that closes stream in the finally block (safely checking for null
and swallowing/ logging any IOException on close) so the bytes are returned as
before but the InputStream is always closed.
| } catch (e: Exception) { | ||
| Logger.exception("Error parsing error_code", e); |
There was a problem hiding this comment.
Misleading exception message and overly broad exception handling.
The exception message says "Error parsing error_code" but this method reads the error body stream, not parses error codes. Additionally, catching generic Exception is too broad and may hide specific I/O failures.
Apply this diff:
- } catch (e: Exception) {
- Logger.exception("Error parsing error_code", e);
+ } catch (e: IOException) {
+ Logger.exception("Error reading error response body", e);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (e: Exception) { | |
| Logger.exception("Error parsing error_code", e); | |
| } catch (e: IOException) { | |
| Logger.exception("Error reading error response body", e); |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/NetworkUtils.kt around lines 22-23,
replace the overly broad catch of Exception and the misleading message "Error
parsing error_code" with a catch for the specific I/O exception thrown when
reading the error body (e.g., IOException). Update the Logger call to a correct
message such as "Error reading error body" and log the caught IOException; do
not swallow other exceptions—either rethrow them or allow them to propagate so
higher layers can handle them.
| /** | ||
| * Extracts error_code and error_sub_code from a JSON error response body. | ||
| * If the stream is null or parsing fails, returns empty strings for both codes. | ||
| * | ||
| * @param stream InputStream of the error response body | ||
| * @return Pair of error_code and error_sub_code | ||
| */ | ||
| @JvmStatic | ||
| fun getErrorCodes(errorBody: String): Pair<String, String> { | ||
| var errorCode = "" | ||
| var errorSubCode = "" | ||
| try { | ||
| val json = JSONObject(errorBody) | ||
| errorCode = json.optString("error_code", ""); | ||
| errorSubCode = json.optString("error_sub_code", ""); | ||
| } catch (e: Exception) { | ||
| Logger.exception("Error parsing error_code", e); | ||
| } | ||
| return Pair(errorCode, errorSubCode) | ||
| } |
There was a problem hiding this comment.
Fix incorrect KDoc parameter and refine exception handling.
The KDoc at line 32 references @param stream InputStream but the actual parameter is errorBody: String. Additionally, catching generic Exception should be narrowed to JSONException for clarity.
Apply this diff:
/**
* Extracts error_code and error_sub_code from a JSON error response body.
* If the stream is null or parsing fails, returns empty strings for both codes.
*
- * @param stream InputStream of the error response body
+ * @param errorBody String containing the JSON error response body
* @return Pair of error_code and error_sub_code
*/
@JvmStatic
fun getErrorCodes(errorBody: String): Pair<String, String> {
var errorCode = ""
var errorSubCode = ""
try {
val json = JSONObject(errorBody)
errorCode = json.optString("error_code", "");
errorSubCode = json.optString("error_sub_code", "");
- } catch (e: Exception) {
+ } catch (e: org.json.JSONException) {
Logger.exception("Error parsing error_code", e);
}
return Pair(errorCode, errorSubCode)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Extracts error_code and error_sub_code from a JSON error response body. | |
| * If the stream is null or parsing fails, returns empty strings for both codes. | |
| * | |
| * @param stream InputStream of the error response body | |
| * @return Pair of error_code and error_sub_code | |
| */ | |
| @JvmStatic | |
| fun getErrorCodes(errorBody: String): Pair<String, String> { | |
| var errorCode = "" | |
| var errorSubCode = "" | |
| try { | |
| val json = JSONObject(errorBody) | |
| errorCode = json.optString("error_code", ""); | |
| errorSubCode = json.optString("error_sub_code", ""); | |
| } catch (e: Exception) { | |
| Logger.exception("Error parsing error_code", e); | |
| } | |
| return Pair(errorCode, errorSubCode) | |
| } | |
| /** | |
| * Extracts error_code and error_sub_code from a JSON error response body. | |
| * If the stream is null or parsing fails, returns empty strings for both codes. | |
| * | |
| * @param errorBody String containing the JSON error response body | |
| * @return Pair of error_code and error_sub_code | |
| */ | |
| @JvmStatic | |
| fun getErrorCodes(errorBody: String): Pair<String, String> { | |
| var errorCode = "" | |
| var errorSubCode = "" | |
| try { | |
| val json = JSONObject(errorBody) | |
| errorCode = json.optString("error_code", ""); | |
| errorSubCode = json.optString("error_sub_code", ""); | |
| } catch (e: org.json.JSONException) { | |
| Logger.exception("Error parsing error_code", e); | |
| } | |
| return Pair(errorCode, errorSubCode) | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/NetworkUtils.kt around lines 28 to 47,
the KDoc incorrectly documents a non-existent InputStream parameter and the
catch is too broad; update the KDoc to describe the actual parameter errorBody:
String (replace the @param stream line with @param errorBody the JSON error
response as String) and narrow the exception catch to catch
org.json.JSONException (import if necessary) so only JSON parse errors are
handled; keep returning Pair(errorCode, errorSubCode).
| Logger.log( | ||
| LogTypes.TYPE_ERROR_SERVER_COMMS, | ||
| errorMessage | ||
| ) | ||
| Logger.exception(LogTypes.TYPE_ERROR_SERVER_COMMS, Throwable(errorMessage)) |
There was a problem hiding this comment.
Critical: Incorrect Logger.exception signature on line 72.
Line 72 calls Logger.exception(LogTypes.TYPE_ERROR_SERVER_COMMS, Throwable(errorMessage)), which appears to pass a log type constant as the first parameter. Comparing with line 90 (Logger.exception(errorMessage, t)), the signature should be Logger.exception(message: String, throwable: Throwable).
Apply this diff to fix the call:
Logger.log(
LogTypes.TYPE_ERROR_SERVER_COMMS,
errorMessage
)
-Logger.exception(LogTypes.TYPE_ERROR_SERVER_COMMS, Throwable(errorMessage))
+Logger.exception(errorMessage, Throwable(errorMessage))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Logger.log( | |
| LogTypes.TYPE_ERROR_SERVER_COMMS, | |
| errorMessage | |
| ) | |
| Logger.exception(LogTypes.TYPE_ERROR_SERVER_COMMS, Throwable(errorMessage)) | |
| Logger.log( | |
| LogTypes.TYPE_ERROR_SERVER_COMMS, | |
| errorMessage | |
| ) | |
| Logger.exception(errorMessage, Throwable(errorMessage)) |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/NetworkUtils.kt around lines 68-72, the
Logger.exception call uses the wrong parameter order; change it to call
Logger.exception(message: String, throwable: Throwable). Replace the existing
call so the first argument is errorMessage and the second is the throwable
(e.g., Logger.exception(errorMessage, t) if a caught exception variable t
exists, otherwise Logger.exception(errorMessage, Throwable(errorMessage))).
| private fun updateNotificationsIcon() { | ||
| if(shouldShowNotiifcations()) { | ||
| val notifications = NotificationRecordDatabaseHelper().getAllNotifications(activity) | ||
| val hasUnreadNotification = notifications!!.any { !it.readStatus } | ||
|
|
||
| binding.ivNotification.setImageResource( | ||
| if (hasUnreadNotification) R.drawable.ic_new_notification_bell | ||
| else R.drawable.ic_bell | ||
| ) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Unread bell never updates: helper is never invoked.
updateNotificationsIcon() isn’t called; the bell stays at default.
Call it after populating the signed‑in UI (e.g., end of the true branch in refreshDrawerContent):
// after navDrawerAdapter.refreshList(items)
updateNotificationsIcon()Search for any existing calls:
🏁 Script executed:
#!/bin/bash
rg -n 'updateNotificationsIcon\('Length of output: 102
I need to verify the broader context and confirm the suggested fix location is appropriate.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find refreshDrawerContent definition and surrounding context
rg -n -A15 'fun refreshDrawerContent\(' app/src/org/commcare/navdrawer/BaseDrawerController.kt
Length of output: 799
🏁 Script executed:
#!/bin/bash
# Search for any notification-related updates or listeners in the file
rg -n 'notification|Notification' app/src/org/commcare/navdrawer/BaseDrawerController.kt -iLength of output: 946
🏁 Script executed:
#!/bin/bash
# Find navDrawerAdapter.refreshList call location in refreshDrawerContent
rg -n 'navDrawerAdapter\.refreshList' app/src/org/commcare/navdrawer/BaseDrawerController.ktLength of output: 51
Add missing call to updateNotificationsIcon() in refreshDrawerContent().
The function updateNotificationsIcon() is properly implemented but never invoked, leaving the notification bell in its default state. Add the call after navDrawerAdapter.refreshList(items) on line 216:
navDrawerAdapter.refreshList(items)
updateNotificationsIcon()🤖 Prompt for AI Agents
In app/src/org/commcare/navdrawer/BaseDrawerController.kt around lines 222 to
233, updateNotificationsIcon() is implemented but never called; modify
refreshDrawerContent() to invoke updateNotificationsIcon() immediately after the
navDrawerAdapter.refreshList(items) call (i.e., insert a call to
updateNotificationsIcon() on the line following
navDrawerAdapter.refreshList(items)) so the notification bell updates when the
drawer content is refreshed.
| val notifications = NotificationRecordDatabaseHelper().getAllNotifications(activity) | ||
| val hasUnreadNotification = notifications!!.any { !it.readStatus } | ||
|
|
||
| binding.ivNotification.setImageResource( | ||
| if (hasUnreadNotification) R.drawable.ic_new_notification_bell | ||
| else R.drawable.ic_bell | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid NPE on notifications list; use safe check.
- if(shouldShowNotiifcations()) {
- val notifications = NotificationRecordDatabaseHelper().getAllNotifications(activity)
- val hasUnreadNotification = notifications!!.any { !it.readStatus }
+ if (shouldShowNotifications()) {
+ val notifications = NotificationRecordDatabaseHelper().getAllNotifications(activity)
+ val hasUnreadNotification = notifications?.any { !it.readStatus } == trueCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/navdrawer/BaseDrawerController.kt around lines 224 to
231, the code assumes
NotificationRecordDatabaseHelper().getAllNotifications(activity) returns
non-null and uses !! which can cause an NPE; update the call to use a null-safe
pattern (e.g., val notifications =
NotificationRecordDatabaseHelper().getAllNotifications(activity) ?: emptyList())
and then compute hasUnreadNotification with notifications.any { !it.readStatus }
so the null case is treated as empty and no force-unwrapping is used; set the
notification icon based on that result.
📝 WalkthroughWalkthroughThis PR implements multiple interconnected updates to the CommCare Android application, including a version downgrade (2.61 → 2.60.0), removal of the messaging menu item from ConnectActivity, and addition of PIN and fingerprint authentication verification message strings across multiple locales (Spanish, French, Hindi, Portuguese, Swahili, Tigrinya, and English). The PR refactors network error-handling infrastructure by updating method signatures across IApiCallback, BaseApi, and related classes to propagate error bodies as strings rather than InputStreams. It introduces a new NetworkUtils singleton for centralized error body parsing and logging. UI logic for PersonalIdBiometricConfigFragment is consolidated to streamline fingerprint/PIN option determination. A NOTIFICATIONS feature flag is added to PersonalIdFeatureFlagChecker and integrated into ConnectActivity and BaseDrawerController. Job status preservation logic prevents status regression during updates. Exception logging is enhanced in multiple task classes, and FCMMessageData serialization is corrected to persist both actionType and action fields. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR spans 30+ files with heterogeneous changes including method signature updates across multiple API layers, significant UI refactoring in PersonalIdBiometricConfigFragment, new networking infrastructure (NetworkUtils), feature flag integration, and localization updates. The changes are not repetitive but require separate reasoning for error-handling flow updates, UI logic consolidation, and feature-flag propagation across multiple components. Possibly related PRs
Suggested labels
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 |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
app/src/org/commcare/connect/network/base/BaseApi.kt (1)
39-46: Also log the success-body read failure.If reading the success body fails, log as a failed response for parity with non-2xx errors before delegating to processFailure.
Apply this diff:
} catch (e: IOException) { - Logger.exception("Error reading response stream", e); - // Handle error when reading the stream - callback.processFailure(response.code(), endPoint, "") + Logger.exception("Error reading response stream", e) + // Log as failed response for consistency + logFailedResponse("Error reading response stream", response.code(), endPoint, "") + callback.processFailure(response.code(), endPoint, "") }app/src/org/commcare/activities/connect/ConnectActivity.java (1)
74-82: Guard against missing job when GO_TO_JOB_STATUS is true.handleInfoRedirect() requires a non-null job; add a fallback to avoid NPE if extras didn’t resolve a job.
Apply this diff:
- if (getIntent().getBooleanExtra(GO_TO_JOB_STATUS, false)) { - startDestinationId = handleInfoRedirect(startArgs); + if (getIntent().getBooleanExtra(GO_TO_JOB_STATUS, false)) { + if (job == null) { + org.javarosa.core.services.Logger.exception( + "GO_TO_JOB_STATUS set but no job resolved from extras", + new Exception("Missing job")); + return startDestinationId; + } + startDestinationId = handleInfoRedirect(startArgs);app/src/org/commcare/connect/network/ConnectNetworkHelper.java (2)
329-349: Close errorStream to prevent resource leaks.errorStream is read but never closed; this can exhaust connections/file descriptors under load.
Apply this diff:
- } else { - String errorBody = NetworkUtils.getErrorBody(errorStream); - NetworkUtils.logFailedResponse("", responseCode, url, errorBody); - handler.processFailure(responseCode, url, errorBody); - } + } else { + String errorBody = ""; + try { + errorBody = NetworkUtils.getErrorBody(errorStream); + } finally { + StreamsUtil.closeStream(errorStream); + } + NetworkUtils.logFailedResponse("", responseCode, url, errorBody); + handler.processFailure(responseCode, url, errorBody); + }
192-213: Bug: headers map not populated for form-encoded POST.Reassigning outputHeaders breaks propagation to the caller; version/content headers are lost.
Apply this diff:
if (useFormEncoding) { Multimap<String, String> multimap = ArrayListMultimap.create(); for (Map.Entry<String, Object> entry : params.entrySet()) { multimap.put(entry.getKey(), entry.getValue().toString()); } requestBody = ModernHttpRequester.getPostBody(multimap); - outputHeaders = getContentHeadersForXFormPost(requestBody); + HashMap<String, String> contentHeaders = getContentHeadersForXFormPost(requestBody); + outputHeaders.putAll(contentHeaders); } else {app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (2)
43-58: Fix double-callback and stream leak in processSuccess; use UTF‑8; crash on JSON contract breaches
- onSuccess is called even after parse failures (after onFailure) → double callback.
- When parser is null, responseData isn’t closed → resource leak.
- Use UTF‑8 when constructing the JSON string.
- Per team convention, JSONException should crash (contract bug), not be handled gracefully. Based on learnings.
@@ - public void processSuccess(int responseCode, InputStream responseData) { - if (parser != null) { - try (InputStream in = responseData) { - JSONObject json = new JSONObject(new String(StreamsUtil.inputStreamToByteArray(in))); - parser.parse(json, sessionData); - } catch (JSONException e) { - Logger.exception("JSON error parsing API response", e); - onFailure(PersonalIdOrConnectApiErrorCodes.JSON_PARSING_ERROR, e); - } catch (IOException e) { - Logger.exception("Error parsing API response", e); - onFailure(PersonalIdOrConnectApiErrorCodes.NETWORK_ERROR, e); - } - } - onSuccess((T)sessionData); - } + public void processSuccess(int responseCode, InputStream responseData) { + try (InputStream in = responseData) { + if (parser != null) { + JSONObject json = new JSONObject( + new String(StreamsUtil.inputStreamToByteArray(in), java.nio.charset.StandardCharsets.UTF_8)); + parser.parse(json, sessionData); + } + onSuccess((T) sessionData); + } catch (JSONException e) { + Logger.exception("JSON error parsing API response", e); + throw new RuntimeException("JSON contract violation parsing API response", e); + } catch (IOException e) { + Logger.exception("Error parsing API response", e); + onFailure(PersonalIdOrConnectApiErrorCodes.NETWORK_ERROR, e); + } + }Also add:
+import java.nio.charset.StandardCharsets;(Based on learnings)
90-97: Add createCallback overload for parser-first argument patternThe four call sites at lines 96, 136, 151, and 156 pass arguments in reversed order relative to the existing method signature. The current
createCallback(PersonalIdSessionData, PersonalIdApiResponseParser)method expects session data first, but these calls provide the parser first.Adding an overload
createCallback(PersonalIdApiResponseParser<T>, T)resolves the mismatch and provides proper handling for non-session flows:private IApiCallback createCallback(PersonalIdSessionData sessionData, PersonalIdApiResponseParser parser) { return new BaseApiCallback<T>(this) { ... }; } + + // Overload for non-session flows (e.g., ConnectUserRecord, requestId, or null). + private IApiCallback createCallback(final PersonalIdApiResponseParser<T> parser, final T data) { + return new BaseApiCallback<T>(this) { + @Override + public void processSuccess(int responseCode, InputStream responseData) { + try (InputStream in = responseData) { + if (parser != null) { + JSONObject json = new JSONObject( + new String(StreamsUtil.inputStreamToByteArray(in), java.nio.charset.StandardCharsets.UTF_8)); + parser.parse(json, data); + } + onSuccess(data); + } catch (JSONException e) { + Logger.exception("JSON error parsing API response", e); + throw new RuntimeException("JSON contract violation parsing API response", e); + } catch (IOException e) { + Logger.exception("Error parsing API response", e); + onFailure(PersonalIdOrConnectApiErrorCodes.NETWORK_ERROR, e); + } + } + + @Override + public void processFailure(int responseCode, String url, String errorBody) { + // No session-aware error-code handling for generic flows + super.processFailure(responseCode, url, errorBody); + } + }; + }
🧹 Nitpick comments (10)
app/src/org/commcare/adapters/EntityStringFilterer.java (1)
77-79: Good addition of exception logging for observability.This change improves debugging by surfacing errors that were previously silently ignored when stopping Firebase Performance Monitoring traces. Since tracing is a non-critical monitoring concern, catching and logging the exception while allowing execution to continue is appropriate.
Optional formatting nit: The log message has a trailing space (
"Failed to stop tracing "). Consider removing it for consistency:- Logger.exception("Failed to stop tracing ", e); + Logger.exception("Failed to stop tracing", e);app/src/org/commcare/views/QuestionsView.java (1)
362-373: Use consistent approach for class name extraction.Line 367 explicitly calls
.toString()on the class object, but line 369 relies on implicit toString() via string concatenation. For consistency and clarity, use the same approach for both.Additionally, line 369 has inconsistent spacing in the concatenation operator.
Apply this diff for consistency:
@NonNull private String getFocusedViewClassName() { View focusedView = findFocus(); String focusedViewClassName = "None"; if (focusedView != null) { focusedViewClassName = focusedView.getClass().toString(); if (focusedView.getParent() != null) { - focusedViewClassName += "/"+ focusedView.getParent().getClass(); + focusedViewClassName += "/" + focusedView.getParent().getClass().toString(); } } return focusedViewClassName; }app/src/org/commcare/connect/database/JobStoreManager.java (1)
104-107: Add logging when status is preserved to prevent regression.The status preservation logic is a good defensive measure, but it operates silently. If the server sends a lower status value, this could indicate a data inconsistency or bug that should be investigated.
Apply this diff to add observability:
if(existingJob.getStatus() > job.getStatus()) { //Status should never go backwards + Logger.log("JobStoreManager", "Preventing status downgrade for job " + + job.getJobId() + " from " + existingJob.getStatus() + + " to " + job.getStatus()); job.setStatus(existingJob.getStatus()); }app/src/org/commcare/tasks/ModernHttpTask.java (2)
37-37: Stream ownership/closure: avoid leaked connectionsHolding another InputStream increases the risk of not closing ResponseBody/errorBody. Please clarify ownership and close both success/error streams (and bodies) after processing to prevent OkHttp connection leaks.
Example patch (only if not already closed downstream):
@@ - ModernHttpRequester.processResponse( + try { + ModernHttpRequester.processResponse( httpResponseProcessor, mResponse.code(), new ResponseStreamAccessor() { ... - }); + }); + } finally { + closeQuietly(responseDataStream); + closeQuietly(errorResponseDataStream); + if (mResponse != null) { + ResponseBody body = mResponse.body(); + if (body != null) body.close(); + ResponseBody errorBody = mResponse.errorBody(); + if (errorBody != null) errorBody.close(); + } + } @@ + private static void closeQuietly(@Nullable InputStream in) { + if (in != null) { + try { in.close(); } catch (IOException ignored) {} + } + }Based on learnings.
73-75: Do error-body parsing off the UI threaddeliverResult runs on the main thread. Ensure error stream parsing happens in doTaskBackground (or is pre-buffered) to avoid UI jank. Optionally pre-buffer to bytes/string there and pass a ByteArrayInputStream (with a reasonable cap).
Example (illustrative):
@@ - errorResponseDataStream = requester.getErrorResponseStream(mResponse); + ResponseBody eb = mResponse.errorBody(); + if (eb != null) { + byte[] bytes = eb.bytes(); // worker thread + errorResponseDataStream = new java.io.ByteArrayInputStream(bytes); + } else { + errorResponseDataStream = null; + }app/src/org/commcare/activities/connect/ConnectActivity.java (1)
131-133: Typo in local variable name.Rename notiificationsMenuItem → notificationsMenuItem for clarity.
Apply this diff:
- MenuItem notiificationsMenuItem = menu.findItem(R.id.action_bell); - notiificationsMenuItem.setVisible(PersonalIdFeatureFlagChecker.isFeatureEnabled(NOTIFICATIONS)); + MenuItem notificationsMenuItem = menu.findItem(R.id.action_bell); + notificationsMenuItem.setVisible(PersonalIdFeatureFlagChecker.isFeatureEnabled(NOTIFICATIONS));app/src/org/commcare/connect/network/ConnectNetworkHelper.java (2)
144-146: Typo in log tag.“Netowrk” → “Network”.
Apply this diff:
- Logger.log("Netowrk Error", error); + Logger.log("Network Error", error);
374-381: Unify IO error logging with NetworkUtils.Leverage centralized logging for consistency.
Apply this diff:
- } else { - Logger.exception("IO Exception during API call", exception); - handler.processFailure(-1, url, ""); - } + } else { + NetworkUtils.logNetworkError(exception, url); + handler.processFailure(-1, url, ""); + }app/src/org/commcare/connect/network/NetworkUtils.kt (1)
66-73: Nit: endpoint is non-null.Elvis on a non-null parameter is redundant. Optional clean-up if you touch this next.
Also applies to: 85-91
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (1)
61-65: processFailure: guard for non-JSON bodies and nulls; keep logs quietgetErrorCodes() logs exceptions when bodies aren’t JSON. That’s fine, but consider short-circuiting when errorBody is empty to avoid noisy logs on HTML/text errors; pass a non-null string to Kotlin BaseApiCallback to avoid boundary NPEs.
- Pair<String, String> errorCodes = getErrorCodes(errorBody); + Pair<String, String> errorCodes = getErrorCodes(errorBody == null ? "" : errorBody); if (!handleErrorCodeIfPresent(errorCodes.getFirst(), errorCodes.getSecond(), sessionData)) { super.processFailure(responseCode, url, errorBody); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
app/AndroidManifest.xml(1 hunks)app/res/menu/menu_connect.xml(0 hunks)app/res/values-es/strings.xml(1 hunks)app/res/values-fr/strings.xml(1 hunks)app/res/values-hi/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/activities/InstallFromListActivity.java(1 hunks)app/src/org/commcare/activities/PostRequestActivity.java(1 hunks)app/src/org/commcare/activities/QueryRequestActivity.java(1 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(5 hunks)app/src/org/commcare/adapters/EntityStringFilterer.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java(1 hunks)app/src/org/commcare/connect/MessageManager.java(5 hunks)app/src/org/commcare/connect/database/JobStoreManager.java(1 hunks)app/src/org/commcare/connect/network/ConnectNetworkHelper.java(7 hunks)app/src/org/commcare/connect/network/IApiCallback.java(1 hunks)app/src/org/commcare/connect/network/NetworkUtils.kt(1 hunks)app/src/org/commcare/connect/network/base/BaseApi.kt(3 hunks)app/src/org/commcare/connect/network/base/BaseApiCallback.kt(2 hunks)app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java(3 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java(1 hunks)app/src/org/commcare/navdrawer/BaseDrawerController.kt(3 hunks)app/src/org/commcare/network/GetAndParseActor.java(1 hunks)app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt(2 hunks)app/src/org/commcare/services/FCMMessageData.java(1 hunks)app/src/org/commcare/tasks/DataPullTask.java(2 hunks)app/src/org/commcare/tasks/EntityLoaderTask.java(1 hunks)app/src/org/commcare/tasks/FormLoaderTask.java(1 hunks)app/src/org/commcare/tasks/ModernHttpTask.java(3 hunks)app/src/org/commcare/views/QuestionsView.java(2 hunks)
💤 Files with no reviewable changes (1)
- app/res/menu/menu_connect.xml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 Learning: 2025-06-13T15:53:12.951Z
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.
Applied to files:
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
📚 Learning: 2025-05-08T11:08:18.530Z
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.
Applied to files:
app/src/org/commcare/activities/connect/ConnectActivity.java
🧬 Code graph analysis (9)
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (2)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(43-608)app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java (1)
AnalyticsParamValue(7-194)
app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt (1)
isFeatureEnabled(21-28)
app/src/org/commcare/connect/MessageManager.java (1)
app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
processFailure(20-53)
app/src/org/commcare/connect/network/base/BaseApi.kt (1)
app/src/org/commcare/connect/network/NetworkUtils.kt (1)
logFailedResponse(49-73)
app/src/org/commcare/connect/network/IApiCallback.java (1)
app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
processFailure(20-53)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (2)
app/src/org/commcare/utils/BiometricsHelper.java (1)
BiometricsHelper(33-258)app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (1)
PIN(33-35)
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (3)
app/src/org/commcare/connect/network/NetworkUtils.kt (1)
getErrorCodes(35-47)app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
processFailure(20-53)app/src/org/commcare/connect/network/base/BaseApiHandler.kt (1)
onFailure(16-16)
app/src/org/commcare/activities/connect/ConnectActivity.java (3)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-49)app/src/org/commcare/connect/database/ConnectJobUtils.java (1)
ConnectJobUtils(25-383)app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt (1)
isFeatureEnabled(21-28)
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (2)
app/src/org/commcare/connect/network/NetworkUtils.kt (2)
getErrorBody(15-26)logFailedResponse(49-73)app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
processFailure(20-53)
🔇 Additional comments (34)
app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (1)
259-266: LGTM! Good addition of telemetry for locked account tracking.The analytics call is appropriately placed to capture account lockout failures during the backup code recovery flow. The placement after logging the recovery result but before clearing fields and navigating ensures the event is captured at the right moment in the flow.
app/src/org/commcare/tasks/FormLoaderTask.java (1)
157-159: Good improvement to error visibility.Logging exceptions during trace cleanup provides valuable debugging information that was previously lost.
app/src/org/commcare/tasks/DataPullTask.java (1)
285-287: Good improvement to error visibility.Logging exceptions during trace cleanup provides valuable debugging information for both success and error paths.
Also applies to: 335-337
app/src/org/commcare/tasks/EntityLoaderTask.java (1)
72-74: Good improvement to error visibility.Logging exceptions during trace cleanup provides valuable debugging information that was previously lost. This change is consistent with similar improvements across other task classes.
app/src/org/commcare/views/QuestionsView.java (2)
43-44: LGTM!The AndroidX NonNull import is appropriate for the annotation used on the new helper method.
351-360: LGTM! Good defensive logging.The catch-log-rethrow pattern appropriately captures diagnostic information about the IllegalArgumentException while preserving the original exception behavior. This will help investigate crashes related to view resizing.
app/src/org/commcare/activities/InstallFromListActivity.java (1)
327-329: No action required — errorStream non-usage is consistent with the codebase pattern.The
errorStreamparameter is unused inInstallFromListActivity.processClientError(), but this is intentional and consistent with similar implementations (QueryRequestActivity,PostRequestActivity,GetAndParseActor). WhileConnectNetworkHelperdoes parse the error body viaNetworkUtils.getErrorBody(errorStream), it has a specialized use case requiring detailed error propagation to anIApiCallbackhandler.
InstallFromListActivity.handleRequestError()only acceptsresponseCodeand a boolean flag, reflecting simpler error handling (retry or show results). To use the error body here would require modifying the method signature, which is outside the scope of this interface compliance update.This design is appropriate for the activity's error-handling strategy.
app/AndroidManifest.xml (1)
5-5: Version name alignment looks goodMatching the manifest versionName to 2.60.0 is consistent with this back-merge of the 2.60 branch. No further changes needed.
app/res/values/strings.xml (1)
346-348: New unlock variants look solidAdding explicit PIN and fingerprint variants alongside the generic unlock message keeps the copy clear and matches the new flows.
app/res/values-fr/strings.xml (1)
274-276: French strings are consistentThe new PIN and fingerprint messages follow the existing tone and terminology—looks good.
app/res/values-sw/strings.xml (1)
276-278: Swahili copy reads wellThe additional strings mirror the English intent while keeping terminology consistent with the rest of the locale.
app/res/values-ti/strings.xml (1)
267-269: Tigrinya updates align with the new flowAll three messages were added with consistent terminology—looks good from my side.
app/res/values-pt/strings.xml (1)
279-281: Portuguese variants are spot onThe new PIN and fingerprint instructions are clear and match the rest of the locale.
app/res/values-hi/strings.xml (1)
274-276: Localization strings properly formatted and consistent across all locales.The Hindi localization strings are correctly updated with two new authentication verification messages (PIN and fingerprint) and capitalization standardization from "PersonalId" to "PersonalID". XML formatting is valid, and verification confirms all corresponding strings exist consistently across all locale files (English, Spanish, French, Portuguese, Swahili, and Tigrinya).
app/src/org/commcare/services/FCMMessageData.java (1)
123-129: Serialization logic is now correct, but note the breaking change.The fix properly serializes both
actionType(as enum string) andaction(as raw string), which aligns with the class design where both fields serve different purposes:
actionType: parsed enum for programmatic useaction: original raw string from payloadThe serialization order correctly matches the deserialization order in
readExternal.However, this format change is incompatible with previously serialized data (see comment on
readExternal).app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java (1)
566-576: Status filter removal verified — confirm this change is intentional.The STATUS_APPROVED filter has been confirmed removed from the
numberOfDeliveriesToday()method. In commit 2689b7c, it originally containedrecord.getStatus().equals(STATUS_APPROVED), but the current code no longer includes this condition.This changes the behavior at line 626: all deliveries from today (approved, pending, rejected) now count toward the daily visit limit, whereas previously only approved deliveries counted. This could prevent users from submitting new deliveries if they have pending or rejected deliveries from today.
Confirm whether this change in counting logic is intentional or if the status filter should be retained.
app/src/org/commcare/tasks/ModernHttpTask.java (1)
100-105: Confirm getResponseStream() nullability annotation and verify external interface contractThe anonymous ResponseStreamAccessor implementation at line 94 is the only implementer in this codebase and properly implements
getErrorResponseStream()with@Nullableannotation. However, there's an asymmetry:getErrorResponseStream()is annotated@NullablebutgetResponseStream()is not, despite both fields being initialized to null and conditionally populated. If error responses (non-2xx, 204, HEAD) causegetResponseStream()to be called, it could return null and should also be annotated@Nullablefor consistency.Since
ResponseStreamAccessoris from the externalorg.commcare.core.interfaceslibrary, verify:
- The interface contract for both methods' nullability
- Whether other implementers exist outside this codebase
- Whether
getResponseStream()requires the@Nullableannotation to match its actual behaviorThe
javax.annotation.Nullableimport is consistent with the file's convention.app/src/org/commcare/activities/QueryRequestActivity.java (1)
176-178: Signature updated to match interface contract.The
processClientErrormethod signature now accepts anInputStream errorStreamparameter to align with the broader error-handling refactor across the codebase. Note thaterrorStreamis currently unused in this implementation.app/src/org/commcare/connect/network/IApiCallback.java (1)
12-12: Interface signature updated to use String errorBody.The
processFailuremethod signature has been updated from(int, @Nullable InputStream, String)to(int, String, String), replacing the InputStream-based error handling with a String-based approach. This aligns with the broader network error-handling refactor across the codebase.app/src/org/commcare/activities/PostRequestActivity.java (1)
182-196: Signature updated to match interface contract.The
processClientErrormethod signature now includes anInputStream errorStreamparameter to align with theHttpResponseProcessorinterface update. The parameter is currently unused as the implementation relies solely on the response code for error messaging.app/src/org/commcare/network/GetAndParseActor.java (1)
83-85: Signature updated to match interface contract.The
processClientErroroverride now accepts anInputStream errorStreamparameter to comply with the updatedHttpResponseProcessorinterface. The parameter is unused as the implementation delegates toprocessErrorResponsewhich only requires the response code.app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt (2)
15-15: New NOTIFICATIONS feature flag added.The NOTIFICATIONS feature flag constant has been added to support notification-related feature toggling across the application. The implementation follows the established pattern for feature flags.
22-28: Feature flag handling implemented correctly.The
isFeatureEnabledmethod has been updated to handle the new NOTIFICATIONS flag, returningfalseto keep the feature disabled. The implementation maintains the exhaustive switch pattern with proper error handling for unknown flags.app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
20-53: Signature updated to match IApiCallback interface.The
processFailuremethod signature has been updated to accepterrorBodyas a String instead of an InputStream, aligning with the IApiCallback interface change. The error handling logic based on HTTP status codes remains unchanged, with the errorBody parameter available for future use.app/src/org/commcare/navdrawer/BaseDrawerController.kt (4)
23-25: Feature flag imports added for cleaner code.The companion functions
isFeatureEnabledand feature flag constants are now imported directly, making the code more concise compared to qualifying calls with the class name.
242-245: Refactored to use imported feature flag helper.The method now uses the imported
isFeatureEnabledfunction for cleaner, more concise code.
247-249: Helper method consolidates notification visibility logic.The new
shouldShowNotiifcationsmethod centralizes the logic for determining notification feature visibility based on login state and feature flag. Note: the typo in the method name is addressed in a previous comment.
131-145: Notification icon logic refactored to dedicated method.The notification icon update logic has been extracted to
updateNotificationsIcon(), makingrefreshDrawerContentcleaner and more maintainable.app/src/org/commcare/connect/MessageManager.java (1)
90-92: All processFailure implementations updated consistently.All five
IApiCallbackanonymous class implementations have been updated to match the new interface signature(int, String, String). Each implementation maintains its existing behavior of delegating tolistener.connectActivityComplete(false). The url and errorBody parameters are available for future error logging or detailed error handling.Also applies to: 142-144, 194-198, 254-256, 308-310
app/src/org/commcare/connect/network/base/BaseApi.kt (2)
48-56: Good: error stream parsed and deterministically closed.getErrorBody + finally-close is correct and prevents resource leaks. No change needed.
60-65: Network failure path is consistent.Logging + processNetworkFailure() is appropriate. LGTM.
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (2)
134-153: Consolidated decision tree reads well.Clear prioritization: fingerprint configured → agree; else hardware present → configure; PIN branch overlays correctly. LGTM.
161-174: Title/message normalization is sound.Good fallback to no‑hardware error; avoids partial UI states. LGTM.
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
86-92: Defensive parse: avoid NumberFormatException on OPPORTUNITY_ID string.Parsing untrusted extras can crash the activity. Add a safe parse with logging.
Apply this diff:
- if (opportunityId == -1) { - String opportunityIdStr = getIntent().getStringExtra(ConnectConstants.OPPORTUNITY_ID); - if (!StringUtils.isEmpty(opportunityIdStr)) { - opportunityId = Integer.parseInt(opportunityIdStr); - } - } + if (opportunityId == -1) { + String opportunityIdStr = getIntent().getStringExtra(ConnectConstants.OPPORTUNITY_ID); + if (!StringUtils.isEmpty(opportunityIdStr)) { + try { + opportunityId = Integer.parseInt(opportunityIdStr.trim()); + } catch (NumberFormatException nfe) { + org.javarosa.core.services.Logger.exception( + "Invalid OPPORTUNITY_ID extra (non-integer): " + opportunityIdStr, nfe); + } + } + }⛔ Skipped due to learnings
Learnt from: OrangeAndGreen PR: dimagi/commcare-android#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.
| <string name="connect_verify_message">Configura un método a continuación para desbloquear tanto tu dispositivo como tu cuenta de PersonalID</string> | ||
| <string name="connect_verify_message_pin">Configure un PIN o patrón a continuación para desbloquear su dispositivo y su cuenta PersonalID</string> | ||
| <string name="connect_verify_message_fingerprint">Configure su huella digital a continuación para desbloquear tanto su dispositivo como su cuenta PersonalID</string> |
There was a problem hiding this comment.
Keep the imperative tone consistent
Line 212 retains the informal “Configura…”, but the newly added lines switch to the formal “Configure…”. Please align the new strings with the existing tone.
- <string name="connect_verify_message_pin">Configure un PIN o patrón a continuación para desbloquear su dispositivo y su cuenta PersonalID</string>
- <string name="connect_verify_message_fingerprint">Configure su huella digital a continuación para desbloquear tanto su dispositivo como su cuenta PersonalID</string>
+ <string name="connect_verify_message_pin">Configura un PIN o patrón a continuación para desbloquear tu dispositivo y tu cuenta PersonalID</string>
+ <string name="connect_verify_message_fingerprint">Configura tu huella digital a continuación para desbloquear tanto tu dispositivo como tu cuenta PersonalID</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <string name="connect_verify_message">Configura un método a continuación para desbloquear tanto tu dispositivo como tu cuenta de PersonalID</string> | |
| <string name="connect_verify_message_pin">Configure un PIN o patrón a continuación para desbloquear su dispositivo y su cuenta PersonalID</string> | |
| <string name="connect_verify_message_fingerprint">Configure su huella digital a continuación para desbloquear tanto su dispositivo como su cuenta PersonalID</string> | |
| <string name="connect_verify_message">Configura un método a continuación para desbloquear tanto tu dispositivo como tu cuenta de PersonalID</string> | |
| <string name="connect_verify_message_pin">Configura un PIN o patrón a continuación para desbloquear tu dispositivo y tu cuenta PersonalID</string> | |
| <string name="connect_verify_message_fingerprint">Configura tu huella digital a continuación para desbloquear tanto tu dispositivo como tu cuenta PersonalID</string> |
🤖 Prompt for AI Agents
In app/res/values-es/strings.xml around lines 212 to 214, the existing string at
line 212 uses the informal imperative "Configura..." while the two new strings
on 213–214 use the formal "Configure..."; update the new strings to the informal
imperative to match tone—replace "Configure" with "Configura" in both
connect_verify_message_pin and connect_verify_message_fingerprint so all three
strings use the same informal phrasing.
| private boolean handleErrorCodeIfPresent(String errorCode, String errorSubCode, PersonalIdSessionData sessionData) { | ||
| try { | ||
| if (errorResponse != null) { | ||
| byte[] errorBytes = StreamsUtil.inputStreamToByteArray(errorResponse); | ||
| String jsonStr = new String(errorBytes, java.nio.charset.StandardCharsets.UTF_8); | ||
| JSONObject json = new JSONObject(jsonStr); | ||
|
|
||
| String errorCode = json.optString("error_code", ""); | ||
| sessionData.setSessionFailureCode(errorCode); | ||
| if ("LOCKED_ACCOUNT".equalsIgnoreCase(errorCode)) { | ||
| onFailure(PersonalIdOrConnectApiErrorCodes.ACCOUNT_LOCKED_ERROR, null); | ||
| return true; | ||
| } else if ("INTEGRITY_ERROR".equalsIgnoreCase(errorCode)) { | ||
| if (json.has("sub_code")) { | ||
| String subErrorCode = json.optString("sub_code"); | ||
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Integrity error with subcode " + subErrorCode); | ||
| sessionData.setSessionFailureSubcode(subErrorCode); | ||
| onFailure(PersonalIdOrConnectApiErrorCodes.INTEGRITY_ERROR, null); | ||
| } | ||
| return true; | ||
| } | ||
| sessionData.setSessionFailureCode(errorCode); | ||
| if ("LOCKED_ACCOUNT".equalsIgnoreCase(errorCode)) { | ||
| onFailure(PersonalIdOrConnectApiErrorCodes.ACCOUNT_LOCKED_ERROR, null); | ||
| return true; | ||
| } else if ("INTEGRITY_ERROR".equalsIgnoreCase(errorCode)) { | ||
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Integrity error with subcode " + errorSubCode); | ||
| sessionData.setSessionFailureSubcode(errorSubCode); | ||
| onFailure(PersonalIdOrConnectApiErrorCodes.INTEGRITY_ERROR, null); | ||
| return true; | ||
| } | ||
| } catch (Exception e) { | ||
| Logger.exception("Error parsing error_code", e); | ||
| Logger.exception("Error handling error code", e); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Don’t clobber session failure code/subcode when empty
When parsing fails or codes aren’t present, you set empty strings on sessionData, wiping prior state.
- sessionData.setSessionFailureCode(errorCode);
+ if (errorCode != null && !errorCode.isEmpty()) {
+ sessionData.setSessionFailureCode(errorCode);
+ }
@@
- sessionData.setSessionFailureSubcode(errorSubCode);
+ if (errorSubCode != null && !errorSubCode.isEmpty()) {
+ sessionData.setSessionFailureSubcode(errorSubCode);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private boolean handleErrorCodeIfPresent(String errorCode, String errorSubCode, PersonalIdSessionData sessionData) { | |
| try { | |
| if (errorResponse != null) { | |
| byte[] errorBytes = StreamsUtil.inputStreamToByteArray(errorResponse); | |
| String jsonStr = new String(errorBytes, java.nio.charset.StandardCharsets.UTF_8); | |
| JSONObject json = new JSONObject(jsonStr); | |
| String errorCode = json.optString("error_code", ""); | |
| sessionData.setSessionFailureCode(errorCode); | |
| if ("LOCKED_ACCOUNT".equalsIgnoreCase(errorCode)) { | |
| onFailure(PersonalIdOrConnectApiErrorCodes.ACCOUNT_LOCKED_ERROR, null); | |
| return true; | |
| } else if ("INTEGRITY_ERROR".equalsIgnoreCase(errorCode)) { | |
| if (json.has("sub_code")) { | |
| String subErrorCode = json.optString("sub_code"); | |
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Integrity error with subcode " + subErrorCode); | |
| sessionData.setSessionFailureSubcode(subErrorCode); | |
| onFailure(PersonalIdOrConnectApiErrorCodes.INTEGRITY_ERROR, null); | |
| } | |
| return true; | |
| } | |
| sessionData.setSessionFailureCode(errorCode); | |
| if ("LOCKED_ACCOUNT".equalsIgnoreCase(errorCode)) { | |
| onFailure(PersonalIdOrConnectApiErrorCodes.ACCOUNT_LOCKED_ERROR, null); | |
| return true; | |
| } else if ("INTEGRITY_ERROR".equalsIgnoreCase(errorCode)) { | |
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Integrity error with subcode " + errorSubCode); | |
| sessionData.setSessionFailureSubcode(errorSubCode); | |
| onFailure(PersonalIdOrConnectApiErrorCodes.INTEGRITY_ERROR, null); | |
| return true; | |
| } | |
| } catch (Exception e) { | |
| Logger.exception("Error parsing error_code", e); | |
| Logger.exception("Error handling error code", e); | |
| } | |
| return false; | |
| private boolean handleErrorCodeIfPresent(String errorCode, String errorSubCode, PersonalIdSessionData sessionData) { | |
| try { | |
| if (errorCode != null && !errorCode.isEmpty()) { | |
| sessionData.setSessionFailureCode(errorCode); | |
| } | |
| if ("LOCKED_ACCOUNT".equalsIgnoreCase(errorCode)) { | |
| onFailure(PersonalIdOrConnectApiErrorCodes.ACCOUNT_LOCKED_ERROR, null); | |
| return true; | |
| } else if ("INTEGRITY_ERROR".equalsIgnoreCase(errorCode)) { | |
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Integrity error with subcode " + errorSubCode); | |
| if (errorSubCode != null && !errorSubCode.isEmpty()) { | |
| sessionData.setSessionFailureSubcode(errorSubCode); | |
| } | |
| onFailure(PersonalIdOrConnectApiErrorCodes.INTEGRITY_ERROR, null); | |
| return true; | |
| } | |
| } catch (Exception e) { | |
| Logger.exception("Error handling error code", e); | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
around lines 70 to 85, avoid overwriting existing session failure code/subcode
with empty or null values: only call
sessionData.setSessionFailureCode(errorCode) and
sessionData.setSessionFailureSubcode(errorSubCode) when the provided values are
non-null and non-empty (use StringUtils.isEmpty or equivalent checks),
preserving prior values when parsing fails or codes are absent; keep the
existing error-specific handling (LOCKED_ACCOUNT/INTEGRITY_ERROR) and logging
intact, and ensure you do not set empty strings before invoking onFailure.
| fun getErrorBody(stream: InputStream?): String { | ||
| try { | ||
| if (stream != null) { | ||
| val errorBytes = StreamsUtil.inputStreamToByteArray(stream) | ||
| return String(errorBytes, StandardCharsets.UTF_8) | ||
| } | ||
| } catch (e: Exception) { | ||
| Logger.exception("Error parsing error_code", e); | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Close the stream inside getErrorBody to make callers safer.
Using use ensures closure even on exceptions and reduces duplication/leak risk.
Apply this diff:
- fun getErrorBody(stream: InputStream?): String {
- try {
- if (stream != null) {
- val errorBytes = StreamsUtil.inputStreamToByteArray(stream)
- return String(errorBytes, StandardCharsets.UTF_8)
- }
- } catch (e: Exception) {
- Logger.exception("Error parsing error_code", e);
- }
- return ""
- }
+ fun getErrorBody(stream: InputStream?): String {
+ if (stream == null) return ""
+ return try {
+ stream.use { input ->
+ val errorBytes = StreamsUtil.inputStreamToByteArray(input)
+ String(errorBytes, StandardCharsets.UTF_8)
+ }
+ } catch (e: Exception) {
+ Logger.exception("Error parsing error_code", e)
+ ""
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun getErrorBody(stream: InputStream?): String { | |
| try { | |
| if (stream != null) { | |
| val errorBytes = StreamsUtil.inputStreamToByteArray(stream) | |
| return String(errorBytes, StandardCharsets.UTF_8) | |
| } | |
| } catch (e: Exception) { | |
| Logger.exception("Error parsing error_code", e); | |
| } | |
| return "" | |
| } | |
| fun getErrorBody(stream: InputStream?): String { | |
| if (stream == null) return "" | |
| return try { | |
| stream.use { input -> | |
| val errorBytes = StreamsUtil.inputStreamToByteArray(input) | |
| String(errorBytes, StandardCharsets.UTF_8) | |
| } | |
| } catch (e: Exception) { | |
| Logger.exception("Error parsing error_code", e) | |
| "" | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/NetworkUtils.kt around lines 16 to 26,
the InputStream passed into getErrorBody is not closed on all paths; change the
method to call stream?.use { ... } so the stream is automatically closed even if
StreamsUtil.inputStreamToByteArray or String conversion throws; inside the use
block read the bytes and return the UTF-8 string, keep the catch to log
exceptions, and return an empty string on null/exception — this removes manual
close duplication and prevents leaks.
| private fun updateNotificationsIcon() { | ||
| if(shouldShowNotiifcations()) { | ||
| val notifications = NotificationRecordDatabaseHelper().getAllNotifications(activity) | ||
| val hasUnreadNotification = notifications!!.any { !it.readStatus } | ||
|
|
||
| binding.ivNotification.setImageResource( | ||
| if (hasUnreadNotification) R.drawable.ic_new_notification_bell | ||
| else R.drawable.ic_bell | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Typo in method name.
The method name contains a typo: shouldShowNotiifcations should be shouldShowNotifications (extra 'i'). This typo appears in multiple locations (lines 224, 239, 247).
Apply this diff to fix the typo:
- private fun updateNotificationsIcon() {
- if(shouldShowNotiifcations()) {
+ private fun updateNotificationsIcon() {
+ if(shouldShowNotifications()) {
val notifications = NotificationRecordDatabaseHelper().getAllNotifications(activity)
val hasUnreadNotification = notifications!!.any { !it.readStatus }
binding.ivNotification.setImageResource(
if (hasUnreadNotification) R.drawable.ic_new_notification_bell
else R.drawable.ic_bell
)
}
}And update the other occurrences:
- binding.notificationView.visibility = if (shouldShowNotiifcations()) View.VISIBLE else View.GONE
+ binding.notificationView.visibility = if (shouldShowNotifications()) View.VISIBLE else View.GONE- private fun shouldShowNotiifcations(): Boolean {
+ private fun shouldShowNotifications(): Boolean {
return PersonalIdManager.getInstance().isloggedIn() && isFeatureEnabled(NOTIFICATIONS);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun updateNotificationsIcon() { | |
| if(shouldShowNotiifcations()) { | |
| val notifications = NotificationRecordDatabaseHelper().getAllNotifications(activity) | |
| val hasUnreadNotification = notifications!!.any { !it.readStatus } | |
| binding.ivNotification.setImageResource( | |
| if (hasUnreadNotification) R.drawable.ic_new_notification_bell | |
| else R.drawable.ic_bell | |
| ) | |
| } | |
| } | |
| private fun updateNotificationsIcon() { | |
| if(shouldShowNotifications()) { | |
| val notifications = NotificationRecordDatabaseHelper().getAllNotifications(activity) | |
| val hasUnreadNotification = notifications!!.any { !it.readStatus } | |
| binding.ivNotification.setImageResource( | |
| if (hasUnreadNotification) R.drawable.ic_new_notification_bell | |
| else R.drawable.ic_bell | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/navdrawer/BaseDrawerController.kt around lines 223 to
233 (and other occurrences at lines 224, 239, 247), the method name is
misspelled as shouldShowNotiifcations; rename all occurrences (calls and the
method declaration) to shouldShowNotifications, update import/overrides/usages
accordingly, and ensure compilation by refactoring any references across the
file to the corrected name.
| public void readExternal(DataInputStream in, PrototypeFactory pf) throws IOException, DeserializationException { | ||
| actionType = ActionTypes.valueOf(ExtUtil.readString(in)); | ||
| action = ExtUtil.readString(in); | ||
| username = ExtUtil.readString(in); | ||
| domain = ExtUtil.readString(in); | ||
| creationTime = new DateTime(ExtUtil.readLong(in)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Breaking change in serialization format requires migration strategy.
Adding the action field read at line 116 changes the serialization format and breaks backward compatibility. Any existing serialized FCMMessageData objects will fail to deserialize because:
- Old format wrote only one string (action) at the position where
actionTypeis now expected - The new code expects two strings in sequence (actionType, then action)
- Reading old data will cause field misalignment or deserialization failure
Additionally, line 115 uses ActionTypes.valueOf() which throws IllegalArgumentException if the string doesn't match a valid enum constant, with no error handling.
Verify the following:
Consider adding:
- A migration strategy or version field to handle old serialized data
- Error handling around
ActionTypes.valueOf():
@Override
public void readExternal(DataInputStream in, PrototypeFactory pf) throws IOException, DeserializationException {
- actionType = ActionTypes.valueOf(ExtUtil.readString(in));
+ try {
+ actionType = ActionTypes.valueOf(ExtUtil.readString(in));
+ } catch (IllegalArgumentException e) {
+ Logger.log("FCMMessageData", "Invalid action type during deserialization, defaulting to INVALID");
+ actionType = ActionTypes.INVALID;
+ }
action = ExtUtil.readString(in);
username = ExtUtil.readString(in);
domain = ExtUtil.readString(in);
creationTime = new DateTime(ExtUtil.readLong(in));
}🏁 Script executed:
#!/bin/bash
# Search for any persisted FCMMessageData instances that need migration
rg -n "FCMMessageData" --type java -C3 | rg -i "serializ|persist|save|store"Length of output: 2097
The web search results confirm the critical nature of the issue. Externalizable classes are responsible for their own versioning which is tied to the external format, and all data and objects must be read from or skipped in the stream in the same order as they were written. Adding a field read at line 116 violates this contract without versioning support.
Now let me check the actual writeExternal method and ActionTypes enum:
Based on CommCare's own documentation, when code is changed for a serializable class, a data migration check is needed, especially when adding a new field to the class that you want to persist across serializations. Let me verify the writeExternal method in the repository:
🏁 Script executed:
#!/bin/bash
# Find writeExternal method in FCMMessageData
rg -n "writeExternal" "app/src/org/commcare/services/FCMMessageData.java" -A10Length of output: 452
🏁 Script executed:
#!/bin/bash
# Find ActionTypes enum definition
rg -n "enum ActionTypes" --type java -A20Length of output: 1939
🏁 Script executed:
#!/bin/bash
# Check if there's any versioning mechanism in the class
rg -n "serialVersionUID|version|VERSION" "app/src/org/commcare/services/FCMMessageData.java"Length of output: 0
Critical: Breaking change in serialization format requires migration strategy for existing persisted data.
The code adds actionType as a new first field in the external format (line 124 changed from writing action to writing actionType.toString()), breaking backward compatibility with all existing serialized FCMMessageData objects stored in SharedPreferences and Bundles.
When old data is deserialized by the new code:
- Line 115 reads the old
actionstring asactionType(invalid enum value) - Line 116 reads the old
usernameasaction - Field alignment fails, causing deserialization crashes on app upgrade
Additionally, ActionTypes.valueOf() at line 115 throws IllegalArgumentException with no error handling. While ActionTypes.INVALID exists as a fallback, add try-catch to handle invalid enum values gracefully:
@Override
public void readExternal(DataInputStream in, PrototypeFactory pf) throws IOException, DeserializationException {
- actionType = ActionTypes.valueOf(ExtUtil.readString(in));
+ try {
+ actionType = ActionTypes.valueOf(ExtUtil.readString(in));
+ } catch (IllegalArgumentException e) {
+ actionType = ActionTypes.INVALID;
+ }
action = ExtUtil.readString(in);
username = ExtUtil.readString(in);
domain = ExtUtil.readString(in);
creationTime = new DateTime(ExtUtil.readLong(in));
}Required: Implement a data migration strategy or versioning mechanism in readExternal() to handle existing serialized data from users upgrading the app.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void readExternal(DataInputStream in, PrototypeFactory pf) throws IOException, DeserializationException { | |
| actionType = ActionTypes.valueOf(ExtUtil.readString(in)); | |
| action = ExtUtil.readString(in); | |
| username = ExtUtil.readString(in); | |
| domain = ExtUtil.readString(in); | |
| creationTime = new DateTime(ExtUtil.readLong(in)); | |
| } | |
| public void readExternal(DataInputStream in, PrototypeFactory pf) throws IOException, DeserializationException { | |
| try { | |
| actionType = ActionTypes.valueOf(ExtUtil.readString(in)); | |
| } catch (IllegalArgumentException e) { | |
| actionType = ActionTypes.INVALID; | |
| } | |
| action = ExtUtil.readString(in); | |
| username = ExtUtil.readString(in); | |
| domain = ExtUtil.readString(in); | |
| creationTime = new DateTime(ExtUtil.readLong(in)); | |
| } |
avazirna
force-pushed
the
commcare_2.60
branch
from
f6b9096 to
338ddf4
Compare
|
@damagatchi retest this please |
4 similar comments
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
1 similar comment
|
@damagatchi retest this please |
Product Description
Backmerge Commcare 2.60.0
cross-request: dimagi/commcare-core#1507