Toggle Connect visibility #3295
Conversation
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant ConnectDB as ConnectUserDatabaseUtil
participant Nav as ConnectNavHelper
User->>App: Open Login/Setup screen
App->>ConnectDB: hasConnectAccess(context)
ConnectDB-->>App: true/false
App->>User: Show/enable Connect button if true
User->>App: Tap Connect
App->>Nav: goToConnectJobsListChecked(context)
Nav->>ConnectDB: hasConnectAccess(context)
alt access allowed
Nav-->>App: Start ConnectActivity
else no access
Nav-->>App: Throw IllegalStateException
end
sequenceDiagram
participant DB as Connect DB
participant Upgrader as ConnectDatabaseUpgrader
participant V14 as ConnectUserRecordV14
participant V16 as ConnectUserRecordV16
participant New as ConnectUserRecord
DB->>Upgrader: oldVersion=14..16
alt 14->15
Upgrader->>DB: Read V14 records
Upgrader->>V16: fromV14(old)
Upgrader->>DB: Write V16 records
end
alt 16->17
Upgrader->>DB: Read V16 records
Upgrader->>DB: Check ConnectJobRecord presence
Upgrader->>New: fromV16(old, hasConnectAccess)
Upgrader->>DB: Write New records (preserve IDs)
end
sequenceDiagram
participant FCM as FirebaseMessagingUtil
participant ConnectDB as ConnectUserDatabaseUtil
FCM->>ConnectDB: turnOnConnectAccess(context)
ConnectDB-->>FCM: Access enabled if user exists
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🔭 Outside diff range comments (1)
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
674-680: Use the checked navigation helper to prevent potential crashes.The click handler currently calls goToConnectJobsList, which throws IllegalStateException if access is absent. While the button is gated by isConnectEnabled, future regressions could enable the button prematurely and cause a crash. Prefer the “checked” helper for safety.
Apply:
- installFragment.updateConnectButton(isConnectEnabled, v -> { - PersonalIdManager.getInstance().unlockConnect(this, success -> { - ConnectNavHelper.INSTANCE.goToConnectJobsList(this); - }); - }); + installFragment.updateConnectButton(isConnectEnabled, v -> { + PersonalIdManager.getInstance().unlockConnect(this, success -> { + ConnectNavHelper.INSTANCE.goToConnectJobsListChecked(this); + }); + });
🧹 Nitpick comments (13)
app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (1)
24-26: Add invitedUser flag: fix comment typo; confirm default-false semanticsThe addition looks good. Two small things:
- Typo in the comment.
- Please confirm that defaulting to false is intentional for older server responses that may not include invited_user; i.e., access will be turned on via push/migration.
Apply this diff to fix the comment:
- var invitedUser: Boolean = false // indicates if user has has been invited to Connect + var invitedUser: Boolean = false // indicates if the user has been invited to Connectapp/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java (1)
30-30: Deduplicate JSON key "invited_user" via a shared constantThe same key is used across multiple parsers. Extract a shared constant to avoid typos and simplify future changes.
Consider adding a constant to the shared interface and using it here (and in CompleteProfileResponseParser):
Add in PersonalIdApiResponseParser:
// e.g., in PersonalIdApiResponseParser.java String JSON_INVITED_USER = "invited_user";Then change here:
- sessionData.setInvitedUser(json.optBoolean("invited_user", false)); + sessionData.setInvitedUser(json.optBoolean(JSON_INVITED_USER, false));And similarly in CompleteProfileResponseParser:
- sessionData.setInvitedUser(json.optBoolean("invited_user", false)); + sessionData.setInvitedUser(json.optBoolean(JSON_INVITED_USER, false));app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
194-194: Enabling access on any CCC push: confirm intended behavior; consider adding audit loggingTurning on Connect access upon any CCC push centralizes the “invite” mechanism and seems aligned with the PR goals. Two follow-ups:
- Confirm it’s acceptable to enable access even if the specific CCC action is unrecognized (default case) or has an invalid payload; the call happens before the switch handling.
- Consider adding a lightweight log/analytics event when access is enabled via push to aid debugging/audit.
app/src/org/commcare/connect/PersonalIdManager.java (1)
225-225: Post-signin navigation now silently no-ops without access; consider user feedback.Switching to goToConnectJobsListChecked avoids crashes but provides no UX signal if access is absent. Consider surfacing a brief message so users know why they aren’t navigated.
Example change (if you prefer handling it here instead of in the helper):
- ConnectNavHelper.INSTANCE.goToConnectJobsListChecked(activity); + if (ConnectUserDatabaseUtil.hasConnectAccess(activity)) { + ConnectNavHelper.INSTANCE.goToConnectJobsList(activity); + } else { + // Optional: replace with a dedicated string if available + Toast.makeText(activity, activity.getString(R.string.connect_unlock_unavailable), Toast.LENGTH_SHORT).show(); + }app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
135-136: Typo: Rename hasConnecrAccess → hasConnectAccess.Minor readability nit; keeps naming consistent with the util method.
- val hasConnecrAccess = ConnectUserDatabaseUtil.hasConnectAccess(activity) + val hasConnectAccess = ConnectUserDatabaseUtil.hasConnectAccess(activity)Also update the reference below (line 143) accordingly:
- isEnabled = hasConnecrAccess + isEnabled = hasConnectAccessapp/src/org/commcare/connect/ConnectNavHelper.kt (1)
20-20: Nit: Remove unnecessary semicolon.Kotlin doesn’t need it here.
- checkConnectAccess(context); + checkConnectAccess(context)app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (1)
230-236: Parameter alignment issue in ConnectUserRecord constructorThe constructor call has inconsistent spacing between parameters
personalIdSessionData.getDemoUser()andpersonalIdSessionData.getRequiredLock().Apply this diff to fix the spacing:
- personalIdSessionData.getDemoUser(),personalIdSessionData.getRequiredLock(), + personalIdSessionData.getDemoUser(), personalIdSessionData.getRequiredLock(),app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java (1)
133-141: Parameter alignment issue in ConnectUserRecord constructorThe constructor call has inconsistent spacing between parameters
personalIdSessionData.getDemoUser()andpersonalIdSessionData.getRequiredLock().Apply this diff to fix the spacing:
- personalIdSessionData.getDemoUser(),personalIdSessionData.getRequiredLock(), + personalIdSessionData.getDemoUser(), personalIdSessionData.getRequiredLock(),app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (2)
83-84: Add Javadoc for the new hasConnectAccess fieldBased on the learning about documenting fields with Javadoc comments, the new
hasConnectAccessfield should include documentation explaining its purpose and relationship with the Connect feature access control.Add documentation above the field:
+ /** + * Indicates whether the user has access to Connect features. + * This flag is controlled by the invited_user status from the server + * and can be enabled through push notifications or server responses. + */ @Persisting(value = 16) private boolean hasConnectAccess;
95-109: Constructor parameter list is becoming unwieldyThe constructor now has 10 parameters, which makes it difficult to use correctly and prone to errors. Consider using a builder pattern or a parameter object to improve maintainability.
Consider refactoring to use a builder pattern for better maintainability:
public static class Builder { private String primaryPhone; private String userId; private String password; private String name; private String pin; private Date lastPinVerifyDate; private String photo; private boolean isDemo; private String requiredLock = PersonalIdSessionData.PIN; private boolean hasConnectAccess; public Builder withPrimaryPhone(String primaryPhone) { this.primaryPhone = primaryPhone; return this; } // ... other builder methods public ConnectUserRecord build() { ConnectUserRecord record = new ConnectUserRecord(); // ... set all fields return record; } }This would make the code more readable and less error-prone when creating instances.
app/src/org/commcare/android/database/connect/models/ConnectUserRecordV16.java (3)
11-12: Add a brief class-level Javadoc to clarify this is an intermediate, migration-only model.This helps future maintainers avoid adding business logic or evolving this class beyond its migration purpose.
Apply this diff:
@Table(ConnectUserRecordV16.STORAGE_KEY) -public class ConnectUserRecordV16 extends Persisted { +/** + * Intermediate persisted model for DB v16 used only during the 14→15→16 and 16→17 upgrades. + * Mirrors the legacy ConnectUserRecord schema to facilitate data migration. + * Do not add business logic or evolve this class beyond migration needs. + */ +public class ConnectUserRecordV16 extends Persisted {
67-69: Annotate requiredLock with DeviceAuthType for compile-time safety.Since only PIN or BIOMETRIC_TYPE are valid, use the existing annotation to catch invalid assignments at compile time.
Apply this diff:
- @Persisting(value = 15) - private String requiredLock; + @Persisting(value = 15) + @PersonalIdSessionData.DeviceAuthType + private String requiredLock;And annotate the getter’s return type:
- public String getRequiredLock() { + public @PersonalIdSessionData.DeviceAuthType String getRequiredLock() { return requiredLock; }Also applies to: 141-143
19-69: Sensitive fields are persisted; ensure we never log them.password and pin are plain strings in the model (expected for compatibility). Please confirm that:
- We don’t log instances of this class (or override toString()).
- Any debug logs redact these fields.
If needed, I can propose a minimal toString() override that redacts sensitive fields specifically in this intermediate class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/src/org/commcare/activities/CommCareSetupActivity.java(2 hunks)app/src/org/commcare/activities/LoginActivityUIController.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java(6 hunks)app/src/org/commcare/android/database/connect/models/ConnectUserRecordV16.java(1 hunks)app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt(1 hunks)app/src/org/commcare/connect/ConnectNavHelper.kt(2 hunks)app/src/org/commcare/connect/PersonalIdManager.java(1 hunks)app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java(1 hunks)app/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParser.java(1 hunks)app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java(1 hunks)app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java(4 hunks)app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java(1 hunks)app/src/org/commcare/navdrawer/BaseDrawerController.kt(1 hunks)app/src/org/commcare/utils/FirebaseMessagingUtil.java(2 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecordV8.java:107-107
Timestamp: 2025-01-28T08:41:32.380Z
Learning: When upgrading from ConnectLinkedAppRecordV3 to ConnectLinkedAppRecordV8, the connectIdLinked field is intentionally set to true as part of the database migration strategy since this field was not available from the server in V3 and is introduced in V8.
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
PR: dimagi/commcare-android#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/activities/CommCareSetupActivity.javaapp/src/org/commcare/connect/PersonalIdManager.javaapp/src/org/commcare/utils/FirebaseMessagingUtil.javaapp/src/org/commcare/connect/ConnectNavHelper.kt
📚 Learning: 2025-05-22T14:28:35.959Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Applied to files:
app/src/org/commcare/activities/CommCareSetupActivity.java
📚 Learning: 2025-02-04T15:29:12.993Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2927
File: app/src/org/commcare/activities/connect/HQUserInviteActivity.java:0-0
Timestamp: 2025-02-04T15:29:12.993Z
Learning: In HQUserInviteActivity, parameters from Intent data (domain, inviteCode, username, etc.) are required and the code is designed to fail immediately if they're missing, rather than adding null checks. This helps catch integration issues early.
Applied to files:
app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java
📚 Learning: 2025-02-19T15:15:01.935Z
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2956
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java:58-60
Timestamp: 2025-02-19T15:15:01.935Z
Learning: Error handling for message retrieval in ConnectMessageChannelListFragment's retrieveMessages callback is not required as per user preference.
Applied to files:
app/src/org/commcare/utils/FirebaseMessagingUtil.java
📚 Learning: 2025-07-29T14:14:07.954Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3248
File: app/src/org/commcare/adapters/ConnectMessageAdapter.java:0-0
Timestamp: 2025-07-29T14:14:07.954Z
Learning: In the CommCare Android Connect messaging system (ConnectMessageAdapter.java), the team prefers to let getMessage() potentially return null and crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately during development.
Applied to files:
app/src/org/commcare/utils/FirebaseMessagingUtil.java
📚 Learning: 2025-05-09T10:57:41.073Z
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
Applied to files:
app/src/org/commcare/utils/FirebaseMessagingUtil.java
📚 Learning: 2025-07-29T14:09:49.805Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3248
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java:0-0
Timestamp: 2025-07-29T14:09:49.805Z
Learning: In the CommCare Android Connect messaging system, message retry logic is implemented at the MessageManager level via MessageManager.sendUnsentMessages(). This method is called automatically when the channel list loads (ConnectMessageChannelListFragment), and it attempts to resend any outgoing messages that are not yet confirmed. Individual message fragments do not need to implement their own retry logic.
Applied to files:
app/src/org/commcare/utils/FirebaseMessagingUtil.java
📚 Learning: 2025-01-28T08:41:32.380Z
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecordV8.java:107-107
Timestamp: 2025-01-28T08:41:32.380Z
Learning: When upgrading from ConnectLinkedAppRecordV3 to ConnectLinkedAppRecordV8, the connectIdLinked field is intentionally set to true as part of the database migration strategy since this field was not available from the server in V3 and is introduced in V8.
Applied to files:
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.javaapp/src/org/commcare/android/database/connect/models/ConnectUserRecord.java
📚 Learning: 2025-01-21T17:28:09.007Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#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/ConnectUserRecordV16.javaapp/src/org/commcare/android/database/connect/models/ConnectUserRecord.javaapp/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.javaapp/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java
📚 Learning: 2025-01-23T21:38:17.427Z
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecordV3.java:1-68
Timestamp: 2025-01-23T21:38:17.427Z
Learning: The versioned database model classes (e.g., ConnectLinkedAppRecordV3) intentionally maintain their original design patterns, including their security implementation, to preserve backward compatibility.
Applied to files:
app/src/org/commcare/android/database/connect/models/ConnectUserRecordV16.java
📚 Learning: 2025-01-26T14:32:09.266Z
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecordV5.java:72-76
Timestamp: 2025-01-26T14:32:09.266Z
Learning: In ConnectUserRecordV5, initializing lastPasswordDate and connectTokenExpiration to new Date() in the constructor is intentional.
Applied to files:
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java
🧬 Code Graph Analysis (7)
app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (1)
app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt (1)
context(11-38)
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (1)
ConnectUserDatabaseUtil(10-56)
app/src/org/commcare/connect/PersonalIdManager.java (1)
app/src/org/commcare/connect/ConnectNavHelper.kt (1)
goToConnectJobsListChecked(31-35)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (1)
ConnectUserDatabaseUtil(10-56)
app/src/org/commcare/activities/LoginActivityUIController.java (3)
app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
isPersonalIdLoggedIn(21-25)app/src/org/commcare/connect/PersonalIdManager.java (1)
PersonalIdManager(64-622)app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (1)
ConnectUserDatabaseUtil(10-56)
app/src/org/commcare/android/database/connect/models/ConnectUserRecordV16.java (1)
app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (1)
requiredLock(9-40)
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (1)
app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (1)
requiredLock(9-40)
🔇 Additional comments (9)
app/src/org/commcare/connect/network/connectId/parser/CompleteProfileResponseParser.java (1)
26-26: Parse invited_user: LGTMUsing optBoolean with a false fallback is appropriate and consistent with the gating strategy described in the PR.
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
30-30: Import of ConnectUserDatabaseUtil: LGTMapp/src/org/commcare/activities/CommCareSetupActivity.java (1)
33-33: Import looks correct.The new import for ConnectUserDatabaseUtil aligns with usage below.
app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
141-143: Gating OPPORTUNITIES by Connect access is appropriate.Enabling the parent item based on persisted access state is consistent with the rest of the PR’s access-control pattern.
app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (1)
44-55: LGTM: Minimal, clear APIs for access check and enablement.Both hasConnectAccess and turnOnConnectAccess read well and match the storage model introduced by the migration.
app/src/org/commcare/activities/LoginActivityUIController.java (1)
565-576: Good refactoring to improve code organizationThe extraction of the welcome message logic into a separate method improves code organization and readability. The access control logic correctly combines both login state and Connect access checks to determine button visibility.
app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java (1)
601-632: Review migrate has_connect_access logic for edge casesThe migration in
ConnectDatabaseUpgrader.java:upgradeSixteenSeventeencalculateshasConnectAccesssolely by checking for existingConnectJobRecordentries. No special handling is present for demo/test or invited users, and we found no documentation on aninvited_userflag.Please confirm that this aligns with product requirements for:
- Users whose job records have been deleted
- Demo/test users created via
DemoUserBuilder- Any invited-user onboarding flows
Locations for review:
- app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java (lines 601–632)
- app/src/org/commcare/models/database/user/DemoUserBuilder.java
- app/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.java
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (1)
199-216: Verify invited_user mapping and migration logic for hasConnectAccessIt looks like
fromV16correctly uses thehasConnectAccessparameter, but no references to the server’sinvited_userflag were found in the codebase. Please manually verify that:
- The JSON parser/model for the auth or user endpoint (e.g.
ConnectAuthResponse,PersonalIdServerParser) reads theinvited_userfield.- That
invited_useris passed into theConnectUserRecordV17(or equivalent) constructor and eventually intohasConnectAccess.- Your database migration tests cover both invited and non-invited user scenarios to ensure new and migrated users receive the correct
hasConnectAccessvalue.app/src/org/commcare/android/database/connect/models/ConnectUserRecordV16.java (1)
70-77: Confirm defaults won’t be used for new records outside migrations.Setting verifySecondaryPhoneByDate and connectTokenExpiration to new Date() in the constructor is fine for migration scaffolding, but if this constructor is ever used for non-migration instantiation, these defaults could be semantically incorrect.
Would you like me to scan the PR branch to verify that ConnectUserRecordV16 is only used by the upgrader path and never for creating new users?
| public static ConnectUserRecordV16 fromV14(ConnectUserRecordV14 oldRecord) { | ||
| ConnectUserRecordV16 newRecord = new ConnectUserRecordV16(); | ||
| newRecord.userId = oldRecord.getUserId(); | ||
| newRecord.password = oldRecord.getPassword(); | ||
| newRecord.name = oldRecord.getName(); | ||
| newRecord.primaryPhone = oldRecord.getPrimaryPhone(); | ||
| newRecord.alternatePhone = ""; | ||
| newRecord.registrationPhase = oldRecord.getRegistrationPhase(); | ||
| newRecord.lastPasswordDate = oldRecord.getLastPasswordDate(); | ||
| newRecord.connectToken = oldRecord.getConnectToken(); | ||
| newRecord.connectTokenExpiration = oldRecord.getConnectTokenExpiration(); | ||
| newRecord.secondaryPhoneVerified = true; | ||
| newRecord.photo = oldRecord.getPhoto(); | ||
| newRecord.isDemo = oldRecord.isDemo(); | ||
| newRecord.requiredLock = PersonalIdSessionData.PIN; | ||
| return newRecord; | ||
| } |
There was a problem hiding this comment.
Migration currently loses secondary phone verification state and deadline. Preserve values from V14 instead of forcing defaults.
Forcing secondaryPhoneVerified = true and leaving verifySecondaryPhoneByDate at constructor default overwrites existing user data and can change behavior. Even if deprecated, preserving fidelity during migration avoids unintended side effects.
Apply this diff:
public static ConnectUserRecordV16 fromV14(ConnectUserRecordV14 oldRecord) {
ConnectUserRecordV16 newRecord = new ConnectUserRecordV16();
newRecord.userId = oldRecord.getUserId();
newRecord.password = oldRecord.getPassword();
newRecord.name = oldRecord.getName();
newRecord.primaryPhone = oldRecord.getPrimaryPhone();
newRecord.alternatePhone = "";
newRecord.registrationPhase = oldRecord.getRegistrationPhase();
newRecord.lastPasswordDate = oldRecord.getLastPasswordDate();
newRecord.connectToken = oldRecord.getConnectToken();
newRecord.connectTokenExpiration = oldRecord.getConnectTokenExpiration();
- newRecord.secondaryPhoneVerified = true;
+ // Preserve legacy state for deprecated fields to avoid behavior changes during migration
+ newRecord.secondaryPhoneVerified = oldRecord.isSecondaryPhoneVerified();
+ newRecord.verifySecondaryPhoneByDate = oldRecord.getVerifySecondaryPhoneByDate();
newRecord.photo = oldRecord.getPhoto();
newRecord.isDemo = oldRecord.isDemo();
newRecord.requiredLock = PersonalIdSessionData.PIN;
return newRecord;
}If the intent is to discard these fields entirely in v17+, consider explicitly nulling them (and marking them nullable) or removing them during the v16→v17 transformation instead of mutating values here.
📝 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.
| public static ConnectUserRecordV16 fromV14(ConnectUserRecordV14 oldRecord) { | |
| ConnectUserRecordV16 newRecord = new ConnectUserRecordV16(); | |
| newRecord.userId = oldRecord.getUserId(); | |
| newRecord.password = oldRecord.getPassword(); | |
| newRecord.name = oldRecord.getName(); | |
| newRecord.primaryPhone = oldRecord.getPrimaryPhone(); | |
| newRecord.alternatePhone = ""; | |
| newRecord.registrationPhase = oldRecord.getRegistrationPhase(); | |
| newRecord.lastPasswordDate = oldRecord.getLastPasswordDate(); | |
| newRecord.connectToken = oldRecord.getConnectToken(); | |
| newRecord.connectTokenExpiration = oldRecord.getConnectTokenExpiration(); | |
| newRecord.secondaryPhoneVerified = true; | |
| newRecord.photo = oldRecord.getPhoto(); | |
| newRecord.isDemo = oldRecord.isDemo(); | |
| newRecord.requiredLock = PersonalIdSessionData.PIN; | |
| return newRecord; | |
| } | |
| public static ConnectUserRecordV16 fromV14(ConnectUserRecordV14 oldRecord) { | |
| ConnectUserRecordV16 newRecord = new ConnectUserRecordV16(); | |
| newRecord.userId = oldRecord.getUserId(); | |
| newRecord.password = oldRecord.getPassword(); | |
| newRecord.name = oldRecord.getName(); | |
| newRecord.primaryPhone = oldRecord.getPrimaryPhone(); | |
| newRecord.alternatePhone = ""; | |
| newRecord.registrationPhase = oldRecord.getRegistrationPhase(); | |
| newRecord.lastPasswordDate = oldRecord.getLastPasswordDate(); | |
| newRecord.connectToken = oldRecord.getConnectToken(); | |
| newRecord.connectTokenExpiration = oldRecord.getConnectTokenExpiration(); | |
| // Preserve legacy state for deprecated fields to avoid behavior changes during migration | |
| newRecord.secondaryPhoneVerified = oldRecord.isSecondaryPhoneVerified(); | |
| newRecord.verifySecondaryPhoneByDate = oldRecord.getVerifySecondaryPhoneByDate(); | |
| newRecord.photo = oldRecord.getPhoto(); | |
| newRecord.isDemo = oldRecord.isDemo(); | |
| newRecord.requiredLock = PersonalIdSessionData.PIN; | |
| return newRecord; | |
| } |
🤖 Prompt for AI Agents
In
app/src/org/commcare/android/database/connect/models/ConnectUserRecordV16.java
around lines 79 to 95, the migration from V14 currently forces
secondaryPhoneVerified = true and leaves verifySecondaryPhoneByDate at the
constructor default, which overwrites existing V14 state; instead copy the
secondary phone verification fields from the old record (e.g.,
newRecord.secondaryPhoneVerified = oldRecord.isSecondaryPhoneVerified();
newRecord.verifySecondaryPhoneByDate =
oldRecord.getVerifySecondaryPhoneByDate()) so the original verification flag and
deadline are preserved during migration; if the V14 getters differ, call the
appropriate getters or add them, and only default/null them here if the explicit
intent is to discard these fields (in which case make that behavior explicit and
documented).
| private fun checkConnectAccess(context: Context) { | ||
| if (!ConnectUserDatabaseUtil.hasConnectAccess(context)) { | ||
| throw IllegalStateException("Cannot navigate to Connect Jobs List without access") | ||
| } | ||
| } | ||
|
|
||
| fun goToConnectJobsListChecked(context: Context) { | ||
| if (ConnectUserDatabaseUtil.hasConnectAccess(context)) { | ||
| goToConnectJobsList(context) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Throwing on missing access can crash legacy call sites; verify and consider a softer failure mode.
goToConnectJobsList and goToActiveInfoForJob now throw IllegalStateException via checkConnectAccess. If any legacy caller didn’t update to the checked variant or pre-gate, the app will crash.
Two-part recommendation:
- Verify all call sites are either pre-gated or now use goToConnectJobsListChecked.
- Consider returning early with logging (or using the checked variant exclusively) instead of throwing, to make navigation failures non-fatal in production.
Run to find call sites that may need updating:
If you prefer a non-throwing helper, here’s a diff to soften the behavior:
private fun checkConnectAccess(context: Context) {
- if (!ConnectUserDatabaseUtil.hasConnectAccess(context)) {
- throw IllegalStateException("Cannot navigate to Connect Jobs List without access")
- }
+ if (!ConnectUserDatabaseUtil.hasConnectAccess(context)) {
+ // Non-fatal guardrail; callers using the unchecked method won't crash.
+ // Consider logging here if you have a logger available.
+ return
+ }
}Optionally, extend the pattern to messaging to keep all Connect surfaces uniformly gated (outside the changed range):
// Additional change outside the selected lines
fun goToMessagingChecked(context: Context) {
if (ConnectUserDatabaseUtil.hasConnectAccess(context)) {
goToMessaging(context)
}
}Also applies to: 37-39
🏁 Script executed:
#!/bin/bash
# Find all direct calls to goToConnectJobsList and goToActiveInfoForJob
# Expectation: Calls either happen behind ConnectUserDatabaseUtil.hasConnectAccess checks
# or use goToConnectJobsListChecked.
set -e
echo "Kotlin call sites:"
rg -n "ConnectNavHelper\.(goToConnectJobsList|goToActiveInfoForJob)\(" -S -g '!**/build/**'
echo
echo "Java call sites:"
rg -n "ConnectNavHelper\.INSTANCE\.(goToConnectJobsList|goToActiveInfoForJob)\(" -S -g '!**/build/**'Length of output: 149
🏁 Script executed:
#!/bin/bash
# Find all direct calls to goToConnectJobsList and goToActiveInfoForJob,
# but don't exit on no matches so we get both Kotlin and Java results.
echo "Kotlin call sites:"
rg -n "ConnectNavHelper\.(goToConnectJobsList|goToActiveInfoForJob)\(" -S -g '!**/build/**' || true
echo
echo "Java call sites:"
rg -n "ConnectNavHelper\.INSTANCE\.(goToConnectJobsList|goToActiveInfoForJob)\(" -S -g '!**/build/**' || trueLength of output: 834
🏁 Script executed:
#!/bin/bash
# Show context around each Java call site to see if they're already gated.
echo "CommCareSetupActivity.java context:"
rg -n -C3 "goToConnectJobsList" app/src/org/commcare/activities/CommCareSetupActivity.java
echo
echo "LoginActivity.java context:"
rg -n -C3 "goToConnectJobsList" app/src/org/commcare/activities/LoginActivity.java
echo
echo "DispatchActivity.java context (goToConnectJobsList):"
rg -n -C3 "goToConnectJobsList" app/src/org/commcare/activities/DispatchActivity.java
echo
echo "DispatchActivity.java context (goToActiveInfoForJob):"
rg -n -C3 "goToActiveInfoForJob" app/src/org/commcare/activities/DispatchActivity.javaLength of output: 2305
Ensure Connect navigation calls are gated or use the “Checked” variants
We found two unguarded calls in DispatchActivity that will throw IllegalStateException at runtime if the user lacks connect access:
- app/src/org/commcare/activities/DispatchActivity.java:
• Line 208:ConnectNavHelper.INSTANCE.goToConnectJobsList(this);
• Line 212:ConnectNavHelper.INSTANCE.goToActiveInfoForJob(this, job, true);
These should be guarded with ConnectUserDatabaseUtil.hasConnectAccess(context) or swapped to the checked helpers:
- ConnectNavHelper.INSTANCE.goToConnectJobsList(this);
+ if (ConnectUserDatabaseUtil.hasConnectAccess(this)) {
+ ConnectNavHelper.INSTANCE.goToConnectJobsList(this);
+ }
- ConnectNavHelper.INSTANCE.goToActiveInfoForJob(this, job, true);
+ if (ConnectUserDatabaseUtil.hasConnectAccess(this)) {
+ ConnectNavHelper.INSTANCE.goToActiveInfoForJob(this, job, true);
+ }Alternatively, call the checked variants:
ConnectNavHelper.INSTANCE.goToConnectJobsListChecked(this);
ConnectNavHelper.INSTANCE.goToActiveInfoForJobChecked(this, job, true);Note: Other sites (in CommCareSetupActivity and LoginActivity) invoke navigation only after unlockConnect(success → …), so they’re already gated.
🤖 Prompt for AI Agents
In app/src/org/commcare/activities/DispatchActivity.java around lines 208–212,
two unguarded ConnectNavHelper calls will throw when the user lacks connect
access; replace each direct call with either a guard using
ConnectUserDatabaseUtil.hasConnectAccess(context) before invoking the current
method, or call the checked variants instead: use
ConnectNavHelper.INSTANCE.goToConnectJobsListChecked(this) in place of
goToConnectJobsList(this), and use
ConnectNavHelper.INSTANCE.goToActiveInfoForJobChecked(this, job, true) in place
of goToActiveInfoForJob(this, job, true); ensure any added guard uses the
current activity/context reference consistently.
| */ | ||
| private static final int CONNECT_DB_VERSION = 16; | ||
| private static final int CONNECT_DB_VERSION = 17; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
DB bump to 17: verify upgrade path and align changelog
- Version bump and note look correct. Please ensure ConnectDatabaseUpgrader implements a robust 16→17 migration that adds has_connect_access (and infers it per spec).
- Changelog jumps from V.14 to V.16. If V.15 existed, consider adding a brief V.15 note for consistency.
Run this script to verify upgrade/migration artifacts exist and reference has_connect_access:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate ConnectDatabaseUpgrader and check for 16->17 logic"
fd -HI ConnectDatabaseUpgrader.java | sed -n '1,200p'
rg -n "onUpgrade|upgrade\\(|17|16|has_connect_access|V17" -A 3 -B 3
echo
echo "Confirm ConnectUserRecord has has_connect_access column mapping"
fd -HI ConnectUserRecord.java
rg -n "has_connect_access|hasConnectAccess" -A 2 -B 2
echo
echo "Check presence of V16 model used for migration"
fd -HI "ConnectUserRecordV16.java"
rg -n "ConnectUserRecordV16|fromV16|fromV14" -A 2 -B 2Length of output: 307548
To verify the 16→17 migration and confirm has_connect_access is handled correctly, let’s dump the upgrader and related classes and search for relevant patterns:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Inspect ConnectDatabaseUpgrader ==="
sed -n '1,200p' app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java
echo
echo "=== 2) Look for 16→17 logic in upgrader ==="
grep -nE 'case\s+16|case\s+17|upgrade.*16|upgrade.*17' \
app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java || echo "no matches"
echo
echo "=== 3) Search for has_connect_access across repo ==="
grep -Rn "has_connect_access" .
echo
echo "=== 4) Check ConnectUserRecord mapping ==="
grep -Rn "has_connect_access" \
app/src/org/commcare/models/database/connect/ConnectUserRecord.java || echo "no mapping"
echo
echo "=== 5) Verify presence of V16 model ==="
find app/src -type f -name "ConnectUserRecordV16.java" -print
grep -Rn "class ConnectUserRecordV16" app/src || echo "V16 model not found"Length of output: 9401
Let’s inspect the 16→17 upgrader and the ConnectUserRecord classes to confirm the new column and migration logic:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Show upgradeSixteenSeventeen in ConnectDatabaseUpgrader ==="
grep -R -n "upgradeSixteenSeventeen" -A20 app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java
echo
echo "=== 2) Verify has_connect_access in current ConnectUserRecord ==="
grep -n "has_connect_access" -A5 -B5 app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java
echo
echo "=== 3) Confirm V16 model lacks this column ==="
grep -n "has_connect_access" -A5 -B5 app/src/org/commcare/android/database/connect/models/ConnectUserRecordV16.javaLength of output: 2667
Let’s locate the actual ConnectUserRecord.java and check how (or if) it maps the new column:
This will confirm whether the new boolean column is declared and migrated in the V16→V17 path.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate ConnectUserRecord.java ==="
fd -HI ConnectUserRecord.java
echo
echo "=== Inspect its fields (first 200 lines) ==="
sed -n '1,200p' app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java
echo
echo "=== Look for hasConnectAccess in that file ==="
grep -Rn "hasConnectAccess" app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java || echo "no getter/setter for hasConnectAccess"
echo
echo "=== Confirm V16 has no such field ==="
grep -Rn "hasConnectAccess" app/src/org/commcare/android/database/connect/models/ConnectUserRecordV16.java || echo "V16 unchanged"Length of output: 6926
Confirm 16→17 migration and fix changelog consistency
- The
upgradeSixteenSeventeenmethod inConnectDatabaseUpgrader
• Reads existing job records and infershasConnectAccess = jobStorage.getNumRecords() > 0
• Loads allConnectUserRecordV16, callsConnectUserRecord.fromV16(old, hasConnectAccess), and writes the newConnectUserRecord - The
ConnectUserRecordclass defines
•@Persisting(16) private boolean hasConnectAccess;
• AfromV16(…, boolean)factory that assignshasConnectAccess
• PublichasConnectAccess()getter andsetHasConnectAccess(...)setter ConnectUserRecordV16has nohasConnectAccessfield, as expected- Changelog in
DatabaseConnectOpenHelper.javajumps from V.14 to V.16. Since there is anupgradeFourteenFifteenstep, please add a brief V.15 entry (e.g. “V15 – <summary of 14→15 changes>”) for consistency.
…nectAccess on Personal ID login
- Throws an Exception if code tries to open Connect screen without access - Adds a method to check for access and navigate to Connect
shubham1g5
force-pushed
the
connectVisibility
branch
from
8fca944 to
6202109
Compare
shubham1g5
force-pushed
the
connectVisibility
branch
from
a535725 to
8eb1d0c
Compare
app/res/values/strings.xml
Outdated
| <string name="nav_drawer_close">Close navigation drawer</string> | ||
|
|
||
| <string name="setup_refresh_opportunities_no_jobs">No Opportunities found</string> | ||
| <string name="setup_refresh_opportunities_with_jobs">"New opportunities found, click on Opportunities to learn more. "</string> |
There was a problem hiding this comment.
Extra quotes in the string
Product Description
https://dimagi.atlassian.net/browse/CCCT-1613
vidma_recorder_14082025_223355.mp4
Technical Summary
Spec
QA Plan
Part of 2.59
Labels and Review