CCCT-2016 Create Release Toggles DB Table #3482
Conversation
Created a new class for the release toggles table in the database.
📝 WalkthroughWalkthroughThis pull request introduces a new Kotlin data model class Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (2)
68-68: Increment database version for the new table.A new table
ConnectReleaseToggleRecordis being added, butCONNECT_DB_VERSIONremains at 20. The version should be incremented to 21 to ensure the upgrade path is triggered for existing users.🔧 Proposed fix
/** * V.2 - Added ConnectJobRecord, ConnectAppInfo, and ConnectLearningModuleInfo tables * V.3 - Added date_claimed column to ConnectJobRecord, * and reason column to ConnectJobDeliveryRecord * V.4 - Added confirmed and confirmedDate fields to ConnectJobPaymentRecord * Added link offer info to ConnectLinkedAppRecord * V.5 - Added projectStartDate and isActive to ConnectJobRecord * V.6 - Added pin,secondaryPhoneVerified, and registrationDate fields to ConnectUserRecord * V.7 - Added ConnectPaymentUnitRecord table * V.8 - Added is_user_suspended to ConnectJobRecord * V.9 - Added using_local_passphrase to ConnectLinkedAppRecord * V.10 - Added last_accessed column to ConnectLinkedAppRecord * V.11 - Added daily start and finish times to ConnectJobRecord * V.12 - Added ConnectMessagingChannelRecord table and ConnectMessagingMessageRecord table * V.13 - Added ConnectJobDeliveryFlagRecord table * V.14 - Added a photo and isDemo field to ConnectUserRecord * V.16 - Added personal_id_credential table * V17 - Added a new column has_connect_access to ConnectUserRecord * V18 - Added new columns to personal_id_credential table (previously the table was unused) * V.19 - Added push_notification_history * V.20 Added acknowledged column in push_notification_history + * V.21 - Added connect_release_toggles table */ - private static final int CONNECT_DB_VERSION = 20; + private static final int CONNECT_DB_VERSION = 21;
174-178: Add upgrade logic for ConnectReleaseToggleRecord table.Without an upgrade path in
ConnectDatabaseUpgrader, existing users upgrading from version 20 to 21 won't have theconnect_release_togglestable created. TheonCreate()method only runs for fresh installs. You need to add migration logic in the upgrader class to create this table for existing users.Generate a script to locate the upgrade implementation file:
#!/bin/bash # Description: Find the ConnectDatabaseUpgrader class to add migration logic # Locate the upgrader class fd -e java -e kt ConnectDatabaseUpgrader # Show the structure to understand how to add the migration rg -A 20 'class ConnectDatabaseUpgrader' --type=java --type=kotlinThe migration should add table creation logic similar to this pattern (to be added in
ConnectDatabaseUpgrader):if (oldVersion <= 20) { TableBuilder builder = new TableBuilder(ConnectReleaseToggleRecord.class); db.execSQL(builder.getTableCreateString()); }
🤖 Fix all issues with AI agents
In
@app/src/org/commcare/android/database/connect/models/ConnectReleaseToggleRecord.kt:
- Line 54: The local variable name is misspelled as relaseToggle; rename it to
releaseToggle wherever it's declared and referenced (the instance of
ConnectReleaseToggleRecord created with ConnectReleaseToggleRecord().apply { ...
}) to fix the typo and keep consistent naming.
- Around line 50-59: The model declares slug, active, createdAt, and modifiedAt
as nullable but fromJson() treats them as required; update the
ConnectReleaseToggleRecord class to make these properties non-nullable (String,
Boolean, Date) and adjust any constructors/initializers accordingly so the class
matches the API contract; ensure fromJson() remains using
json.getString/getBoolean and DateUtils.parseDateTime to allow failures to
surface, and update any usages or tests that assumed nullable values to handle
the non-null types.
🧹 Nitpick comments (1)
app/src/org/commcare/android/database/connect/models/ConnectReleaseToggleRecord.kt (1)
15-33: Add KDoc comments for persisted fields.Based on learnings from similar Connect model classes, persisted fields should be documented with KDoc comments explaining their purpose, format requirements, and relationships. This aids maintainability and helps future developers understand the data contract.
📝 Suggested documentation
+ /** + * Unique identifier/slug for the release toggle feature. + */ @Persisting(1) @MetaField(META_SLUG) var slug: String? = null private set + /** + * Whether this release toggle is currently active/enabled. + */ @Persisting(2) @MetaField(META_ACTIVE) var active: Boolean? = null private set + /** + * Timestamp when this release toggle record was created. + */ @Persisting(3) @MetaField(META_CREATED_AT) var createdAt: Date? = null private set + /** + * Timestamp when this release toggle record was last modified. + */ @Persisting(4) @MetaField(META_MODIFIED_AT) var modifiedAt: Date? = null private setBased on learnings, ConnectUserRecord and other Connect model classes document their fields with Javadoc comments.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/org/commcare/android/database/connect/models/ConnectReleaseToggleRecord.ktapp/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 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.
📚 Learning: 2025-01-21T17:28:09.007Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:66-71
Timestamp: 2025-01-21T17:28:09.007Z
Learning: The ConnectUserRecord class in CommCare Android uses Persisting annotations with sequential indices for field persistence, and MetaField annotations for fields that have corresponding META constants. Fields should be documented with Javadoc comments explaining their purpose, format requirements, and relationships with other fields.
Applied to files:
app/src/org/commcare/android/database/connect/models/ConnectReleaseToggleRecord.ktapp/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
📚 Learning: 2025-04-18T20:13:29.655Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.
Applied to files:
app/src/org/commcare/android/database/connect/models/ConnectReleaseToggleRecord.ktapp/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
📚 Learning: 2025-01-27T09:08:32.722Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 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.
Applied to files:
app/src/org/commcare/android/database/connect/models/ConnectReleaseToggleRecord.kt
📚 Learning: 2025-12-10T16:39:05.007Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3448
File: app/src/org/commcare/gis/EntityMapUtils.kt:214-234
Timestamp: 2025-12-10T16:39:05.007Z
Learning: In Kotlin files (here: app/src/org/commcare/gis/EntityMapUtils.kt), follow fail-fast guidance: treat conditions that indicate a programmer error or broken invariant as exceptions rather than defensive handling. If a condition represents a bug (e.g., internal API assumptions like array/list size mismatches), crash early to alert developers instead of attempting to recover. Use clear runtime exceptions (e.g., IllegalStateException, IllegalArgumentException) or assertions where appropriate, and avoid swallowing or masking bugs. Ensure invariants are checked and failures are surfaced with actionable messages for quick debugging.
Applied to files:
app/src/org/commcare/android/database/connect/models/ConnectReleaseToggleRecord.kt
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 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.
Applied to files:
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
📚 Learning: 2025-04-22T16:44:26.867Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3043
File: app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java:86-99
Timestamp: 2025-04-22T16:44:26.867Z
Learning: In ConnectMessagingChannelRecord, fields like META_CHANNEL_ID, META_CONSENT, META_CHANNEL_NAME, and META_KEY_URL are required fields, and the code is intentionally designed to crash if they're missing to raise awareness of data integrity issues rather than handling them silently.
Applied to files:
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
📚 Learning: 2025-04-22T17:04:26.780Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3043
File: app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java:0-0
Timestamp: 2025-04-22T17:04:26.780Z
Learning: In the Connect API contract, the server doesn't include the fields `created`, `answered_consent`, and `key` in JSON responses for messaging channels, which is why the `fromJson` method in `ConnectMessagingChannelRecord` explicitly initializes these values rather than parsing them.
Applied to files:
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
📚 Learning: 2025-12-08T12:01:48.545Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 3455
File: app/src/org/commcare/connect/network/connectId/parser/RetrieveNotificationsResponseParser.kt:64-93
Timestamp: 2025-12-08T12:01:48.545Z
Learning: In the CommCare Android Connect notification system (RetrieveNotificationsResponseParser.kt), messages are only decrypted using channels that already exist in the database with their encryption keys. When new channels arrive without keys, a separate network flow fetches the channel keys, and then the notification API is re-called to decrypt any messages for those channels. This explains why parseAndSeparateNotifications() correctly uses existingChannels from the database rather than newly parsed channels.
Applied to files:
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
📚 Learning: 2025-04-22T17:04:26.780Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3043
File: app/src/org/commcare/android/database/connect/models/ConnectMessagingChannelRecord.java:0-0
Timestamp: 2025-04-22T17:04:26.780Z
Learning: In the Connect API contract, the server doesn't include the fields `created`, `answered_consent`, and `key` in JSON responses for messaging channels, which is why the `fromJson` method in `ConnectMessagingChannelRecord` explicitly initializes these values rather than parsing them from JSON.
Applied to files:
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
🧬 Code graph analysis (1)
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (1)
app/src/org/commcare/android/database/connect/models/ConnectReleaseToggleRecord.kt (1)
ConnectReleaseToggleRecord(13-64)
⏰ 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/models/database/connect/DatabaseConnectOpenHelper.java (1)
143-144: Table creation placement looks good.The
ConnectReleaseToggleRecordtable is correctly created using theTableBuilderpattern, consistent with other Connect tables in theonCreate()flow.
app/src/org/commcare/android/database/connect/models/ConnectReleaseToggleRecord.kt
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/ConnectReleaseToggleRecord.kt
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/ConnectReleaseToggleRecord.kt
Outdated
Show resolved
Hide resolved
Converted table properties to non-null. Simplified fromJson() function.
Added the new table to the database upgrade flow. Removed an unused parameter from ConnectDatabaseUpgrader.upgrade().
Optimized imports.
Formatted code.
Ran Ktlint.
Bumped database version up to 21.
conroy-ricketts
requested review from
Jignesh-dimagi,
OrangeAndGreen,
jaypanchal-13 and
shubham1g5
| db.beginTransaction(); | ||
|
|
||
| try { | ||
| SqlStorage.dropTable(db, ConnectReleaseToggleRecord.STORAGE_KEY); |
There was a problem hiding this comment.
no need to drop table here as it won't exist before version 21.
There was a problem hiding this comment.
Ah true great catch, I think I can also get rid of db.setTransactionSuccessful() and the entire try-catch block here because that is already called inside addTableForNewModel()
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
Show resolved
Hide resolved
| fun fromJson(json: JSONObject): ConnectReleaseToggleRecord = | ||
| ConnectReleaseToggleRecord().apply { | ||
| val createdAtDateString = json.getString(META_CREATED_AT) | ||
| val modifiedAtDateString = json.getString(META_MODIFIED_AT) | ||
|
|
||
| slug = json.getString(META_SLUG) | ||
| active = json.getBoolean(META_ACTIVE) | ||
| createdAt = DateUtils.parseDateTime(createdAtDateString) | ||
| modifiedAt = DateUtils.parseDateTime(modifiedAtDateString) | ||
| } |
There was a problem hiding this comment.
the api payload here is little different where slug is present as the key to Json itself. So I don't think this method will work here as written.
There was a problem hiding this comment.
Aaah I see I misinterpreted this at first
|
You will also need to add the new model to the list here for the tests to pass |
|
@shubham1g5 Just trying to see how data model will satisfy internal configuration as mentioned here or I have missed some concern here? |
Replied on Doc. |
…nto task/CCCT-2016-release-toggles-database-table
Removed unnecessary code from the upgradeTwentyTwentyOne() function.
Marked slug column as unique. Changed fromJson() logic to accomadate expected API response structure.
Added ConnectReleaseToggleRecord to list of externalizable classes in FormStorageTest.
Made correction in fromJson() logic.
Optimized imports.
| @Throws(JSONException::class) | ||
| fun fromJson(json: JSONObject): List<ConnectReleaseToggleRecord> { |
There was a problem hiding this comment.
@conroy-ricketts Do we want to handle the JSONException here only as we should ignore the corrupt ones and use non-corrupted.
There was a problem hiding this comment.
I'm not sure I understand, can you please clarify a bit more?
There was a problem hiding this comment.
@Jignesh-dimagi Think the exception should be handled by the caller here and just logged as a non-fatal, The release toggles won't be user visible as such and user won't have any knowledge of this feature. So there is no reason to handle corrupt toggles separately, but we also should not crash the app due to this as all the toggle will be fetched in background only.
There was a problem hiding this comment.
@shubham1g5 instead of handling at each calling function if this function passes only non corrupted toggles from here, will be easier?
There was a problem hiding this comment.
agree it should be in one place but think that one place should be the corresponding parser ?
There was a problem hiding this comment.
Yeah but just trying to see how does parser receives non corrupted toggles if raising exception from here?
There was a problem hiding this comment.
When we say "corrupt" here do we mean that the code threw a JSONException for a particular toggle? If so, @Jignesh-dimagi are you suggesting that if a JSONException is thrown, then we should use some default values here instead when creating a ConnectReleaseToggleRecord?
There was a problem hiding this comment.
Ahh I understand what you are saying now.
@conroy-ricketts What Jignesh is trying to get to is that we should not be breaking functionality for all toggles because one of the toggle has incorrect JSON data in the corresponding block. To support that we will need to catch JsonException per release toggle and catch and log it inside this method so that we can return the toggles for which the data is correct in a fail-safe manner.
There was a problem hiding this comment.
That sounds like a good idea to me! I'll update this soon
…nto task/CCCT-2016-release-toggles-database-table
Ran Ktlint.
Tweaked function for creating release toggles from json so that one corrupt release toggle doesn't prevent non-corrupt release toggles from being created.
…nto task/CCCT-2016-release-toggles-database-table
conroy-ricketts
deleted the
task/CCCT-2016-release-toggles-database-table
branch
CCCT-2016
Technical Summary
I added a new table for release toggles in our database.
In
ConnectDatabaseUpgrader.upgrade(), I removed thenewVersionparameter because it's not being used anywhere - let me know if I should add that back.Example of release toggle json:
Safety Assurance
Safety story
I did a quick smoke test to make sure the app builds successfully and that I didn't break anything.
QA Plan
I imagine that this will be thoroughly tested indirectly through other tickets. So there is probably no need for QA on this particular one.