Preserving local job status when updating list from API call. #3336
Conversation
OrangeAndGreen
requested review from
Jignesh-dimagi,
avazirna and
shubham1g5
📝 WalkthroughWalkthroughThe update logic in JobStoreManager now preserves an existing job’s status during an update. When an incoming job matches an existing job by jobId, the code copies both the ID and the status from the existing record to the incoming job before saving. New job creation and subsequent storage of app info, modules, and payment units remain unchanged. No public signatures were modified. Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant JSM as JobStoreManager
participant DB as Database
Caller->>JSM: store/update jobs
JSM->>DB: find job by jobId
alt Existing job found
DB-->>JSM: existingJob
note right of JSM: Copy ID and status from existingJob to incoming job
JSM->>DB: save(updated job with preserved status)
DB-->>JSM: ack
else No existing job
DB-->>JSM: not found
JSM->>DB: insert(new job)
DB-->>JSM: ack
end
JSM-->>Caller: result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/src/org/commcare/connect/database/JobStoreManager.java (6)
101-106: Avoid freezing legitimate server-driven transitions (add a guard)If any non-learning statuses are (now or later) derived from payload changes, the unconditional override could freeze progress. Consider only overriding when the incoming status is the default/derived “available” state.
- job.setStatus(existingJob.getStatus()); + if (job.getStatus() == ConnectJobRecord.STATUS_AVAILABLE + || job.getStatus() == ConnectJobRecord.STATUS_AVAILABLE_NEW) { + job.setStatus(existingJob.getStatus()); + }
88-88: Duplicate lastUpdate set — pick oneYou set lastUpdate in both processAndStoreJobs and storeOrUpdateJob. Keep only the inner one to avoid redundant assignments.
- job.setLastUpdate(new Date());Also applies to: 116-116
77-82: Delete children before parent to avoid FK/consistency issuesIf FK constraints or cascading are enabled now or in the future, remove modules/payment units/app infos before jobs.
- jobStorage.removeAll(jobIds); - appInfoStorage.removeAll(appInfoIds); - moduleStorage.removeAll(moduleIds); - paymentUnitStorage.removeAll(paymentUnitIds); + moduleStorage.removeAll(moduleIds); + paymentUnitStorage.removeAll(paymentUnitIds); + appInfoStorage.removeAll(appInfoIds); + jobStorage.removeAll(jobIds);
84-96: Avoid O(n²) lookups when matching jobsFor large lists, scanning existingJobs per incoming job is quadratic. Pre-index by jobId.
- private int processAndStoreJobs(List<ConnectJobRecord> existingJobs, List<ConnectJobRecord> jobs) { + private int processAndStoreJobs(List<ConnectJobRecord> existingJobs, List<ConnectJobRecord> jobs) { int newJobs = 0; + Map<Integer, ConnectJobRecord> existingById = new HashMap<>(); + for (ConnectJobRecord ej : existingJobs) { + existingById.put(ej.getJobId(), ej); + } for (ConnectJobRecord job : jobs) { - job.setLastUpdate(new Date()); - boolean isExisting = storeOrUpdateJob(existingJobs, job); + boolean isExisting = storeOrUpdateJob(existingById, job); if (!isExisting) { newJobs++; } } return newJobs; } - private boolean storeOrUpdateJob(List<ConnectJobRecord> existingJobs, ConnectJobRecord job) { + private boolean storeOrUpdateJob(Map<Integer, ConnectJobRecord> existingById, ConnectJobRecord job) { // Check if the job already exists - boolean isExisting = false; - for (ConnectJobRecord existingJob : existingJobs) { - if (existingJob.getJobId() == job.getJobId()) { - job.setID(existingJob.getID()); // Set ID for updating - job.setStatus(existingJob.getStatus()); - isExisting = true; - break; - } - } + ConnectJobRecord existingJob = existingById.get(job.getJobId()); + boolean isExisting = existingJob != null; + if (isExisting) { + job.setID(existingJob.getID()); // Set ID for updating + job.setStatus(existingJob.getStatus()); // Preserve local status + }Also applies to: 98-123
133-139: Null-safety for app infosAssuming both learn and delivery app infos are always non-null. If not guaranteed by ConnectJobRecord, add guards to avoid NPEs during ID/field updates.
- for (ConnectAppRecord existing : existingAppInfos) { + for (ConnectAppRecord existing : existingAppInfos) { if (existing.getIsLearning()) { - job.getLearnAppInfo().setID(existing.getID()); // Set ID for updating + if (job.getLearnAppInfo() != null) { + job.getLearnAppInfo().setID(existing.getID()); + } } else { - job.getDeliveryAppInfo().setID(existing.getID()); // Set ID for updating + if (job.getDeliveryAppInfo() != null) { + job.getDeliveryAppInfo().setID(existing.getID()); + } } }Also applies to: 140-147
98-123: Add a focused unit test for the learning-preserve caseHigh-value test: existing job with STATUS_LEARNING remains LEARNING after storeJobs with incoming AVAILABLE.
I can draft a small JUnit (Robolectric) test exercising storeOrUpdateJob with mocked storage if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/connect/database/JobStoreManager.java(1 hunks)
🧰 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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (1)
app/src/org/commcare/connect/database/JobStoreManager.java (1)
101-106: Preserve local status on updates — LGTMCopying the existing record’s status achieves the PR goal of preventing regressions to AVAILABLE on refresh.
|
@damagatchi retest this please |
https://dimagi.atlassian.net/browse/CCCT-1713
Product Description
Job status is preserved when performing updates from the server (learning won't reset to available).
Technical Summary
Keeping the local status when updating storage from the list that was received from the API.
The server doesn't send storage, so we try to calculate it.
But if we knew the status before making the API call, that should be treated as the latest value.
Feature Flag
Connect
Safety Assurance
Safety story
Small change, only affects job status
Dev tested, dev approved
Automated test coverage
None
QA Plan
Invite user to a new job, begin learning, but don't complete and learning modules. Return to the jobs list, and after refreshing the job should still show "learning" status (should not go back to "available" status).