Code clean up for Secondry Phone number #3193
Conversation
📝 WalkthroughWalkthroughThis change removes all secondary phone verification functionality from the codebase. It deletes methods and logic related to displaying, updating, and handling the secondary phone confirmation tile in the home screen and UI controller. Methods for managing secondary phone verification in the Sequence Diagram(s)sequenceDiagram
participant User
participant StandardHomeActivity
participant UIController
participant PersonalIdManager
User->>StandardHomeActivity: Open Home Screen
StandardHomeActivity->>UIController: (No longer calls updateSecondaryPhoneConfirmationTile)
UIController--xPersonalIdManager: (No check for secondary phone verification)
Note over StandardHomeActivity,UIController: Secondary phone verification UI and logic are skipped
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
app/src/org/commcare/activities/connect/PersonalIdActivity.java (2)
31-35:⚠️ Potential issuePotential NPE when the expected fragment is not active
getCurrentFragment()can returnnull; the comment even acknowledges the crash potential, yet the call to
getCurrentFragment().handleFinishedPinActivity(...)is unconditional.- getCurrentFragment().handleFinishedPinActivity(requestCode, resultCode, data); + PersonalIdBiometricConfigFragment frag = getCurrentFragment(); + if (frag != null) { + frag.handleFinishedPinActivity(requestCode, resultCode, data); + }This removes the crash risk without changing behaviour.
66-74:⚠️ Potential issueIllegal switch syntax – will not compile
case NotIntroduced, Registering:is only legal with the arrow (->) switch expression syntax (Java 14+).
With a colon switch statement you must repeat thecasetoken:- switch (PersonalIdManager.getInstance().getStatus()) { - case NotIntroduced, Registering: + switch (PersonalIdManager.getInstance().getStatus()) { + case NotIntroduced: + case Registering:Without this fix the class fails to build.
🧹 Nitpick comments (1)
app/src/org/commcare/activities/connect/PersonalIdActivity.java (1)
25-26:beginRegistration()invocation happens beforenavControlleris guaranteed to be ready
beginRegistration()eventually callsnavController.navigate(), butnavControlleris initialised byNavigationHostCommCareActivityaftersetContentView()in its ownonCreate().
BecausebeginRegistration()is executed immediately aftersuper.onCreate(savedInstanceState)we are safe only if the base-class initialisesnavControllersynchronously inside itsonCreate()before control returns here.
If that implicit ordering ever changes, this will throw an NPE at app start-up.Consider defensive-guarding or moving the call into
onPostCreate().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/org/commcare/activities/StandardHomeActivity.java(0 hunks)app/src/org/commcare/activities/StandardHomeActivityUIController.java(0 hunks)app/src/org/commcare/activities/connect/PersonalIdActivity.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java(1 hunks)app/src/org/commcare/connect/PersonalIdManager.java(0 hunks)
💤 Files with no reviewable changes (3)
- app/src/org/commcare/activities/StandardHomeActivity.java
- app/src/org/commcare/activities/StandardHomeActivityUIController.java
- app/src/org/commcare/connect/PersonalIdManager.java
| /** | ||
| * @Deprecated should no longer be used, just here to avoid a db migration for the moment | ||
| */ | ||
| private boolean secondaryPhoneVerified; | ||
|
|
||
| @Persisting(12) | ||
| @MetaField(META_VERIFY_SECONDARY_PHONE_DATE) | ||
| @Deprecated | ||
| /** | ||
| * @Deprecated should no longer be used, just here to avoid a db migration for the moment | ||
| */ | ||
| private Date verifySecondaryPhoneByDate; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stale persisted columns still actively written
The deprecated fields remain persisted and are still assigned in:
- default constructor (
secondaryPhoneVerified = true,verifySecondaryPhoneByDate = new Date();) fromV14()migration method.
If the intent is to freeze these columns, continuing to mutate them is misleading and bloats the on-disk diff set.
Either:
- Stop writing to them (simplest), or
- Remove them entirely and bump the DB version.
At minimum, add a comment explaining why they are still written while considered deprecated.
🤖 Prompt for AI Agents
In app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java
around lines 58 to 69, the deprecated persisted fields secondaryPhoneVerified
and verifySecondaryPhoneByDate are still being assigned values in the default
constructor and fromV14() migration method, which is misleading and bloats the
on-disk diff. To fix this, stop assigning values to these deprecated fields in
the constructor and migration method if you want to freeze them, or
alternatively remove these fields entirely and increment the database version.
If you choose to keep writing to them temporarily, add a clear comment
explaining why they are still assigned despite being deprecated.
pm-dimagi
left a comment
pm-dimagi
left a comment
There was a problem hiding this comment.
we can also clean the corresponding xml of standardHomeActivity
| super.onCreate(savedInstanceState); | ||
| handleRedirection(getIntent()); | ||
| updateBackButton(); | ||
| beginRegistration(); |
There was a problem hiding this comment.
@shubham1g5 Can we remove beginRegistration method also? Already app:startDestination="@id/personalid_phone_fragment" in nav graph.
Removes secondry phone number handling and mark it as deperecated.
Compilation should ensure good enough safety on code removal
Labels and Review