Job status fix for learning #2990
Conversation
…status is farther than remote. Handles a special case when transitioning to learning.
📝 WalkthroughWalkthroughThis pull request introduces an additional conditional block within the Sequence Diagram(s)sequenceDiagram
participant DBHelper as ConnectDatabaseHelper.storeJobs
participant Existing as ExistingJobRecord
participant Incoming as IncomingJobRecord
DBHelper->>Existing: Retrieve current status
DBHelper->>Incoming: Retrieve new job status
alt Existing.status > Incoming.status
DBHelper->>Incoming: Update incoming status to existing status
else
DBHelper->>Incoming: Retain incoming status
end
DBHelper->>DBHelper: Persist job record
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ 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 (
|
| if(existing.getStatus() > incoming.getStatus()) { | ||
| incoming.setStatus(existing.getStatus()); | ||
| } |
There was a problem hiding this comment.
Can status ever change on server side ? I wonder if it can cause server changes to get overwritten in some cases ?
There was a problem hiding this comment.
Not really since the server doesn't actually send a status for each job, just data that mobile uses to determine the status. Status changes do occur this way though, but only in the "forward" direction (i.e. increasing integer values), i.e. Available >> Learning >> Delivering.
That's why the check here only overrides the value when existing status is greater than incoming status.
There was a problem hiding this comment.
can we always do incoming.setStatus(existing.getStatus()); in that case to simplify the reasoning here to respect mobile statuses over server always. (Assuming that the final status will be same either way)
There was a problem hiding this comment.
Hmm, yeah, that could probably work. I was thinking there could be cases where a server-side change is the impetus for moving to a new state, but currently that's not ever the case.
But for example, if we were to add a special state for "assessment", i.e. the user completed all learn modules but hasn't passed the assessment yet. In a case like that, the user would submit an assessment form, and then sometime later mobile would learn (via the jobs API call) that the user had passed the assessment and progressed to delivery state.
So it seemed to me that allowing the server to move us forward in state could logically make sense, if not now then in the future. But nothing should ever move the user backward in the workflow, at least not as we've designed it so far.
Your thoughts?
There was a problem hiding this comment.
Sounds good to me to keep it as it is based on your reasoning here.
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Approving with an optional suggestion
No ticket, fixing a quick bug for Jon.
Product Description
Fixes a special case where the user has transitioned to learning but hasn't completed a learn module yet. This ensures that the status stays in "learning" and doesn't revert to "available".
Technical Summary
The underlying issue is that the server doesn't send a status along with each job when we retrieve the jobs. So when parsing and storing the job data, we use a couple checks to determine the current status. Unfortunately, the check to determine whether we're in learning state is whether the user has >0 learn completion (i.e. they've completed at least one learn module).
When the user starts learning, the mobile code changes the status for the job locally to learning, but when the job gets downloaded again we calculate the status fresh from the payload, resulting in "available" status again. This is handled by taking whichever status is later in the workflow when comparing newly received jobs to what's already in storage. So if local knows we're in learning, we'll override available to learning before storing the job in the DB.
Feature Flag
Connect
Safety Assurance
Safety story
Small change to enforce expected behavior
Automated test coverage
None
QA Plan
Not required
Labels and Review