Conversation
Fixed some bugs regarding incorrect UI state at various stages of an opportunity's workflow.
conroy-ricketts
requested review from
Jignesh-dimagi,
OrangeAndGreen,
jaypanchal-13 and
shubham1g5
📝 WalkthroughWalkthroughThis pull request modifies job progress tracking and navigation logic across three fragments in the ConnectHome app. The adapter now uses the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (1)
216-257:⚠️ Potential issue | 🟠 MajorUse composite status for list classification to avoid state mismatch.
At Line 236, the switch still branches on
job.getStatus()while the row model now usescompositeJob. If these differ, entries can be categorized under the wrong stage while rendering composite-derived data.🔧 Proposed fix
- boolean isLearnAppInstalled = - AppUtils.isAppInstalled(job.getLearnAppInfo().getAppId()); - boolean isDeliverAppInstalled = - AppUtils.isAppInstalled(job.getDeliveryAppInfo().getAppId()); ConnectJobRecord compositeJob = ConnectJobUtils.getCompositeJob(requireActivity(), job.getJobUUID()); Objects.requireNonNull(compositeJob); + boolean isLearnAppInstalled = + AppUtils.isAppInstalled(compositeJob.getLearnAppInfo().getAppId()); + boolean isDeliverAppInstalled = + AppUtils.isAppInstalled(compositeJob.getDeliveryAppInfo().getAppId()); boolean userCompletedDelivery = compositeJob.getStatus() == ConnectJobRecord.STATUS_DELIVERING && compositeJob.getDeliveryProgressPercentage() == 100; @@ - switch (job.getStatus()) { + switch (compositeJob.getStatus()) { case STATUS_AVAILABLE_NEW, STATUS_AVAILABLE: newJobs.add(createJobModel( compositeJob,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java` around lines 216 - 257, The switch is using job.getStatus() while the row model is created from compositeJob, which can cause misclassification; change the switch to branch on compositeJob.getStatus() (the ConnectJobRecord instance returned by ConnectJobUtils.getCompositeJob) so createJobModel(...) calls and list additions (newJobs, inProgressJobs, finishedJobs) use the composite status consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java`:
- Around line 216-257: The switch is using job.getStatus() while the row model
is created from compositeJob, which can cause misclassification; change the
switch to branch on compositeJob.getStatus() (the ConnectJobRecord instance
returned by ConnectJobUtils.getCompositeJob) so createJobModel(...) calls and
list additions (newJobs, inProgressJobs, finishedJobs) use the composite status
consistently.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/org/commcare/adapters/JobListConnectHomeAppsAdapter.javaapp/src/org/commcare/fragments/connect/ConnectJobDetailBottomSheetDialogFragment.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
One thing to consider but otherwise looks fine to me.
One thing to note: I think long-term the goal with the ConnectLoginJobListModel should be not to have the job be included in the object, but instead have any per-item state values be calculated in advance and included directly in the model. That way the adapter simply looks up answers, it doesn't calculate anything on its own.
The interim solution being used now isn't too bad since the calculations are pretty cheap or also just direct lookups on the job. Before we had each row retrieving the composite job from the DB, which had more potential to create performance issues when scrolling.
| progress = item.getLearningProgress(); | ||
| progressColor = ContextCompat.getColor(context, R.color.connect_blue_color); | ||
| } else { | ||
| progress = item.getDeliveryProgress(); |
There was a problem hiding this comment.
I'm pretty sure this will always be zero since the job is still in learning state, but that's what we want. Should we just hard-code it to zero so we're not relying on a value that technically is in an undefined state?
|
Oh, might be worth fixing this issue Brendon brought up before merging this |
Tweaked the UI for the different opportunity states again.
…nto CCCT-2199-landing-page-bug-fixes
conroy-ricketts
dismissed stale reviews from OrangeAndGreen and shubham1g5
via
768026f
|
@shubham1g5 @OrangeAndGreen Pushed another commit just now to address the thing Brendon brought up in Slack, so basically now we show the learning icon with a full 100% when an assessment is passed. As I understand it, this will be a temporary solution until we implement CCCT-2138 |
|
I just now updated the video in the description |
@OrangeAndGreen I like this a lot! Will definitely be a good thing to do soon |
|
@conroy-ricketts Curious if we still need this PR assuming we implement https://dimagi.atlassian.net/browse/CCCT-2138 in the release ? |
|
@shubham1g5 Yeah I think this PR may not be necessary anymore as I'll be changing this functionality again with CCCT-2138, and this PR is relatively small anyways. I'll close this PR and include any changes with my new PR for CCCT-2138! |
3 participants
I'm linking these bugs to CCCT-2199, but these fixes were inspired by QA-8393.
Product Description
The TLDR is that I fixed some bugs regarding incorrect UI state at various stages of an opportunity.
Here is a video showing what the entire workflow looks like with the changes in this PR (keep your eyes on "Test Opp"):
Screen_Recording_20260227_141613_CommCare.Debug.mp4
Technical Summary
So, when I was trying to reproduce the QA ticket (QA-8393), I noticed a bug where if you complete all learn modules for a new opportunity, but do not start the assessment, the "Proceed" button is shown on the landing page which takes you to the Delivery overview page (the correct UX here is that we show the "Resume" button and tapping on that takes the user to the Learn Progress screen which has the "Go to assessment" button).
Here is a video demonstrating the incorrect UX that I described above (the opportunity called "The Best Opportunity Ever" in my video has all learn modules complete but I had not attempted the assessment yet):
Screen_Recording_20260227_102443_CommCare.Debug.mp4
Here is a video demonstrating the correct UX for that same opportunity:
Screen_Recording_20260227_104813_CommCare.Debug.mp4
Also, an important note here is that when getting the
passedAssessmentvalue from a job, we need the "composite job" fromConnectJobUtils.getCompositeJob()to get the correct value.I feel like this composite job thing has caused the team quite a bit of unnecessary headache in the past, so I just went ahead and refactored the code to use the composite job whenever we create the
ConnectLoginJobListModel's inConnectJobsListsFragment.setJobListData()since that is what is being passed around everywhere in the code. This allows us to cleanup the code a bit, and hopefully we no longer have anymore issues regarding this.Safety Assurance
Safety story
I ran multiple opportunities through the entire flow and checked the landing page every step of the way to verify that both the opportunity card tile and the "View Info" bottom sheet shows the correct UI given the current state of the opportunities.
QA Plan
Run a brand new opportunity through the entire workflow and check the landing page every step of the way to verify that both the opportunity card tile and the "View Info" bottom sheet shows the correct UI given the current state of your opportunity.