Connect 472000 hotfix #3035
Connect 472000 hotfix #3035
Conversation
…me after logging in
…ncies-during-jcenter-sunset Remove directly bundled libraries
… is >= Tiramisu. Removed auto SMS code retrieval from deactivation OTP page (user must type it manually).
…e refresh menu option.
Logging empty push notifications
Register broadcast receivers with proper security
…tton in toolbar from each relevant fragment
…rences to Context objects. Updating tab height at bottom of page when changing tabs (to fix a vertical squishing bug).
…status is farther than remote. Handles a special case when transitioning to learning.
Additional logging for failed HQ token auth in ConnectID-managed apps.
Fix sync buttons in learn and delivery progress pages
Logs SSL exception when trying to download app
Commcare 2.56
…nto connect_468051_hotfix
Only init cache if cache is enabled
Added analytics event when database is rekeyed with new passphrase.
OrangeAndGreen
added
don't pull
cross requested
skip-integration-tests
📝 WalkthroughWalkthroughThis pull request introduces a broad set of changes across the Android application, touching on resource files, UI layouts, authentication flows, entity cache management, logging, and dependency updates. The version name in the manifest is incremented to 2.57. Several layout XML files are refactored for improved structure, constraint usage, and new UI elements, including updates to ConnectID registration and delivery progress screens. Multiple string resource files are updated for new features, improved translations, and corrected XML entity encodings. On the backend, entity cache invalidation logic is refactored, with scheduling responsibilities moved from Sequence Diagram(s)sequenceDiagram
participant User
participant App (UI)
participant ConnectManager
participant BiometricsHelper
participant PrimeEntityCacheHelper
participant WorkManager
%% ConnectID Authentication Flow (simplified)
User->>App (UI): Initiates ConnectID authentication
App (UI)->>ConnectManager: unlockConnect(activity, callback)
ConnectManager->>BiometricsHelper: authenticatePinOrBiometric(activity, biometricManager, callback)
BiometricsHelper-->>ConnectManager: Success/Failure callback
ConnectManager-->>App (UI): Callback invoked (success/failure)
%% Entity Cache Invalidation Scheduling
App (UI)->>PrimeEntityCacheHelper: scheduleEntityCacheInvalidation()
PrimeEntityCacheHelper->>WorkManager: Enqueue EntityCacheInvalidationWorker
WorkManager-->>PrimeEntityCacheHelper: Worker runs, processes cache
WorkManager->>PrimeEntityCacheHelper: On completion, schedulePrimeEntityCacheWorker()
%% EXIF Metadata Preservation During Image Scaling
App (UI)->>FileUtil: scaleAndSaveImage(originalImage, finalFilePath, maxDimen)
FileUtil->>FileUtil: Read EXIF from original image
FileUtil->>FileUtil: Scale and save image
FileUtil->>FileUtil: Copy EXIF to scaled image and save
%% Menu Handling in Fragments
App (UI)->>Fragment: onCreateView()
Fragment->>MenuHost: addMenuProvider(MenuProvider)
User->>Fragment: Selects "sync" menu action
Fragment->>Fragment: refreshData()
Possibly related PRs
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
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:
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: 2
🧹 Nitpick comments (21)
app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java (1)
234-249: Consider fixing typos in method and class names.The method name
registerBrodcastRecieverhas two spelling errors. Additionally, the class name itself has a typo (Fragmnetinstead ofFragment). While not affecting functionality, these could be fixed in a future PR.-public void registerBrodcastReciever() { +public void registerBroadcastReceiver() {app/src/org/commcare/adapters/ConnectDeliveryProgressReportAdapter.java (1)
48-66: Improved user experience with contextual remaining time messagesThe refactored code now provides more specific and user-friendly messages based on the context (today, tomorrow, days over, etc.). The switch expression is well-used and makes the code concise and readable.
Consider extracting this logic to a separate method to improve readability and maintainability:
- int pending = connectDeliveryDetails.getPendingCount(); - String remaining; - if(pending > 0) { - remaining = switch ((int) connectDeliveryDetails.getRemainingDays()) { - case 0 -> - context.getString(R.string.connect_results_summary_days_over); - case 1 -> - context.getString(R.string.connect_results_summary_remaining_today, pending); - case 2 -> - context.getString(R.string.connect_results_summary_remaining_tomorrow, pending); - default -> - context.getString(R.string.connect_results_summary_remaining_days, - pending, connectDeliveryDetails.getRemainingDays()); - }; - } else { - remaining = context.getString(R.string.connect_results_summary_visits_done); - } - holder.binding.tvRemaining.setText(remaining); + holder.binding.tvRemaining.setText(getFormattedRemainingText(connectDeliveryDetails)); // Add this method to the class: private String getFormattedRemainingText(ConnectDeliveryDetails details) { int pending = details.getPendingCount(); if(pending > 0) { return switch ((int) details.getRemainingDays()) { case 0 -> context.getString(R.string.connect_results_summary_days_over); case 1 -> context.getString(R.string.connect_results_summary_remaining_today, pending); case 2 -> context.getString(R.string.connect_results_summary_remaining_tomorrow, pending); default -> context.getString(R.string.connect_results_summary_remaining_days, pending, details.getRemainingDays()); }; } else { return context.getString(R.string.connect_results_summary_visits_done); } }app/res/values/strings.xml (3)
493-493: Consider a more direct user prompt.Current text "Please configure a screen unlock mechanism" is functional, but you might consider rephrasing for clarity or urgency, e.g. "You must set up a screen unlock method to proceed."
616-619: Refine user-facing phrases for clarity.Phrases like "Days Over" and "All Visits Done" might be perceived as abrupt. Consider re-wording to maintain a more user-friendly tone, e.g. "Time Limit Reached" or "All Visits Completed."
798-798: Fix grammatical structure.Change the string to a more grammatically correct form:
-<string name="primary_and_alternate_phone_number">Primary and alternate phone number are same</string> +<string name="primary_and_alternate_phone_number">Primary and alternate phone numbers are the same</string>app/res/values-pt/strings.xml (2)
421-421: Add comma for readability in Portuguese.Consider "Por favor, configure um mecanismo de desbloqueio de tela" to match regional writing style standards.
512-515: Improve user-friendly wording.For strings like "%d até Amanhã" and "Dias Acima," consider more natural expressions such as "Dias Excedidos" or "Prazo Encerrado" to better communicate the message.
app/res/layout/fragment_connect_delivery_progress.xml (4)
14-19: Use color resources instead of hard-coded hex.Rather than
android:background="#fff", consider referencing a color resource (e.g.,@color/white) to maintain consistency and theming control across the app.-android:background="#fff" +android:background="@color/white"
35-104: Confirm button text size for accessibility.The
RoundedButtonusesapp:roundButtonTextSize="5.0sp", which may be quite small for some users. Verify that this font size meets accessibility guidelines.
106-155: Consider switching from RelativeLayout to ConstraintLayout.RelativeLayout is functional, but aligning new UI elements might be simpler in a ConstraintLayout for consistency throughout the layout hierarchy.
178-182: Check if match_parent benefits ViewPager2.Currently
android:layout_height="wrap_content"might cause the ViewPager’s height to fluctuate if pages differ in size. If a stable height is desired, considermatch_parentor a fixed dimension for a smoother user experience.app/res/layout/fragment_signup.xml (3)
41-48: Use adaptive sizing for the logo.Enforcing a fixed height (40dp) on the logo might cause scaling issues on some screen densities. Consider a wrap_content approach with adjustable padding or constraints to maintain consistent appearance.
185-193: Centralize error color references.Here, direct usage of
@android:color/holo_red_lightmight lead to inconsistent theming. Use custom color resources for possible theming or brand guidelines.
229-240: Revisit the button label.Currently the button text retrieves
@string/review, which may be unclear for new users. Consider a clearer label like “Continue” or “Next” if it suits the signup flow better.app/src/org/commcare/tasks/EntityLoaderHelper.kt (2)
27-29: Consider constructor overloading vs. mutable property.Declaring
var factory: NodeEntityFactory?in the primary constructor, while also supporting theinBackgroundparameter, can be streamlined with an overloaded constructor or a more explicit injection pattern.
36-50: Multiple factory strategies are logical.Deferring factory creation based on detail optimization or async strategy is a clear approach. If expansion is planned, you might consider an injection framework for better maintainability.
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (2)
142-147: Potential measuring timing issue
Adjusting theViewPager2height based on the selected fragment’s measured height is a helpful approach, but ensure the measurement is valid if layout inflation is delayed. Consider using aview.postor a layout listener if you notice sizing glitches.
223-232: Avoid duplicating menu handling
Since you now useMenuProvider, thisonOptionsItemSelectedoverride may be redundant. Consider removing it to avoid confusion, if not needed:-@Override -public boolean onOptionsItemSelected(MenuItem item) { - if(item.getItemId() == R.id.action_sync) { - refreshData(); - return true; - } - return false; -}app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (2)
103-108: Exception logging clarity
Including the relevant details in"Exhausted biometrics"logging is good. Consider clarifying the'configured' vs 'allowed'distinction in the log message for future troubleshooting.
209-214: Obsolete password lock route
This code path logs an exception and then finishes. If this legacy path is truly unused, consider removing it to avoid confusion or accidental invocation.app/src/org/commcare/utils/BiometricsHelper.java (1)
80-92: Unified PIN or biometric launch
authenticatePinOrBiometricmerges flows effectively. Consider clarifying prompt text if the user has both options simultaneously.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (67)
app/AndroidManifest.xml(1 hunks)app/assets/locales/android_translatable_strings.txt(1 hunks)app/build.gradle(7 hunks)app/res/layout/fragment_connect_delivery_details.xml(2 hunks)app/res/layout/fragment_connect_delivery_progress.xml(2 hunks)app/res/layout/fragment_signup.xml(2 hunks)app/res/layout/view_job_card.xml(3 hunks)app/res/layout/view_progress_job_card.xml(3 hunks)app/res/navigation/nav_graph_connectid.xml(1 hunks)app/res/values-es/strings.xml(1 hunks)app/res/values-fr/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(4 hunks)app/res/values-sw/strings.xml(1 hunks)app/res/values-ti/strings.xml(4 hunks)app/res/values/strings.xml(5 hunks)app/src/org/commcare/AppUtils.java(1 hunks)app/src/org/commcare/CommCareApplication.java(3 hunks)app/src/org/commcare/activities/EntitySelectActivity.java(2 hunks)app/src/org/commcare/activities/LoginActivity.java(2 hunks)app/src/org/commcare/activities/StandardHomeActivityUIController.java(1 hunks)app/src/org/commcare/activities/SyncCapableCommCareActivity.java(4 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(1 hunks)app/src/org/commcare/activities/connect/ConnectIdActivity.java(2 hunks)app/src/org/commcare/adapters/ConnectDeliveryProgressReportAdapter.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java(5 hunks)app/src/org/commcare/android/database/connect/models/ConnectPaymentUnitRecord.java(2 hunks)app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java(1 hunks)app/src/org/commcare/android/logging/ForceCloseLogSerializer.java(0 hunks)app/src/org/commcare/connect/ConnectManager.java(2 hunks)app/src/org/commcare/connect/database/ConnectAppDatabaseUtil.java(1 hunks)app/src/org/commcare/connect/database/ConnectDatabaseHelper.java(3 hunks)app/src/org/commcare/connect/database/ConnectJobUtils.java(2 hunks)app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java(1 hunks)app/src/org/commcare/engine/resource/ResourceInstallUtils.java(1 hunks)app/src/org/commcare/entity/AndroidAsyncNodeEntityFactory.kt(2 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java(8 hunks)app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java(4 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java(3 hunks)app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java(5 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java(0 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java(2 hunks)app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java(1 hunks)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java(1 hunks)app/src/org/commcare/logging/DeviceReportWriter.java(3 hunks)app/src/org/commcare/logging/analytics/TimedStatsTracker.java(2 hunks)app/src/org/commcare/network/CommcareRequestGenerator.java(1 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(3 hunks)app/src/org/commcare/tasks/DataPullTask.java(3 hunks)app/src/org/commcare/tasks/EntityCacheInvalidationWorker.kt(2 hunks)app/src/org/commcare/tasks/EntityLoaderHelper.kt(6 hunks)app/src/org/commcare/tasks/EntityLoaderTask.java(1 hunks)app/src/org/commcare/tasks/ManageKeyRecordTask.java(4 hunks)app/src/org/commcare/tasks/PrimeEntityCache.kt(1 hunks)app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt(6 hunks)app/src/org/commcare/utils/BiometricsHelper.java(6 hunks)app/src/org/commcare/utils/FileUtil.java(4 hunks)app/src/org/commcare/views/dialogs/AlertDialogFragment.java(0 hunks)app/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.java(2 hunks)app/unit-tests/src/org/commcare/android/tests/formsave/CyclicCasesPurgeTest.java(2 hunks)
💤 Files with no reviewable changes (3)
- app/src/org/commcare/android/logging/ForceCloseLogSerializer.java
- app/src/org/commcare/views/dialogs/AlertDialogFragment.java
- app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java
🧰 Additional context used
🧠 Learnings (1)
app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (1)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:235-236
Timestamp: 2025-02-04T21:29:29.594Z
Learning: The empty performPasswordUnlock method in ConnectIdBiometricConfigFragment is intentionally left empty and should not be flagged in reviews.
🧬 Code Graph Analysis (14)
app/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.java (1)
app/src/org/commcare/CommCareApplication.java (1)
CommCareApplication(155-1270)
app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (1)
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
ConnectDatabaseHelper(28-125)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1)
CCAnalyticsEvent(7-55)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java (1)
app/src/org/commcare/activities/CommCareActivity.java (1)
CommCareActivity(82-994)
app/src/org/commcare/network/CommcareRequestGenerator.java (1)
app/src/org/commcare/connect/ConnectManager.java (1)
ConnectManager(86-1066)
app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java (1)
app/src/org/commcare/connect/ConnectManager.java (1)
ConnectManager(86-1066)
app/src/org/commcare/connect/database/ConnectJobUtils.java (2)
app/src/org/commcare/connect/ConnectManager.java (1)
ConnectManager(86-1066)app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
ConnectDatabaseHelper(28-125)
app/src/org/commcare/tasks/DataPullTask.java (1)
app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (1)
scheduleEntityCacheInvalidation(89-101)
app/src/org/commcare/tasks/EntityLoaderTask.java (2)
app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (2)
entityLoaderHelper(36-256)evalCtx(212-214)app/src/org/commcare/tasks/EntityLoaderHelper.kt (1)
detail(23-131)
app/src/org/commcare/activities/connect/ConnectIdActivity.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-79)
app/src/org/commcare/CommCareApplication.java (1)
app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (1)
clearState(219-226)
app/src/org/commcare/activities/LoginActivity.java (1)
app/src/org/commcare/connect/ConnectManager.java (1)
ConnectManager(86-1066)
app/src/org/commcare/tasks/EntityLoaderHelper.kt (1)
app/src/org/commcare/models/database/user/models/CommCareEntityStorageCache.java (1)
CommCareEntityStorageCache(42-438)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (2)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java (1)
ConnectDeliveryProgressDeliveryFragment(33-179)app/src/org/commcare/fragments/connect/ConnectResultsSummaryListFragment.java (1)
ConnectResultsSummaryListFragment(34-297)
🔇 Additional comments (155)
app/src/org/commcare/logging/DeviceReportWriter.java (3)
3-3: Import ofandroid.os.Buildis correct and necessary.This import is required for accessing device model and Android version information.
102-103: Device model and Android version are now included in the report header.Adding
device_modelandandroid_versionto the device report header improves diagnostics and device tracking. The use ofBuild.MODELandBuild.VERSION.RELEASEis standard and appropriate.
132-133: Method signature reformatting improves readability.Splitting the
throwsclause across two lines is a stylistic improvement and does not affect functionality.app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java (1)
245-245: Good security enhancement for newer Android versions.The change to check for Android SDK version Tiramisu (API 33) instead of Oreo (API 26) is appropriate. Using the
RECEIVER_NOT_EXPORTEDflag for broadcast receivers on newer Android versions aligns with security best practices and prevents unintended exposure to other applications.app/res/values-no/strings.xml (1)
152-152: Correct XML entity escaping for<manifest>tag.The update properly escapes the
<manifest>tag for display in the error message, fixing a localization bug.app/res/values-fr/strings.xml (1)
151-151: Correct XML entity escaping for<manifest>tag.The update properly escapes the
<manifest>tag for display in the error message, fixing a localization bug.app/res/values-es/strings.xml (1)
151-151: Correct XML entity escaping for<manifest>tag.The update properly escapes the
<manifest>tag for display in the error message, fixing a localization bug.app/AndroidManifest.xml (1)
5-5: Version name updated for new build.The version bump from 2.56 to 2.57 is correct for a new release or hotfix.
app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1)
53-53: Add analytics event constant for database rekeying.The new constant is well-named and fits the existing pattern for analytics events.
app/src/org/commcare/engine/resource/ResourceInstallUtils.java (1)
266-268: Good addition of diagnostic logging for SSL exceptions.Adding explicit logging for SSL exceptions will help diagnose connectivity and certificate issues during resource installation. This is especially valuable for troubleshooting field deployments.
app/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.java (2)
36-36: Appropriate import for WorkManager.This import is needed for the WorkManager initialization added to the test.
109-109: Essential WorkManager initialization for entity cache operations.Initializing WorkManager before launching EntitySelectActivity ensures that the test environment properly supports entity cache operations, which appear to use WorkManager for scheduling in the real application.
app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java (1)
140-140: Fixed variable name and resource ID for job description.The variable name and corresponding view ID were corrected from "discrepation" to "description", which improves code clarity and consistency with layout resources.
Also applies to: 148-148
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java (2)
19-19: Required import for CommCareActivity.This import is needed for the type checking and casting in the added navigation code.
87-89: Improved navigation with "up" button support.Adding the up button to the action bar enhances user navigation, providing a consistent way to return to the previous screen. The implementation properly checks for null values and appropriate type before making the change.
app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java (1)
396-402: Reordering of registration phase setup improves state transition sequenceThe code now sets the registration phase after setting up navigation and updating the user record, creating a more logical sequence of operations. This ensures the UI state (navigation) and user data are prepared before changing the application phase.
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
515-517: New analytics tracking for database rekey eventsThe added method allows tracking database rekey operations, which enhances observability of security-related operations within the app.
app/src/org/commcare/logging/analytics/TimedStatsTracker.java (1)
9-10: Improved logging with centralized utilitiesThe change replaces Android's native
Log.icalls with the app's centralizedLogger.logutility using specific log types. This promotes consistent log handling and better integration with the app's analytics and error reporting infrastructure.Also applies to: 62-65
app/res/navigation/nav_graph_connectid.xml (1)
290-292: Enhanced navigation flow with direct path to secondary phone screenAdding a direct navigation action from signup fragment to secondary phone fragment improves the ConnectID registration flow and aligns with other navigation and validation improvements in the registration process.
app/src/org/commcare/activities/StandardHomeActivityUIController.java (2)
85-85: Fixed variable name typo for better code clarityChanged variable name from likely
tvJobDiscrepationtotvJobDescriptionfor clarity and consistency.
92-92: Fixed setText reference to match the renamed variableCorresponding update to use the renamed variable
tvJobDescriptionwhen setting text content.app/res/values-lt/strings.xml (1)
152-152: Fixed XML entity encoding for proper renderingCorrected the XML entity encoding from the incorrect
&lt;manifest\>to the proper<manifest\>for correct rendering of angle brackets in UI.app/src/org/commcare/tasks/EntityCacheInvalidationWorker.kt (2)
17-19: Added check for empty cache before processingGood defensive programming by adding a check to avoid unnecessary processing when the cache is empty.
23-26: Added finally block to ensure worker schedulingGreat improvement to ensure that
PrimeEntityCacheHelper.schedulePrimeEntityCacheWorker()is always called, regardless of whether an exception occurs or not.app/src/org/commcare/services/CommCareFirebaseMessagingService.java (2)
222-225: Added logging for empty push notificationsGood addition to log an exception when both notification title and text are empty or whitespace, which helps with debugging notification issues.
237-237: Improved code formattingMinor formatting improvement by adding a space after the
ifkeyword for better code consistency.app/res/layout/view_job_card.xml (1)
28-28: LGTM - Fixed typo in TextView IDThe ID was correctly changed from
tv_job_discrepationtotv_job_descriptionand all corresponding layout constraints were updated accordingly.Also applies to: 47-47, 58-58
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (1)
172-172: LGTM - Method simplified to return value directlyThe
getSecondaryPhoneVerifyByDate()method was simplified to directly return the field value without conditional checking, following standard getter pattern best practices.app/src/org/commcare/tasks/DataPullTask.java (2)
454-454: LGTM - Refactored cache invalidation schedulingReplaced application instance method call with static helper method, centralizing entity cache invalidation logic in
PrimeEntityCacheHelper.
483-483: LGTM - Added helpful log statementAdded informative log statement before releasing user resources, improving maintainability and debugging capabilities.
app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (1)
57-57: LGTM - Added proper database teardownAdded call to
ConnectDatabaseHelper.teardown()after resetting the broken flag, ensuring proper database connection cleanup when forgetting a user.app/res/layout/view_progress_job_card.xml (3)
28-28: Fixed typo in TextView ID.The ID has been properly corrected from the misspelled
tv_job_discrepationto the correcttv_job_description, improving code readability and alignment with other parts of the codebase.
48-48: Updated constraint reference to use corrected ID.Properly updated the constraint reference to use the renamed TextView ID.
58-58: Updated constraint reference to use corrected ID.Properly updated another constraint reference to use the renamed TextView ID, maintaining layout consistency.
app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java (1)
125-132: Added validation to prevent primary and secondary phone numbers from being identical.Added important validation logic that prevents users from using the same phone number for both primary and secondary contacts. This enhancement improves user experience by providing immediate feedback when the numbers match.
The implementation:
- Retrieves the user's primary phone number
- Compares it with the entered secondary phone number
- Shows an error message and disables the continue button if they match
- Hides the error message when they don't match
app/src/org/commcare/android/database/connect/models/ConnectPaymentUnitRecord.java (2)
22-23: Added constant for server-side identifier.Added a well-documented constant
META_IDto represent the server-side identifier key, with a helpful comment explaining the difference between server and local database naming conventions.
63-63: Updated JSON field mapping to use server-side identifier.Changed the JSON field mapping to use the server-provided
idfield instead ofunit_id, aligning with server response format while maintaining the local database field structure.app/src/org/commcare/network/CommcareRequestGenerator.java (1)
187-190: Simplified error handling using centralized ConnectManager check.Replaced complex try-catch logic with a streamlined call to
ConnectManager.checkForFailedConnectIdAuth(username), which encapsulates the ConnectID authentication failure detection. This refactoring simplifies the code and centralizes the error condition check.app/src/org/commcare/activities/connect/ConnectActivity.java (1)
185-185: Improved architecture using MenuProvidersThe comment documents an important architectural change where fragments now handle the sync button actions individually through MenuProviders, rather than delegating through the activity.
This change aligns with modern Android development practices and improves component separation, allowing each fragment to manage its own menu interactions in a lifecycle-aware manner.
app/src/org/commcare/entity/AndroidAsyncNodeEntityFactory.kt (2)
56-62: Adding factory reference for improved cache coordinationThe addition of
thisparameter properly passes the factory instance to the prime cache helper, supporting coordinated caching operations.This change is part of a broader refactoring of the entity caching mechanism, improving coordination between components.
70-76: Adding factory reference for improved cache coordinationSimilar to the previous change, the factory instance is now passed to the prime cache helper for better coordination of entity loading and caching.
This maintains consistency with the earlier change, ensuring the factory reference is provided in all cases.
app/assets/locales/android_translatable_strings.txt (1)
209-210: Updated version identification string formatsThe parameter ordering in the version identification strings has been updated to reflect changes in how version information is passed to these strings.
These changes align with modifications in
AppUtils.javaand the version increment to 2.57 in the manifest, ensuring consistent version reporting throughout the app.app/unit-tests/src/org/commcare/android/tests/formsave/CyclicCasesPurgeTest.java (4)
3-6: Improved imports with static assertionsUpdated import statements to use static imports for assertions, improving code readability.
Static imports for assertions help make test code more concise and readable.
11-12: Updated to AndroidX test runnerUpdated the JUnit runner import to use AndroidX's version.
This change is part of the migration to AndroidX, which is the recommended approach for modern Android development.
59-60: Enhanced UI thread task executionAdded explicit call to run UI thread tasks including delayed ones before idling the main looper.
This change ensures all UI thread tasks, including those with delays, are executed before proceeding with the test, improving test reliability.
62-68: Improved dialog verification approachReplaced the previous approach of accessing the dialog message with a more direct and robust method using
ShadowDialog.getLatestDialog().The new approach:
- Gets the dialog directly from the shadow
- Verifies the dialog exists and is showing
- Extracts the error message text directly from the dialog's view
This makes the test more reliable by explicitly verifying the state of the dialog before testing its content.
app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java (7)
5-7: Added menu-related imports for AndroidXThese new imports are necessary for implementing the AndroidX menu provider pattern that's being added to this fragment.
29-35: Updated to AndroidX annotations and added menu-related importsThe code now properly uses AndroidX annotations and imports the necessary classes for menu handling. This aligns with modern Android development practices.
84-100: Implemented AndroidX MenuHost/MenuProvider patternThe fragment now uses the recommended AndroidX approach for handling menu items, which provides better lifecycle awareness and cleaner separation of concerns. The implementation correctly:
- Gets the MenuHost from the activity
- Adds a MenuProvider with proper lifecycle scoping
- Handles the sync action menu item
This is a good architectural improvement.
306-306: Fixed variable name typoChanged from
tvJobDiscrepationtotvJobDescription, which matches the actual purpose of this text view.
312-312: Updated method call to use corrected variable nameThe method call now correctly uses the fixed variable name
tvJobDescription.
5-7: Improved menu handling with AndroidX's MenuHost/MenuProvider APIsThe addition of imports and implementation of MenuHost/MenuProvider APIs modernizes the menu handling approach. Using these APIs properly integrates with the fragment's lifecycle through
getViewLifecycleOwner()andLifecycle.State.RESUMED, ensuring menu items are only active when the fragment is visible and interactive.Also applies to: 29-35, 84-100
306-312: Fixed variable name typoThe variable name has been changed from
tvJobDiscrepationtotvJobDescription, which correctly reflects its purpose and maintains consistency with other parts of the codebase.app/src/org/commcare/activities/EntitySelectActivity.java (4)
499-500: Added null check to prevent potential NullPointerExceptionThis change prevents a crash when accessing properties of a potentially null triple object from the observer.
1023-1036: Improved progress calculation and presentationThe progress calculation has been enhanced to:
- Calculate a weighted progress percentage across multiple phases when caching is enabled
- Only update the UI when the progress value actually changes (preventing unnecessary updates)
- Use a consistent "Loading..." message instead of phase-specific messages
This change provides a smoother, more intuitive progress indication to users during entity loading.
499-500: Added null check to prevent potential NPEGood addition of a null check when accessing the observed triple to prevent null pointer exceptions. This makes the code more robust when handling asynchronous data updates.
1023-1036: Improved progress calculation logicThe progress reporting has been significantly improved:
- Added support for weighted progress calculation across multiple phases when caching is enabled
- Optimized UI updates by only refreshing the display when the progress value actually changes
- Simplified the progress message to use a standard "Loading x%" format
This change enhances the user experience by providing more accurate progress feedback during entity loading operations.
app/src/org/commcare/tasks/EntityLoaderTask.java (2)
45-45: Updated constructor call to match new EntityLoaderHelper signatureThe constructor now passes
nullfor the new optional factory parameter in EntityLoaderHelper. This aligns with the updated signature of EntityLoaderHelper which now accepts an optional NodeEntityFactory parameter.
45-45: Updated constructor to support the new EntityLoaderHelper APIThe constructor now passes a
nullparameter for the factory argument to theEntityLoaderHelperconstructor, aligning with the updated API that accepts an optional factory parameter.Based on the provided context snippets, this change is part of a broader enhancement to the entity cache management system, where
EntityLoaderHelpernow properly coordinates with the prime entity cache helper to cancel and reschedule background cache work during synchronous entity loading.app/src/org/commcare/AppUtils.java (2)
175-176: Updated version string parameter orderingThe parameter order for the localization call has been modified, removing BuildConfig.VERSION_NAME from the parameters list. This change should align with corresponding updates in the localization string resources.
Make sure this change is coordinated with updates to the localization string with key
app_version_stringin the resource files to ensure the parameters are correctly matched.
176-176:Details
✅ Verification successful
Removed BuildConfig.VERSION_NAME from version string parameters
The version information formatting has been updated by removing
BuildConfig.VERSION_NAMEfrom the parameter array passed toLocalization.get. This change likely aligns with modifications to the version identification string templates in localization resources.
🏁 Script executed:
#!/bin/bash # Check for corresponding changes in localization strings grep -r "app_version_string" app/assets/locales/ app/res/Length of output: 334
Change Verified: Updated Version Parameter Formatting
- The removal of
BuildConfig.VERSION_NAMEfrom the parameters passed toLocalization.getinapp/src/org/commcare/AppUtils.java(line 176) has been verified.- A check of the localization strings in
app/res/values/strings.xml,app/res/values-small/strings.xml, andapp/res/values-sw/strings.xmlconfirms that the version formatting templates (e.g.,version.id.long,version.id.short,toleo.id.nde) are consistent with the updates.app/res/layout/fragment_connect_delivery_details.xml (1)
158-190: Improved layout structure with ConstraintLayoutThe refactoring from LinearLayout to ConstraintLayout provides better layout flexibility and performance. The use of constraint-based positioning offers more robust behavior across different screen sizes.
app/src/org/commcare/connect/database/ConnectJobUtils.java (2)
14-14: Import added to support ConnectID configuration check
414-422: Added ConnectID configuration check before database queryThis defensive change prevents unnecessary database queries when ConnectID is not configured, improving efficiency and preventing potential errors.
app/src/org/commcare/connect/database/ConnectAppDatabaseUtil.java (2)
6-6: Import added to support ConnectID configuration check
12-21: Added ConnectID configuration check before database querySimilar to the change in ConnectJobUtils, this defensive programming prevents unnecessary database operations when ConnectID is not configured, maintaining consistency across the codebase.
app/src/org/commcare/activities/SyncCapableCommCareActivity.java (4)
25-25: Import for PrimeEntityCacheHelperThis import supports the refactored entity cache invalidation approach.
34-34: Import for LoggerRequired for the new logging functionality in the authentication failure handling.
109-114: Improved ConnectID authentication failure handlingThe code now centrally detects ConnectID authentication failures and logs them appropriately, enhancing the diagnostic capabilities of the application.
278-278: Refactored entity cache invalidationSchedule entity cache invalidation is now delegated to PrimeEntityCacheHelper, improving modularity and separation of concerns.
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (1)
106-122: MenuProvider implementation correctly follows modern Android lifecycle-aware patternsThe refactoring from overriding
onOptionsItemSelectedto using AndroidX'sMenuHostandMenuProviderAPIs is a good modernization that properly integrates with the fragment lifecycle. The implementation correctly:
- Attaches the menu provider to the fragment's view lifecycle
- Sets the active state to RESUMED
- Handles the sync action appropriately
app/src/org/commcare/tasks/ManageKeyRecordTask.java (4)
126-128: Good addition of error logging for null user after key record requestAdding detailed error logging when the logged-in user is unexpectedly null helps with diagnostics and troubleshooting session issues.
140-141: Improved session management observability with maintenance loggingThis logging statement clearly identifies when and why a user session is being closed, which will help track down authentication and session lifecycle issues.
517-517: Enhanced user setup loggingThe maintenance-level logging for user setup provides better visibility into the session establishment process.
522-523: Added exception logging for session availabilityThis log accurately captures when session retrieval fails after login, which is an important diagnostic point.
app/src/org/commcare/tasks/PrimeEntityCache.kt (2)
14-14: Nullable variable declaration is a necessary changeMaking
primeEntityCacheHelpernullable aligns with the broader entity caching system changes mentioned in the summary.
26-27: Appropriate use of safe calls in cleanup methodsThe safe calls (
?.) in thefinallyblock andonStopped()method correctly handle the possibility of null values, preventing potential null pointer exceptions during cleanup operations.Also applies to: 32-33
app/src/org/commcare/activities/connect/ConnectIdActivity.java (2)
37-38: Added safety comment for PIN unlock pathThe comment clarifies an important constraint: PIN unlock requests should only happen when the BiometricConfig fragment is active to prevent crashes. This is a good defensive programming practice.
104-107: Implemented previously commented-out case for alternate phone registrationThis change properly implements navigation for the
CONNECT_REGISTRATION_ALTERNATE_PHONEcase that was previously commented out. It correctly uses the navigation component's directions pattern to transition from the signup fragment to the secondary phone fragment with the appropriate parameters.app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (4)
14-14: Added Firebase Analytics importAdded import for the FirebaseAnalyticsUtil class that will be used for reporting database rekeying events.
36-41: Enhanced database rekeying with analyticsThe implementation now only rekeys the database when necessary (when it's open and the passphrase has changed) and reports this important security event through analytics.
43-44: Improved passphrase storage commentAdded a clarifying comment explaining that the received passphrase is stored as what's in use locally, providing better code documentation.
77-78: Added logging for legacy database accessThis change adds valuable diagnostic information when the database is accessed using the legacy byte array passphrase method, including whether the passphrase is encrypted or not. This will help identify and monitor outdated code patterns that should be migrated.
app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java (3)
268-271: Improved error message handling for phone number validationThe code now consistently makes the error text view visible before checking phone validity, providing better visual feedback to users during the validation process.
270-275: Enhanced validation for alternate phone numbersAdded specific error handling when a user tries to use their existing alternate phone number as their primary phone, showing a clear error message. Otherwise, it shows a "checking" message to indicate that validation is in progress.
293-293: Fixed indentation for the error response handlerMinor formatting improvement for better code readability.
app/src/org/commcare/activities/LoginActivity.java (3)
560-561: Added null-safety check for app selectionThe change adds protection against potential
IndexOutOfBoundsExceptionby checking if the dropdown list is empty before trying to access an element at the selected index. This is a good defensive programming practice.
562-564: Simplified app state logicImproved the logic for determining the ConnectID app management state by using a direct condition check rather than nested conditionals. This makes the code more readable and maintainable.
876-879: Added ConnectID authentication failure detection and loggingThis enhancement checks for failed ConnectID authentication during login and logs detailed information when token authentication fails for ConnectID-managed apps. This will help with diagnosing authentication issues.
app/src/org/commcare/CommCareApplication.java (3)
370-370: Improved logging for user session lifecycle events.Adding detailed logging when closing a user session to start a new one will help with debugging and monitoring session lifecycle events.
383-383: Improved logging for user session lifecycle events.Adding detailed logging when closing a user session will help with debugging and monitoring session lifecycle events.
820-821: Improved entity cache management.Clearing the state of the prime entity cache helper before scheduling the prime entity cache worker ensures a clean state for the new cache operation, which is good practice. This change aligns with the centralization of cache invalidation scheduling in
PrimeEntityCacheHelper.app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java (4)
78-80: Good practice: Added time formatting constants.Creating constants for time formatting patterns improves maintainability and reduces the risk of inconsistencies throughout the code.
212-212: Code simplification: Improved boolean assignment.Using
json.optBoolean(META_IS_ACTIVE, true)directly is cleaner and more straightforward than the previous implementation, with the same functionality.
443-449: Improved date calculation logic for project validity.This change correctly accounts for the fact that a project with an end date is valid until midnight, not just until 00:00. Adding almost 24 hours (86399999 ms) to the time difference ensures that the days remaining calculation is more accurate.
460-475: Added backward compatibility for time formatting.Good implementation of conditional time formatting based on SDK version. Using
DateTimeFormatterfor newer Android versions (26+) while falling back toSimpleDateFormatfor older versions ensures both backward compatibility and optimal performance.app/src/org/commcare/connect/ConnectManager.java (3)
254-254: Simplified biometric authentication call.The code simplifies the fingerprint authentication call by removing the unnecessary parameter, making the code cleaner.
260-261: Improved user feedback for authentication failures.Adding a toast notification when no unlock method is available improves the user experience by providing clear feedback about why the unlock failed. The accompanying log entry is also good for debugging purposes.
575-588: Added centralized ConnectID authentication status check.This new method centralizes the logic for checking if ConnectID authentication has failed, with proper exception handling and logging. It checks if ConnectID is configured and if the app record exists and is worker-linked, which helps standardize authentication failure handling across the app.
app/src/org/commcare/utils/FileUtil.java (5)
10-10: Updated to AndroidX ExifInterface library.Migrating to the AndroidX version of ExifInterface provides more consistent behavior across different Android versions.
68-88: Added comprehensive EXIF metadata tag list.The EXIF_TAGS array provides a complete set of metadata tags to preserve when processing images, ensuring important information like GPS coordinates, timestamps, orientation, and copyright is retained.
621-635: Added method to preserve EXIF metadata.The new
copyExifDatamethod provides a clean way to transfer EXIF metadata from the original image to a scaled image, while updating dimension tags appropriately. This is essential for maintaining important metadata during image processing.
651-657: Added EXIF data extraction before image scaling.Reading the original EXIF data before scaling ensures this valuable metadata is preserved for later use.
666-674: Preserved EXIF metadata in scaled images.Updating the image saving process to copy and save EXIF metadata ensures that critical information like GPS coordinates, timestamps, and orientation is retained in scaled images.
app/res/values-sw/strings.xml (1)
1-611: LGTM! Comprehensive Swahili translation appears complete and properly formatted.The file contains a complete localization of all application strings to Swahili, covering all essential UI elements, messages, and features. All placeholder variables (%s, %d) are preserved correctly, and the translations appear to maintain consistent terminology.
app/build.gradle (7)
26-26: Added Maven repository for Liferay correctly.The Liferay public repository addition supports the updated dependencies while following standard Gradle repository definition patterns.
35-35: Added AndroidX ExifInterface dependency for improved image handling.This dependency supports proper EXIF metadata handling during image operations, which is critical for preserving image orientation and metadata during scaling operations.
76-76: Simplified dependency management by using remote SimPrints library.Replaced multiple local AAR dependencies with a single remote dependency, which improves maintainability and version tracking.
108-110: Updated Firebase dependencies to more recent versions.Security and feature improvements from upgrading:
- firebase-analytics from 17.5.0 to 20.1.2
- firebase-crashlytics from 17.2.1 to 18.3.7
127-134: Improved Kujaku dependency management.Explicitly managing Volley and Storage dependencies while excluding them from Kujaku reduces potential version conflicts and provides more control over the dependency graph.
587-590: Improved instrumentation test detection with regex pattern.Replaced hardcoded task names with a reusable function that uses regex pattern matching, making the build script more maintainable and flexible.
671-674: Added helper function for test task identification.The implementation uses a clean regex pattern that matches both "assemble" and "connected" task prefixes with "AndroidTest" suffixes, covering all instrumentation test scenarios.
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressDeliveryFragment.java (6)
121-122: LGTM - Minor syntax improvement.Small readability enhancement while maintaining the same functionality.
148-150: Improved delivery counting logic.The refactored code more clearly computes the remaining deliveries as the difference between maximum allowed and approved counts, improving readability and maintainability.
152-153: Simplified total amount calculation.Using totalApproved directly in the calculation makes the code more consistent and easier to follow.
155-155: Moved days remaining calculation to job object.Replacing local calculation with
job.getDaysRemaining()centralizes this logic in the job class where it logically belongs, promoting better code organization.
157-158: Enhanced percentage calculation with null safety.Added a guard clause to prevent division by zero when calculating the approval percentage.
162-163: Consistent use of calculated values.Setting pendingCount to the previously calculated remaining value improves consistency and reduces redundant calculations.
app/res/values-ti/strings.xml (4)
211-211: Fixed XML entity encoding in error message.Correctly escaped XML entities in the root_element_error string, ensuring proper display of angle brackets.
409-409: Added missing unlock availability string resource.This string is needed to properly inform users when screen unlock mechanisms need configuration.
428-428: Added configuration failure message with parameter support.Provides informative error feedback with parameter placeholder (%s) for detailed error information.
496-499: Added Connect results summary strings for time-based reporting.These four new string resources support displaying remaining visits with different timeframes (tomorrow, today) and completion states (all visits done, days over).
app/res/values-pt/strings.xml (1)
223-223: Escaped root element fix looks good.This change ensures proper XML escaping of
<manifest>. No issues found.app/res/layout/fragment_connect_delivery_progress.xml (2)
2-7: NestedScrollView with fillViewport.Switching to a NestedScrollView and enabling fillViewport is a standard approach for scrollable content. No immediate concerns found.
20-26: Sufficient accessibility for refresh icon?The
contentDescriptionreferences a string resource, which is good for localization. Be sure this icon is purely decorative or has a meaningful content description to assist screen readers.app/res/layout/fragment_signup.xml (2)
3-9: Adopting ConstraintLayout as the root.Using ConstraintLayout provides flexibility for positioning elements. This approach typically improves performance over nested linear or relative layouts. Looks good.
208-218: Confirm accessibility label for CheckBox.Ensure the check box is readily parsed by screen readers. If the descriptive text is separate, either supply a contentDescription or confirm the view’s label is properly associated with the check box.
app/src/org/commcare/tasks/EntityLoaderHelper.kt (3)
61-64: Concurrent load concerns.Canceling background cache work before synchronous load avoids DB lock conflicts. Ensure no concurrent tasks could override each other if multiple entity loaders run simultaneously.
75-77: Good practice resuming the cache worker.Rescheduling the prime entity cache after synchronous loading ensures the cache remains correct and up to date. No issues found.
129-129: Check for null safety.Calling
factory!!.markAsCancelled()uses a non-null assertion. While the logic ensuresfactoryisn’t null by initialization time, confirm no external calls can forcibly unsetfactorybefore cancellation.app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (6)
6-8: Imports for menu handling look fine
These imports neatly support the subsequent menu-related logic in the fragment.
16-17: Good move toMenuHost/MenuProvider
Using AndroidXMenuHostandMenuProvideraligns with modern menu handling approaches.
54-54: Confirm that switchingConnectRegularTextViewtoTextViewpreserves styling
If any custom property was tied toConnectRegularTextView, ensure it’s still applied after switching to a plainTextView.
74-74: Method access and annotations
PubliconCreateViewwith@NonNullparameters is consistent with common patterns. No issues here.
172-189: Menu creation viaMenuProvider
Implementing the in-fragment menu provider is clear and cohesive with the rest of the fragment. Ensure that if you have multiple providers, the order of creation is deliberate.
203-211: Use of medium text for job description
Switching toConnectMediumTextViewfor job description ensures consistent styling with other Connect UI elements. Implementation looks good.app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (6)
38-38: Doc comment update
Changing the class comment to reflect its biometric configuration purpose is accurate.
60-60: ConsistentonCreateViewoverride
The fragment’sonCreateViewsignature is clear; no concerns here.
97-99: Auto PIN fallback
Automatically trying PIN after fingerprint errors is a convenient approach. Ensure users are aware that a fallback was triggered to avoid confusion in the UX.
135-148: Early exit when no biometrics are available
Immediately callingfinish(true, true);if neither fingerprint nor PIN is available is fine, but ensure the user receives a suitable UI message clarifying why they can’t proceed.
153-170: Adaptive button text
Setting button text based on the biometric configuration statuses ensures clarity for end-users.
203-203: State refresh on resume
CallingupdateState()inonResumehelps keep the biometric UI current with any external changes in device settings.app/src/org/commcare/utils/BiometricsHelper.java (6)
14-22: New imports
All newly added imports are valid and align with the updated code changes.
46-46: Straightforward fingerprint status check
ReturningcheckFingerprintStatus(...) == Configuredis clean and concise.
53-56: Consolidated fingerprint authentication
Delegating toauthenticatePinOrBiometriccentralizes logic well.
60-66: Simplified PIN status checks
Relying oncheckStatusfor PIN ensures consistent logic with fingerprint checks.
120-128: Logging unhandled biometric status
Raising an exception for an unexpected status is helpful for debug. If you find a scenario hitting this path often, add a user-facing notice.
158-159: Configuration failure exception
If enrollment setup is unavailable, ensure the user sees a suitable fallback or help text.app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (7)
7-15: Flow system change from StateFlow to SharedFlowThe imports reflect a shift in how completionStatus and cachedEntitiesState are handled, changing from state-based to event-based flow architecture. This is appropriate for events that don't need to maintain current state values.
52-56: Design improvement: event-based flow implementationSwitching from
MutableStateFlowtoMutableSharedFlowwithreplay = 0changes these flows from state-based to purely event-based. This is appropriate when we only care about new events rather than maintaining a current value.
59-59: Adding explicit nullability to progressStateAdding explicit nullability to the LiveData return type improves type safety and code clarity.
64-64: New cache invalidation scheduling mechanismThe addition of the
scheduleEntityCacheInvalidation()method provides a centralized way to schedule cache invalidation work. UsingOutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUESTis a good choice for non-critical background work.Also applies to: 89-101
72-82: Improved resource efficiency with session checksAdding checks for session activity and entity caching capability before scheduling work prevents unnecessary background operations, improving battery life and resource usage.
194-195: Improved flexibility with optional factory parameterAdding an optional factory parameter allows for more flexible entity loading strategies and better coordination between background and synchronous operations.
120-122:Details
✅ Verification successful
Thread blocking concerns with runBlocking
Using
runBlockingto emit values can block the current thread. This could potentially cause UI freezes if called from the main thread.Please verify this code is not called from the main thread by running:
Also applies to: 143-145, 205-207
🏁 Script executed:
#!/bin/bash # Check if these methods are called from the main thread rg -A 3 -B 3 "primeEntityCache\(\)" --glob "*.kt" --glob "*.java" rg -A 3 -B 3 "primeEntityCacheForDetail\(" --glob "*.kt" --glob "*.java" rg -A 3 -B 3 "primeCacheForDetail\(" --glob "*.kt" --glob "*.java"Length of output: 5833
Thread Blocking Safe in Background Tasks
The grep investigation shows that all invocations of
primeEntityCache(),primeEntityCacheForDetail(...), andprimeCacheForDetail(...)are executed within task-related contexts (in files such asPrimeEntityCache.ktandAndroidAsyncNodeEntityFactory.kt) rather than from UI components. This confirms that the use ofrunBlockinginPrimeEntityCacheHelper.kt(lines 120-122, 143-145, and 205-207) does not risk blocking the main thread.No changes are required here, but please remain cautious with any future refactors to ensure these helper methods continue to run exclusively in background threads.
| if (CommCareApplication.isSessionActive()) { | ||
| primeEntityCacheHelper = CommCareApplication.instance().currentApp.primeEntityCacheHelper | ||
| primeEntityCacheHelper.primeEntityCache() | ||
| primeEntityCacheHelper!!.primeEntityCache() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a safe call instead of non-null assertion
The non-null assertion (!!) could lead to a NullPointerException if primeEntityCacheHelper is null for any reason.
- primeEntityCacheHelper!!.primeEntityCache()
+ primeEntityCacheHelper?.primeEntityCache() ?: return Result.failure()📝 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.
| primeEntityCacheHelper!!.primeEntityCache() | |
| primeEntityCacheHelper?.primeEntityCache() ?: return Result.failure() |
| factory: AsyncNodeEntityFactory? = null | ||
| ) { |
There was a problem hiding this comment.
Non-null assertion on nullable factory parameter
Line 195 uses a non-null assertion (!!) on the factory parameter, which is defined as nullable. This could cause a runtime exception if factory is null.
Consider replacing the non-null assertion with a safe approach:
- it.factory!!.setEntityProgressListener(this)
+ it.factory?.setEntityProgressListener(this) ?: throw IllegalStateException("Factory cannot be null when entity loading is in progress")Or ensure the factory is properly initialized in the EntityLoaderHelper constructor.
Also applies to: 189-190, 194-195
|
@damagatchi retest this please |
|
@damagatchi retest this please |
3 similar comments
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
…nto connect_472000_hotfix
|
@damagatchi retest this please |
|
@damagatchi retest this please |
1 similar comment
|
@damagatchi retest this please |
Connect Beta 472000 hotfix.
PR is for generating a build only, do not merge.
Related PRs:
#3033
#3032
cross-request: dimagi/commcare-core#1455