-fixes from regression qa fixes #3098
Conversation
pm-dimagi
requested review from
OrangeAndGreen and
shubham1g5
and removed request for
shubham1g5
📝 WalkthroughWalkthroughThis set of changes refactors Connect-related logic by removing several methods and fields from Sequence Diagram(s)sequenceDiagram
participant LoginActivity
participant ConnectManager
participant ConnectAppRecord
participant ConnectJobRecord
LoginActivity->>ConnectManager: wasAppLaunchedFromConnect(appId)
LoginActivity->>ConnectManager: setConnectJobForApp(context, appId)
ConnectManager->>ConnectAppRecord: getAppRecord(appId)
ConnectManager->>ConnectJobRecord: getCompositeJob(appRecord)
ConnectManager->>ConnectManager: set activeJob
ConnectManager-->>LoginActivity: return ConnectJobRecord
sequenceDiagram
participant StandardHomeActivityUIController
participant ConnectManager
StandardHomeActivityUIController->>ConnectManager: shouldShowJobStatus(activity, appId)
ConnectManager-->>StandardHomeActivityUIController: returns boolean
Possibly related PRs
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/org/commcare/views/connect/CustomOtpView.java (1)
103-103: Maintain consistent type casting style.The type cast
(EditText)getChildAt(index - 1)now lacks a space between the cast and method call, while all other casts in this file (lines 88, 91, 133, 178, 218, 238) maintain a space:(EditText) getChildAt(...). Consider either reverting this change or updating all other casts for consistency.- EditText previousEditText = (EditText)getChildAt(index - 1); + EditText previousEditText = (EditText) getChildAt(index - 1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/res/layout/fragment_connect_delivery_progress.xml(1 hunks)app/src/org/commcare/activities/LoginActivity.java(2 hunks)app/src/org/commcare/activities/StandardHomeActivityUIController.java(1 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(0 hunks)app/src/org/commcare/activities/connect/ConnectIdActivity.java(1 hunks)app/src/org/commcare/connect/ConnectIDManager.java(0 hunks)app/src/org/commcare/connect/ConnectManager.java(3 hunks)app/src/org/commcare/connect/database/ConnectJobUtils.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java(2 hunks)app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java(1 hunks)app/src/org/commcare/views/connect/CustomOtpView.java(1 hunks)
💤 Files with no reviewable changes (2)
- app/src/org/commcare/activities/connect/ConnectActivity.java
- app/src/org/commcare/connect/ConnectIDManager.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shubham1g5
PR: dimagi/commcare-android#0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
🧬 Code Graph Analysis (4)
app/src/org/commcare/activities/StandardHomeActivityUIController.java (1)
app/src/org/commcare/connect/ConnectManager.java (1)
ConnectManager(67-657)
app/src/org/commcare/activities/connect/ConnectIdActivity.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-84)
app/src/org/commcare/activities/LoginActivity.java (1)
app/src/org/commcare/connect/ConnectManager.java (1)
ConnectManager(67-657)
app/src/org/commcare/connect/database/ConnectJobUtils.java (1)
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
ConnectDatabaseHelper(28-125)
🔇 Additional comments (15)
app/src/org/commcare/activities/connect/ConnectIdActivity.java (1)
167-173: Improved code formatting for better readability.The method call to
ConnectIDSignupFragmentDirections.actionConnectidPhoneFragmentToConnectidMessagehas been nicely reformatted with parameters on separate lines, which makes the code more readable and easier to maintain.app/src/org/commcare/activities/StandardHomeActivityUIController.java (1)
240-240:Details
✅ Verification successful
Static method call replacement aligns with Connect refactoring.
The change from
ConnectIDManager.getInstance().shouldShowJobStatus()toConnectManager.shouldShowJobStatus()is part of the broader refactoring to centralize Connect job management logic inConnectManagerusing static methods instead of instance methods inConnectIDManager.
🏁 Script executed:
#!/bin/bash # Check for any remaining references to ConnectIDManager.getInstance().shouldShowJobStatus # that might have been missed during the refactoring rg --type java "ConnectIDManager\.getInstance\(\)\.shouldShowJobStatus" -A 2 -B 2Length of output: 83
🏁 Script executed:
#!/bin/bash # Ensure no leftover instance-based ConnectIDManager usage rg --type java "ConnectIDManager" -n # Verify ConnectManager defines the static shouldShowJobStatus method rg --type java "class ConnectManager" -A5 -B5 # List all occurrences of shouldShowJobStatus to confirm calls and definitions rg --type java -n "shouldShowJobStatus" -C2Length of output: 18986
Static
shouldShowJobStatuscall refactoring confirmedAll occurrences of
ConnectIDManager.getInstance().shouldShowJobStatus()have been removed and replaced with the static
ConnectManager.shouldShowJobStatus(Context, String)method. The new static method is defined atConnectManager.java:208and the call in
StandardHomeActivityUIController.java:240correctly uses it.app/res/layout/fragment_connect_delivery_progress.xml (2)
76-76: Improved button height flexibility.Changing from fixed
35dpheight towrap_contentallows the button to adjust its height based on content, improving adaptability across different screen sizes and text settings.
86-86: Consistent button layout improvement.Similarly to the yes button, changing the no button height to
wrap_contentcreates consistency between the buttons and allows for proper text display regardless of device settings or content size.app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (1)
288-294: Improved code formatting for navigation logic.The conditional assignment for the
CONNECT_RECOVERY_CONFIGURE_BIOMETRICScase has been nicely reformatted from a single line to multiple lines with proper indentation, making the code more readable without changing its functionality.app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java (1)
12-12: Added Guava Strings utility import.This import supports the enhanced string validation in the delivery description handling.
app/src/org/commcare/connect/database/ConnectJobUtils.java (1)
227-238: Enhanced transaction safety with proper try-finally blockThe modification properly wraps database operations in a try-finally block to ensure that
endTransaction()is always called, even if an exception occurs during the database operations. This prevents potential transaction leaks when removing existing delivery flags and inserting new ones.app/src/org/commcare/activities/LoginActivity.java (2)
144-144: Method call changed from ConnectIDManager to ConnectManagerThis change aligns with the refactoring that moves Connect job management responsibilities from
ConnectIDManagerto static methods inConnectManager.
471-471: Method call changed from ConnectIDManager instance to ConnectManager static methodThis change is part of the centralization of Connect job management in
ConnectManager. The code now uses the staticsetConnectJobForAppmethod instead of the instance method onConnectIDManager.app/src/org/commcare/connect/ConnectManager.java (6)
165-165: Changed activeJob from instance variable to static variableThis change supports the refactoring to centralize Connect job management in static methods.
175-176: Updated setActiveJob to use static fieldThe method now updates the static
activeJobfield directly instead of via an instance, maintaining consistency with the refactoring approach.
179-180: Updated getActiveJob to return static fieldThe method now returns the static
activeJobfield directly, consistent with the refactoring to centralize state in static methods.
193-194: Updated parent activity assignmentThe
goToMessagingmethod now sets theparentActivityvia the singleton instance rather than directly on a static field.
198-206: Added static utility method for retrieving and setting job for appThis new method encapsulates the logic for retrieving the
ConnectAppRecordfor a given app ID, obtaining the associated composite job, setting it as the active job, and returning it. This centralizes job management that was previously handled inConnectIDManager.
208-216: Added static utility method for job status display logicThis new method determines whether the job status should be displayed based on app record and job status. It returns false if either record or active job is null, or if the app is a learning app with a job status of "delivering".
| if (Strings.isNullOrEmpty(descriptionText)) { | ||
| if (delivery.getFlags() != null) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhanced null and empty string handling.
Replacing the simple null check with Strings.isNullOrEmpty() improves robustness by handling both null and empty strings, which prevents potential display issues when descriptions are empty but not null.
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java
around lines 195 to 196, replace the existing null check on descriptionText with
Strings.isNullOrEmpty(descriptionText) to handle both null and empty string
cases, ensuring more robust validation and preventing display issues when
descriptions are empty but not null.
Product Description
Regression testing qa bug fixes
https://dimagi.atlassian.net/browse/QA-7662?linkSource=email
cross-request: dimagi/commcare-core#1475
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review