-
-
Notifications
You must be signed in to change notification settings - Fork 45
- Created personal_id_credential table #3199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Added migration
📝 WalkthroughWalkthroughA new database model class, Sequence Diagram(s)sequenceDiagram
participant App
participant DatabaseOpenHelper
participant Database
participant PersonalIdCredential
App->>DatabaseOpenHelper: onCreate(SQLiteDatabase db)
DatabaseOpenHelper->>Database: Execute SQL to create tables
DatabaseOpenHelper->>Database: Create PersonalIdCredential table
App->>DatabaseOpenHelper: onUpgrade(db, oldVersion=15, newVersion=16)
DatabaseOpenHelper->>Database: upgradeFifteenSixteen(db)
DatabaseOpenHelper->>Database: Add PersonalIdCredential table
App->>PersonalIdCredential: fromJson(JSONArray)
PersonalIdCredential->>PersonalIdCredential: Parse JSON, create instances
Suggested reviewers
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (1)
54-57: Document missing V.15 entry for historical clarityThe changelog jumps from
V.14toV.16. Even if V.15 contained no schema change, leaving a placeholder entry keeps the version history unambiguous for future maintainers.app/src/org/commcare/android/database/connect/models/PersonalIdCredential.java (3)
18-21: AddserialVersionUIDto maintain serialization compatibility
Serializableclasses should declare an explicitprivate static final long serialVersionUIDto avoid unexpectedInvalidClassExceptionif the class shape changes.public class PersonalIdCredential extends Persisted implements Serializable { + private static final long serialVersionUID = 1L;
22-28: Reuse defined meta constants when parsing JSONThe key strings are duplicated later in
fromJson. Referencing the existingMETA_*constants prevents drift if a column name ever changes.- credential.setAppName(obj.optString("app_name")); - credential.setSlug(obj.optString("slug")); - credential.setType(obj.optString("type")); - credential.setIssuedDate(obj.optString("issued_date")); - credential.setTitle(obj.optString("title")); - credential.setCredential(obj.optString("credential")); + credential.setAppName(obj.optString(META_APP_NAME)); + credential.setSlug(obj.optString(META_SLUG)); + credential.setType(obj.optString(META_TYPE)); + credential.setIssuedDate(obj.optString(META_ISSUED_DATE)); + credential.setTitle(obj.optString(META_TITLE)); + credential.setCredential(obj.optString(META_CREDENTIAL));
95-97: ReplaceprintStackTracewith structured logging
printStackTracecan flood logcat in production and bypass existing crash reporting. UseCrashUtil.log(e)(or the project’s preferred logger) instead.- } catch (JSONException e) { - e.printStackTrace(); + } catch (JSONException e) { + CrashUtil.log(e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/org/commcare/android/database/connect/models/PersonalIdCredential.java(1 hunks)app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java(3 hunks)app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (2)
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (1)
134-136: Table creation looks correctThe new
TableBuilder(PersonalIdCredential.class)invocation cleanly integrates the table into fresh installs. No further action required.app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java (1)
117-120: Sequential upgrade path intactThe additional branch for
oldVersion == 15maintains the one-step-at-a-time upgrade contract used throughout the class. Good consistency.
| private void upgradeFifteenSixteen(SQLiteDatabase db) { | ||
| addTableForNewModel(db, PersonalIdCredential.STORAGE_KEY, new PersonalIdCredential()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider explicit logging / idempotency guard
addTableForNewModel is wrapped in a transaction, but if the upgrade is accidentally re-run (for example after a partially-applied migration) the CREATE TABLE will throw.
Adding a IF NOT EXISTS in TableBuilder#getTableCreateString() or catching the specific exception here would make the upgrade idempotent and safer on retry.
🤖 Prompt for AI Agents
In app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java
around lines 592 to 594, the upgrade method calls addTableForNewModel which
attempts to create a table but does not handle the case where the table already
exists, causing an exception if the upgrade is retried. To fix this, modify the
table creation logic in TableBuilder#getTableCreateString() to include "IF NOT
EXISTS" in the CREATE TABLE statement or catch the specific exception thrown
when the table exists in upgradeFifteenSixteen and handle it gracefully to make
the upgrade idempotent and safe to rerun.
app/src/org/commcare/android/database/connect/models/PersonalIdCredential.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/PersonalIdCredential.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/PersonalIdCredential.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/PersonalIdCredential.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/PersonalIdCredential.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/PersonalIdCredential.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/database/connect/models/PersonalIdCredential.java
Outdated
Show resolved
Hide resolved
shubham1g5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking, but would be good to correct nomenclature here.
app/src/org/commcare/android/database/connect/models/PersonalIdCredential.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Persisting(2) | ||
| @MetaField(META_SLUG) | ||
| private String slug; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a comment on spec reg. the nomenclature here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shubham1g5 Renamed to app_id as of now
|
@jaypanchal-13 Seems like |
|
| , "org.commcare.android.database.global.models.GlobalErrorRecord" | ||
|
|
||
| ,"org.commcare.android.database.connect.models.ConnectUserRecordV14" | ||
| ,"org.commcare.android.database.connect.models.PersonalIdCredential" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a comment above this line saying //Added in 2.58 similar to other version comments above
Product Description
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review