Adds Credential in Left Side Drawer #3340
Conversation
📝 WalkthroughWalkthroughRemoves the credential toolbar menu item from menu_connect.xml and its handling in ConnectActivity.onOptionsItemSelected. Centralizes post-unlock navigation in ConnectNavHelper via a new private unlockAndGoTo helper. Adds public navigation methods unlockAndGoToCredentials and goToCredentials to launch PersonalIdCredentialActivity. Updates nav drawer: adds NavItemType.CREDENTIAL, drawer item creation gated by shouldShowCredential(), and a navigateToCredential() path in BaseDrawerActivity that uses unlockAndGoToCredentials and closes the drawer on success. Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Drawer as BaseDrawerActivity
participant Controller as BaseDrawerController
participant Nav as ConnectNavHelper
participant Unlock as Unlock Flow
participant Cred as PersonalIdCredentialActivity
User->>Drawer: Tap "Credential" in drawer
Drawer->>Controller: onItemClick(CREDENTIAL)
Controller-->>Drawer: Dispatch CREDENTIAL
Drawer->>Nav: unlockAndGoToCredentials(activity, listener)
Nav->>Unlock: Prompt user to unlock (if required)
alt unlock success
Unlock-->>Nav: success
Nav->>Cred: startActivity()
Cred-->>Drawer: onComplete
Drawer->>Drawer: closeDrawer()
else unlock canceled/failure
Unlock-->>Nav: canceled/error
Nav-->>Drawer: no-op
end
sequenceDiagram
autonumber
participant Caller as Any Feature
participant Nav as ConnectNavHelper
participant Unlock as Unlock Flow
participant Target as Navigation Target
Note over Nav: New centralized helper
Caller->>Nav: unlockAndGoTo(activity, listener, navigationAction)
Nav->>Unlock: Ensure unlocked
alt success
Unlock-->>Nav: ok
Nav->>Target: navigationAction()
Target-->>Caller: done
else failure
Unlock-->>Nav: fail/cancel
Nav-->>Caller: return without navigation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
224-226: Redundant helper; keep or strengthen.This method mirrors the enclosing
if (isloggedIn()). Either inline the check (YAGNI) or expand it if additional constraints are required later (e.g., role/feature flag).app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
116-124: Nit: fix extra space; matches sibling methods.Minor style tweak.
- protected fun navigateToCredential() { + protected fun navigateToCredential() {app/src/org/commcare/connect/ConnectNavHelper.kt (1)
45-53: Credentials navigation added correctly; consider @JvmStatic for Java callers.If any Java code will call these, add
@JvmStaticfor parity withgoToNotification.- fun unlockAndGoToCredentials(activity: CommCareActivity<*>, listener: ConnectActivityCompleteListener) { + @JvmStatic + fun unlockAndGoToCredentials(activity: CommCareActivity<*>, listener: ConnectActivityCompleteListener) { unlockAndGoTo(activity, listener, ::goToCredentials) } - fun goToCredentials(context: Context) { + @JvmStatic + fun goToCredentials(context: Context) { val i = Intent(context, PersonalIdCredentialActivity::class.java) context.startActivity(i) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/res/menu/menu_connect.xml(0 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(0 hunks)app/src/org/commcare/connect/ConnectNavHelper.kt(2 hunks)app/src/org/commcare/navdrawer/BaseDrawerActivity.kt(3 hunks)app/src/org/commcare/navdrawer/BaseDrawerController.kt(3 hunks)
💤 Files with no reviewable changes (2)
- app/src/org/commcare/activities/connect/ConnectActivity.java
- app/res/menu/menu_connect.xml
🧰 Additional context used
🧠 Learnings (1)
📚 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/connect/ConnectNavHelper.ktapp/src/org/commcare/navdrawer/BaseDrawerActivity.kt
🧬 Code graph analysis (1)
app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (2)
app/src/org/commcare/connect/ConnectNavHelper.kt (1)
unlockAndGoToCredentials(45-47)app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
closeDrawer(228-230)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (5)
app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)
43-44: Enum addition looks good.
NavItemType.CREDENTIALintegrates cleanly with the existing drawer flow.app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
84-85: Drawer route wiring is correct.
NavItemType.CREDENTIAL -> navigateToCredential()aligns with existing patterns.app/src/org/commcare/connect/ConnectNavHelper.kt (3)
17-28: Nice centralization; confirm callback thread.
unlockAndGoTois a good abstraction. Please confirmPersonalIdManager.unlockConnectinvokes its callback on the main thread; starting activities off-main can be problematic.
30-33: Refactor to helper looks good.
unlockAndGoToMessagingnow delegates; behavior should be unchanged.
59-60: Access check retained — good.
goToConnectJobsListstill guards viacheckConnectAccess(context).
|
Credentials have been changed to worker history, should we change in code also that naming convection at major places where it is reflecting the worker history like from |
| fun unlockAndGoToCredentials(activity: CommCareActivity<*>, listener: ConnectActivityCompleteListener) { | ||
| unlockAndGoTo(activity, listener, ::goToCredentials) | ||
| } | ||
|
|
||
| fun goToCredentials(context: Context) { | ||
| val i = Intent(context, PersonalIdCredentialActivity::class.java) | ||
| context.startActivity(i) | ||
| } | ||
|
|
There was a problem hiding this comment.
+1 for making it generic, nit pic (optional):
fun unlockAndGoToCredentials(activity: CommCareActivity<*>, listener: ConnectActivityCompleteListener) {
unlockAndGoTo(activity, listener){
val i = Intent(activity, PersonalIdCredentialActivity::class.java)
activity.startActivity(i)
}
}
There was a problem hiding this comment.
I think we are going to change this code soon to only require unlock in some cases (based on a time threshold) and not everytime, so keeping the non unlock version of these methods separately might be helpful to achive that.
@Jignesh-dimagi yeah, I will do a separate PR here to rename everything related to |
|
|
||
| private fun shouldShowCredential(): Boolean { | ||
| // we are keeping this off for now until we have go ahead to release this feature | ||
| return PersonalIdManager.getInstance().isloggedIn() && false; |
There was a problem hiding this comment.
@shubham1g5 can we create variable in ConnectConstant so that it can be used at other places too to disable this feature and enable with this single variable only.
There was a problem hiding this comment.
I do like the idea of it being gverned by a variable. We are going to transition soon to Feature flag variables where server tells us what features are enabled for the user with the help of an API call as well. Given that I am thinking to create a new FeatureToggle class that has methods like isWorkHistoryEnabled() and just return a hardcoded value of false at the moment but in future can fetch this from DB. Thoughts ?
There was a problem hiding this comment.
Seems like good idea if Feature Flag functionality is going to be in our plate soon. I guess in this PR only you can create and use that FeatureFlag or FeatureToogle class so that minimal changes when implementing Feature Flag functionality.
|
@damagatchi retest this please |
1 similar comment
|
@damagatchi retest this please |
Product Description
https://dimagi.atlassian.net/browse/CCCT-1660
Labels and Review