Commcare 2.61 #3438
Commcare 2.61 #3438
Conversation
…662-location-indicator-signup # Conflicts: # app/res/values-es/strings.xml # app/res/values-fr/strings.xml # app/res/values-hi/strings.xml # app/res/values-lt/strings.xml # app/res/values-no/strings.xml # app/res/values-pt/strings.xml # app/res/values-sw/strings.xml # app/res/values-ti/strings.xml # app/res/values/strings.xml
Added privacy link in tooltip
Added privacy link in tooltip
…indicator-signup # Conflicts: # app/res/values-es/strings.xml # app/res/values-fr/strings.xml # app/res/values-hi/strings.xml # app/res/values-lt/strings.xml # app/res/values-no/strings.xml # app/res/values-pt/strings.xml # app/res/values-sw/strings.xml # app/res/values-ti/strings.xml # app/res/values/strings.xml
Some side nav bar changes
Location indicator for Personal ID Signup
Solved small size issue when capturing photo during personalID sign up
…estionview-2 Improve logging during resizing in QuestionsView
Notification and work history screen title issue solved
Added Receiver to listen location service status
Supported older version before API 24
Earned and Transfer amount on the payment tab issue solved
Crash solved while moving back and forth in connect message screen
📝 WalkthroughWalkthroughThis PR introduces location-aware functionality to the PersonalID feature alongside UI refinements and lifecycle management updates. Changes include a new location service change broadcast receiver mechanism in Sequence DiagramsequenceDiagram
participant OS as OS/System
participant Receiver as LocationChangeReceiverBroadcast
participant Controller as CommCareFusedLocationController
participant Listener as CommCareLocationListener<br/>(Implementations)
OS->>Receiver: ACTION_PROVIDERS_CHANGED broadcast
Receiver->>Controller: onReceive() triggered
Controller->>Controller: Check if location services<br/>globally enabled/disabled
alt Location Services Enabled
Controller->>Controller: call start()
Controller->>Listener: onLocationResult(location)
else Location Services Disabled
Controller->>Controller: call stop()
end
Controller->>Listener: onLocationServiceChange(boolean)
Note over Listener: Implementations:<br/>GeoPointActivity<br/>PersonalIdPhoneFragment<br/>PollSensorController
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/org/commcare/utils/GeoUtils.java (1)
150-166: ThelocationServicesEnabledGloballymethod should remain private as it is only used internally within the same class.The search found only one caller: the
goToProperLocationSettingsScreenmethod at line 141, which is in the same file. No external usages from other classes or files were discovered. Making this method public expands the API surface unnecessarily without justification from current usage patterns.
🧹 Nitpick comments (9)
app/res/layout/screen_personalid_photo_capture.xml (1)
60-75: Consider adding content descriptions to buttons for accessibility.While LinearLayout itself is navigable, adding explicit
android:contentDescriptionattributes to both buttons can improve screen reader experience and align with accessibility best practices, especially for a photo capture UI where user intent clarity is valuable.<com.google.android.material.button.MaterialButton android:id="@+id/take_photo_button" android:layout_width="0dp" android:layout_height="wrap_content" android:layout_weight="1" android:layout_marginEnd="5dp" android:contentDescription="@string/personalid_photo_capture_take_photo_button" android:text="@string/personalid_photo_capture_take_photo_button" /> <com.google.android.material.button.MaterialButton android:id="@+id/save_photo_button" android:layout_width="0dp" android:layout_height="wrap_content" android:layout_weight="1" android:enabled="false" android:layout_marginStart="5dp" android:contentDescription="@string/personalid_photo_capture_save_photo_button" android:text="@string/personalid_photo_capture_save_photo_button" />app/res/drawable/ic_blue_triangle.xml (1)
8-8: Reference the color resource instead of hardcoding.The hardcoded color
#3843CFmatches the newly added@color/sapphire_blueresource. Use the color reference for consistency and easier maintenance.Apply this diff:
- android:fillColor="#3843CF"/> + android:fillColor="@color/sapphire_blue"/>app/res/drawable/ic_location_info.xml (1)
8-8: Consider using a color resource for consistency.The hardcoded color
#3B43C5is similar to@color/sapphire_blue(#3843CF). For better maintainability, consider either using the existing color resource or defining a new named color resource in colors.xml if this specific shade is intentional.app/src/org/commcare/activities/GeoPointActivity.java (1)
264-267: Consider adding a clarifying comment.The empty implementation is likely intentional since GeoPointActivity handles location service issues through other callbacks (
onLocationRequestFailure,handleNoLocationProviders). However, a brief comment explaining why this callback doesn't need handling would improve code clarity.Example:
@Override public void onLocationServiceChange(boolean locationServiceEnabled) { - + // No action needed - location service state is handled via onLocationRequestFailure }app/src/org/commcare/android/javarosa/PollSensorController.java (1)
131-134: Consider adding a clarifying comment.The empty implementation is likely correct since PollSensorController handles location failures through
onLocationRequestFailureand has its own timeout mechanism viaPollingTimeoutTask. A brief comment would clarify the intentional no-op behavior.Example:
@Override public void onLocationServiceChange(boolean locationServiceEnabled) { - + // No action needed - service state changes handled via onLocationRequestFailure }app/src/org/commcare/location/CommCareLocationListener.kt (1)
22-29: Document the contract foronLocationServiceChangefor future implementorsAdding this callback is reasonable, but it would help to briefly document when it’s fired (e.g., “called when system location providers are toggled on/off”) and what guarantees callers can rely on (frequency, threading, whether it’s called only while a request is active, etc.). That will make it easier for future
CommCareLocationListenerimplementors to handle it correctly.app/src/org/commcare/location/CommCareFusedLocationController.kt (2)
54-69: Receiver lifecycle is sound; consider minimal logging for swallowedIllegalArgumentExceptionThe pattern of
restartLocationServiceChangeReceiver()→stopthenstart, and thetry/catcharoundunregisterReceiverare reasonable to avoid duplicate-registration and “Receiver not registered” crashes.To aid debugging subtle lifecycle issues, you might want to log the
IllegalArgumentExceptionat a verbose/debug level instead of fully swallowing it:fun stopLocationServiceChangeReceiver() { try { mContext?.unregisterReceiver(mLocationServiceChangeReceiver) } catch (e: IllegalArgumentException) { - // This can happen if stop is called multiple times + // This can happen if stop is called multiple times + // Optional: log at debug level if you need to diagnose receiver lifecycle issues. + // Log.d("CommCareFusedLocationController", "Receiver already unregistered", e) } }Not required, but it can save time if you ever chase receiver-registration bugs.
Also applies to: 84-104
89-104: Clarify intended usage ofstartLocationServiceChangeReceiverwhen location is initially disabled
LocationChangeReceiverBroadcastcorrectly togglesstart()/stop()and feedsonLocationServiceChange(locationServiceEnabled)to the listener onPROVIDERS_CHANGED_ACTION. Right now, this receiver is only guaranteed to be (re)registered viarestartLocationServiceChangeReceiver(), which you call on the success path ofstart().If the initial
checkLocationSettingsfails because location is off, the receiver won’t be (re)started from here. That’s fine as long as callers explicitly invokestartLocationServiceChangeReceiver()(e.g., when showing a “please enable location” UI). It’d be good to double‑check call sites to ensure that’s happening and maybe capture the intended pattern in a short KDoc on these methods.Also applies to: 106-122
app/res/layout/screen_personalid_phoneno.xml (1)
135-193: Tidy up constraints and accessibility for the new location/consent blockThe new ConstraintLayout block (
ll_location, tooltip, consent row, continue button) looks structurally sound, but there are a couple of small improvements you might consider:
Make constraints explicit for clarity
ll_privacy_policyis only constrained vialayout_constraintTop_toBottomOf="@+id/iv_location_info"; horizontally it relies on the default left anchoring plus margins. Similarly,iv_locationis only constrainedend_toStartOf="@+id/tv_location". This works, but being explicit (e.g.,app:layout_constraintStart_toStartOf="parent"foriv_locationandll_privacy_policy) will make the layout easier to reason about and less fragile to future edits.Accessibility for clickable location/tooltip UI
tv_location,tooltipText, and the info icon (iv_location_info) participate in an interactive pattern, but only the text views are clickable/focusable and none of the icons havecontentDescription. Consider:
- Adding appropriate
android:contentDescriptiontoiv_location/iv_location_info.- Ensuring the focus order and hit areas make sense for TalkBack users (e.g., the info icon might also be clickable and share the same action as
tv_locationif that matches the UX).These are polish items; functionally the layout should behave as intended.
Also applies to: 216-265
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
app/AndroidManifest.xml(1 hunks)app/res/drawable/bg_location_tooltip.xml(1 hunks)app/res/drawable/ic_blue_triangle.xml(1 hunks)app/res/drawable/ic_location_info.xml(1 hunks)app/res/layout/nav_drawer_base.xml(3 hunks)app/res/layout/nav_drawer_footer.xml(2 hunks)app/res/layout/nav_drawer_header.xml(2 hunks)app/res/layout/nav_drawer_list_item.xml(1 hunks)app/res/layout/screen_personalid_phoneno.xml(5 hunks)app/res/layout/screen_personalid_photo_capture.xml(1 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-lt/strings.xml(1 hunks)app/res/values-no/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/colors.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/GeoPointActivity.java(1 hunks)app/src/org/commcare/activities/PushNotificationActivity.kt(1 hunks)app/src/org/commcare/activities/connect/PersonalIdWorkHistoryActivity.kt(4 hunks)app/src/org/commcare/activities/connect/viewmodel/PersonalIdWorkHistoryViewModel.kt(3 hunks)app/src/org/commcare/android/javarosa/PollSensorController.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectJobDetailBottomSheetDialogFragment.java(0 hunks)app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java(0 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java(0 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java(0 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java(0 hunks)app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java(0 hunks)app/src/org/commcare/fragments/personalId/PersonalIdNameFragment.java(0 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(6 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java(0 hunks)app/src/org/commcare/fragments/personalId/WorkHistoryEarnedFragment.kt(0 hunks)app/src/org/commcare/fragments/personalId/WorkHistoryPendingFragment.kt(0 hunks)app/src/org/commcare/location/CommCareFusedLocationController.kt(3 hunks)app/src/org/commcare/location/CommCareLocationListener.kt(1 hunks)app/src/org/commcare/utils/GeoUtils.java(1 hunks)app/src/org/commcare/views/QuestionsView.java(3 hunks)commcare-support-library/src/main/java/org/commcare/commcaresupportlibrary/CommCareLauncher.java(2 hunks)
💤 Files with no reviewable changes (10)
- app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java
- app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java
- app/src/org/commcare/fragments/connect/ConnectJobDetailBottomSheetDialogFragment.java
- app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java
- app/src/org/commcare/fragments/personalId/PersonalIdNameFragment.java
- app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java
- app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java
- app/src/org/commcare/fragments/personalId/WorkHistoryPendingFragment.kt
- app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
- app/src/org/commcare/fragments/personalId/WorkHistoryEarnedFragment.kt
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
📚 Learning: 2025-05-22T14:32:53.133Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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-no/strings.xmlapp/res/values/strings.xmlapp/res/values-es/strings.xmlapp/res/values-lt/strings.xml
📚 Learning: 2025-05-07T06:48:32.621Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3070
File: app/res/values/themes.xml:80-80
Timestamp: 2025-05-07T06:48:32.621Z
Learning: The CommCare Android app doesn't require backward compatibility for support library features like the vector drawable compatibility provided by app:srcCompat. It uses android:src directly for drawable references in themes.
Applied to files:
app/res/drawable/ic_location_info.xml
📚 Learning: 2025-05-07T06:33:26.714Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3070
File: app/res/drawable/shape_connect_delivery_pending.xml:1-5
Timestamp: 2025-05-07T06:33:26.714Z
Learning: For drawables in the CommCare Android codebase, the preferred approach is to let consuming layouts explicitly specify required width and height rather than defining intrinsic sizes within shape drawables.
Applied to files:
app/res/drawable/ic_location_info.xmlapp/res/drawable/bg_location_tooltip.xmlapp/res/drawable/ic_blue_triangle.xml
📚 Learning: 2025-05-22T14:28:35.959Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaapp/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java
📚 Learning: 2025-06-04T19:17:21.213Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:62-64
Timestamp: 2025-06-04T19:17:21.213Z
Learning: In ConnectUnlockFragment.java, the user prefers to let getArguments() potentially throw NullPointerException rather than adding null checks, as the arguments are required for proper navigation flow and their absence indicates a programming error that should fail fast.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
📚 Learning: 2025-05-22T14:26:41.341Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In SelectInstallModeFragment.java, the showConnectErrorMessage method intentionally omits null checks because it's called at a specific point in the startup flow where UI is guaranteed to be loaded. It's designed to crash if activity or view is null to make potential issues immediately visible rather than hiding them with defensive programming.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
📚 Learning: 2025-03-10T08:16:59.436Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 2949
File: app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java:58-59
Timestamp: 2025-03-10T08:16:59.436Z
Learning: All fragments using view binding should implement proper cleanup in onDestroyView() by setting binding to null to prevent memory leaks.
Applied to files:
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.javaapp/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.javaapp/src/org/commcare/activities/connect/viewmodel/PersonalIdWorkHistoryViewModel.ktapp/src/org/commcare/activities/connect/PersonalIdWorkHistoryActivity.kt
📚 Learning: 2025-02-19T15:15:01.935Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2956
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java:58-60
Timestamp: 2025-02-19T15:15:01.935Z
Learning: Error handling for message retrieval in ConnectMessageChannelListFragment's retrieveMessages callback is not required as per user preference.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java
📚 Learning: 2025-04-21T15:02:17.492Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 3042
File: app/src/org/commcare/fragments/BreadcrumbBarViewModel.java:50-55
Timestamp: 2025-04-21T15:02:17.492Z
Learning: ViewModels should not store View or Activity references as this can cause memory leaks. Unlike Fragments with setRetainInstance(true), ViewModels don't have automatic view detachment mechanisms. When migrating from Fragments to ViewModels, view references should be replaced with data-only state in the ViewModel.
Applied to files:
app/src/org/commcare/activities/connect/viewmodel/PersonalIdWorkHistoryViewModel.ktapp/src/org/commcare/activities/connect/PersonalIdWorkHistoryActivity.kt
🧬 Code graph analysis (3)
app/src/org/commcare/activities/GeoPointActivity.java (1)
app/src/org/commcare/location/CommCareLocationListener.kt (1)
onLocationServiceChange(22-22)
app/src/org/commcare/android/javarosa/PollSensorController.java (1)
app/src/org/commcare/location/CommCareLocationListener.kt (1)
onLocationServiceChange(22-22)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
app/src/org/commcare/location/CommCareLocationListener.kt (1)
onLocationServiceChange(22-22)
🪛 detekt (1.23.8)
app/src/org/commcare/location/CommCareFusedLocationController.kt
[warning] 101-101: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ 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 (30)
app/res/layout/screen_personalid_photo_capture.xml (1)
52-76: Solid UI refactoring using proper LinearLayout weight distribution.The new horizontal LinearLayout with equal weight distribution (layout_weight=1, width=0dp) is the correct Android pattern for proportional button sizing. The 20dp horizontal margins and 5dp inter-button spacing maintain visual consistency and provide appropriate padding.
app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java (1)
42-48: Eagerly initializing the summary view inonCreateViewis appropriateCalling
updateSummaryView()right aftersetupRecyclerView()ensures the summary amounts are correctly populated on first render instead of waiting for an externalupdateView()call. SincesetupRecyclerView()already depends on a non-nulljoband binding, this doesn’t introduce any new nullability risks.app/src/org/commcare/views/QuestionsView.java (4)
11-12: LGTM! Import additions support the enhanced focus diagnostics.The new imports (
ViewGroup,ViewParent,ComboboxWidget) are properly utilized in the refactored focus diagnostic methods.Also applies to: 27-27
356-357: LGTM! Enhanced diagnostic logging for resize exceptions.The additional focus context (parent view hierarchy) will help diagnose resize failures more effectively.
377-395: Enhanced focus diagnostics improve debugging capability.The refactored method provides more detailed context for resize exceptions, especially the special handling for
ComboboxWidgetthat includes question ID and answer value. The null-safe implementation and clear fallback logic are well-structured.
385-390: Pattern matchinginstanceofis fully supported—no compatibility issue.The project is configured with
sourceCompatibility JavaVersion.VERSION_17andtargetCompatibility JavaVersion.VERSION_17inapp/build.gradle, along with desugaring enabled (coreLibraryDesugaringEnabled true). Java 17 fully supports pattern matching forinstanceofas a standard feature (available since Java 16). The syntax used on line 385 is valid and requires no changes.app/res/values/colors.xml (1)
224-225: LGTM!The new color resources for the location UI feature are well-defined and follow the existing naming conventions.
commcare-support-library/src/main/java/org/commcare/commcaresupportlibrary/CommCareLauncher.java (1)
25-46: LGTM!The refactoring to use
entrySet()iteration is cleaner and more efficient than the previouskeySet()approach, avoiding repeated map lookups. The added null/empty check is good defensive programming.app/res/values-hi/strings.xml (1)
517-521: LGTM!The Hindi localization strings for PersonalID location features are properly formatted and consistent with the additions in other language locales.
app/res/values-no/strings.xml (1)
126-130: LGTM!The Norwegian localization strings for PersonalID location features are properly formatted and align with the additions in other language locales.
app/AndroidManifest.xml (1)
5-5: Based on my investigation, I cannot verify the core claim in the original review comment. Here are my findings:
Unable to confirm the version downgrade: The current AndroidManifest.xml shows
versionName="2.61.0"andversionCode="106". However, I cannot find evidence that the previous version was 2.62.No reference to 2.62 in the repository: A search for "2.62" across the repository returned no results.
Limited git history: The git log shows only the current commit ("Automated: adding hotfix version to AndroidManifest") and one prior merge commit. The repository history doesn't allow me to see what the version was before this change.
Commit message suggests intentional change: The message "Automated: adding hotfix version to AndroidManifest" indicates this could be an automated or deliberate version management action, possibly part of a release process.
Rewritten Review Comment:
Verify the versionName and versionCode changes.
Unable to confirm from git history whether
versionName="2.61.0"(versionCode="106") represents a downgrade or intentional hotfix version. Please clarify:
- What was the previous versionName and versionCode?
- Is this an intentional hotfix or release branch merge?
- Should the versionCode increment align with the version change?
app/res/values-sw/strings.xml (1)
526-530: LGTM! Location strings localized consistently.The four new location-related strings for PersonalID are properly localized in Swahili and align with the additions in other locale files across the PR.
app/res/values-ti/strings.xml (1)
510-513: LGTM! Tigrinya localization added correctly.The location-related strings are properly localized and consistent with other locale additions in the PR.
app/res/values-pt/strings.xml (1)
527-530: LGTM! Portuguese localization complete.The location strings are properly translated and align with the localization pattern across all language files in this PR.
app/src/org/commcare/activities/connect/PersonalIdWorkHistoryActivity.kt (2)
17-17: LGTM! Base class migration to CommCareActivity completed properly.The activity has been successfully migrated from
CommonBaseActivitytoCommCareActivity<PersonalIdWorkHistoryActivity>()with all required lifecycle methods implemented:
onPostCreateadded for action bar configurationgenerateProgressDialogoverride added as required by the base classThe changes align with the modern architecture patterns in the codebase.
Also applies to: 51-58, 107-108
82-82: Context passed to ViewModel method.The activity context is now passed to the ViewModel's
retrieveAndProcessWorkHistorymethod. This ties to the API change in the ViewModel. Please see the verification request in the ViewModel file review to ensure the context is not retained in the call chain.app/res/layout/nav_drawer_list_item.xml (1)
12-12: LGTM! Navigation drawer theming update.The background color change to
connect_blue_coloris consistent with the broader navigation drawer theming updates across multiple layout files in this PR.app/res/layout/nav_drawer_footer.xml (1)
9-9: LGTM! Footer theming updated for consistency and contrast.The background color change to
connect_blue_colorand the addition of white text color ensure proper visual contrast and align with the navigation drawer theming updates throughout the PR.Also applies to: 101-101
app/res/drawable/bg_location_tooltip.xml (1)
1-5: LGTM! Tooltip background drawable follows best practices.The new tooltip background uses the
sapphire_bluecolor with an 8dp corner radius. The implementation correctly avoids defining intrinsic dimensions, allowing consuming layouts to specify the required size.Based on learnings, this is the preferred approach for drawables in the codebase.
app/res/layout/nav_drawer_base.xml (1)
5-5: LGTM! Consistent color scheme update.The navigation drawer color updates align with the broader UI theming changes across related layouts (nav_drawer_header.xml, nav_drawer_footer.xml, etc.) for visual consistency.
Also applies to: 13-13, 51-51, 71-71
app/res/layout/nav_drawer_header.xml (1)
9-9: LGTM! Color scheme updates align with drawer theming.The background and tint changes are consistent with the updated color palette (persian_indigo and connect_blue_color) applied across navigation drawer components.
Also applies to: 23-23
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (3)
123-140: LGTM! Well-structured tooltip helper method.The
setLocationToolTipmethod appropriately:
- Updates UI elements based on location availability
- Displays different icons and messages for found vs. not-found states
- Enables clickable links in the tooltip text via LinkMovementMethod
182-194: LGTM! Tooltip visibility toggling works correctly.The click listeners appropriately:
- Toggle tooltip info visibility when clicking the info icon
- Hide tooltip info when clicking the main layout (acting as a backdrop dismiss)
567-574: Location service re-enablement is already handled automatically by the BroadcastReceiver.The
CommCareFusedLocationController.LocationChangeReceiverBroadcastlistens forPROVIDERS_CHANGED_ACTIONbroadcasts. When location service is re-enabled, the receiver automatically callsstart()(line 119 in the controller) before invoking theonLocationServiceChange()callback (line 122). Therefore, location acquisition is already restarted immediately when the user re-enables the service via the notification shade—no additional action is needed inPersonalIdPhoneFragment.onLocationServiceChange(). The current implementation is correct: it handles the disabled case by clearing location data, and appropriately does nothing for the enabled case since location restart is already initiated by the controller.Likely an incorrect or invalid review comment.
app/src/org/commcare/activities/PushNotificationActivity.kt (2)
65-72: LGTM! Improved lifecycle management.Moving action bar configuration to
onPostCreateis a better practice that ensures the action bar is set up after the activity is fully created and the view hierarchy is established. This follows Android best practices for action bar initialization.
74-74: LGTM! Good encapsulation.Making
initViews()private is appropriate since it's only called internally fromonCreate().app/res/values/strings.xml (1)
696-699: LGTM! Clear and informative location messages.The new location-related strings provide:
- Clear status indicators for location availability
- Helpful guidance in failure scenarios (enable permissions or try later)
- Privacy policy link in success case for transparency
app/res/values-fr/strings.xml (1)
522-525: LGTM! French translations provided for location strings.The French translations for the new location-related strings are properly formatted and maintain the HTML link structure for the privacy policy.
app/res/values-lt/strings.xml (1)
126-129: Location tooltip strings wiring looks correctThe new
personalid_*location*strings use the expected keys, preserve the\n\nline breaks, and include the HTML link consistently with other locales. From a resource/wiring perspective this looks good.app/res/values-es/strings.xml (1)
518-522: Spanish PersonalID location strings are consistent with base and other localesThe four new
personalid_*location*strings match the base keys and structure (same line breaks, same privacy link), and the Spanish phrasing aligns with the rest of the PersonalID copy. No issues from a resources or wiring standpoint.
| val profilePhoto: String? = user.photo | ||
|
|
||
| fun retrieveAndProcessWorkHistory() { | ||
| fun retrieveAndProcessWorkHistory(context: Context) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the PersonalIdWorkHistoryViewModel implementation
fd -t f "PersonalIdWorkHistoryViewModel.kt" --exec head -100 {} \;
# Also search for PersonalIdApiHandler to check its implementation
fd -t f "PersonalIdApiHandler" --exec cat {} \;
# Search for retrieveWorkHistory method signature and implementation
rg -nP --type=kotlin "fun retrieveWorkHistory\(" -A 15
# Check for any context field storage in API handler classes
rg -nP --type=kotlin "private.*context.*Context|var.*context.*Context|val.*context.*Context"Repository: dimagi/commcare-android
Length of output: 17893
🏁 Script executed:
#!/bin/bash
# Check the full implementation of RetrieveWorkHistoryResponseParser
cat app/src/org/commcare/connect/network/connectId/parser/RetrieveWorkHistoryResponseParser.kt
# Also check ApiPersonalId.retrieveWorkHistory to see how the parser is used
rg -nP --type=java "retrieveWorkHistory" app/src/org/commcare/connect/network/ -A 20
# Check if the context is stored in the callback or used immediately
rg -nP --type=java "class BaseApiCallback" -A 30Repository: dimagi/commcare-android
Length of output: 6360
🏁 Script executed:
#!/bin/bash
# Check what storeCredentialDataInTable does with the context
rg -nP --type=kotlin "fun storeCredentialDataInTable" -A 20
# Check if getApplication() is available in AndroidViewModel
rg -nP --type=kotlin "class PersonalIdWorkHistoryViewModel" -B 5 -A 5
# Look for how other methods in PersonalIdApiHandler use context
rg -nP --type=java "ApiPersonalId\." app/src/org/commcare/connect/network/ | head -30Repository: dimagi/commcare-android
Length of output: 4095
🏁 Script executed:
#!/bin/bash
# Search for storeCredentialDataInTable implementation
fd -t f "ConnectAppDatabaseUtil.kt" --exec cat {} \;
# Also search for any database-related imports in RetrieveWorkHistoryResponseParser
rg -nP "storeCredentialDataInTable" -A 10 -B 2Repository: dimagi/commcare-android
Length of output: 3953
🏁 Script executed:
#!/bin/bash
# Find ConnectDatabaseHelper and check getConnectStorage implementation
fd -t f "ConnectDatabaseHelper*" --exec cat {} \;
# Also search for getConnectStorage signature
rg -nP "getConnectStorage" -A 5Repository: dimagi/commcare-android
Length of output: 30819
Based on my verification, I found a confirmed critical memory leak issue. The Activity context is being retained in the RetrieveWorkHistoryResponseParser, which is held by the callback chain that can outlive the Activity. Since the ViewModel has access to getApplication() via AndroidViewModel, this issue needs to be addressed.
Verify that the context parameter is not retained in the call chain and replace with application context to prevent memory leaks.
The RetrieveWorkHistoryResponseParser stores the Activity context as a private field (line 13: private val context: Context). While the context is used immediately in the parse() method, the parser itself is retained in the callback chain attached to the retrofit Call object, which can execute asynchronously and potentially outlive the Activity. This creates a reference cycle: Call → Callback → Parser → Activity Context.
Additionally, storeCredentialDataInTable() passes this context to ConnectDatabaseHelper.getConnectStorage(), which stores it in an AndroidDbHelper instance. Database operations work correctly with Application context.
Since PersonalIdWorkHistoryViewModel extends AndroidViewModel, use getApplication() instead of passing the Activity context:
fun retrieveAndProcessWorkHistory() { // Remove context parameter
viewModelScope.launch(Dispatchers.IO) {
object : PersonalIdApiHandler<List<PersonalIdWorkHistory>>() {
// ... rest of implementation
}.retrieveWorkHistory(getApplication(), user.userId, user.password) // Use getApplication()
}
}| object : LocationCallback() { | ||
| override fun onLocationResult(result: LocationResult) { | ||
| result ?: return | ||
| mCurrentLocation = result.lastLocation | ||
| mListener?.onLocationResult(mCurrentLocation!!) | ||
| } |
There was a problem hiding this comment.
Guard against null lastLocation to avoid a potential NPE
LocationResult.lastLocation is nullable; if it’s ever null, mCurrentLocation!! will crash when invoking onLocationResult. You can make this safer without changing behavior when a location is present:
- private val mLocationCallback =
- object : LocationCallback() {
- override fun onLocationResult(result: LocationResult) {
- result ?: return
- mCurrentLocation = result.lastLocation
- mListener?.onLocationResult(mCurrentLocation!!)
- }
- }
+ private val mLocationCallback =
+ object : LocationCallback() {
+ override fun onLocationResult(result: LocationResult) {
+ val location = result.lastLocation ?: return
+ mCurrentLocation = location
+ mListener?.onLocationResult(location)
+ }
+ }This keeps getLocation() semantics but avoids a crash when the fused provider can’t supply a fix.
Also applies to: 76-76
🤖 Prompt for AI Agents
In app/src/org/commcare/location/CommCareFusedLocationController.kt around lines
34 to 39 (and similarly at line 76), the code assumes
LocationResult.lastLocation is non-null and uses mCurrentLocation!! which can
throw an NPE; change the callback to early-return if result.lastLocation is null
(or skip updating/calling the listener) so that mCurrentLocation is only
assigned and mListener.onLocationResult invoked when lastLocation is non-null,
preserving existing behavior when a location is present but avoiding a crash
when the fused provider returns no fix.
| private String getFocusedViewParentView() { | ||
| View focusedView = findFocus(); | ||
| if (focusedView == null || focusedView.getParent() == null || focusedView.getParent().getParent() == null) { | ||
| return "None"; | ||
| } | ||
| ViewParent grandParent = focusedView.getParent().getParent(); | ||
| try { | ||
| String resourceName = focusedView.getResources().getResourceEntryName(((ViewGroup)grandParent).getId()); | ||
| return grandParent.getClass().getSimpleName() + "/" + resourceName; | ||
| } catch (Exception e) { | ||
| return grandParent.getClass().getSimpleName() + "/NoParentViewName"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add instanceof check before casting to ViewGroup.
Line 369 casts grandParent to ViewGroup without verifying the type. Since ViewParent is an interface with implementations that may not be ViewGroup (e.g., ViewRootImpl), this risks a ClassCastException that would mask the original resize exception being diagnosed.
Apply this diff to add a type check:
private String getFocusedViewParentView() {
View focusedView = findFocus();
if (focusedView == null || focusedView.getParent() == null || focusedView.getParent().getParent() == null) {
return "None";
}
ViewParent grandParent = focusedView.getParent().getParent();
try {
- String resourceName = focusedView.getResources().getResourceEntryName(((ViewGroup)grandParent).getId());
- return grandParent.getClass().getSimpleName() + "/" + resourceName;
- } catch (Exception e) {
+ if (grandParent instanceof ViewGroup) {
+ String resourceName = focusedView.getResources().getResourceEntryName(((ViewGroup)grandParent).getId());
+ return grandParent.getClass().getSimpleName() + "/" + resourceName;
+ }
+ return grandParent.getClass().getSimpleName() + "/NotAViewGroup";
+ } catch (Resources.NotFoundException e) {
return grandParent.getClass().getSimpleName() + "/NoParentViewName";
}
}📝 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 String getFocusedViewParentView() { | |
| View focusedView = findFocus(); | |
| if (focusedView == null || focusedView.getParent() == null || focusedView.getParent().getParent() == null) { | |
| return "None"; | |
| } | |
| ViewParent grandParent = focusedView.getParent().getParent(); | |
| try { | |
| String resourceName = focusedView.getResources().getResourceEntryName(((ViewGroup)grandParent).getId()); | |
| return grandParent.getClass().getSimpleName() + "/" + resourceName; | |
| } catch (Exception e) { | |
| return grandParent.getClass().getSimpleName() + "/NoParentViewName"; | |
| } | |
| } | |
| private String getFocusedViewParentView() { | |
| View focusedView = findFocus(); | |
| if (focusedView == null || focusedView.getParent() == null || focusedView.getParent().getParent() == null) { | |
| return "None"; | |
| } | |
| ViewParent grandParent = focusedView.getParent().getParent(); | |
| try { | |
| if (grandParent instanceof ViewGroup) { | |
| String resourceName = focusedView.getResources().getResourceEntryName(((ViewGroup)grandParent).getId()); | |
| return grandParent.getClass().getSimpleName() + "/" + resourceName; | |
| } | |
| return grandParent.getClass().getSimpleName() + "/NotAViewGroup"; | |
| } catch (Resources.NotFoundException e) { | |
| return grandParent.getClass().getSimpleName() + "/NoParentViewName"; | |
| } | |
| } |
| xmlns:tools="http://schemas.android.com/tools" | ||
| android:versionCode="106" | ||
| android:versionName="2.62"> | ||
| android:versionName="2.61.0"> |
There was a problem hiding this comment.
Once merged, we will need to correct this back to 2.62
Technical Summary
Merging commcare_2.61 into master
Release: here
Labels and Review