Worst-case Error Messaging for Connect #3121
Conversation
Upgrade code for global database.
New ConnectDatabaseHelper.crashDb functions to handle configuring the logout error message and then crashing. Improved error message for DB crash (more generic about cause, and explicitly referencing PersonalID).
📝 WalkthroughWalkthroughThis change introduces enhanced error handling and user feedback for connection errors related to PersonalID management in the application. It adds new UI elements (TextViews) for displaying connection error messages on both the login and installation mode selection screens. The Sequence Diagram(s)sequenceDiagram
participant User
participant LoginActivity
participant PersonalIdManager
participant LoginActivityUIController
User->>LoginActivity: Launches app / Navigates to login
LoginActivity->>PersonalIdManager: init(context)
PersonalIdManager->>PersonalIdManager: checkForLogoutErrorMessage(context)
alt Error message exists
PersonalIdManager-->>LoginActivity: return error message
LoginActivity->>LoginActivityUIController: setConnectErrorMessageUI(error message)
LoginActivityUIController->>LoginActivityUIController: Display error in UI
else No error
PersonalIdManager-->>LoginActivity: return null
LoginActivity->>LoginActivity: Continue normal flow
end
sequenceDiagram
participant User
participant CommCareSetupActivity
participant PersonalIdManager
participant SelectInstallModeFragment
User->>CommCareSetupActivity: Launches setup activity
CommCareSetupActivity->>PersonalIdManager: init(context)
PersonalIdManager->>PersonalIdManager: checkForLogoutErrorMessage(context)
alt Error message exists
PersonalIdManager-->>CommCareSetupActivity: return error message
CommCareSetupActivity->>SelectInstallModeFragment: showConnectErrorMessage(error message)
SelectInstallModeFragment->>SelectInstallModeFragment: Display error in UI
else No error
PersonalIdManager-->>CommCareSetupActivity: return null
CommCareSetupActivity->>CommCareSetupActivity: Continue normal flow
end
Possibly related PRs
Suggested labels
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ 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:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
app/res/layout/screen_login.xml (1)
76-85: Enhance accessibility and layout robustness for error display.
Sinceconnect_error_msgis updated dynamically, consider adding:android:accessibilityLiveRegion="polite" android:layout_width="match_parent"to ensure screen readers announce the error and long messages wrap correctly across the full width.
app/src/org/commcare/activities/LoginActivityUIController.java (1)
408-411: Consider clearing the error on successful login / retry
setConnectErrorMessageUIonly ever shows the message; it is never hidden.
If the user resolves the issue (e.g. finishes the re-configuration flow and returns to login), the stale error text will continue to occupy space.Add a complementary helper such as
clearConnectErrorMessageUI()that sets the view toGONE, and call it fromsetStyleDefault()or after a successful login attempt.
Otherwise the UI can appear “stuck” in an error state.app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
97-111: New database crash and error persistence mechanismThe new
crashDb()methods provide a structured way to:
- Store an error message ID in the global database
- Force an app crash with a consistent exception message
This improves the user experience by ensuring error messages persist across app restarts.
However, consider adding a log entry before throwing the exception to help with debugging:
+ Logger.log(LogTypes.TYPE_ERROR, "Forcing Connect database crash with message ID: " + message); throw new RuntimeException("Connect database crash");app/src/org/commcare/android/database/global/models/ConnectKeyRecord.java (1)
27-29: New field for storing logout error messageThe new
logoutErrorMessagefield is properly annotated with@Persisting(3)and@MetaField(LOGOUT_MESSAGE)for database persistence. The default value of -1 indicates no error message is set.Consider extracting the default value (-1) to a named constant for better code readability:
+ public static final int NO_LOGOUT_ERROR = -1; - int logoutErrorMessage = -1; + int logoutErrorMessage = NO_LOGOUT_ERROR;app/src/org/commcare/android/database/global/models/ConnectKeyRecordV7.java (1)
1-45: New versioned model class for database migrationThis class provides a clean migration path for the database schema upgrade. It correctly:
- Defines the same table structure as the previous version
- Provides the necessary fields and accessors
- Includes a migration path from V6
This is a good practice for database versioning, ensuring backward compatibility.
However, there's a minor discrepancy in the comments:
/** * DB model for storing the encrypted/encoded Connect DB passphrase - * Used up to global DB V6 + * Used up to global DB V7 * * @author dviggiano */app/src/org/commcare/connect/PersonalIdManager.java (1)
141-153: New method to check for logout error messagesThe
checkForLogoutErrorMessageimplementation correctly:
- Retrieves the key record from global storage
- Checks if a logout error message is set (value > 0)
- Calls
forgetUserwith the error message string- Returns the error message to the caller
This encapsulates the error detection and user state cleanup in a single place.
Consider adding a comment explaining why
> 0is used as the check for a valid message ID:- if(message > 0) { + // Resource IDs are always positive integers, -1 means no message + if(message > 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/res/layout/screen_login.xml(1 hunks)app/res/layout/select_install_mode_fragment.xml(1 hunks)app/res/values-fr/strings.xml(1 hunks)app/res/values-pt/strings.xml(1 hunks)app/res/values-sw/strings.xml(1 hunks)app/res/values-ti/strings.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/CommCareSetupActivity.java(3 hunks)app/src/org/commcare/activities/LoginActivity.java(1 hunks)app/src/org/commcare/activities/LoginActivityUIController.java(2 hunks)app/src/org/commcare/android/database/global/models/ConnectKeyRecord.java(2 hunks)app/src/org/commcare/android/database/global/models/ConnectKeyRecordV7.java(1 hunks)app/src/org/commcare/connect/PersonalIdManager.java(3 hunks)app/src/org/commcare/connect/database/ConnectDatabaseHelper.java(4 hunks)app/src/org/commcare/connect/database/ConnectDatabaseUtils.java(1 hunks)app/src/org/commcare/fragments/SelectInstallModeFragment.java(1 hunks)app/src/org/commcare/models/database/global/DatabaseGlobalOpenHelper.java(1 hunks)app/src/org/commcare/models/database/global/GlobalDatabaseUpgrader.java(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
app/src/org/commcare/connect/PersonalIdManager.java (1)
PersonalIdManager(64-685)
app/src/org/commcare/android/database/global/models/ConnectKeyRecordV7.java (1)
app/src/org/commcare/android/database/global/models/ConnectKeyRecord.java (1)
Table(15-60)
🔇 Additional comments (17)
app/src/org/commcare/models/database/global/DatabaseGlobalOpenHelper.java (1)
35-37: Database version increment for the new logout_error_message columnThe version increment and documentation comment correctly reflect the addition of the new
logout_error_messagecolumn to theConnectKeyRecordtable, which will support the worst-case error messaging feature described in the PR objectives.app/src/org/commcare/connect/database/ConnectDatabaseUtils.java (1)
44-44: Method visibility change from package-private to publicChanging the visibility of
getKeyRecordto public is appropriate since this method now needs to be accessible from other packages to support the error message detection functionality described in the PR objectives.app/res/values-sw/strings.xml (1)
361-361: Updated error message for improved user guidanceThe updated error message provides clearer instructions for users when they encounter a database corruption issue, directing them to reconfigure their Personal ID account rather than using technical language about database problems.
app/res/values-ti/strings.xml (1)
350-350: Correct localized error string.
The Tigrinya update forconnect_db_corruptaligns with the default English message, cleanly instructing the user to re-authenticate their PersonalID account without extra context.app/res/values-pt/strings.xml (1)
362-362: Approve updated Portuguese error instruction.
The revised Portuguese string matches the simplified English version and clearly tells the user to reconfigure their PersonalID account.app/res/values/strings.xml (1)
896-896: Message improvement for PersonalID account errors.The error message has been updated to provide clearer guidance to users when they encounter an issue with their PersonalID account, instructing them to reconfigure their account rather than using technical language about database corruption.
app/res/layout/select_install_mode_fragment.xml (1)
31-41: UI enhancement for displaying connection errors.A new TextView has been added to show connection error messages to users during the installation process. The element is properly configured with appropriate styling (bold text, red color) and is initially hidden until an error needs to be displayed.
This matches the similar error message display that was added to the login screen, providing consistent error handling across the app.
app/res/values-fr/strings.xml (1)
227-227: Properly localized error message for French users.The French translation of the connection error message has been correctly updated to match the semantic changes made to the English version.
app/src/org/commcare/activities/LoginActivity.java (1)
141-145: Improved error handling for PersonalID initialization failures.The code now properly captures error messages returned from
personalIdManager.init()and displays them in the UI, enhancing the user experience when connection errors occur. This is a critical part of the new error handling system that propagates database corruption or logout errors to the UI.This change complements the new database schema which now supports storing error messages and the UI changes that display these messages to users.
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
178-179: Repeated initialisation on rotation may overwrite a previously shown error
PersonalIdManager.getInstance().init(this)is called every timeonCreateruns.
If the first run returned an error message and the user rotates before acting, a second call could returnnull, clearingconnectErrorand hiding the message.Cache the value the first time:
if (connectError == null) { connectError = PersonalIdManager.getInstance().init(this); }app/src/org/commcare/models/database/global/GlobalDatabaseUpgrader.java (1)
180-213: Good transactional safety and data conversionThe new
upgradeSevenEightstep correctly:
- Adds the
LOGOUT_MESSAGEcolumn.- Converts V7 rows to the final
ConnectKeyRecordmodel.- Wraps the migration in a transaction and preserves primary keys.
Nice work keeping the upgrade path resilient and explicit!
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (2)
43-43: Replacement of error handling with crash mechanismThe implementation now forces the app to crash when encountering an error with DB passphrase handling, rather than using a previous error handling approach. This creates a predictable failure path that's easier to recover from upon app restart.
79-80: Consistent crash behavior for database corruptionThe change ensures consistent handling of database corruption by calling
crashDb(), which will store the error message and force an app crash. This approach is more reliable than the previous handling as it ensures the app will restart in a clean state.app/src/org/commcare/android/database/global/models/ConnectKeyRecord.java (3)
19-19: New constant for logout error message fieldAdding the field constant follows the pattern used for other persisted fields in this class.
49-55: Added getter and setter for logout error messageThe implementation correctly provides access to the new field.
57-59: Updated migration method for V7The migration implementation correctly converts from V7 to the current version, preserving the required fields. Note that the new
logoutErrorMessagefield will be initialized with the default value of -1, indicating no error message.app/src/org/commcare/connect/PersonalIdManager.java (1)
115-139: Modified initialization to check for logout error messagesThe
initmethod now returns aStringthat can contain an error message instead ofvoid. This allows the calling activity to display appropriate error messages to the user. The implementation correctly:
- Checks for logout error messages at the start of initialization
- Returns the error message if one is found, preventing further initialization
- Uses the new
crashDb()method for handling corrupt databases- Returns
nullwhen initialization completes normallyThis provides a clean way to propagate error conditions to the UI layer.
| private String connectError = null; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
connectError is not persisted across configuration changes
The field is initialised in onCreate but is never written to / restored from onSaveInstanceState. Rotating the device will therefore lose the message and the user will no longer see the reason they must re-configure.
// onSaveInstanceState
-outState.putBoolean(KEY_SHOW_NOTIFICATIONS_BUTTON, showNotificationsButton);
+outState.putBoolean(KEY_SHOW_NOTIFICATIONS_BUTTON, showNotificationsButton);
+outState.putString("connect-error", connectError); // NEWand in loadStateFromInstance:
showNotificationsButton = savedInstanceState.getBoolean(KEY_SHOW_NOTIFICATIONS_BUTTON, false);
+connectError = savedInstanceState.getString("connect-error");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private String connectError = null; | |
| // -- in CommCareSetupActivity.java -- | |
| @Override | |
| protected void onSaveInstanceState(Bundle outState) { | |
| super.onSaveInstanceState(outState); | |
| outState.putBoolean(KEY_SHOW_NOTIFICATIONS_BUTTON, showNotificationsButton); | |
| + outState.putString("connect-error", connectError); // NEW | |
| } | |
| private void loadStateFromInstance(Bundle savedInstanceState) { | |
| showNotificationsButton = savedInstanceState.getBoolean(KEY_SHOW_NOTIFICATIONS_BUTTON, false); | |
| + connectError = savedInstanceState.getString("connect-error"); | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/activities/CommCareSetupActivity.java around lines
168-169, the connectError field is not saved or restored during configuration
changes like device rotation. To fix this, override onSaveInstanceState to save
connectError into the Bundle, and update onCreate or loadStateFromInstance to
restore connectError from the savedInstanceState Bundle, ensuring the error
message persists across rotations.
📝 WalkthroughWalkthroughThis change introduces enhanced error handling and messaging for connection issues in the app. New Sequence Diagram(s)sequenceDiagram
participant User
participant LoginActivity
participant PersonalIdManager
participant LoginActivityUIController
User->>LoginActivity: Launches app / login screen
LoginActivity->>PersonalIdManager: init(context)
PersonalIdManager-->>LoginActivity: returns connectError (String or null)
alt connectError is not null
LoginActivity->>LoginActivityUIController: setConnectErrorMessageUI(connectError)
LoginActivityUIController->>LoginActivityUIController: Show error message in TextView
else connectError is null
LoginActivity->>LoginActivityUIController: Continue normal login flow
end
sequenceDiagram
participant App
participant ConnectDatabaseHelper
participant ConnectKeyRecord
participant GlobalStorage
App->>ConnectDatabaseHelper: Open or access Connect DB
alt DB is corrupted or inaccessible
ConnectDatabaseHelper->>ConnectKeyRecord: Retrieve or create record
ConnectDatabaseHelper->>ConnectKeyRecord: setLogoutErrorMessage()
ConnectDatabaseHelper->>GlobalStorage: Write updated record
ConnectDatabaseHelper->>App: Throw RuntimeException (crash app)
else DB is fine
ConnectDatabaseHelper->>App: Proceed as normal
end
Suggested labels
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ 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:
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.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
app/src/org/commcare/models/database/global/GlobalDatabaseUpgrader.java (1)
145-178: 🛠️ Refactor suggestionType mismatch between storage adapter and object written
newStorageis constructed withConnectKeyRecord.classbut receivesConnectKeyRecordV7instances.
While this works because generics are erased, it is brittle and may break if reflection code expects matching classes.- SqlStorage<Persistable> newStorage = new SqlStorage<>( - ConnectKeyRecord.STORAGE_KEY, - ConnectKeyRecord.class, + SqlStorage<Persistable> newStorage = new SqlStorage<>( + ConnectKeyRecord.STORAGE_KEY, + ConnectKeyRecordV7.class, new ConcreteAndroidDbHelper(c, db));
♻️ Duplicate comments (1)
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
391-401: Missed opportunity to restoreconnectErrorafter rotation
KEY_ERROR_MESSAGEis saved, but not the new Connect error.
See earlier comment: add the extraputString/getStringpair for"connect-error"so the red banner survives rotations.
🧹 Nitpick comments (6)
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
176-179: InitializePersonalIdManagerearlier to reduce side-effects
PersonalIdManager.getInstance().init(this)can crash the app (viaConnectDatabaseHelper.crashDb()).
Placing the call aftercheckForMultipleAppsViolation()but before any UI is shown means a partial UI may flash and then crash, which feels abrupt.
Consider invoking this before any fragment work (even beforesetContentView) so that a crash occurs during a blank splash, yielding a cleaner UX.app/src/org/commcare/activities/LoginActivityUIController.java (1)
408-411: Hide standard error container when showing Connect error
setConnectErrorMessageUImakes the message visible but leaves the regular error container intact.
Displaying both simultaneously can confuse users.protected void setConnectErrorMessageUI(String errorMessage) { - connectErrorMessage.setVisibility(View.VISIBLE); - connectErrorMessage.setText(errorMessage); + clearErrorMessage(); // hide username/password errors + connectErrorMessage.setVisibility(View.VISIBLE); + connectErrorMessage.setText(errorMessage); }app/src/org/commcare/models/database/global/GlobalDatabaseUpgrader.java (2)
63-67: Loop will not upgrade past v8 if more steps are added
oldVersiongets updated in eachifblock but the method ends without a final check thatoldVersion == newVersion.
A future developer adding v9 logic could forget to extend this chain. Consider awhile (oldVersion < newVersion)with aswitchinstead.
180-213: No cleanup of obsolete column after migrationAfter adding
LOGOUT_MESSAGE, any devices already on v8 will keep the redundantisLocalcolumn from v7 even if unused.
Although harmless, you can drop it inside the same transaction to keep the schema tidy:db.execSQL("ALTER TABLE " + ConnectKeyRecord.STORAGE_KEY + " DROP COLUMN " + ConnectKeyRecord.IS_LOCAL);app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (2)
43-44: Cascading crashes risk losing diagnostic information
crashDb()throws a genericRuntimeException("Connect database crash").
Consider creating a dedicated exception (ConnectDatabaseCorruptionException) so crash analytics can distinguish this path and attach context (e.g., stack trace,dbBrokenflag).
97-111: Threading: Toast removed, but handler import remains
android.os.Handleris no longer used after the rewrite; remove the unused import to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/res/layout/screen_login.xml(1 hunks)app/res/layout/select_install_mode_fragment.xml(1 hunks)app/res/values-fr/strings.xml(1 hunks)app/res/values-pt/strings.xml(1 hunks)app/res/values-sw/strings.xml(1 hunks)app/res/values-ti/strings.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/CommCareSetupActivity.java(3 hunks)app/src/org/commcare/activities/LoginActivity.java(1 hunks)app/src/org/commcare/activities/LoginActivityUIController.java(2 hunks)app/src/org/commcare/android/database/global/models/ConnectKeyRecord.java(2 hunks)app/src/org/commcare/android/database/global/models/ConnectKeyRecordV7.java(1 hunks)app/src/org/commcare/connect/PersonalIdManager.java(3 hunks)app/src/org/commcare/connect/database/ConnectDatabaseHelper.java(4 hunks)app/src/org/commcare/connect/database/ConnectDatabaseUtils.java(1 hunks)app/src/org/commcare/fragments/SelectInstallModeFragment.java(1 hunks)app/src/org/commcare/models/database/global/DatabaseGlobalOpenHelper.java(1 hunks)app/src/org/commcare/models/database/global/GlobalDatabaseUpgrader.java(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
app/src/org/commcare/connect/PersonalIdManager.java (1)
PersonalIdManager(64-685)
app/src/org/commcare/android/database/global/models/ConnectKeyRecordV7.java (1)
app/src/org/commcare/android/database/global/models/ConnectKeyRecord.java (1)
Table(15-60)
🔇 Additional comments (22)
app/res/layout/screen_login.xml (1)
76-85: New TextView added for displaying connection error messages.This new UI element provides a clear way to communicate connection errors to users when they encounter issues with their PersonalID account. The styling choices (bold, red text, and centered positioning) appropriately indicate the error nature of the message.
app/src/org/commcare/models/database/global/DatabaseGlobalOpenHelper.java (1)
35-37:Details
✅ Verification successful
Incremented database version to support new error messaging feature.
The database version has been incremented from 7 to 8 to accommodate the new
logout_error_messagecolumn in the ConnectKeyRecord table. This is the correct approach for handling database schema changes.
🏁 Script executed:
#!/bin/bash # Verify that the database upgrader implements the migration for version 8 echo "Checking for GlobalDatabaseUpgrader migration code for version 8..." rg "case 7:" -A 15 app/src/org/commcare/models/database/global/GlobalDatabaseUpgrader.javaLength of output: 237
To ensure we’re inspecting the correct file and that the version‐8 migration is implemented, let’s locate the upgrader and list its case handlers and any references to the new column:
🏁 Script executed:
#!/bin/bash set -e # 1. Locate the upgrader file echo "Locating GlobalDatabaseUpgrader.java..." UPGRADER_PATH=$(fd GlobalDatabaseUpgrader.java) echo "Found upgrader at: $UPGRADER_PATH" # 2. List all version cases in the switch echo -e "\nListing all 'case' statements:" rg "case [0-9]" -n "$UPGRADER_PATH" # 3. Check for migration logic touching logout_error_message echo -e "\nSearching for logout_error_message references:" rg "logout_error_message" -n "$UPGRADER_PATH"Length of output: 586
Let’s inspect the upgrader’s
upgrade(…)method and look for any raw-SQL or ALTER statements touching the new column:
🏁 Script executed:
#!/bin/bash set -e # Locate the upgrader UPGRADER=$(fd GlobalDatabaseUpgrader.java) echo "Using upgrader at $UPGRADER" # Show the upgrade method signature + body header echo -e "\n--- upgrade(...) method context ---" rg -n 'void upgrade' -A20 -B5 "$UPGRADER" # Search for migration SQL or column references echo -e "\n--- Migration SQL / column references ---" rg -n 'execSQL\|ALTER TABLE\|ConnectKeyRecord\|logout_error_message' "$UPGRADER"Length of output: 1504
We still need to confirm whether the upgrader implements migrations for versions 5 through 8 (particularly the 7→8 step). Let’s search for all of those
if (oldVersion == X)blocks and anyupgradeSevenEightmethod:
🏁 Script executed:
#!/bin/bash set -e # Locate the upgrader UPGRADER=$(fd GlobalDatabaseUpgrader.java) echo "Using upgrader at: $UPGRADER" # Check for upgrade logic from v5 through v8 echo -e "\n--- Looking for oldVersion == 5,6,7,8 blocks ---" rg -n 'if \(oldVersion == [5-8]\)' "$UPGRADER" # Check for a named upgrade method for 7→8 echo -e "\n--- Looking for upgradeSevenEight method ---" rg -n 'upgradeSevenEight' -n "$UPGRADER"Length of output: 953
Let’s inspect the full body of
upgradeSevenEightto confirm whether it actually adds thelogout_error_messagecolumn:
🏁 Script executed:
#!/bin/bash set -e # Locate the upgrader file UPGRADER=$(fd GlobalDatabaseUpgrader.java) echo "Upgrader path: $UPGRADER" # Dump the entire upgradeSevenEight method echo -e "\n--- upgradeSevenEight(...) body ---" rg -n 'private boolean upgradeSevenEight' -A200 "$UPGRADER"Length of output: 4714
Database migration for version 8 verified
GLOBAL_DB_VERSIONis correctly updated to 8 inapp/src/org/commcare/models/database/global/DatabaseGlobalOpenHelper.java.- The
upgradeSevenEight(SQLiteDatabase db)method inGlobalDatabaseUpgrader.javaadds thelogout_error_messagecolumn to theConnectKeyRecordtable and migrates existing data.No further changes needed.
app/src/org/commcare/fragments/SelectInstallModeFragment.java (1)
201-205: New method added to display connection error messages in the setup screen.The
showConnectErrorMessagemethod will display error messages related to PersonalID connection issues during the setup phase. This matches the similar functionality added to the login screen and creates a consistent user experience for error handling across both entry points.app/res/layout/select_install_mode_fragment.xml (1)
31-40: Good addition of error message display componentThis TextView is well-positioned to show connection error messages to users during app setup. The styling (bold red text) appropriately conveys an error state, and setting it initially to
goneensures it only appears when needed.app/res/values/strings.xml (1)
896-896: Improved user-friendly error messageThe updated error message uses more generalized language that focuses on the action needed (reconfiguring the account) rather than exposing technical details about database corruption. This matches the PR's goal of providing clear guidance when a device needs to be reconfigured.
app/res/values-pt/strings.xml (1)
362-362: Consistent localization of error messageThe Portuguese translation has been updated to maintain consistency with the English version, providing clear instructions for the user to reconfigure their PersonalID account when needed.
app/src/org/commcare/connect/database/ConnectDatabaseUtils.java (1)
44-44: Appropriate visibility change to support error message checkingChanging the
getKeyRecordmethod from package-private to public is necessary to allow components outside this package (like login/setup screens) to access ConnectKeyRecord objects and check for error messages.This change aligns with the PR objective of adding error messaging for critical failure scenarios, as it enables UI components to retrieve the stored error message from the global database.
app/res/values-fr/strings.xml (1)
227-227: Good localization update for error message.The French localization for the database corruption error has been updated to provide clear instructions to users, aligning with the PR objective of introducing error messaging for critical failure scenarios.
app/res/values-sw/strings.xml (1)
361-361: Good localization update for error message.The Swahili localization for the database corruption error has been updated to provide clear instructions to users, maintaining consistency with the changes in other language files.
app/src/org/commcare/activities/LoginActivity.java (1)
141-144: Good error handling implementation.The code now properly captures potential error messages from
personalIdManager.init()and displays them in the UI. This implements the error propagation flow required for the worst-case error messaging feature.app/src/org/commcare/activities/LoginActivityUIController.java (1)
97-99:connectErrorMessageelement missing initial state checkEnsure the layout sets
android:visibility="gone"for@id/connect_error_msg; otherwise the message may be visible with stale text beforesetConnectErrorMessageUI()is called.app/src/org/commcare/connect/PersonalIdManager.java (6)
27-27: Added import for ConnectKeyRecord.This import is correctly added to support the new error handling functionality that checks for logout error messages stored in the ConnectKeyRecord.
115-115: Method signature changed to return String.The init method now returns a String instead of void to support propagating error messages up to the UI layers.
118-121: New error handling flow checks for logout error messages.This code properly checks for error messages at initialization and short-circuits the process if an error is found, returning the message to be displayed in the UI.
134-134: Database corruption handling updated.Changed from calling
handleCorruptDb(parent)tocrashDb(), which presumably sets the error message and forces the app to crash, implementing the critical failure handling described in the PR.
138-138: Added explicit return null.This ensures the method always returns a value (null when no error is found) to match the updated return type.
141-153: New method to check for logout error messages.This method retrieves the ConnectKeyRecord, checks if it contains an error message code, and if found:
- Gets the localized error message string
- Calls forgetUser to reset the user's state
- Returns the error message to be displayed
This is a clean implementation of the error handling flow described in the PR objectives.
app/src/org/commcare/android/database/global/models/ConnectKeyRecordV7.java (1)
1-44: Well-structured intermediate database model for migration.This new class serves as an intermediate model for database migration, representing the V7 version of ConnectKeyRecord before the addition of the logoutErrorMessage field. It correctly:
- Maintains the same storage key and field structure as the original record
- Includes proper field annotations for persistence
- Provides a migration path from V6 via the fromV6 static method
- Has appropriate getters for accessing the fields
This approach allows for clean database schema evolution through explicit version models.
app/src/org/commcare/android/database/global/models/ConnectKeyRecord.java (4)
19-19: Added constant for the logout message field.The new LOGOUT_MESSAGE constant properly defines the database column name for the error message field.
27-29: Added persistent field for storing error messages.The logoutErrorMessage field is:
- Correctly annotated with @persisting(3) to maintain field order
- Properly annotated with @metafield to link it to the column name
- Initialized with a default value of -1 to indicate no error
This implementation allows for storing error message resource IDs in the database.
49-55: Added getter and setter for the error message.These methods allow for retrieving and setting the error message ID in a clean, encapsulated way.
57-58: Added migration method from V7.The static fromV7 method ensures proper data migration from the V7 version of the record while preserving the existing fields. Note that the new logoutErrorMessage field will be initialized with its default value (-1) for migrated records.
| private String connectError = null; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
connectError is not persisted across configuration changes
connectError is declared here but never written to onSaveInstanceState() (or restored from it).
If the user rotates the device (or the activity is otherwise recreated) before clearing the error, the message will silently disappear.
@@ onSaveInstanceState(Bundle outState)
- outState.putString(KEY_ERROR_MESSAGE, errorMessageToDisplay);
+ outState.putString(KEY_ERROR_MESSAGE, errorMessageToDisplay);
+ outState.putString("connect-error", connectError);and in loadStateFromInstance():
- errorMessageToDisplay = savedInstanceState.getString(KEY_ERROR_MESSAGE);
+ errorMessageToDisplay = savedInstanceState.getString(KEY_ERROR_MESSAGE);
+ connectError = savedInstanceState.getString("connect-error");🤖 Prompt for AI Agents
In app/src/org/commcare/activities/CommCareSetupActivity.java around lines
168-169, the connectError field is not saved or restored during configuration
changes, causing the error message to disappear on activity recreation. To fix
this, override onSaveInstanceState() to save the connectError string into the
Bundle, and update loadStateFromInstance() to restore connectError from the
savedInstanceState Bundle if present.
|
@OrangeAndGreen Curious if we have considered deleting the connect db as soon as we are in the worst case scenario so that user will get an option to sign up again with Personal Id ? |
|
@shubham1g5 Hmm, that could probably be done since the error message is stored in the global DB (ConnectKeyRecord). Eventually we need to delete both, i.e. clear the ConnectKeyRecord table in global DB and delete the entire Connect DB, and in the first implementation I thought it cleaner to do all the deletions together in one place. But that meant keeping the "crashed DB" around along with the ConnectKeyRecord until after the crash, so the error message could be retrieved and everything could be deleted at once on the next app startup. Instead, I could try to rework the code so that the Connect DB is deleted immediately, but the ConnectKeyRecord isn't cleaned up until the message has been consumed (next app startup). Note this would differ from the intentional "forget PersonalID" user action, which would probably still delete all the related storage in one go. So we might end up with two ways to deconfigure the device, which seems less than ideal. Note that currently the behavior is as desired, i.e. on app startup after the crash the user sees the Setup or Login page (as appropriate) with PersonalID not configured and an error message at the top of the screen. They can then go to the menu to begin the Device Configuration workflow again. |
| public void init(Context parent) { | ||
| public String init(Context parent) { | ||
| parentActivity = parent; | ||
| if (personalIdSatus == PersonalIdStatus.NotIntroduced) { |
There was a problem hiding this comment.
@OrangeAndGreen Just a question: Why we check status for user when its still not introduced?
There was a problem hiding this comment.
This is just to prevent the check from being performed multiple times if the code is called again after startup
|
@OrangeAndGreen Can we apply same logic when app fails to retrieve the valid connectId token? |
|
@Jignesh-dimagi Good idea, I changed the code that handles token denied to crash the DB and show the message that was in the toast when the app restarts |
…nto dv/worst_case_message Also crashing DB when token denied in new photo upload page.
I find that a bit odd that we need to store an error message in DB as well. We should probably store some flag/status code instead on the basis of which we can deduce errors.
I don't think I understand what's the reasoning here for waiting for next app startup. I would instead act as soon as we identify that there is a problem and showing maybe blocking message at the same time to the user i.e. Your DB is corrupt and you need to restup your connect account -> OK -> Auto Forget User -> navigate to device config flow ? (I am assuming use won't be able to do anything meaningful anyway with their connect ID account ?) |
…ing to ConnectKeyRecord). Now storing a GlobalErrors enum for the error instead of a string ID (enum converted to a string for display). Supporting multiple global errors being stored and displayed.
…mcare-android into dv/worst_case_message
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Left some Minor lint related comments, but looking quite good overall!
app/src/org/commcare/android/database/global/models/GlobalErrorRecord.java
Outdated
Show resolved
Hide resolved
| if(globalError != null) { | ||
| installFragment.showConnectErrorMessage(globalError); | ||
| } |
There was a problem hiding this comment.
optional: how about putting all of this error handling in the fragment itself inside a single method, it will be cleaner and more localized that way.
There was a problem hiding this comment.
Hmm, it's not clear to me how to accomplish that. We need to handle global errors as early as possible in order to possibly delete the Connect DB before attempting to initialize PersonalIdManager, but at that time the SelectInstallModeFragment hasn't had onCreateView called yet.
One idea: I could move the code for checking for global errors into the install fragment, such that:
- Setup activity could call installFragment.checkForGlobalErrors during onCreate
- The fragment could do the check and store the resulting string for later
- Once onCreateView gets called, the fragment could automatically display the error message if present
The code would still happen in two places that way, but there would only be one interaction from SetupActivity to InstallFragment
| } else if (ConnectDatabaseHelper.isDbBroken()) { | ||
| //Corrupt DB, inform user to recover | ||
| ConnectDatabaseHelper.handleCorruptDb(parent); | ||
| ConnectDatabaseHelper.crashDb(); |
There was a problem hiding this comment.
optional: I kind of prefer calling this handleCorruptDb as crashDb seems like we are destroying the database.
There was a problem hiding this comment.
Two thoughts:
- When we call crashDb we effectively are destroying the database, since that function crashes the app and on the next startup the DB is deleted after handling the global error that is specific to Connect.
- I renamed the method because it isn't only used in the corrupt DB case anymore. For example, we're now calling it when a PersonalID OAuth request gets denied, and in the future there may be other uses for it
There was a problem hiding this comment.
Sounds good, throwing markDbForDeletionAndCrash as another option.
| import org.commcare.dalvik.R; | ||
|
|
||
| public enum GlobalErrors { | ||
| PERSONALID_GENERIC_ERROR(R.string.connect_db_corrupt), |
There was a problem hiding this comment.
Probably good to be specific with error codes and call it PERSONALID_DB_CORRUPT and TOKEN_DENIED i.e naming them based on what causes them instead of what action we need to take for them .
There was a problem hiding this comment.
Ah, currently I have the first error intentionally set as the generic, and used in the default case where we want to crash the DB but don't have a more-specific error message to show the user. So the corrupt DB scenario is the current example, where we aren't going to tell the user that the DB was corrupt and instead just want to show a "something went wrong" message. I renamed the corresponding string to be inline: personalid_generic_error
For the second error, I was thinking of the real cause as "lost configuration", while "token denied" is just the action that led us to discover that we lost the configuration. Once we start supporting profile edits (i.e. changing name or uploading a new photo), the update_profile API call will be another place where we might learn that we lost config, but that would happen via a failed basic auth (not involving a token request).
All that said, I'm fine with renaming the errors if you still think that's the best way forward.
There was a problem hiding this comment.
the corrupt DB scenario is the current example, where we aren't going to tell the user that the DB was corrupt and instead just want to show a "something went wrong" message
Think it's fine to show a generic error message for these error codes without making the error code generic, I don't want these errors to stand for "What user sees as an error" but more to represent "The error state of App" if that makes sense.
I was thinking of the real cause as "lost configuration", while "token denied" is just the action that led us to discover that we lost the configuration.
That makes sense to me on naming, although curious what does "lost configuration` signify here ? As in, what exactly has been lost in the config that results into this.
…t to be more clear.
…ler, and rename function to indicate not Connect-specific.
|
@damagatchi retest this please |
2 similar comments
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
2 similar comments
|
@damagatchi retest this please |
|
@damagatchi retest this please |
…nto dv/worst_case_message
Product Description
In the "worst-case scenario" for Connect (where we need to de-configure the user on the device and have them complete the Device Configuration workflow again), the user will now be shown an error message on the Setup or Login page (depending on which gets shown at app startup) when they restart the CommCare app.
Technical Summary
Added a column to ConnectKeyRecord (global DB) to hold the ID for a localized error message string.
Added a function to "crash the DB": set an error message and crash the app
Added init code to check for the error message and, if found, delete all Connect-related storage
Added TextViews in LoginActivity and SelectInstallModeFragment (used by SetupActivity) to display the error message when encountered
Feature Flag
PersonalID
Safety Assurance
Safety story
Tested locally by manually forcing a DB crash and observing the message on restart.
Restarted the app again and observed no error message.
Automated test coverage
None
QA Plan
This may be tricky for QA to test since it handles worst-case scenarios that shouldn't be encountered in normal usage.