Pm beta relase #3001
Pm beta relase #3001
Conversation
… 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
… into dv/delivery_progress_ui_fix
Delivery Progress UI fix
Solved smaller screens UI issues
Job status fix for learning
Fixed a bad translation (across languages) involving malformed escape XML.
# Conflicts: # app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java # app/src/org/commcare/android/database/connect/models/ConnectPaymentUnitRecord.java # app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
📝 WalkthroughWalkthroughThis pull request includes multiple changes across the codebase. Layout files have been restructured—for example, replacing LinearLayout and FrameLayout with ConstraintLayout, adding new view components, and updating layout parameters to improve flexibility. String resources in various locales have been corrected to properly display XML tags (e.g., Sequence Diagram(s)sequenceDiagram
participant LA as LoginActivity
participant CM as ConnectManager
participant Logger as Logger
LA->>LA: handlePullTaskResult(AUTH_FAILED)
LA->>CM: checkForFailedConnectIdAuth(username)
CM-->>LA: returns true/false
alt if true
LA->>Logger: Log token authentication error for Connect ID
end
LA->>LA: raiseLoginMessage(bad credentials)
sequenceDiagram
participant Activity as FragmentActivity
participant BH as BiometricsHelper
participant Prompt as BiometricPrompt
Activity->>BH: authenticatePinOrBiometric(callback)
BH->>Prompt: Initiate authentication (DEVICE_CREDENTIAL, BIOMETRIC_STRONG, BIOMETRIC_WEAK)
Prompt-->>Activity: Return authentication result
Possibly related PRs
✨ 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: 8
🧹 Nitpick comments (12)
app/res/values-ti/strings.xml (1)
211-211: Corrected HTML Entity Encoding inroot_element_errorThe string resource now uses
<manifest\>in place of the incorrect&lt;manifest\>. Please verify whether the backslash before>is intended or if the desired display should be the plain<manifest>(i.e. encoded as<manifest>).app/res/values-es/strings.xml (1)
151-151: Updated HTML Entity forroot_element_errorThe error message string has been corrected from displaying
&lt;manifiesto\>to<manifest\>, ensuring that the tag appears accurately in the UI. Please double-check if the backslash before>is needed or if it should be removed for proper rendering.app/src/org/commcare/activities/connect/ConnectActivity.java (1)
184-184: Architecture improvement: Refactored menu handling to fragment-levelThe code now uses a more modern approach where fragments handle their menu actions individually through MenuProviders instead of the activity delegating to them. This follows the Android architectural best practice of giving fragments more autonomy over their UI interactions.
This change represents a good architectural improvement, making the fragments more self-contained and reducing dependencies on the hosting activity. If there are other similar activity-to-fragment delegations in the codebase, consider applying the same pattern for consistency.
app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (1)
239-239: Simplified biometric authentication logicThe code now uses a streamlined approach for fingerprint authentication by removing the
allowOtherOptionsparameter. This aligns with the backend changes inBiometricsHelperwhere authentication methods have been consolidated.This simplification is good and aligns with the consolidated approach used in
BiometricsHelper.authenticatePinOrBiometric(). The code now follows a more unified authentication flow while maintaining the same functionality.app/src/org/commcare/models/database/user/models/CommCareEntityStorageCache.java (1)
287-289: Add null check to new getCacheKey method.The method correctly constructs a cache key string, but like the previous method, it lacks null checks which could lead to NullPointerExceptions.
public String getCacheKey(String detailId, String mFieldId, ValueType valueType) { + if (detailId == null || mFieldId == null || valueType == null) { + Logger.log(LogTypes.TYPE_MAINTENANCE, "Null parameters provided to getCacheKey"); + return ""; + } return valueType + "_" + detailId + "_" + mFieldId; }app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java (1)
165-165: Clarify purpose of static startDate variable.The startDate variable seems to track the maximum project start date across all jobs parsed, but its purpose and scope are not clear from the naming alone.
- static Date startDate = new Date(); + static Date maxProjectStartDate = new Date(); // Tracks the maximum project start date across all jobsapp/src/org/commcare/android/database/connect/models/ConnectPaymentUnitRecord.java (2)
57-71: Improved error handling with graceful fallback.The try-catch block with proper error logging is a good addition, but returning null on exception could lead to NullPointerExceptions if callers don't check for null returns.
Consider adding a comment or Javadoc to document that this method can return null, or use an Optional return type if your Android API level supports it.
/** * Creates a payment unit record from JSON data * @param json The JSON object containing payment unit data * @param jobId The job ID to associate with this payment unit * @return A ConnectPaymentUnitRecord object, or null if parsing failed * @throws JSONException If there's an error parsing the JSON * @throws ParseException If there's an error parsing date values */ public static ConnectPaymentUnitRecord fromJson(JSONObject json, int jobId) throws JSONException, ParseException {
74-104: Good addition of getter and setter methods for better encapsulation.The addition of getter and setter methods improves the API's clarity and follows good OOP principles. However, there's an inconsistency in the API design - some properties have both getters and setters (jobId, maxTotal) while others have only getters (name, unitId, maxDaily, amount).
For consistency, consider either:
- Adding the missing setter methods for all properties
- Making all properties immutable (remove setters) except those that truly need to be modified after creation
+ public void setMaxDaily(int maxDaily) { + this.maxDaily = maxDaily; + } + public void setAmount(int amount) { + this.amount = amount; + } + public void setName(String name) { + this.name = name; + } + public void setUnitId(int unitId) { + this.unitId = unitId; + }app/res/layout/fragment_signup.xml (2)
42-101: Consider extracting common styles to reduce duplication.The text views and input fields have many repeated styling attributes like margins, font families, and text colors that could be extracted to styles.
Extract common styling attributes to styles in styles.xml to make the layout more maintainable and consistent:
<!-- In styles.xml --> <style name="ConnectInputLayout"> <item name="android:layout_width">match_parent</item> <item name="android:layout_height">wrap_content</item> <item name="android:layout_marginStart">16dp</item> <item name="android:layout_marginEnd">16dp</item> <item name="android:layout_marginTop">16dp</item> <item name="android:orientation">horizontal</item> </style> <style name="ConnectTextView"> <item name="android:layout_width">wrap_content</item> <item name="android:layout_height">wrap_content</item> <item name="android:textColor">@color/connect_dark_blue_color</item> </style>
219-240: Improve UI consistency by aligning buttons.The continue button layout properties could be improved for better consistency with other buttons in the layout.
Consider making the button width and alignment more consistent with the overall layout:
<com.google.android.material.button.MaterialButton android:id="@+id/continue_button" style="@style/CustomButtonStyle" - android:layout_width="wrap_content" + android:layout_width="match_parent" android:layout_height="wrap_content" - android:layout_gravity="end" + android:layout_gravity="center" android:layout_marginTop="16dp" - android:layout_marginEnd="16dp" + android:layout_marginHorizontal="16dp" android:gravity="start|center" android:text="@string/review" android:drawableRight="@drawable/connect_right_arrow" />app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (2)
223-231: Redundant menu handling with potential conflicts.The fragment implements both a MenuProvider and overrides onOptionsItemSelected, which is redundant and could lead to conflicts.
Since you're already using the MenuProvider API, consider removing the redundant onOptionsItemSelected method:
-@Override -public boolean onOptionsItemSelected(MenuItem item) { - if(item.getItemId() == R.id.action_sync) { - refreshData(); - return true; - } - - return false; -}
376-382: Type casting in refresh method is fragile.The refresh method uses explicit type casting which could be brittle if fragment implementations change.
Consider using an interface to define the refresh behavior instead of relying on explicit type casting:
+ public interface RefreshableFragment { + void updateView(); + } public void refresh() { for (Fragment fragment : fragmentList) { - if (fragment instanceof ConnectDeliveryProgressDeliveryFragment) { - ((ConnectDeliveryProgressDeliveryFragment) fragment).updateView(); - } else if (fragment instanceof ConnectResultsSummaryListFragment) { - ((ConnectResultsSummaryListFragment) fragment).updateView(); + if (fragment instanceof RefreshableFragment) { + ((RefreshableFragment) fragment).updateView(); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
app/res/layout/fragment_connect_delivery_details.xml(2 hunks)app/res/layout/fragment_signup.xml(2 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(1 hunks)app/res/values-sw/strings.xml(1 hunks)app/res/values-ti/strings.xml(1 hunks)app/src/org/commcare/activities/LoginActivity.java(1 hunks)app/src/org/commcare/activities/SyncCapableCommCareActivity.java(2 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java(9 hunks)app/src/org/commcare/android/database/connect/models/ConnectPaymentUnitRecord.java(2 hunks)app/src/org/commcare/connect/ConnectDatabaseHelper.java(2 hunks)app/src/org/commcare/connect/ConnectManager.java(3 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java(6 hunks)app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java(3 hunks)app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java(0 hunks)app/src/org/commcare/logging/AndroidLogger.java(0 hunks)app/src/org/commcare/logging/PreInitLogger.java(0 hunks)app/src/org/commcare/models/database/user/models/CommCareEntityStorageCache.java(3 hunks)app/src/org/commcare/network/CommcareRequestGenerator.java(1 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(3 hunks)app/src/org/commcare/utils/BiometricsHelper.java(2 hunks)
💤 Files with no reviewable changes (3)
- app/src/org/commcare/logging/AndroidLogger.java
- app/src/org/commcare/logging/PreInitLogger.java
- app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java
🧰 Additional context used
🧠 Learnings (3)
app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (2)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:95-131
Timestamp: 2025-03-26T13:12:29.695Z
Learning: Biometric authentication security improvements were considered but skipped in PR #2949 as per user's request. The implementation remained with basic biometric authentication without additional security layers.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:235-236
Timestamp: 2025-03-26T13:12:29.695Z
Learning: The empty performPasswordUnlock method in ConnectIdBiometricConfigFragment is intentionally left empty and should not be flagged in reviews.
app/src/org/commcare/android/database/connect/models/ConnectPaymentUnitRecord.java (1)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectPaymentUnitRecord.java:41-41
Timestamp: 2025-03-26T13:12:29.696Z
Learning: In ConnectPaymentUnitRecord, monetary values (maxTotal, maxDaily, amount) are intentionally stored as integers.
app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java (2)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectJobRecordV2.java:120-121
Timestamp: 2025-03-26T13:12:29.695Z
Learning: The date fields (projectEndDate, lastUpdate, lastLearnUpdate, lastDeliveryUpdate) in ConnectJobRecordV2 and its newer versions are guaranteed to be non-null through the application's initialization logic and data flow. The newer version (ConnectJobRecord) explicitly initializes these in the constructor, while V2 maintains this guarantee through database upgrades and JSON parsing.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java:86-93
Timestamp: 2025-03-26T13:12:29.695Z
Learning: The JSON fields (status, unitName, slug, entityId, entityName, reason) in ConnectJobDeliveryRecord's fromJson method are guaranteed to be non-null through the application's data contract, except for the 'id' field which is explicitly checked.
🧬 Code Definitions (5)
app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (1)
app/src/org/commcare/utils/BiometricsHelper.java (1)
BiometricsHelper(25-164)
app/src/org/commcare/activities/LoginActivity.java (1)
app/src/org/commcare/connect/ConnectManager.java (1)
ConnectManager(81-1059)
app/src/org/commcare/activities/SyncCapableCommCareActivity.java (1)
app/src/org/commcare/connect/ConnectManager.java (1)
ConnectManager(81-1059)
app/src/org/commcare/network/CommcareRequestGenerator.java (1)
app/src/org/commcare/connect/ConnectManager.java (1)
ConnectManager(81-1059)
app/src/org/commcare/connect/ConnectManager.java (2)
app/src/org/commcare/utils/BiometricsHelper.java (1)
BiometricsHelper(25-164)app/src/org/commcare/connect/ConnectDatabaseHelper.java (1)
ConnectDatabaseHelper(56-1024)
🔇 Additional comments (40)
app/res/values-pt/strings.xml (1)
223-223: Correct XML Escape Sequence in Root Element Error MessageThe updated string now correctly uses
<manifest\>to represent the<manifest>tag. This fix ensures proper XML rendering and consistency with other locales.app/res/values-no/strings.xml (1)
152-152: Fix XML Escaping in Norwegian Root Element Error MessageThe error message is now updated to use the correct escape sequence
<manifest\>instead of the erroneous&lt;manifest\>. This change ensures that the error is displayed properly in the UI.app/res/values-lt/strings.xml (1)
152-152: Correct XML Escape Sequence in Lithuanian ResourceThe Lithuanian string now properly replaces the incorrect
&lt;manifest\>with<manifest\>, ensuring that the manifest tag is rendered accurately when the error message is displayed.app/res/values-fr/strings.xml (1)
151-151: Corrected XML tag encoding for proper displayThe change fixes the HTML entity encoding for the
<manifest>tag, replacing the double-encoded&lt;with the correct<. This ensures the error message will properly display as<manifest>rather than showing the literal<manifest>.app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java (1)
244-248:Details
✅ Verification successful
Updated SDK version check for broadcast receiver registration
This change updates the API level check from Android O (API 26) to Android Tiramisu (API 33) for using the
RECEIVER_NOT_EXPORTEDflag when registering the SMS broadcast receiver. This aligns with Android 13's more restrictive broadcast receiver security model.To verify this change is consistent with other similar patterns in the codebase:
🏁 Script executed:
#!/bin/bash # Check for other occurrences of similar broadcast receiver registrations rg "registerReceiver.*RECEIVER_NOT_EXPORTED" --type javaLength of output: 739
Verified: Updated API Tiramisu Check for Broadcast Receiver Registration
- The change in ConnectIdPhoneVerificationFragmnet.java now correctly uses
RECEIVER_NOT_EXPORTEDwhen the SDK level is at least Android Tiramisu (API 33).- A scan of the codebase confirms that similar patterns exist in other files (e.g., SessionRegistrationHelper.java and FormEntryActivity.java), verifying consistency in how broadcast receivers are registered under Android 13’s security model.
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (3)
18-18: LGTM - Added import for string utility classThe added import for
com.google.android.gms.common.util.Stringsprovides access to string utility methods needed for the empty notification check.
221-224: Good defensive coding - Added validation for empty notificationsThis change prevents showing empty notifications to users by checking if both title and text are empty or just whitespace. Empty notifications are now properly logged with their associated action, which will help with debugging.
236-236: LGTM - Minor formatting improvementThe whitespace formatting improvement for the if statement condition makes the code more consistent.
app/src/org/commcare/activities/LoginActivity.java (1)
881-884: Enhanced error logging for Connect ID authentication failuresThis change adds specialized error logging when authentication fails specifically for Connect ID managed applications. The additional logging will help with debugging token authentication issues in this specific scenario.
The check uses the
ConnectManager.checkForFailedConnectIdAuthmethod to determine if the authentication failure is related to a Connect ID managed application, making the error handling more targeted and informative.app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java (3)
5-7: LGTM - Added required imports for menu handlingAdded necessary imports for implementing menu functionality in the fragment.
29-35: LGTM - Reorganized imports for new menu componentsReorganized imports and added necessary menu-related components from the androidx libraries.
84-100: Improved architecture with lifecycle-aware menu handlingGreat improvement implementing a proper
MenuProviderthat follows modern Android architecture patterns. This implementation:
- Uses the activity as a
MenuHost- Adds a menu provider that handles the sync action
- Properly attaches to the fragment's view lifecycle
- Scopes the menu to only appear when the fragment is in the RESUMED state
This is more maintainable than older approaches that override fragment methods directly.
app/src/org/commcare/connect/ConnectDatabaseHelper.java (2)
307-307: Fixed method name typoCorrected method name from
setComletedLearningModulestosetCompletedLearningModules, fixing a typo that could have led to confusion.
345-348: Improved job status handling for consistencyAdded important logic to preserve the highest job status between existing and incoming jobs. This prevents jobs from incorrectly reverting to earlier status values during synchronization, which maintains workflow integrity.
For example, if a job has already transitioned to a "learning" status locally but the server still shows it as "available", this change ensures the local status isn't downgraded.
app/src/org/commcare/network/CommcareRequestGenerator.java (1)
186-189: Improved error handling for Connect ID authenticationThe code now uses a centralized method
ConnectManager.checkForFailedConnectIdAuth(username)to verify if authentication failed for a connect-managed app. This approach is cleaner than the previous implementation which likely had nested logic with try-catch blocks.app/src/org/commcare/activities/SyncCapableCommCareActivity.java (2)
33-33: LGTM - Added Logger importThe import for
Loggerfromorg.javarosa.core.servicesis now properly included to support the exception logging in the error handling logic.
109-112: Improved error handling for ConnectID authentication failuresThis change adds proper logging for ConnectID authentication failures during sync operations, making debugging easier. It replaces what was likely a call to
forgetAppCredentialswith a more targeted approach that logs detailed error information.app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (2)
18-20: LGTM - Added necessary imports for menu handlingThe imports support the new menu handling mechanism using the Jetpack Lifecycle components.
Also applies to: 26-32
105-122: Modernized menu handling with lifecycle-aware componentsThe code now uses the recommended lifecycle-aware MenuProvider pattern instead of overriding fragment methods. This approach:
- Connects the menu to the fragment's view lifecycle
- Improves separation of concerns
- Follows modern Android development practices
This is a good upgrade from the previous approach which likely used an overridden
onOptionsItemSelectedmethod.app/res/layout/fragment_connect_delivery_details.xml (3)
158-167: Converted LinearLayout to ConstraintLayout for better performanceThe layout has been upgraded from LinearLayout to ConstraintLayout with proper constraints. The width is now set to
0dpwith constraints, and right margin has been added.
173-175: Added proper constraints to child viewsThe text views now have proper constraint attributes that define their positioning within the parent ConstraintLayout. This replaces the previous vertical orientation in the LinearLayout.
Also applies to: 184-186
190-190: LGTM - Updated closing tagThe closing tag correctly reflects the change to ConstraintLayout.
app/src/org/commcare/models/database/user/models/CommCareEntityStorageCache.java (2)
21-23: Added appropriate logging imports.The newly added imports for
LogTypesandLoggerare correctly used in the new methods for error logging.
31-32: Added necessary imports for ValueType constants.These static imports are appropriately used in the
getFieldIdFromCacheKeymethod to strip type prefixes from cache keys.app/src/org/commcare/connect/ConnectManager.java (3)
249-249: Simplified fingerprint authentication call.The authentication flow has been improved by removing the conditional parameter and delegating to the consolidated authentication implementation in BiometricsHelper.
568-581: Well-implemented method for checking Connect ID authentication failures.This new method properly checks if Connect ID is configured and retrieves the linked application record, with good error handling for potential exceptions.
Consider using a more specific exception type instead of catching generic Exception to better identify the source of errors:
public static boolean checkForFailedConnectIdAuth(String username) { try { if (isConnectIdConfigured()) { String seatedAppId = CommCareApplication.instance().getCurrentApp().getUniqueId(); ConnectLinkedAppRecord appRecord = ConnectDatabaseHelper.getAppData( CommCareApplication.instance(), seatedAppId, username); return appRecord != null && appRecord.getWorkerLinked(); } - } catch (Exception e){ + } catch (SessionUnavailableException e) { + Logger.exception("Session unavailable while checking ConnectId status", e); + } catch (RuntimeException e) { Logger.exception("Error while checking ConnectId status after failed token auth", e); } return false; }
843-843: Fixed method name typo.Corrected the method name from
setComletedLearningModulestosetCompletedLearningModules, which improves code clarity and consistency.app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java (4)
23-26: Added TimeUnit import for better date calculations.The TimeUnit import is appropriately used in the getDaysRemaining method to convert milliseconds to days, which is more readable and less error-prone than manual calculations.
43-43: Added new job status constant.The addition of STATUS_ALL_JOBS constant is appropriate for extending job status types.
227-232: Good defensive check for null payment unit records.The null check before adding payment units to the list is a good practice to prevent NullPointerExceptions when working with parsed JSON objects.
437-440: Improved time calculation using TimeUnit.The use of TimeUnit.MILLISECONDS.toDays is more readable and less error-prone than manually calculating days from milliseconds.
app/src/org/commcare/utils/BiometricsHelper.java (3)
52-54: Excellent refactoring of authentication methods.The authentication flow has been simplified by removing the allowExtraOptions parameter and delegating to the new consolidation method.
81-84: Simplified PIN authentication.The authentication flow for PIN has been streamlined by delegating to the consolidated method, reducing code duplication.
86-99: Well-implemented consolidated authentication method.The new authenticatePinOrBiometric method effectively unifies the authentication logic for both PIN and biometric methods, checking if either is configured and creating an appropriate BiometricPrompt.
app/src/org/commcare/android/database/connect/models/ConnectPaymentUnitRecord.java (1)
7-7: Good addition of Logger import for error handling.Adding proper logging for JSON parsing errors will help with troubleshooting.
app/res/layout/fragment_signup.xml (1)
3-9: Good upgrade to ConstraintLayout for better flexibility.Switching from FrameLayout to ConstraintLayout is a good improvement for a complex layout like this, as it allows for more flexible positioning of child views and better responsiveness.
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (4)
6-8: Good addition of menu-related imports.Adding the necessary imports for Menu implementation is appropriate for the functionality being added.
Also applies to: 16-17
142-146: Potential null pointer risk when measuring fragment view.The code attempts to measure a fragment view that might not be fully created or initialized yet.
Check for potential null pointer exceptions by verifying if the view is properly inflated and attached before measuring:
View view = viewStateAdapter.createFragment(position).getView(); -if(view != null) { +if(view != null && view.isAttachedToWindow() && view.getMeasuredHeight() > 0) { pager.getLayoutParams().height = view.getMeasuredHeight(); pager.requestLayout(); }
172-188: Good implementation of lifecycle-aware MenuProvider.The implementation of a MenuProvider with proper lifecycle handling is a good modern approach to menu management in Android.
356-373: Improved fragment management with list-based approach.Using a list of fragments is a more maintainable approach than creating fragments on demand, allowing for better lifecycle management.
| public int getFieldIdFromCacheKey(String detailId, String cacheKey) { | ||
| cacheKey = cacheKey.replace(TYPE_SORT_FIELD + "_", ""); | ||
| cacheKey = cacheKey.replace(TYPE_NORMAL_FIELD + "_", ""); | ||
| String intId = cacheKey.substring(detailId.length() + 1); | ||
| try { | ||
| return Integer.parseInt(intId); | ||
| } catch (NumberFormatException nfe) { | ||
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Unable to parse cache key " + cacheKey); | ||
| //TODO: Kill this cache key if this didn't work | ||
| return -1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks to prevent potential NullPointerExceptions.
The method logic correctly extracts field IDs from cache keys, but lacks null checks for the cacheKey and detailId parameters. Also, the substring operation could throw StringIndexOutOfBoundsException if cacheKey doesn't contain detailId.
public int getFieldIdFromCacheKey(String detailId, String cacheKey) {
+ if (detailId == null || cacheKey == null) {
+ Logger.log(LogTypes.TYPE_MAINTENANCE, "Null parameters provided to getFieldIdFromCacheKey");
+ return -1;
+ }
+ if (!cacheKey.contains(detailId)) {
+ Logger.log(LogTypes.TYPE_MAINTENANCE, "Cache key " + cacheKey + " does not contain detail ID " + detailId);
+ return -1;
+ }
cacheKey = cacheKey.replace(TYPE_SORT_FIELD + "_", "");
cacheKey = cacheKey.replace(TYPE_NORMAL_FIELD + "_", "");
String intId = cacheKey.substring(detailId.length() + 1);
try {
return Integer.parseInt(intId);
} catch (NumberFormatException nfe) {
Logger.log(LogTypes.TYPE_MAINTENANCE, "Unable to parse cache key " + cacheKey);
//TODO: Kill this cache key if this didn't work
return -1;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public int getFieldIdFromCacheKey(String detailId, String cacheKey) { | |
| cacheKey = cacheKey.replace(TYPE_SORT_FIELD + "_", ""); | |
| cacheKey = cacheKey.replace(TYPE_NORMAL_FIELD + "_", ""); | |
| String intId = cacheKey.substring(detailId.length() + 1); | |
| try { | |
| return Integer.parseInt(intId); | |
| } catch (NumberFormatException nfe) { | |
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Unable to parse cache key " + cacheKey); | |
| //TODO: Kill this cache key if this didn't work | |
| return -1; | |
| } | |
| } | |
| public int getFieldIdFromCacheKey(String detailId, String cacheKey) { | |
| if (detailId == null || cacheKey == null) { | |
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Null parameters provided to getFieldIdFromCacheKey"); | |
| return -1; | |
| } | |
| if (!cacheKey.contains(detailId)) { | |
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Cache key " + cacheKey + " does not contain detail ID " + detailId); | |
| return -1; | |
| } | |
| cacheKey = cacheKey.replace(TYPE_SORT_FIELD + "_", ""); | |
| cacheKey = cacheKey.replace(TYPE_NORMAL_FIELD + "_", ""); | |
| String intId = cacheKey.substring(detailId.length() + 1); | |
| try { | |
| return Integer.parseInt(intId); | |
| } catch (NumberFormatException nfe) { | |
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Unable to parse cache key " + cacheKey); | |
| //TODO: Kill this cache key if this didn't work | |
| return -1; | |
| } | |
| } |
| job.title = json.getString(META_NAME); | ||
| job.description = json.getString(META_DESCRIPTION); | ||
| job.organization = json.getString(META_ORGANIZATION); | ||
| job.projectEndDate = DateUtils.parseDate(json.getString(META_END_DATE)); | ||
| job.projectStartDate = DateUtils.parseDate(json.getString(META_START_DATE)); | ||
| if (job.projectStartDate != null && job.projectStartDate.after(startDate)) { | ||
| startDate = job.projectStartDate; | ||
| } | ||
| job.maxVisits = json.getInt(META_MAX_VISITS_PER_USER); | ||
| job.maxDailyVisits = json.getInt(META_MAX_DAILY_VISITS); | ||
| job.budgetPerVisit = json.getInt(META_BUDGET_PER_VISIT); | ||
| String budgetPerUserKey = "budget_per_user"; | ||
| job.totalBudget = json.has(budgetPerUserKey) ? json.getInt(budgetPerUserKey) : -1; | ||
| job.currency = json.has(META_CURRENCY) && !json.isNull(META_CURRENCY) ? json.getString(META_CURRENCY) : ""; | ||
| job.shortDescription = json.has(META_SHORT_DESCRIPTION) && !json.isNull(META_SHORT_DESCRIPTION) ? | ||
| json.getString(META_SHORT_DESCRIPTION) : ""; | ||
|
|
||
| job.totalBudget = json.getInt(budgetPerUserKey); | ||
| job.currency = json.getString(META_CURRENCY); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks to JSON parsing to prevent JSONExceptions.
The direct retrieval of JSON values without checking if they exist could lead to JSONExceptions if fields are missing. Consider using opt methods like optString, optInt, etc. with default values.
- job.title = json.getString(META_NAME);
- job.description = json.getString(META_DESCRIPTION);
- job.organization = json.getString(META_ORGANIZATION);
- job.projectEndDate = DateUtils.parseDate(json.getString(META_END_DATE));
- job.projectStartDate = DateUtils.parseDate(json.getString(META_START_DATE));
+ job.title = json.optString(META_NAME, "");
+ job.description = json.optString(META_DESCRIPTION, "");
+ job.organization = json.optString(META_ORGANIZATION, "");
+ String endDateStr = json.optString(META_END_DATE, null);
+ job.projectEndDate = endDateStr != null ? DateUtils.parseDate(endDateStr) : new Date();
+ String startDateStr = json.optString(META_START_DATE, null);
+ job.projectStartDate = startDateStr != null ? DateUtils.parseDate(startDateStr) : new Date();📝 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.
| job.title = json.getString(META_NAME); | |
| job.description = json.getString(META_DESCRIPTION); | |
| job.organization = json.getString(META_ORGANIZATION); | |
| job.projectEndDate = DateUtils.parseDate(json.getString(META_END_DATE)); | |
| job.projectStartDate = DateUtils.parseDate(json.getString(META_START_DATE)); | |
| if (job.projectStartDate != null && job.projectStartDate.after(startDate)) { | |
| startDate = job.projectStartDate; | |
| } | |
| job.maxVisits = json.getInt(META_MAX_VISITS_PER_USER); | |
| job.maxDailyVisits = json.getInt(META_MAX_DAILY_VISITS); | |
| job.budgetPerVisit = json.getInt(META_BUDGET_PER_VISIT); | |
| String budgetPerUserKey = "budget_per_user"; | |
| job.totalBudget = json.has(budgetPerUserKey) ? json.getInt(budgetPerUserKey) : -1; | |
| job.currency = json.has(META_CURRENCY) && !json.isNull(META_CURRENCY) ? json.getString(META_CURRENCY) : ""; | |
| job.shortDescription = json.has(META_SHORT_DESCRIPTION) && !json.isNull(META_SHORT_DESCRIPTION) ? | |
| json.getString(META_SHORT_DESCRIPTION) : ""; | |
| job.totalBudget = json.getInt(budgetPerUserKey); | |
| job.currency = json.getString(META_CURRENCY); | |
| job.title = json.optString(META_NAME, ""); | |
| job.description = json.optString(META_DESCRIPTION, ""); | |
| job.organization = json.optString(META_ORGANIZATION, ""); | |
| String endDateStr = json.optString(META_END_DATE, null); | |
| job.projectEndDate = endDateStr != null ? DateUtils.parseDate(endDateStr) : new Date(); | |
| String startDateStr = json.optString(META_START_DATE, null); | |
| job.projectStartDate = startDateStr != null ? DateUtils.parseDate(startDateStr) : new Date(); | |
| if (job.projectStartDate != null && job.projectStartDate.after(startDate)) { | |
| startDate = job.projectStartDate; | |
| } | |
| job.maxVisits = json.getInt(META_MAX_VISITS_PER_USER); | |
| job.maxDailyVisits = json.getInt(META_MAX_DAILY_VISITS); | |
| job.budgetPerVisit = json.getInt(META_BUDGET_PER_VISIT); | |
| String budgetPerUserKey = "budget_per_user"; | |
| job.totalBudget = json.getInt(budgetPerUserKey); | |
| job.currency = json.getString(META_CURRENCY); |
| android:layout_height="wrap_content" | ||
| android:padding="1dp" | ||
| android:src="@drawable/ic_outline_person_24" /> | ||
|
|
There was a problem hiding this comment.
Fix escaping in string resource reference.
There's an inconsistent escape sequence in the string resource reference that might cause issues.
Fix the escape sequence:
- <string name="clearanswer_confirm">Ondoa jibu kwa \%s\"?"</string>
+ <string name="clearanswer_confirm">Ondoa jibu kwa \"%s\"?</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <string name="clearanswer_confirm">Ondoa jibu kwa \"%s\"?</string> |
| <string name="capture_image">Piga Picha</string> | ||
| <string name="capture_video">Rekodi Video</string> | ||
| <string name="change_language">Badilisha Lugha</string> | ||
| <string name="clearanswer_confirm">Ondoa jibu kwa \%s\"?"</string> |
There was a problem hiding this comment.
Fix inconsistent string escaping.
Several strings have inconsistent escape sequences for quotation marks.
Fix the escape sequences:
- <string name="clearanswer_confirm">Ondoa jibu kwa \%s\"?"</string>
+ <string name="clearanswer_confirm">Ondoa jibu kwa \"%s\"?</string>
- <string name="download_failed_with_error">Fomu \%1$s\" haikufaulu. %2$s."</string>
+ <string name="download_failed_with_error">Fomu \"%1$s\" haikufaulu. %2$s.</string>Also applies to: 119-119
| <string name="migration_possible_failure">CommCare haikuweza kuhamisha data yako yote wakati wa kusasisha programu. Tafadhali anzisha upya CommCare ili kujaribu kurekebisha hili. Ujumbe huu ukiendelea, huenda ukahitaji \'Kufuta data\' kwa CommCare katika menyu ya Mipangilio ya kifaa chako.</string> | ||
| <string name="migration_failure_title">Hitilafu ya Uhamiaji</string> | ||
| <string name="multiple_apps_limit_message">Umefikia kikomo chako cha programu zilizosakinishwa; unaweza tu kuwa na hadi programu %s zilizosakinishwa mara moja.\n\nIkiwa ungependa kusakinisha programu mpya, unaweza kusanidua programu iliyopo kwanza, ambayo inaweza kufanywa kutoka kwa kidhibiti programu.</string> | ||
| <string name="back_to_manager">Rudi kwa Kidhibiti Programu""</string> |
There was a problem hiding this comment.
Fix closing quote in string resource.
The string has an extra quote character at the end which could cause XML parsing issues.
Fix the closing quote:
- <string name="back_to_manager">Rudi kwa Kidhibiti Programu""</string>
+ <string name="back_to_manager">Rudi kwa Kidhibiti Programu</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <string name="back_to_manager">Rudi kwa Kidhibiti Programu""</string> | |
| <string name="back_to_manager">Rudi kwa Kidhibiti Programu</string> |
| <string name="root_element_error">Kipengele cha mizizi si <manifest\> -- kilikuwa %s</string> | ||
| <string name="root_namespace_error">Kipengele cha mizizi Nafasi ya jina si sahihi: %s</string> | ||
| <string name="manifest_tag_error">Ingizo la Manifest %s linakosa lebo moja au zaidi: jina la faili, hashi, au pakuaUrl</string> |
There was a problem hiding this comment.
Fix XML entity escaping in root_element_error string.
The string contains an XML element that needs proper escaping.
Fix the XML entity escaping:
- <string name="root_element_error">Kipengele cha mizizi si <manifest\> -- kilikuwa %s</string>
+ <string name="root_element_error">Kipengele cha mizizi si <manifest> -- kilikuwa %s</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <string name="root_element_error">Kipengele cha mizizi si <manifest\> -- kilikuwa %s</string> | |
| <string name="root_namespace_error">Kipengele cha mizizi Nafasi ya jina si sahihi: %s</string> | |
| <string name="manifest_tag_error">Ingizo la Manifest %s linakosa lebo moja au zaidi: jina la faili, hashi, au pakuaUrl</string> | |
| <string name="root_element_error">Kipengele cha mizizi si <manifest> -- kilikuwa %s</string> | |
| <string name="root_namespace_error">Kipengele cha mizizi Nafasi ya jina si sahihi: %s</string> | |
| <string name="manifest_tag_error">Ingizo la Manifest %s linakosa lebo moja au zaidi: jina la faili, hashi, au pakuaUrl</string> |
| <string name="submission_logs_title">Kuwasilisha Kumbukumbu za CommCare</string> | ||
| <string name="remote_form_payload_url">#VALUE!</string> | ||
| <string name="url_prompt">Chagua URL</string> | ||
| <string name="about_dialog">%. \n* Ishara ya "Penseli" kwa Creologica \n* ishara ya "Daftari" na Carlo \n* ishara ya "Antena" kwa Dima \n* ishara ya "Alama ya Ramani" na P.J. Onori \n* Orodha" ya Landan Lloyd \n* skrini iliyovunjwa na Sam Martin kutoka kwa Nomino Project \n\n\n\n"</string> |
There was a problem hiding this comment.
Inconsistent quoting in string resource.
The closing quote for the about_dialog string is incorrectly escaped.
Fix the quote:
- <string name="about_dialog">%. \n* Ishara ya "Penseli" kwa Creologica \n* ishara ya "Daftari" na Carlo \n* ishara ya "Antena" kwa Dima \n* ishara ya "Alama ya Ramani" na P.J. Onori \n* Orodha" ya Landan Lloyd \n* skrini iliyovunjwa na Sam Martin kutoka kwa Nomino Project \n\n\n\n"</string>
+ <string name="about_dialog">%. \n* Ishara ya "Penseli" kwa Creologica \n* ishara ya "Daftari" na Carlo \n* ishara ya "Antena" kwa Dima \n* ishara ya "Alama ya Ramani" na P.J. Onori \n* Orodha" ya Landan Lloyd \n* skrini iliyovunjwa na Sam Martin kutoka kwa Nomino Project \n\n\n\n</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <string name="about_dialog">%. \n* Ishara ya "Penseli" kwa Creologica \n* ishara ya "Daftari" na Carlo \n* ishara ya "Antena" kwa Dima \n* ishara ya "Alama ya Ramani" na P.J. Onori \n* Orodha" ya Landan Lloyd \n* skrini iliyovunjwa na Sam Martin kutoka kwa Nomino Project \n\n\n\n"</string> | |
| <string name="about_dialog">%. \n* Ishara ya "Penseli" kwa Creologica \n* ishara ya "Daftari" na Carlo \n* ishara ya "Antena" kwa Dima \n* ishara ya "Alama ya Ramani" na P.J. Onori \n* Orodha" ya Landan Lloyd \n* skrini iliyovunjwa na Sam Martin kutoka kwa Nomino Project \n\n\n\n</string> |
| <string name="recovery_measure_unzip_error">Hitilafu imetokea wakati wa kusakinisha programu kutoka kwa faili ya CCZ. Tafadhali jaribu tena au tafuta faili ya CCZ wewe mwenyewe kwa kubofya kitufe cha \Chagua CCZ\" hapa chini."</string> | ||
| <string name="recovery_measure_update_cancelled">Usasishaji umeghairiwa</string> |
There was a problem hiding this comment.
Fix inconsistent escape sequences.
There are inconsistent escape sequences in these error message strings.
Fix the escape sequences:
- <string name="recovery_measure_unzip_error">Hitilafu imetokea wakati wa kusakinisha programu kutoka kwa faili ya CCZ. Tafadhali jaribu tena au tafuta faili ya CCZ wewe mwenyewe kwa kubofya kitufe cha \Chagua CCZ\" hapa chini."</string>
+ <string name="recovery_measure_unzip_error">Hitilafu imetokea wakati wa kusakinisha programu kutoka kwa faili ya CCZ. Tafadhali jaribu tena au tafuta faili ya CCZ wewe mwenyewe kwa kubofya kitufe cha \"Chagua CCZ\" hapa chini.</string>
- <string name="recovery_measure_ccz_scan_failed">Hitilafu ilitokea wakati wa kupakua faili ya CCZ kwenye simu yako. Tafadhali jaribu tena au tafuta faili ya CCZ wewe mwenyewe kwa kubofya kitufe cha \Chagua CCZ\" hapa chini."</string>
+ <string name="recovery_measure_ccz_scan_failed">Hitilafu ilitokea wakati wa kupakua faili ya CCZ kwenye simu yako. Tafadhali jaribu tena au tafuta faili ya CCZ wewe mwenyewe kwa kubofya kitufe cha \"Chagua CCZ\" hapa chini.</string>Committable suggestion skipped: line range outside the PR's diff.
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
1 similar comment
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
1 similar comment
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
This is outdate created new pr with pm_beta |
Product Description
Due to some last code merge there are some crashes so we have reverted the code of merge than created the build for the new beta release
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review
cross-request: dimagi/commcare-core#1465