Connect QA 2025 03 #2991
Connect QA 2025 03 #2991
Conversation
…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.
Cleaned up call chains around the SSO calls to enforce the same flow. Preparing to better handle the token exceptions.
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
📝 WalkthroughWalkthroughThe pull request updates several XML layouts and Java classes to improve layout flexibility, authentication, and menu handling. In XML files, layout containers are transitioned to Sequence Diagram(s)sequenceDiagram
participant User
participant LoginActivity
participant ConnectManager
participant Logger
User->>LoginActivity: Submit login credentials
LoginActivity->>LoginActivity: Execute handlePullTaskResult
LoginActivity->>ConnectManager: checkForFailedConnectIdAuth(username)
alt Authentication Failed
ConnectManager-->>LoginActivity: true
LoginActivity->>Logger: Log token authentication error
else No Failure
LoginActivity->>User: Display generic login error
end
sequenceDiagram
participant Fragment
participant MenuHost
participant MenuProvider
participant DataService
Fragment->>MenuHost: Register MenuProvider (onCreateView)
MenuHost->>MenuProvider: Create menu items
User->>MenuProvider: Select sync menu item
MenuProvider->>Fragment: onMenuItemSelected triggers refreshData()
Fragment->>DataService: Initiate data refresh
Possibly related PRs
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ 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: 1
🧹 Nitpick comments (1)
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (1)
223-231: Retained compatibility with older menu handling.This method provides backward compatibility with older menu handling approaches while still leveraging the new implementation.
Consider adding a comment explaining why both this method and the MenuProvider implementation coexist. Long-term, when minimum API level increases, this method could be removed in favor of the MenuProvider approach only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/res/layout/fragment_connect_delivery_details.xml(2 hunks)app/res/layout/fragment_signup.xml(2 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/connect/ConnectDatabaseHelper.java(1 hunks)app/src/org/commcare/connect/ConnectManager.java(2 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/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 (1)
- app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java
🧰 Additional context used
🧠 Learnings (1)
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-12T16:03:19.352Z
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-12T16:03:19.352Z
Learning: The empty performPasswordUnlock method in ConnectIdBiometricConfigFragment is intentionally left empty and should not be flagged in reviews.
🔇 Additional comments (37)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (3)
18-18: Added necessary import for string validation.The
Strings.isEmptyOrWhitespaceis imported from Google's common utils to enable more robust empty string checks.
221-224: Improved notification handling with empty content validation.This addition enhances error handling by detecting and logging cases where both notification title and text are empty or whitespace-only. This will help identify malformed push notifications that might otherwise be silently displayed without content.
The error logging includes the associated action, which will be valuable for debugging issues with specific notification types.
236-236: Minor formatting improvement for consistency.Small formatting change that aligns with the codebase's style.
app/src/org/commcare/activities/LoginActivity.java (1)
881-884: Enhanced error logging for Connect ID authentication failures.This change improves error handling by adding specific error logging for Connect ID managed apps when authentication fails. This will help differentiate between regular authentication failures and token-specific issues in Connect ID apps.
The error is now documented with a detailed exception message that clearly identifies the context of the failure, which will be valuable for troubleshooting authentication issues.
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
184-184: Updated menu handling to use modern MenuProviders pattern.The sync button is now handled by individual fragments using MenuProviders instead of being forwarded from the activity. This aligns with Android's recommended architecture components and improves separation of concerns.
This change is part of a modernization effort visible in the related fragments (
ConnectDeliveryProgressFragment,ConnectJobsListsFragment, andConnectLearningProgressFragment) according to the AI summary. Each fragment now implements its own menu handling logic for better modularity.app/src/org/commcare/connect/ConnectDatabaseHelper.java (1)
344-348: Status synchronization enhancement to prevent job status regression.This addition ensures that when an existing job in the database has a higher status than an incoming job from the server, the incoming job's status is updated to match the higher status. This prevents jobs from reverting to earlier states due to server synchronization lag.
app/src/org/commcare/activities/SyncCapableCommCareActivity.java (2)
33-33: LGTM - Added Logger import.Clean import addition for the enhanced error logging functionality.
108-113: Enhanced error logging for ConnectID authentication failures.This addition improves visibility into authentication failures specific to ConnectID-managed applications by checking if the failure is related to a ConnectID account and logging a detailed exception message.
app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (1)
239-239: Simplified fingerprint authentication call.The method call has been streamlined by removing the
allowOtherOptionsparameter, aligning with the intention to focus solely on fingerprint authentication without considering fallback options.app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (3)
18-20: LGTM - Added required menu imports.Required imports for the new menu handling implementation.
26-31: LGTM - Added MenuProvider and lifecycle imports.Required imports for implementing the new menu handling with lifecycle awareness.
105-122: Modernized menu handling using MenuProvider interface.The implementation has been updated to use the recommended MenuProvider pattern, which:
- Properly scopes the menu to the fragment's lifecycle
- Ensures menu handling only occurs when the fragment is in the RESUMED state
- Maintains the same functionality of refreshing data when the sync action is selected
This is a good architecture improvement that follows Android's recommended practices for fragments.
app/src/org/commcare/network/CommcareRequestGenerator.java (1)
186-189: Simplified error handling for Connect ID authentication failures.The code now uses a centralized method
ConnectManager.checkForFailedConnectIdAuth()to check for failed Connect ID authentication, which improves code organization and makes error handling more consistent across the application.app/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.java (2)
5-7: Added imports for menu handling.These imports are necessary for the new menu functionality being implemented.
84-100: Modernized menu handling using MenuProvider.The fragment now properly implements a menu system using the recommended MenuProvider pattern instead of activity method overrides. This follows Android best practices for fragment menu handling and ensures the menu is properly scoped to the fragment's lifecycle.
app/res/layout/fragment_connect_delivery_details.xml (1)
158-190: Improved layout flexibility with ConstraintLayout.Replacing LinearLayout with ConstraintLayout and adding proper constraints improves the layout's flexibility and responsiveness. The views now have clear positioning relative to their parent and siblings rather than relying on LinearLayout's simpler constraints.
app/src/org/commcare/connect/ConnectManager.java (2)
249-249: Simplified fingerprint authentication.Removed the unnecessary parameter from the
authenticateFingerprintcall to streamline the biometric authentication process, focusing solely on fingerprint authentication.
568-581: Added centralized method for checking failed Connect ID authentication.This new utility method centralizes the logic for detecting when a Connect ID authentication has failed, allowing consistent error handling across different parts of the application. The method includes proper exception handling to ensure robustness.
app/src/org/commcare/utils/BiometricsHelper.java (3)
50-54: Good refactoring to use unified authentication method.The method has been simplified by delegating to the new
authenticatePinOrBiometricmethod, which reduces code duplication and improves maintainability.
80-84: Improved authentication flow abstraction.The PIN authentication has been streamlined by delegating to the shared authentication method, reducing code duplication and creating a more consistent user experience.
86-99: Well-designed unified authentication method.This new method effectively combines PIN and biometric authentication flows into a single reusable method with these benefits:
- Checks for either PIN or fingerprint configuration before authentication
- Uses combined authenticators (DEVICE_CREDENTIAL, BIOMETRIC_STRONG, BIOMETRIC_WEAK)
- Creates a consistent authentication experience
- Eliminates conditional checks based on Android version
The implementation follows best practices for modern Android biometric authentication.
app/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.java (8)
6-8: Added essential imports for menu handling.The added imports support the new menu handling functionality implemented in this fragment.
74-75: Added nullability annotation for improved type safety.Adding the
@NonNullannotation to theLayoutInflaterparameter enforces type safety and follows Android best practices.
142-146: Fixed ViewPager height measurement.This change dynamically adjusts the ViewPager height based on the selected fragment's measured height, which improves UI layout and prevents display issues.
172-188: Modern menu implementation using MenuProvider.This implementation follows the recommended modern approach for handling menus in fragments:
- Uses the MenuHost-MenuProvider pattern introduced in newer Android versions
- Properly scopes the menu provider to the view lifecycle
- Handles the sync action in a clean, isolated way
This is a significant improvement over the old approach of overriding fragment methods.
356-362: Improved adapter implementation with fragment list.Replacing static fragment references with a dynamic list improves:
- Maintainability: easier to add/remove fragments
- Flexibility: enables runtime fragment management
- Testability: easier to test with mock fragments
367-368: Simplified fragment creation method.Using the fragment list for creating fragments is more maintainable and less error-prone than the previous approach.
372-373: Dynamic item count based on fragment list size.This change makes the adapter more flexible by deriving the count from the actual list size rather than using a hardcoded value.
376-382: Improved refresh logic with proper type checking.The updated refresh method:
- Iterates through all fragments in the list
- Uses proper instanceof checks to safely cast to appropriate types
- Calls the correct update method based on the fragment type
This approach is more maintainable and supports adding new fragment types in the future.
app/res/layout/fragment_signup.xml (8)
3-9: Migrated to ConstraintLayout for better flexibility.Moving from FrameLayout to ConstraintLayout provides better control over the positioning of elements and improves responsiveness across different screen sizes.
12-24: Well-structured top bar with reusable layout.The new top bar implementation:
- Uses an include directive for common app bar layout, promoting reusability
- Has proper constraints for positioning within the parent ConstraintLayout
- Follows a clean component-based design approach
27-33: Improved ScrollView constraints for better layout handling.The ScrollView now:
- Uses 0dp height with constraints for proper sizing
- Is properly constrained between the top bar and bottom recovery section
- Has better adaptability to different screen sizes
41-67: Enhanced branding and visual hierarchy with company logo.The addition of the company logo, divider, and properly styled title text improves:
- Brand presence in the signup flow
- Visual separation between sections
- Overall user experience through clearer visual hierarchy
119-196: Improved phone input with clear visual structure.The phone input section now:
- Uses a consistent icon styling with the name field
- Separates country code and phone number visually
- Has better error handling with a dedicated text view
- Follows material design principles with appropriate spacing and layout
229-240: Updated to MaterialButton with modern styling.Replacing the standard button with MaterialButton:
- Provides a more modern look consistent with Material Design
- Uses proper styling through a style reference
- Includes a right arrow icon to indicate forward progression
- Has better alignment through gravity and layout attributes
246-249: Well-structured bottom recovery section.The bottom section:
- Is properly constrained to the bottom of the screen
- Has a clear visual separator with the divider
- Maintains proper spacing with margins
290-290: Complete transition to ConstraintLayout.The closing tag confirms the full transition to ConstraintLayout, ensuring all elements are properly positioned within the modern layout structure.
|
|
||
| IntentFilter intentFilter = new IntentFilter(SmsRetriever.SMS_RETRIEVED_ACTION); | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Updated API level check for receiver registration to Android 13 (TIRAMISU).
This change modifies when the RECEIVER_NOT_EXPORTED flag is used when registering the SMS broadcast receiver. Now it's only applied on Android 13 (Tiramisu) or higher, rather than Android 8 (Oreo) or higher.
While this change aligns with newer Android requirements, there's a potential issue with the class name: "ConnectIdPhoneVerificationFragmnet" has a typo (should be "Fragment" instead of "Fragmnet"). Consider fixing this class name for consistency and to prevent confusion.
- package org.commcare.fragments.connectId.ConnectIdPhoneVerificationFragmnet;
+ package org.commcare.fragments.connectId.ConnectIdPhoneVerificationFragment;Additionally, the method name registerBrodcastReciever has two spelling errors. Consider correcting it:
- public void registerBrodcastReciever() {
+ public void registerBroadcastReceiver() {📝 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.
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { | |
| - package org.commcare.fragments.connectId.ConnectIdPhoneVerificationFragmnet; | |
| + package org.commcare.fragments.connectId.ConnectIdPhoneVerificationFragment; | |
| public class ConnectIdPhoneVerificationFragment { | |
| // ... other class members and imports ... | |
| - public void registerBrodcastReciever() { | |
| + public void registerBroadcastReceiver() { | |
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { | |
| // Existing logic for registering the SMS broadcast receiver with the RECEIVER_NOT_EXPORTED flag | |
| } | |
| // ... additional method logic ... | |
| } | |
| // ... other methods and class content ... | |
| } |
…nto pm_connectid_merge # Conflicts: # app/res/values/strings.xml
…nto connect_qa
Using CommCare BuildConfig
App home menu using XML resource
…-while-previewing-in-app Respect image orientation while previewing the image in mobile app
… into master_connectid_merge # Conflicts: # app/AndroidManifest.xml # app/build.gradle # app/res/layout-land/home_screen.xml # app/res/layout/fragment_phone_available_bottom_sheet.xml # app/res/layout/fragment_recovery_code.xml # app/res/layout/fragment_secondary_phone_number.xml # app/res/layout/fragment_signup.xml # app/res/layout/home_screen.xml # app/res/layout/screen_connect_message.xml # app/res/layout/screen_connect_password_verify.xml # app/res/layout/screen_connect_phone_verify.xml # app/res/layout/screen_connect_user_deactivate_otp_verify.xml # app/res/layout/screen_connect_verify.xml # app/res/layout/screen_login.xml # app/res/layout/select_install_mode_fragment.xml # app/res/navigation/nav_graph_connectid.xml # app/res/values/colors.xml # app/res/values/strings.xml # app/res/values/themes.xml # app/src/org/commcare/activities/CommCareSetupActivity.java # app/src/org/commcare/activities/DispatchActivity.java # app/src/org/commcare/activities/HomeButtons.java # app/src/org/commcare/activities/HomeScreenBaseActivity.java # app/src/org/commcare/activities/LoginActivity.java # app/src/org/commcare/activities/LoginActivityUiController.java # app/src/org/commcare/activities/SettingsHelper.java # app/src/org/commcare/activities/StandardHomeActivity.java # app/src/org/commcare/activities/StandardHomeActivityUIController.java # app/src/org/commcare/activities/connect/ConnectIdActivity.java # app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java # app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecord.java # app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java # app/src/org/commcare/connect/ConnectConstants.java # app/src/org/commcare/connect/SMSBroadcastReceiver.java # app/src/org/commcare/connect/database/ConnectAppDatabaseUtil.java # app/src/org/commcare/connect/database/ConnectJobUtils.java # app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java # app/src/org/commcare/connect/network/ApiConnect.java # app/src/org/commcare/connect/network/ApiConnectId.java # app/src/org/commcare/connect/network/ConnectNetworkHelper.java # app/src/org/commcare/connect/network/ConnectNetworkServiceFactory.kt # app/src/org/commcare/connect/network/ConnectSsoHelper.java # app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt # app/src/org/commcare/fragments/SelectInstallModeFragment.java # app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java # app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java # app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java # app/src/org/commcare/fragments/connectId/ConnectIdMessageFragment.java # app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java # app/src/org/commcare/fragments/connectId/ConnectIdPhoneAvailableBottomSheet.java # app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java # app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java # app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java # app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java # app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java # app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java # app/src/org/commcare/network/CommcareRequestGenerator.java # app/src/org/commcare/network/HttpUtils.java # app/src/org/commcare/preferences/AppManagerDeveloperPreferences.java # app/src/org/commcare/utils/BiometricsHelper.java # app/src/org/commcare/utils/CrashUtil.java # app/src/org/commcare/utils/EncryptionKeyAndTransform.java # app/src/org/commcare/utils/KeyboardHelper.java # app/src/org/commcare/utils/PhoneNumberHelper.java # build.gradle
…nto pm_missing_code # Conflicts: # app/src/org/commcare/activities/StandardHomeActivity.java
…nto pm_connectid_merge # Conflicts: # app/src/org/commcare/activities/StandardHomeActivity.java
…re-android into pm_missing_code
-update the missing code, and change the request code value
…re-android into master_connectid_merge # Conflicts: # app/src/org/commcare/activities/StandardHomeActivity.java
Master connectid merge
- fix if check
Fixed signup blocker
Technical Summary
PR to create a build for QA before next beta release.
cross-request: dimagi/commcare-core#1455