CCCT-567 || Home Screen UI Fixes #2917
Conversation
Hide job status button in home screen while job is in review
Solved home screen issues
| @@ -27,8 +29,8 @@ | |||
| */ | |||
| public class HomeButtons { | |||
|
|
|||
| private final static String[] buttonNames = | |||
There was a problem hiding this comment.
Apologies, I should have provided more guidance here. Hiding the button can be accomplished more easily and without having to make additional modifications to this code. Instead, we just need to enhance StandardHomeActivityUIController.getHiddenButtons so it correctly indicates when to hide the Job Status button. The list of button names returned by that function is passed through to HomeButtons so buttons can be shown or hidden as indicated.
| @@ -985,4 +986,11 @@ public static String generatePassword() { | |||
|
|
|||
| return password.toString(); | |||
| } | |||
|
|
|||
| public static boolean shouldShowJobStatus(Activity activity) { | |||
There was a problem hiding this comment.
StandardHomeActivityUIController.getHiddenButtons an call this method to determine whether to add "connect" to the list of hidden buttons.
| String appId = CommCareApplication.instance().getCurrentApp().getUniqueId(); | ||
| ConnectAppRecord record = ConnectManager.getAppRecord(activity, appId); | ||
| ConnectJobRecord job = ConnectManager.getActiveJob(); | ||
| return job.getStatus() == STATUS_DELIVERING && record.getIsLearning(); |
There was a problem hiding this comment.
For the logic here, we want to check for two possibilities where we want to show the Job Status button:
- Job in learning state, logged in to learning app
- Job in delivery state, logged in to delivery app
So (status == LEARNING && record.getIsLearning) || (status == DELIVERY && !record.getisLearning)
|
(Testing code rabbit here) |
|
@shubham1g5: I will perform a full review of the pull request as requested. ✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe changes made across the three Java files involve significant structural and logical modifications aimed at enhancing readability, modularity, and functionality. In In Lastly, in Sequence Diagram(s)sequenceDiagram
participant User
participant HomeButtons
participant StandardHomeActivityUIController
participant ConnectManager
User->>HomeButtons: Request button data
HomeButtons->>HomeButtons: Build button data
HomeButtons->>HomeButtons: Create dynamic buttons
HomeButtons-->>User: Return button data
User->>StandardHomeActivityUIController: Update job tile
StandardHomeActivityUIController->>StandardHomeActivityUIController: Check job status
StandardHomeActivityUIController-->>User: Update UI based on job status
User->>ConnectManager: Authenticate user
ConnectManager->>ConnectManager: Check biometric manager
ConnectManager->>ConnectManager: Handle authentication
ConnectManager-->>User: Authentication result
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 4
🧹 Outside diff range and nitpick comments (5)
app/src/org/commcare/activities/StandardHomeActivityUIController.java (2)
Line range hint
113-171: Consider breaking down complex logic into smaller methodsThe method is quite long and handles multiple complex scenarios. This makes it harder to maintain and test.
Consider extracting the payment unit logic into separate methods:
+ private List<String> getExceededTotalMaxUnits(ConnectJobRecord job) { + List<String> totalMaxes = new ArrayList<>(); + Hashtable<String, Integer> totalPaymentCounts = job.getDeliveryCountsPerPaymentUnit(false); + for (ConnectPaymentUnitRecord unit : job.getPaymentUnits()) { + String stringKey = Integer.toString(unit.getUnitId()); + int totalCount = totalPaymentCounts.getOrDefault(stringKey, 0); + if (totalCount >= unit.getMaxTotal()) { + totalMaxes.add(unit.getName()); + } + } + return totalMaxes; + } + private List<String> getExceededDailyMaxUnits(ConnectJobRecord job) { + // Similar extraction for daily max logic + }Also, consider using StringBuilder for string concatenation in loops:
- warningText += activity.getString(...); + StringBuilder warningText = new StringBuilder(); + warningText.append(activity.getString(...));
205-205: Consider documenting the single progress bar limitationThe comment indicates that only a single daily progress bar is shown currently. Consider adding a TODO or creating a ticket for future implementation of multiple progress bars.
app/src/org/commcare/activities/HomeButtons.java (1)
42-44: Simplify localization key selectionThe assignments for
homeMessageKey,syncKey, andlogoutMessageKeyuse ternary operators based onisDemoUser. This can be simplified for better readability and maintainability.Consider creating a helper method or a mapping that returns the appropriate localization key based on the
isDemoUserflag. For example:private static String getLocalizedKey(String baseKey, boolean isDemoUser) { return isDemoUser ? baseKey + ".demo" : baseKey; } // Usage: String homeMessageKey = getLocalizedKey("home.start", isDemoUser); String syncKey = getLocalizedKey("home.sync", isDemoUser); String logoutMessageKey = getLocalizedKey("home.logout", isDemoUser);This reduces code duplication and makes the intent clearer.
app/src/org/commcare/connect/ConnectManager.java (2)
Line range hint
170-175: Potential Thread Safety Issue with Singleton PatternThe
getInstance()method and thebiometricManagerfield are accessed without synchronization. If multiple threads call these methods simultaneously, it could lead to race conditions or inconsistent states.To ensure thread safety, consider synchronizing the
getInstance()andgetBiometricManager()methods:-private static ConnectManager getInstance() { +private static synchronized ConnectManager getInstance() { if (manager == null) { manager = new ConnectManager(); } return manager; } -public static BiometricManager getBiometricManager(CommCareActivity<?> parent) { +public static synchronized BiometricManager getBiometricManager(CommCareActivity<?> parent) { ConnectManager instance = getInstance(); if (instance.biometricManager == null) { instance.biometricManager = BiometricManager.from(parent); } return instance.biometricManager; }
Line range hint
674-691: Variable 'record' May Not Be in Scope Outside If-Else BlocksThe variable
recordis declared within theifandelse ifblocks but is accessed outside these blocks at line 691. This will lead to a compilation error due to scope issues.To fix this, declare
recordbefore the conditional blocks:if (isConnectIdConfigured()) { + ConnectLinkedAppRecord record = null; if (appLaunchedFromConnect) { // Configure some things if we haven't already - ConnectLinkedAppRecord record = ConnectDatabaseHelper.getAppData(context, appId, username); + record = ConnectDatabaseHelper.getAppData(context, appId, username); if (record == null) { record = prepareConnectManagedApp(context, appId, username); } passwordOrPin = record.getPassword(); } else if (uiInAutoLogin) { String seatedAppId = CommCareApplication.instance().getCurrentApp().getUniqueId(); - ConnectLinkedAppRecord record = ConnectDatabaseHelper.getAppData(context, seatedAppId, username); + record = ConnectDatabaseHelper.getAppData(context, seatedAppId, username); passwordOrPin = record != null ? record.getPassword() : null; if (record != null && record.isUsingLocalPassphrase()) { // Report to analytics so we know when this stops happening FirebaseAnalyticsUtil.reportCccAppAutoLoginWithLocalPassphrase(seatedAppId); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/org/commcare/activities/HomeButtons.java(4 hunks)app/src/org/commcare/activities/StandardHomeActivityUIController.java(7 hunks)app/src/org/commcare/connect/ConnectManager.java(11 hunks)
🔇 Additional comments (4)
app/src/org/commcare/activities/StandardHomeActivityUIController.java (3)
3-4: LGTM: Import statement is correctly placed and necessary
The import for STATUS_DELIVERING is properly added and is used in the updateConnectProgress method.
Line range hint 81-100: Consider adding null check for job object
The code assumes that job is non-null when accessing its methods. While this might be guaranteed by the current flow, it's safer to add an explicit null check.
Let's verify if there are any NPE reports or similar issues:
Consider refactoring this section for better null safety and separation of concerns:
if (show) {
+ if (job == null) {
+ return;
+ }
ConnectBoldTextView tvJobTitle = viewJobCard.findViewById(R.id.tv_job_title);
// ... other findViewById calls ...
+ updateJobTitleAndDescription(job, tvJobTitle, tvJobDiscrepation);
+ updateJobPaymentInfo(job, connectJobPay, connectJobEndDate);
+ updateWorkingHours(job, tv_job_time, hoursTitle);
+
+ updateConnectProgress();
}197-200: Verify the impact of hiding recyclerView when job is finished
The new condition hides the recyclerView when a job is both in DELIVERING status and finished. Please verify that this doesn't cause any UI inconsistencies.
Let's check for similar visibility patterns in the codebase:
✅ Verification successful
Let me gather more context about the visibility handling and job status checks.
Let me check one more thing to understand the complete lifecycle of the recyclerView and its initialization.
The recyclerView visibility logic is consistent with the codebase's behavior
The visibility change is part of a well-structured flow where:
- The recyclerView is properly initialized in
updateConnectProgress() - The STATUS_DELIVERING state is consistently checked across multiple activities
- The condition combines both status and completion state appropriately
- The visibility is only hidden when both conditions are met (delivering and finished)
This implementation aligns with other usages in StandardHomeActivity and ConnectActivity, suggesting it's a deliberate part of the UI flow for completed delivery jobs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar recyclerView visibility patterns
ast-grep --pattern 'recyclerView.setVisibility($VISIBILITY)'
Length of output: 187
Script:
#!/bin/bash
# Search for job status and visibility related patterns
rg -A 3 "getStatus\(\)" app/src/org/commcare/activities/StandardHomeActivityUIController.java
# Look for other recyclerView usages
rg -A 3 "recyclerView" app/src/org/commcare/activities/StandardHomeActivityUIController.java
# Search for job finished checks
rg -A 3 "isFinished\(\)" app/src/org/commcare/activities/StandardHomeActivityUIController.java
Length of output: 1391
Script:
#!/bin/bash
# Search for the class and method context where recyclerView is used
rg -B 10 -A 10 "RecyclerView recyclerView" app/src/org/commcare/activities/StandardHomeActivityUIController.java
# Look for STATUS_DELIVERING constant definition
rg "STATUS_DELIVERING" app/src/org/commcare/activities/
Length of output: 1432
app/src/org/commcare/connect/ConnectManager.java (1)
989-995:
Incorrect Logic in 'shouldShowJobStatus' Method
The current logic in shouldShowJobStatus does not account for all required scenarios. Specifically, it should display the job status under two conditions:
- When the job is in the learning state and the app is a learning app.
- When the job is in the delivering state and the app is a delivery app.
Currently, the method only checks for STATUS_DELIVERING and record.getIsLearning(), which may not cover all cases.
Update the return statement to include both conditions:
-return job.getStatus() == STATUS_DELIVERING && record.getIsLearning();
+return (job.getStatus() == STATUS_LEARNING && record.getIsLearning()) ||
+ (job.getStatus() == STATUS_DELIVERING && !record.getIsLearning());This change ensures that the job status is shown appropriately based on the job's status and the app's type.
| if (!hideJobStatus) { | ||
| buttons.add(createStaticButton(Localization.get("home.connect"), R.color.white, R.drawable.baseline_save_24, R.color.orange_500, getConnectButtonListener(activity))); | ||
| } |
There was a problem hiding this comment.
Incorrect logic in condition for adding the "connect" button
The condition if (!hideJobStatus) may be affected by the assignment issue of hideJobStatus. If hideJobStatus is incorrectly set, this condition could lead to the "connect" button being displayed or hidden unintentionally.
After correcting the hideJobStatus assignment, ensure that this condition correctly reflects the intended logic for displaying the "connect" button.
| private static String[] buttonNames = | ||
| new String[]{}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor button management to improve maintainability
The current implementation relies on the order of the buttonNames array aligning with the buttons list, which can be error-prone and difficult to maintain. This tight coupling may lead to issues if buttons are added, removed, or reordered in the future.
Consider using a single data structure, such as a list of custom objects or a map, that encapsulates both the button identifiers and their corresponding HomeCardDisplayData. This approach can eliminate the need for parallel arrays and reduce the risk of indexing errors.
Also applies to: 48-50, 52-65
| String syncKey = isDemoUser ? "home.sync.demo" : "home.sync"; | ||
| String logoutMessageKey = isDemoUser ? "home.logout.demo" : "home.logout"; | ||
|
|
||
| boolean hideJobStatus = ConnectManager.shouldShowJobStatus(activity); |
There was a problem hiding this comment.
Potential logical error in hideJobStatus assignment
The variable hideJobStatus may be incorrectly assigned. The method ConnectManager.shouldShowJobStatus(activity) likely returns true when the job status should be shown, but assigning it to hideJobStatus implies the opposite.
Consider adjusting the assignment to:
- boolean hideJobStatus = ConnectManager.shouldShowJobStatus(activity);
+ boolean hideJobStatus = !ConnectManager.shouldShowJobStatus(activity);This ensures that hideJobStatus accurately represents whether the job status should be hidden.
📝 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.
| boolean hideJobStatus = ConnectManager.shouldShowJobStatus(activity); | |
| boolean hideJobStatus = !ConnectManager.shouldShowJobStatus(activity); |
| if (isConnectIdConfigured()) { | ||
| ConnectUserRecord user = getUser(context); | ||
| show = !user.getSecondaryPhoneVerified(); | ||
| } |
There was a problem hiding this comment.
Add Null Check for 'user' to Prevent NullPointerException
The method getUser(context) may return null, which would cause a NullPointerException when calling user.getSecondaryPhoneVerified(). Ensure that user is not null before accessing its methods.
Apply this diff to add a null check:
if (isConnectIdConfigured()) {
ConnectUserRecord user = getUser(context);
+ if (user != null) {
show = !user.getSecondaryPhoneVerified();
+ } else {
+ show = false; // Handle null user appropriately
+ }
}📝 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 (isConnectIdConfigured()) { | |
| ConnectUserRecord user = getUser(context); | |
| show = !user.getSecondaryPhoneVerified(); | |
| } | |
| if (isConnectIdConfigured()) { | |
| ConnectUserRecord user = getUser(context); | |
| if (user != null) { | |
| show = !user.getSecondaryPhoneVerified(); | |
| } else { | |
| show = false; // Handle null user appropriately | |
| } | |
| } |
Change logic for hide job status button
Summary
https://dimagi.atlassian.net/browse/CCCT-567
Feature Flag
Product Description
PR Checklist
Automated test coverage
Safety story