Beta release changes to master #3276
Conversation
📝 WalkthroughWalkthroughThis change introduces a new string resource, Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdPhoneFragment
participant GoogleApiAvailability
participant ActivityResultLauncher
participant AppConfigFailureHandler
User->>PersonalIdPhoneFragment: Fragment created
PersonalIdPhoneFragment->>GoogleApiAvailability: checkGooglePlayServices()
alt Play Services available
PersonalIdPhoneFragment-->>User: Continue normal flow
else Play Services not available
GoogleApiAvailability-->>PersonalIdPhoneFragment: Error code
alt Error is user-resolvable
PersonalIdPhoneFragment->>ActivityResultLauncher: Launch resolution dialog
ActivityResultLauncher->>PersonalIdPhoneFragment: onActivityResult
alt Result OK
PersonalIdPhoneFragment-->>User: Continue normal flow
else Result not OK
PersonalIdPhoneFragment->>AppConfigFailureHandler: Trigger failure with error message
end
else Error not user-resolvable
PersonalIdPhoneFragment->>AppConfigFailureHandler: Trigger failure with error message
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/res/values-pt/strings.xml (1)
471-471: Translation reads well – optional tweak to product-name capitalisation“serviços do Google Play” is perfectly understandable, but elsewhere in the file branded product names usually retain their original capitalisation (“Google Play Serviços” or simply “Google Play Services”). Consider:
-<string name="play_service_update_error">Certifique-se de que os serviços do Google Play sejam suportados no dispositivo e estejam atualizados.</string> +<string name="play_service_update_error">Certifique-se de que os Serviços do Google Play sejam suportados no dispositivo e estejam atualizados.</string>Purely stylistic; feel free to ignore if current form matches your localisation guide.
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
113-117: RepeatedsetUserPropertycalls may be wasteful
setUserProperties()is invoked for every analytics event, which now meansBUILD_NUMBER(and all other properties) are pushed to Firebase every single time. While this has always been true for the existing properties, the growing list slightly amplifies the overhead.Consider caching a boolean flag (e.g.,
userPropsSet) and setting these properties only once per process start. That pattern already exists in many Firebase integrations and avoids redundant IPC work on the analytics thread.private static boolean userPropsSet = false; private static void setUserProperties(FirebaseAnalytics analyticsInstance) { + if (userPropsSet) { + return; + } ... analyticsInstance.setUserProperty(CCAnalyticsParam.BUILD_NUMBER, String.valueOf(BuildConfig.VERSION_CODE)); + + userPropsSet = true; }app/res/values/strings.xml (1)
634-634: Refine wording for clearer user guidance.The current phrasing (“…services is supported on the device…”) is a bit awkward and may confuse users. “Available” more clearly conveys what they need to check.
- <string name="play_service_update_error">Please make sure Google Play services is supported on the device and is up to date.</string> + <string name="play_service_update_error">Please make sure Google Play services is available on your device and up to date.</string>app/res/values-es/strings.xml (1)
465-465: Update Spanish copy to stay aligned with the revised English text.If the English string gets re-worded as suggested, mirror the change here to keep translations consistent.
- <string name="play_service_update_error">Asegúrese de que los servicios de Google Play sean compatibles con el dispositivo y estén actualizados.</string> + <string name="play_service_update_error">Asegúrese de que Google Play services esté disponible en el dispositivo y actualizado.</string>app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
66-66: Remove unnecessary static import.The static import for
showfromProgressDialogappears to be unused in this file and should be removed to keep imports clean.-import static android.app.ProgressDialog.show;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
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/strings.xml(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(7 hunks)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 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: 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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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.
app/res/values-pt/strings.xml (1)
Learnt from: Jignesh-dimagi
PR: #3070
File: app/res/values/strings.xml:73-73
Timestamp: 2025-05-07T06:50:52.518Z
Learning: The empty remote_form_payload_url string resource in strings.xml is intentional legacy code and should be preserved as-is.
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (4)
Learnt from: OrangeAndGreen
PR: #2912
File: app/src/org/commcare/fragments/connect/ConnectPaymentSetupFragment.java:61-66
Timestamp: 2025-01-21T17:29:58.014Z
Learning: In the CommCare Android app, for non-critical convenience features like phone number auto-population, exceptions should be logged but fail silently when there's a manual fallback available. This approach prevents app crashes while maintaining the ability to debug issues through logs.
Learnt from: OrangeAndGreen
PR: #2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:66-71
Timestamp: 2025-01-21T17:28:09.007Z
Learning: The ConnectUserRecord class in CommCare Android uses @persisting annotations with sequential indices for field persistence, and @metafield annotations for fields that have corresponding META constants. Fields should be documented with Javadoc comments explaining their purpose, format requirements, and relationships with other fields.
Learnt from: OrangeAndGreen
PR: #2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:81-89
Timestamp: 2025-01-21T17:27:58.754Z
Learning: In the CommCare Android codebase, use org.jetbrains.annotations.NotNull for null-safety annotations. Empty strings are acceptable for unspecified values, but null values should be prevented using @NotNull.
Learnt from: OrangeAndGreen
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.
app/res/values/strings.xml (1)
Learnt from: Jignesh-dimagi
PR: #3070
File: app/res/values/strings.xml:73-73
Timestamp: 2025-05-07T06:50:52.518Z
Learning: The empty remote_form_payload_url string resource in strings.xml is intentional legacy code and should be preserved as-is.
app/res/values-es/strings.xml (1)
Learnt from: Jignesh-dimagi
PR: #3070
File: app/res/values/strings.xml:73-73
Timestamp: 2025-05-07T06:50:52.518Z
Learning: The empty remote_form_payload_url string resource in strings.xml is intentional legacy code and should be preserved as-is.
app/res/values-hi/strings.xml (1)
Learnt from: Jignesh-dimagi
PR: #3070
File: app/res/values/strings.xml:73-73
Timestamp: 2025-05-07T06:50:52.518Z
Learning: The empty remote_form_payload_url string resource in strings.xml is intentional legacy code and should be preserved as-is.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (8)
Learnt from: OrangeAndGreen
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.
Learnt from: OrangeAndGreen
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.
Learnt from: OrangeAndGreen
PR: #3037
File: app/src/org/commcare/connect/ConnectIDManager.java:233-243
Timestamp: 2025-04-22T15:48:29.346Z
Learning: Never instantiate Android Activity classes directly with 'new'. Activities should only be created through the Android framework using Intents.
Learnt from: OrangeAndGreen
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.
Learnt from: OrangeAndGreen
PR: #2912
File: app/src/org/commcare/fragments/connect/ConnectPaymentSetupFragment.java:61-66
Timestamp: 2025-01-21T17:29:58.014Z
Learning: In the CommCare Android app, for non-critical convenience features like phone number auto-population, exceptions should be logged but fail silently when there's a manual fallback available. This approach prevents app crashes while maintaining the ability to debug issues through logs.
Learnt from: OrangeAndGreen
PR: #3037
File: app/src/org/commcare/connect/ConnectConstants.java:11-15
Timestamp: 2025-04-21T18:48:08.330Z
Learning: Request codes used for startActivityForResult should be unique throughout the application, even if they're used in different activities. COMMCARE_SETUP_CONNECT_LAUNCH_REQUEST_CODE and STANDARD_HOME_CONNECT_LAUNCH_REQUEST_CODE should have different values.
Learnt from: shubham1g5
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.
Learnt from: shubham1g5
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.
🔇 Additional comments (8)
app/res/values-hi/strings.xml (1)
465-465: String added correctly – no issuesThe Hindi translation is concise and faithful to the English source. No placeholders are involved, so nothing further to verify.
app/res/values-sw/strings.xml (1)
471-472: String resource looks good and matches the English sourceThe new Swahili translation is concise, clear, and uses the correct formal tone. No issues spotted.
app/res/values-fr/strings.xml (1)
465-466: French translation LGTMThe phrasing is accurate and consistent with the French style used in nearby Play-Services-related strings.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (5)
5-5: LGTM: Proper imports for Google Play Services integration.The imports correctly include the necessary classes for Google Play Services availability checking and error handling.
Also applies to: 30-31
83-84: LGTM: Appropriate field declarations for Play Services handling.The fields are properly declared to support Google Play Services error tracking and resolution flow.
106-106: LGTM: Appropriate placement of Play Services check.Calling
checkGooglePlayServices()inonCreateView()ensures the check happens early in the fragment lifecycle, which is the right approach for validating critical dependencies.
129-147: LGTM: Robust Google Play Services availability checking.The implementation follows Android best practices:
- Uses
GoogleApiAvailability.getInstance()to check service status- Properly handles both user-resolvable and non-resolvable errors
- Logs errors for debugging purposes
- Uses localized error messages for user-facing content
- Integrates with the existing error handling flow via
onConfigurationFailure()The error code formatting (
"play_services_" + status) provides useful debugging information while maintaining consistency with the existing error handling patterns.
375-382: LGTM: Proper Play Services resolution launcher implementation.The launcher correctly:
- Uses
StartIntentSenderForResultcontract for handling Play Services resolution dialogs- Only triggers failure callback when user doesn't resolve the issue (
RESULT_OKnot returned)- Uses the appropriate error message and error code stored during the initial check
- Follows the established pattern used by other launchers in this fragment
Product Description
for these change in Beta release pointing to master
related to this PR
https://github.com/dimagi/commcare-android/pull/3269/files
it has Play Store version checks
and user property to analytics
Technical Summary
Feature Flag
Safety Assurance
Safety story
All thisngs are QA verified just doing the back merge
Automated test coverage
QA Plan
Labels and Review