Show flags for deliveries #3040
Conversation
Retrieving flags from delivery_progress API call and storing in DB. Showing flags in delivery list when there are any (and no "reason" has been provided).
OrangeAndGreen
requested review from
Jignesh-dimagi,
avazirna,
pm-dimagi and
shubham1g5
📝 WalkthroughWalkthroughThe changes introduce a new data model, Sequence Diagram(s)sequenceDiagram
participant UI as ConnectDeliveryListFragment
participant DeliveryRecord as ConnectJobDeliveryRecord
participant Utils as ConnectJobUtils
participant DB as Database
UI->>DeliveryRecord: getReason()
alt reason is null
UI->>DeliveryRecord: getFlags()
DeliveryRecord-->>UI: List<FlagRecord>
UI->>UI: Concatenate flag descriptions
UI->>UI: Display concatenated descriptions
else reason is not null
UI->>UI: Display reason
end
sequenceDiagram
participant Utils as ConnectJobUtils
participant DB as Database
Utils->>DB: getDeliveryFlags(deliveryId)
DB-->>Utils: List<ConnectJobDeliveryFlagRecord>
Utils->>DB: storeDeliveryFlags(deliveryId, List<Flags>)
DB-->>Utils: Update/Insert/Delete flags as needed
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/src/org/commcare/connect/database/ConnectJobUtils.java (1)
220-250: Consider adding pruneMissing parameter for consistencyOther similar methods in this class (storeDeliveries, storePayments, etc.) have a
pruneMissingparameter to control whether to delete records that are no longer present.Consider adding this parameter for consistency:
-public static void storeDeliveryFlags(Context context, List<ConnectJobDeliveryFlagRecord> flags, int deliveryId) { +public static void storeDeliveryFlags(Context context, List<ConnectJobDeliveryFlagRecord> flags, int deliveryId, boolean pruneMissing) { SqlStorage<ConnectJobDeliveryFlagRecord> storage = ConnectDatabaseHelper.getConnectStorage(context, ConnectJobDeliveryFlagRecord.class); List<ConnectJobDeliveryFlagRecord> existingFlags = getDeliveryFlags(context, deliveryId, storage); //Delete flags that are no longer present Vector<Integer> recordIdsToDelete = new Vector<>(); // ... - storage.removeAll(recordIdsToDelete); + if (pruneMissing) { + storage.removeAll(recordIdsToDelete); + }app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java (2)
20-38: Consider adding a lastUpdate fieldOther model classes in this codebase typically include a
lastUpdatefield to track when records were last modified. This is useful for synchronization and auditing purposes.Consider adding a lastUpdate field:
@Persisting(3) @MetaField(META_DESCRIPTION) private String description; +@Persisting(4) +private Date lastUpdate; + +public ConnectJobDeliveryFlagRecord() { + lastUpdate = new Date(); +}
17-68: Add equals() and hashCode() methodsThe class doesn't override equals() and hashCode(), which could cause issues when comparing flags or using them in collections like HashSet or HashMap.
Consider adding equals and hashCode methods based on deliveryId and code:
@Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; ConnectJobDeliveryFlagRecord that = (ConnectJobDeliveryFlagRecord) o; return deliveryId == that.deliveryId && Objects.equals(code, that.code); } @Override public int hashCode() { return Objects.hash(deliveryId, code); }Don't forget to add
import java.util.Objects;to the imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java(5 hunks)app/src/org/commcare/connect/database/ConnectJobUtils.java(3 hunks)app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java(2 hunks)app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java(3 hunks)app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java (1)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java:86-93
Timestamp: 2025-01-27T09:08:32.722Z
Learning: The JSON fields (status, unitName, slug, entityId, entityName, reason) in ConnectJobDeliveryRecord's fromJson method are guaranteed to be non-null through the application's data contract, except for the 'id' field which is explicitly checked.
🧬 Code Graph Analysis (1)
app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java (1)
app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java (1)
Table(23-172)
🔇 Additional comments (29)
app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java (4)
19-19: Great addition of the import for ConnectJobDeliveryFlagRecord.This import supports the new code that utilizes flag records from deliveries, enabling the display of flag descriptions in the UI.
193-206: Well-implemented fallback display mechanism for empty reasons.The code now handles the case when a delivery has no reason by falling back to displaying the comma-separated list of flag descriptions. This ensures users always see relevant information about deliveries.
The implementation is robust with good null-checking and appropriate fallback to an empty string when both reason and flags are null.
19-19: Appropriately added import for new model class.The import for
ConnectJobDeliveryFlagRecordsupports the new flag display functionality.
192-206: Well-implemented fallback mechanism for displaying delivery flags.The code now gracefully handles cases where a delivery reason is null by displaying the associated flags' descriptions as a comma-separated list. The implementation properly checks for null values at each step and provides appropriate fallbacks.
- reasonText.setText(delivery.getReason()); + + String descriptionText = delivery.getReason(); + if(descriptionText == null) { + if(delivery.getFlags() != null) { + List<String> flagStrings = new ArrayList<>(); + for (ConnectJobDeliveryFlagRecord flag : delivery.getFlags()) { + flagStrings.add(flag.getDescription()); + } + descriptionText = String.join(", ", flagStrings); + } else { + descriptionText = ""; + } + } + reasonText.setText(descriptionText);app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java (6)
9-9: Appropriate import for the new model class.This import allows the database upgrader to reference the new ConnectJobDeliveryFlagRecord model class.
100-103: Correct database version upgrade handling.The upgrade logic properly handles incrementing from version 12 to 13 by calling the new upgrade method and updating the version number.
521-523: Clean implementation of the database upgrade.The upgradeTwelveThirteen method correctly adds the new table for ConnectJobDeliveryFlagRecord using the existing addTableForNewModel utility method.
9-9: Appropriately added import for new model class.The import for
ConnectJobDeliveryFlagRecordsupports the database upgrade functionality.
100-103: Proper database version upgrade step added.Added upgrade path from version 12 to 13 following the established pattern in the codebase.
521-523: Well-implemented database upgrade method.The
upgradeTwelveThirteenmethod correctly creates the new table forConnectJobDeliveryFlagRecordusing the existing helper method.app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (6)
11-11: Database version and documentation appropriately updated.The import for ConnectJobDeliveryFlagRecord is added, the database version is incremented from 12 to 13, and the documentation is updated to reflect the addition of the new table.
Also applies to: 53-55
128-130: New table creation properly integrated in onCreate.The code to create the table for ConnectJobDeliveryFlagRecord is correctly added to the onCreate method, ensuring the table is created during initial database setup.
11-11: Appropriately added import for new model class.The import for
ConnectJobDeliveryFlagRecordsupports the database creation functionality.
53-53: Documentation updated to reflect new version changes.Comment correctly documents that version 13 adds the ConnectJobDeliveryFlagRecord table.
55-55: Database version properly incremented.The version increment from 12 to 13 will trigger the appropriate upgrade process.
128-130: Table creation added for fresh database installs.Correctly added table creation for the new
ConnectJobDeliveryFlagRecordmodel in theonCreatemethod, ensuring consistency between upgrade and fresh install paths.app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java (11)
15-15: Necessary import and constant for the new flags feature.Added the List import and created a constant for the META_FLAGS JSON field.
Also applies to: 39-39
72-72: Good addition of the flags field.This field will store the list of flag records associated with a delivery.
98-99: Proper integration of flag parsing from JSON.The fromJson method now correctly parses and assigns the flags field from the JSON object.
149-150: Improved reason handling for JSON null values.The getReason method now handles the case where the reason is the string "null" by returning null instead, which aligns with the fallback mechanism in the UI to display flag descriptions.
152-154: Well-defined getter for flags field.Added a clean getter method to access the flags list from outside the class.
156-171:Details
✅ Verification successful
Verify compatibility with fromV2 method.
The fromV2 method doesn't initialize the flags field when creating a new record from a V2 record. This is likely fine since older records wouldn't have flags, but it would be good to verify that the UI code handles this case correctly.
🏁 Script executed:
#!/bin/bash # Check if there are any places in the codebase that might expect non-null flags echo "Searching for usages of getFlags() without null checks..." rg "getFlags\(\)" -A 2 -B 2Length of output: 2087
fromV2 Flags Initialization Is Safe
I checked all usages of getFlags() on ConnectJobDeliveryRecord:
- In ConnectDeliveryListFragment, there’s an explicit
if (delivery.getFlags() != null)guard before iterating.- No other code paths call getFlags() without a null‐check.
Leaving
flagsasnullwhen converting from V2 is safe—the UI handles it correctly.Optional improvement: you could initialize
flagsto an empty list for clarity:public static ConnectJobDeliveryRecord fromV2(ConnectJobDeliveryRecordV2 oldRecord) { ConnectJobDeliveryRecord newRecord = new ConnectJobDeliveryRecord(); newRecord.jobId = oldRecord.getJobId(); … newRecord.reason = ""; + newRecord.flags = new ArrayList<>(); return newRecord; }No further action required.
15-15: Appropriately added import for List.This import supports the new flags field.
39-39: Added constant for new metadata field.The
META_FLAGSconstant follows the naming convention used for other fields.
72-72: Added field to store delivery flags.The field is appropriately defined as a List of
ConnectJobDeliveryFlagRecordobjects.
149-150: Improved handling of null reasons.The
getReasonmethod now correctly handles the case where the string "null" is stored in the database by returning an actual null value. This enhances compatibility with the UI changes.
152-154: Added getter for accessing flags.The
getFlagsmethod follows the standard pattern used for other fields.app/src/org/commcare/connect/database/ConnectJobUtils.java (1)
303-313: LGTM - Flag retrieval implementation is clean and consistentThe implementation for retrieving delivery flags follows the established pattern in the codebase, properly checking for null storage and returning the results as an ArrayList.
app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java (1)
39-55: Silent exceptions could hide problemsThe method catches JSONException and logs it but continues processing, which might silently skip flags with issues. Consider whether this is desired behavior or if certain exceptions should propagate.
Evaluate whether silent exception handling is appropriate for your use case. If certain flags are critical, consider:
try { String key = keys.next(); ConnectJobDeliveryFlagRecord flag = new ConnectJobDeliveryFlagRecord(); flag.deliveryId = deliveryId; flag.code = key; flag.description = json.getString(key); flags.add(flag); } catch (JSONException e) { Logger.exception("Error parsing delivery flag", e); + // Consider: throw new RuntimeException("Critical flag parsing error", e); }
app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java
Outdated
Show resolved
Hide resolved
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
We need to PR the model and DB changes from here directly to master
|
Added these new changes and some other missing ones to the code in master, PR here |
app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java
Outdated
Show resolved
Hide resolved
https://dimagi.atlassian.net/browse/CCCT-607
cross-request: dimagi/commcare-core#1455
Product Description
Showing flags associated with deliveries (such as too short, missing GPS, etc.)
Technical Summary
New table to store flags associated with a Connect job delivery.
Database upgrade code.
API retrieval and data storage.
Showing the flags as a comma-separated list in the delivery list (in delivery progress page).
Feature Flag
Connect
Safety Assurance
Safety story
Tested locally against prod server.
Automated test coverage
None for Connect yet.
QA Plan
In a Connect opportunity, complete one or more deliveries without satisfying the required conditions for the opportunity (i.e. minimum time, GPS requirement, etc.). After submitting the form, go to delivery progress for the opportunity and view the delivery list. The corresponding flags for the delivery should be shown in a comma-separated list.