CCCT-2054 - UUIDs are replaced for int id for Connect#3535
CCCT-2054 - UUIDs are replaced for int id for Connect#3535Jignesh-dimagi wants to merge 16 commits intomasterfrom
Conversation
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThis pull request migrates the Connect module from integer-based job and payment unit identifiers to string-based UUID identifiers. Changes span model classes, database schema and migrations, API methods, fragments, and helper utilities. Key modifications include: ConnectActivity now extracts OPPORTUNITY_UUID instead of OPPORTUNITY_ID from intents; database models (ConnectAppRecord, ConnectDeliveryDetails, ConnectJobDeliveryRecord, etc.) expose UUID accessors and update schema versioning from 22 to 23; API methods update parameter types from int jobId to String jobUUID; helper utilities and fragments update job lookups to use jobUUID; push notification handling switches to UUID-based payload keys. A migration helper ConnectJobDeliveryRecordV22 is introduced to support the schema transition. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 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.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/src/org/commcare/android/database/connect/models/ConnectPaymentUnitRecord.java (1)
29-29:⚠️ Potential issue | 🟡 MinorMisleading constant name:
META_PAYMENT_UNIT_UUIDsuggests UUID but contains a string ID.The constant name and field name (
unitUUID) both suggest this is a UUID, but line 94 reveals it's actuallyString.valueOf(connectPaymentUnitRecordV21.getUnitId())— a string representation of an integer ID, not a UUID format. Consider renamingMETA_PAYMENT_UNIT_UUIDtoMETA_PAYMENT_UNIT_IDand the field fromunitUUIDtounitId(or similar) for consistency and clarity, or add a comment explaining that this field stores a stringified integer identifier despite its name.app/src/org/commcare/android/database/connect/models/ConnectJobPaymentRecord.java (1)
90-91:⚠️ Potential issue | 🔴 CriticalLine 91 uses the wrong constant to parse
paymentUUID.The
paymentUUIDfield is defined with@MetaField(META_PAYMENT_UUID)(line 66), which maps to the JSON key"payment_uuid". However, line 91 reads fromMETA_PAYMENT_ID("payment_id") instead. This will cause the UUID field to be populated with the wrong value from the server response. TheMETA_PAYMENT_UUIDconstant exists specifically for this purpose (line 30) and is consistent with field definitions and server API contract.🐛 Proposed fix
payment.paymentId = json.getString("id"); - payment.paymentUUID = json.getString(META_PAYMENT_ID); + payment.paymentUUID = json.getString(META_PAYMENT_UUID);app/src/org/commcare/android/database/connect/models/ConnectDeliveryDetails.java (1)
10-22:⚠️ Potential issue | 🟠 MajorMissing getter (and setter) for
unitUUID.The field is assigned in the constructor but never exposed. Every other field in this class has a matching getter/setter pair—this is an inconsistency that leaves
unitUUIDinaccessible after construction.Proposed fix
public void setApprovedPercentage(double approvedPercentage) { this.approvedPercentage = approvedPercentage; } + + public String getUnitUUID() { + return unitUUID; + } + + public void setUnitUUID(String unitUUID) { + this.unitUUID = unitUUID; + } }app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java (1)
179-196:⚠️ Potential issue | 🟠 Major
slugUUIDis assigned fromoldRecord.getSlug()instead of a UUID value.Line 193 assigns
newRecord.slugUUID = oldRecord.getSlug(), which populates a field documented as "This is payment unit uuid" (line 78) with a human-readable slug string. The V22 model does not have aslugUUIDfield—onlyslug. However, assigning the slug (payment unit ID per line 60) to a field namedslugUUIDthat should contain a server-assigned UUID creates semantic confusion and inconsistency with the field's documented purpose.The fromJson() method (line 105) correctly populates
slugUUIDfrom server data (json.getString(META_SLUG_UUID)), which will provide proper UUIDs during normal synchronization. During migration from V22, consider explicitly documenting whether this slug-to-UUID assignment is intentional as a temporary placeholder, or if the field should remain uninitialized/null until the server provides actual UUID values.
🤖 Fix all issues with AI agents
In
`@app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecordV22.kt`:
- Around line 70-84: Rename the factory method
ConnectJobDeliveryRecordV22.fromV22 to fromV21 (it currently accepts a
ConnectJobDeliveryRecordV21 and returns ConnectJobDeliveryRecordV22), update its
declaration and all callers to use fromV21 (e.g. the invocation in
ConnectDatabaseUpgrader), and run a compile to fix any remaining references;
keep the implementation identical and only change the method name to reflect it
converts from V21 to V22.
In `@app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java`:
- Around line 399-406: The query is comparing
PushNotificationRecord.META_OPPORTUNITY_ID (a String column) against jobId (an
int), causing a type mismatch; change the query to pass a String representation
of jobId (e.g., String.valueOf(jobId)) when calling
notificationStorage.getRecordsForValues so the WHERE binds use the correct type,
then continue to call pushNotificationRecord.setOpportunityUUID(jobUUID) and
notificationStorage.write(pushNotificationRecord) as before to persist the
migration.
- Around line 205-209: The fromJson(JSONObject) parser in ConnectJobRecord
currently calls migrateOtherModelsForUUID(job.jobId, job.jobUUID), which causes
heavy DB reads/writes during JSON parsing; remove that call from
ConnectJobRecord.fromJson so parsing has no side effects, and instead invoke
migrateOtherModelsForUUID from the storage layer where DB operations belong (for
example inside JobStoreManager.storeOrUpdateJob or the method that persists
ConnectJobRecord). Update callers to ensure migration runs only when
storing/updating a job and add tests to validate parsing without DB access and
migration execution during persistence.
- Around line 345-352: The storage query for ConnectLearnModuleSummaryRecord
uses the wrong constant: replace ConnectAppRecord.META_JOB_ID with
ConnectLearnModuleSummaryRecord.META_JOB_ID in the
moduleStorage.getRecordsForValues call so the query uses the correct
record-class constant (leave the loop that calls
connectLearnModuleSummaryRecord.setJobUUID(jobUUID) and moduleStorage.write(...)
unchanged).
- Around line 305-408: migrateOtherModelsForUUID is being invoked on every
fromJson parse and currently queries/writes 9 tables; to avoid the heavy work,
add an early idempotency check right after fetching connectJobRecords from
connectJobStorage: inspect the matching ConnectJobRecord(s) (from
connectJobRecords) and if any existing record already has getJobUUID() equal to
the incoming jobUUID, return immediately without touching other storages; keep
this check inside migrateOtherModelsForUUID (referencing connectJobStorage,
connectJobRecords and fromJson where it's called) so normal migration runs only
when the stored job UUID differs from the incoming UUID.
In `@app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java`:
- Around line 958-987: The migration in upgradeTwentyTwoTwentyThree (which calls
upgradeConnectJobDeliveryRecordToV23) performs multiple DB operations but lacks
a transaction; wrap the entire body of upgradeConnectJobDeliveryRecordToV23 in a
transaction by calling db.beginTransaction() before altering the
schema/iterating writes, call db.setTransactionSuccessful() after all operations
succeed, and ensure db.endTransaction() runs in a finally block so partial work
is rolled back on failure and the subsequent retry won't re-run a partially
applied migration.
In `@app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt`:
- Around line 230-237: The variable name oppotunityUUID is misspelled; rename it
to opportunityUUID in both startLearningSyncWorker and startDeliverySyncWorker,
update all usages (including the check TextUtils.isEmpty(opportunityUUID), the
cccCheckPassed call, and the startWorkRequest tag concatenation that uses
"-$oppotunityUUID"), and ensure any other occurrences in this file are similarly
corrected so references and string interpolation match the new identifier.
🧹 Nitpick comments (6)
app/src/org/commcare/models/connect/ConnectLoginJobListModel.java (1)
214-220: Nit:toStringstill labels the field as"id=".Consider updating the label to
"uuid="for consistency with the renamed field.Proposed fix
return "AddListItem{" + "name='" + name + '\'' + - ", id=" + uuid + + ", uuid=" + uuid + ", date=" + date +app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java (1)
103-105:delivery.reasonis assigned twice (pre-existing).Line 103 assigns
json.getString(META_REASON)and line 104 immediately overwrites it withoptStringSafe(...). The first assignment is dead code. Not introduced by this PR, but it's directly adjacent to the new line 105.Proposed cleanup
- delivery.reason = json.getString(META_REASON); delivery.reason = JsonExtensions.optStringSafe(json, META_REASON,""); delivery.slugUUID = json.getString(META_SLUG_UUID);app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecordV22.kt (1)
40-42: Minor:unitNamedefaults to""while other nullable fields default tonull.All other nullable String fields (
status,slug,entityId,entityName,reason,jobUUID) default tonull, butunitNamedefaults to"". This inconsistency could cause subtle issues if null-checking logic is used elsewhere.app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java (1)
417-418: New externalizable classes registered correctly.Both
PushNotificationRecordV21andConnectJobDeliveryRecordV22are added to the history list.Minor nit: consider adding a version comment (e.g.,
// Added in 2.XX) above these entries to match the existing pattern used throughout the list.app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (1)
68-68: Version bump to 23 is correct.Consider adding a
V.23entry to the version comment block above (lines 44-67) to document the addition ofslugUUIDtoConnectJobDeliveryRecord, following the existing documentation pattern.app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt (1)
4-4: Prefer Kotlin-idiomaticisNullOrEmpty()overTextUtils.isEmpty().In Kotlin, you can use
notificationPayload[OPPORTUNITY_UUID].isNullOrEmpty()instead of importingandroid.text.TextUtils. This is more idiomatic and avoids the Android framework dependency in a Kotlin file.
app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecordV22.kt
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt
Outdated
Show resolved
Hide resolved
Jignesh-dimagi
changed the title
|
Hey @Jignesh-dimagi, I'm seeing a bit of an odd issue with these changes. The opportunities list is failing to load a few of my opportunities: 
Here are the links to the failing opportunities if it helps: |
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java
Outdated
Show resolved
Hide resolved
Yes @conroy-ricketts. It's not fully working with production server as mentioned in the description of this ticket. You can check with Staging, where it's working fine. |
|
@Jignesh-dimagi Ah ok I missed that thanks, sounds good! |
| ConnectJobRecord job = new ConnectJobRecord(); | ||
|
|
||
| job.jobId = json.getInt(META_JOB_ID); | ||
| job.jobId = json.getInt(META_JOB_ID); // This will be eventually removed |
There was a problem hiding this comment.
Instead of a comment here, can we add a @deprecated to all the old model fields so that it's obvious to anyone writing code to not use these fields.
There was a problem hiding this comment.
@shubham1g5 I was thinking even of changing it to json.optInt(META_JOB_ID); so if the server stops sending it in the future, our app can keep running without issue. This will be applicable at some other places too. Your thoughts?
There was a problem hiding this comment.
+1 to both of the above ideas.
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
Show resolved
Hide resolved
| boolean isExisting = false; | ||
| for (ConnectJobRecord existingJob : existingJobs) { | ||
| if (existingJob.getJobId() == job.getJobId()) { | ||
| if (existingJob.getJobUUID().equals(job.getJobUUID())) { |
There was a problem hiding this comment.
We can't guarantee here that the uuid stored in mobile is stored as the old id or the new uuid and think we need to instead check
if (existingJob.getJobUUID().equals(job.getJobUUID()) || existingJob.getJobUUID().equals(job.getJobIId())
There was a problem hiding this comment.
As per the solution adapted here, every object including existing DB should have same value of uuid.
There was a problem hiding this comment.
@shubham1g5 I did test the flow before and after; it seems it's working after changes were made as per our offline talk.
| * @param jobUUID | ||
| */ | ||
| private static void migrateOtherModelsForJobUUID(int jobId, String jobUUID) { | ||
| if (jobId != -1 && !TextUtils.isEmpty(jobUUID)) { |
There was a problem hiding this comment.
can this condition be false ever besides a bug ?
There was a problem hiding this comment.
This can be false if server stops sending jobId in future.
There was a problem hiding this comment.
got it, and what about !TextUtils.isEmpty(jobUUID) ? Can that be empty ever ?
There was a problem hiding this comment.
@shubham1g5 It cannot be now, as the server has made the necessary changes. It was at the time when I was developing this code. Not sure if we want to handle it or not.
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
I don't think I have any additional concerns to add here 👍
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Left a few things to consider but nothing I would consider blocking. Well done on a pretty big lift for this one!
| ConnectJobRecord job = new ConnectJobRecord(); | ||
|
|
||
| job.jobId = json.getInt(META_JOB_ID); | ||
| job.jobId = json.getInt(META_JOB_ID); // This will be eventually removed |
There was a problem hiding this comment.
+1 to both of the above ideas.
| * @param jobId | ||
| * @param jobUUID | ||
| */ | ||
| private static void migrateOtherModelsForJobUUID(int jobId, String jobUUID) { |
There was a problem hiding this comment.
A couple thoughts on this method, nothing crucial:
-
It seems a bit unfortunate to add a bunch of code to this class which is only relevant for a limited time, or at least to a limited scope. I wonder if it would make more sense to put it in something like ConnectDatabaseUpgrader? This would be a deviation from the existing pattern in that class, but seems like it makes more sense there in the long-term so it isn't nearly doubling the size of this class for code that we know will eventually be outdated.
-
It also seems unfortunate that this code will run every time the jobs API call succeeds and for every received job (running a query for each job to see no migration necessary), when in reality the upgrade process only needs to happen once (or maybe never).
A couple ideas:
2a. We could do one check before going into the loop for all jobs, and see if we have a job in storage that needs to be upgraded (i.e. check any job and I think see if jobId==jobUUID). If any job has been upgraded then they all have, and we can skip the additional queries
2b. We could also employ a simple SharedPref to remember that we did the migration already (including new versions that never needed to do a migration).
There was a problem hiding this comment.
@OrangeAndGreen Yeah, this was chosen to make the code less complex to handle UUID vs ID. Now with these migrations, we are sure that values of Job UUID are uniform across the models, which is the main source of fetching any record. This will reduce the ambiguity which may arise due to different values of UUID in different models.
-
ConnectDatabaseUpgrader is mostly used for managing the structure / schema of the database and not for storing the real-time values. Also, during the V2 version of UUID changes, it is intended to remove all ID-related stuff, in which removing this function is also planned.
-
Your point is absolutely correct and makes sense. We already have that mechanism in place to not run the migration code multiple times. Once the migrations are done, this code will prevent doing it again. So this is one time operation only.
|
|
||
| // migrate ConnectJobRecord | ||
| for (ConnectJobRecord connectJobRecord : connectJobRecords) { | ||
| connectJobRecord.setJobUUID(jobUUID); |
There was a problem hiding this comment.
Nit: Given the goal of no longer exposing the PK IDs to users, is there any safety gain in resetting that value here so it doesn't linger in the DB?
There was a problem hiding this comment.
Not sure if I understand your question, but its lifecycle is the same as ConnectJobRecord int ID only. It will be removed once the record is no longer linked to the user.
Jignesh-dimagi
dismissed stale reviews from OrangeAndGreen and conroy-ricketts
via
4f8698d
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
4 participants
Product Description
This PR doesn't have direct UI changes, but it has changes which can affect the UI as data models are changed.
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-2054
The aim of these changes is that the application should start using the UUID instead of the integer ID. This would apply to:
Local DB models
API calling
Parsing the API response and storing the new UUID
Using UUID for fetching any data object and not using Int ID
Concept used for migration:
Whenever an application is upgraded from an older version, it will not have a UUID but only an Int ID. The application will default all UUIDs to their corresponding Int IDs.
The application will be using these UUIDs only for all the operations, as their default value is set to Int Id.
Now, whenever an application has a new UUID field value for ConnectJobRecord, which is the main record for referencing the Connect opportunities, all the other models are updated for its UUID in the
jobUUIDfield. So now the application has a uniform UUID record for opportunity.The entire app is using the UUID field to set, get and update the data models, as the application has the UUID field value always there, which can be the default Int Id or the new UUID actual value.
API calls are made using the UUID field only for the reason as stated in the above point.
Safety Assurance
Safety story
I have tested the application in the following scenarios:
Installing the 2.61 version and downloading the One Learn and Delivery app. Also, sends the push notification
Upgrading the version to these changes, and checking
DB migrations are good.
CommCare apps are still being used from the left navigation drawer without any issue.
Push notifications also working fine
Fetching the opportunities, installing the Learn and Delivery app from there and being able to sync the form successfully.
Connect opportunity showing correct job status, payments and payment units
Sending a new push notification, navigating to the correct page after clicking on it from the device tray. The same is true for push notifications from the notification screen.
QA Plan
QA should upgrade to these version from older and make sure that every thing is working.
Note: Due to some missing UUIDs in the production server, the app is working for the staging server correctly as of now. The web team will upload these changes on 23rd Feb, so it should be a good test on the production server after that.
Labels and Review