CCCT-2204 - Add email - tech spec, implementation plan and its tickets#3655
CCCT-2204 - Add email - tech spec, implementation plan and its tickets#3655Jignesh-dimagi wants to merge 2 commits intomasterfrom
Conversation
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughA new documentation file specifying an end-to-end implementation plan was added to define the integration of optional email-entry and OTP verification into the PersonalID signup and recovery flow. The plan covers database schema changes for email fields, session-data management, Retrofit API endpoint additions for OTP operations, new UI fragments and layouts, navigation graph updates, and persistence logic. It also details a legacy-user post-login email offer dialog with frequency capping tracked via counters and timestamps, alongside associated unit tests. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/superpowers-add-email-to-personalid-signup.md`:
- Around line 16-29: The Markdown tables in this document are missing required
blank-line padding around them, causing MD058 lint errors; add a single blank
line before and after the top table that begins with "| File | Purpose |" (the
file list snapshot block referencing ConnectUserRecordV24.java,
PersonalIdEmailFragment.kt, etc.) and likewise add a blank line before and after
the "Files to Modify" table so both tables have one empty line above and below
to satisfy the linter.
- Around line 1949-1960: The fenced dependency-order block (the diagram starting
with "Ticket 1 (DB)" and containing Ticket 2/3/4/5/6/7/8) lacks a language tag
and triggers MD040; update the opening fence from ``` to ```text so the block
becomes a text code fence (e.g., change the line before "Ticket 1 (DB)" to
```text) to improve readability and satisfy the linter.
- Around line 194-210: The fromV24 migration in ConnectUserRecord drops
persisted fields (e.g., pin, verifySecondaryPhoneByDate) and forces
secondaryPhoneVerified=true, risking silent data loss; update
ConnectUserRecord.fromV24 to copy pin and verifySecondaryPhoneByDate from the
ConnectUserRecordV24 instance and preserve the original secondaryPhoneVerified
value instead of hardcoding it, and add corresponding getter methods (e.g.,
getPin(), getVerifySecondaryPhoneByDate(), getSecondaryPhoneVerified()) to
ConnectUserRecordV24 so the migration can read those deprecated fields safely.
- Around line 77-79: Replace the incorrect test field names emailOfferDate1 and
emailOfferDate2 with the schema fields emailOfferCount and lastEmailOfferDate:
update assertions so lastEmailOfferDate is asserted null
(assertNull(new.lastEmailOfferDate)) and emailOfferCount is asserted to its
initial value (e.g., assertEquals(0, new.emailOfferCount) or the project's
chosen default) to match the proposed schema and avoid compilation failures.
- Line 1094: The resend flow currently force-unwraps personalIdSessionData.email
with personalIdSessionData.email!! when calling
sendEmailOtpCall(requireActivity(), ...), which can crash if the session was
restored without an email; update the handler that triggers sendEmailOtpCall
(the resend logic around sendEmailOtpCall and requireActivity()) to first
null-check personalIdSessionData.email, handle the null case by
logging/reporting a recoverable error and navigating the user to an email-entry
or recovery screen (or showing a retryable error UI), and only call
sendEmailOtpCall when the email is non-null to avoid crashes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 539be715-cf45-4823-ad99-486d01e2524d
📒 Files selected for processing (1)
docs/superpowers/plans/superpowers-add-email-to-personalid-signup.md
| | File | Purpose | | ||
| |------|---------| | ||
| | `app/src/org/commcare/android/database/connect/models/ConnectUserRecordV24.java` | Snapshot of v24 ConnectUserRecord (for DB migration) | | ||
| | `app/src/org/commcare/fragments/personalId/PersonalIdEmailFragment.kt` | Email entry UI fragment | | ||
| | `app/src/org/commcare/fragments/personalId/PersonalIdEmailVerificationFragment.kt` | Email OTP verification UI fragment | | ||
| | `app/res/layout/fragment_personalid_email.xml` | Layout for email entry screen | | ||
| | `app/res/layout/fragment_personalid_email_verification.xml` | Layout for email OTP screen | | ||
| | `app/src/org/commcare/connect/network/connectId/parser/SendEmailOtpResponseParser.kt` | Parses send-email-OTP server response | | ||
| | `app/src/org/commcare/connect/network/connectId/parser/VerifyEmailOtpResponseParser.kt` | Parses verify-email-OTP server response | | ||
| | `app/unit-tests/src/org/commcare/connect/PersonalIdEmailOfferTest.kt` | Unit tests for evaluateEmailOffer logic | | ||
| | `app/unit-tests/src/org/commcare/connect/PersonalIdEmailValidationTest.kt` | Unit tests for client-side email format validation | | ||
|
|
||
| ### Files to Modify | ||
| | File | Change | |
There was a problem hiding this comment.
Resolve markdown table lint violations (MD058)
Line 16 and Line 29 table blocks are missing required blank-line padding. This is minor, but it will keep markdownlint noisy for this doc.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 16-16: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 29-29: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/superpowers-add-email-to-personalid-signup.md` around
lines 16 - 29, The Markdown tables in this document are missing required
blank-line padding around them, causing MD058 lint errors; add a single blank
line before and after the top table that begins with "| File | Purpose |" (the
file list snapshot block referencing ConnectUserRecordV24.java,
PersonalIdEmailFragment.kt, etc.) and likewise add a blank line before and after
the "Files to Modify" table so both tables have one empty line above and below
to satisfy the linter.
| assertNull(new.emailOfferDate1) | ||
| assertNull(new.emailOfferDate2) | ||
| } |
There was a problem hiding this comment.
Fix migration test field names to match the proposed schema
Line 77 and Line 78 reference emailOfferDate1 / emailOfferDate2, but the plan defines emailOfferCount and lastEmailOfferDate. Following this as written will produce a compile-failing test and mislead implementers.
Suggested doc fix
- assertNull(new.emailOfferDate1)
- assertNull(new.emailOfferDate2)
+ assertEquals(0, new.emailOfferCount)
+ assertNull(new.lastEmailOfferDate)📝 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.
| assertNull(new.emailOfferDate1) | |
| assertNull(new.emailOfferDate2) | |
| } | |
| assertEquals(0, new.emailOfferCount) | |
| assertNull(new.lastEmailOfferDate) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/superpowers-add-email-to-personalid-signup.md` around
lines 77 - 79, Replace the incorrect test field names emailOfferDate1 and
emailOfferDate2 with the schema fields emailOfferCount and lastEmailOfferDate:
update assertions so lastEmailOfferDate is asserted null
(assertNull(new.lastEmailOfferDate)) and emailOfferCount is asserted to its
initial value (e.g., assertEquals(0, new.emailOfferCount) or the project's
chosen default) to match the proposed schema and avoid compilation failures.
| public static ConnectUserRecord fromV24(ConnectUserRecordV24 oldRecord) { | ||
| ConnectUserRecord newRecord = new ConnectUserRecord(); | ||
| 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 = oldRecord.getRequiredLock(); | ||
| newRecord.hasConnectAccess = oldRecord.hasConnectAccess(); | ||
| // email defaults to null, emailVerified to false, emailOfferCount to 0, lastEmailOfferDate to null |
There was a problem hiding this comment.
fromV24() currently drops existing persisted data
The migration factory copies many fields but omits at least pin and verifySecondaryPhoneByDate (and hardcodes secondaryPhoneVerified). That can silently lose user data during v24→v25 migration.
Suggested doc fix
public static ConnectUserRecord fromV24(ConnectUserRecordV24 oldRecord) {
ConnectUserRecord newRecord = new ConnectUserRecord();
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.pin = oldRecord.getPin();
+ newRecord.secondaryPhoneVerified = oldRecord.isSecondaryPhoneVerified();
+ newRecord.verifySecondaryPhoneByDate = oldRecord.getVerifySecondaryPhoneByDate();
newRecord.photo = oldRecord.getPhoto();
newRecord.isDemo = oldRecord.isDemo();
newRecord.requiredLock = oldRecord.getRequiredLock();
newRecord.hasConnectAccess = oldRecord.hasConnectAccess();
// email defaults to null, emailVerified to false, emailOfferCount to 0, lastEmailOfferDate to null
return newRecord;
}Also add corresponding getters in ConnectUserRecordV24 for the deprecated fields if they’re used in migration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/superpowers-add-email-to-personalid-signup.md` around
lines 194 - 210, The fromV24 migration in ConnectUserRecord drops persisted
fields (e.g., pin, verifySecondaryPhoneByDate) and forces
secondaryPhoneVerified=true, risking silent data loss; update
ConnectUserRecord.fromV24 to copy pin and verifySecondaryPhoneByDate from the
ConnectUserRecordV24 instance and preserve the original secondaryPhoneVerified
value instead of hardcoding it, and add corresponding getter methods (e.g.,
getPin(), getVerifySecondaryPhoneByDate(), getSecondaryPhoneVerified()) to
ConnectUserRecordV24 so the migration can read those deprecated fields safely.
| ) { | ||
| showError(PersonalIdOrConnectApiErrorHandler.handle(requireActivity(), failureCode, t)) | ||
| } | ||
| }.sendEmailOtpCall(requireActivity(), personalIdSessionData.email!!, personalIdSessionData) |
There was a problem hiding this comment.
Avoid force-unwrapping nullable session email in resend flow
Line 1094 uses personalIdSessionData.email!!; if process state is restored without that value, this becomes a crash path. Add a null guard and surface a recoverable error/navigation fallback.
Suggested doc fix
- }.sendEmailOtpCall(requireActivity(), personalIdSessionData.email!!, personalIdSessionData)
+ val email = personalIdSessionData.email
+ if (email.isNullOrBlank()) {
+ showError(getString(R.string.personalid_email_invalid_format))
+ return
+ }
+ }.sendEmailOtpCall(requireActivity(), email, personalIdSessionData)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/superpowers-add-email-to-personalid-signup.md` at line
1094, The resend flow currently force-unwraps personalIdSessionData.email with
personalIdSessionData.email!! when calling sendEmailOtpCall(requireActivity(),
...), which can crash if the session was restored without an email; update the
handler that triggers sendEmailOtpCall (the resend logic around sendEmailOtpCall
and requireActivity()) to first null-check personalIdSessionData.email, handle
the null case by logging/reporting a recoverable error and navigating the user
to an email-entry or recovery screen (or showing a retryable error UI), and only
call sendEmailOtpCall when the email is non-null to avoid crashes.
| ``` | ||
| Ticket 1 (DB) | ||
| └── Ticket 2 (API/Session) | ||
| ├── Ticket 3 (Email Entry Fragment) | ||
| │ ├── Ticket 4 (Validation + Keyboard) | ||
| │ └── Ticket 7 (Navigation) | ||
| ├── Ticket 5 (OTP Fragment) | ||
| │ ├── Ticket 4 (Validation + Keyboard) | ||
| │ └── Ticket 7 (Navigation) | ||
| └── Ticket 6 (Signup Completion) | ||
| Ticket 7 (Navigation) ──► Ticket 8 (Legacy Prompt) | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced dependency-order block (MD040)
The fenced block starting at Line 1949 has no language tag. Use text for readability and lint compliance.
Suggested doc fix
-```
+```text
Ticket 1 (DB)
└── Ticket 2 (API/Session)
├── Ticket 3 (Email Entry Fragment)
│ ├── Ticket 4 (Validation + Keyboard)
│ └── Ticket 7 (Navigation)
├── Ticket 5 (OTP Fragment)
│ ├── Ticket 4 (Validation + Keyboard)
│ └── Ticket 7 (Navigation)
└── Ticket 6 (Signup Completion)
Ticket 7 (Navigation) ──► Ticket 8 (Legacy Prompt)</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 1949-1949: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/superpowers-add-email-to-personalid-signup.md` around
lines 1949 - 1960, The fenced dependency-order block (the diagram starting with
"Ticket 1 (DB)" and containing Ticket 2/3/4/5/6/7/8) lacks a language tag and
triggers MD040; update the opening fence from ``` to ```text so the block
becomes a text code fence (e.g., change the line before "Ticket 1 (DB)" to
```text) to improve readability and satisfy the linter.
@Jignesh-dimagi are you able to add a web ticket for these 2 in the current epic and define the payload and request data for these in that ticket. |
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Looks good overall, Think we need to create implementation tickets on this and ensure smaller PRs. Probably a good functional split of tickets on this work would be -
Mobile:
- DB changes to store email
- network layer changes
- Email Page along with validations and keyboard handling
- Email verification page along with account creation and navigation flow
- Email Prompts reminders
Web:
- Probably create a spec ticket for web to dig deeper on this. We should provide the API payloads for this and ask web to spec out the
- API implementations
- Actual Implmentation to generate and send the otp to user email.
| // createAndSaveConnectUser() will read this to initialise emailOfferCount = 1 | ||
| // so the post-login dialog does not fire again immediately. | ||
| personalIdSessionData.emailSkippedDuringSignup = true | ||
| if (isLegacyFlow) { |
There was a problem hiding this comment.
Why is this getting skipped for legacy users ? If so, why do even open this fragment for these users ?
There was a problem hiding this comment.
It's like the user has agreed to open the email fragment from the reminder prompt for the legacy user but doesn't want to complete it, so they pressed skip.
There was a problem hiding this comment.
where are they pressing skip in this workflow ?
| private fun createEmailWatcher() = object : TextWatcher { | ||
| override fun beforeTextChanged(s: CharSequence?, start: Int, count: Int, after: Int) {} | ||
| override fun onTextChanged(s: CharSequence?, start: Int, before: Int, count: Int) { | ||
| enableContinueButton(isValidEmail(s?.toString())) | ||
| } | ||
| override fun afterTextChanged(s: Editable?) {} | ||
| } |
There was a problem hiding this comment.
can we define error state here for when the email is not valid, think we would like to show an error text to the user in those cases saying "email is invalid" ?
There was a problem hiding this comment.
@shubham1g5 You mean the app should show the error message when the user is still typing?
There was a problem hiding this comment.
yeah that's what I meant, thoughts ?
There was a problem hiding this comment.
How about adding a debounce here that only shows the error after the user stopped typing for something like 3 seconds? So that way the message is not intrusive when the user knows what to type, but also serves as a hint if the user gets stuck
| personalIdSessionData.getInvitedUser()); | ||
| // Carry over email verification state from the signup flow | ||
| user.setEmail(personalIdSessionData.getEmail()); | ||
| user.setEmailVerified(personalIdSessionData.getEmailVerified()); |
There was a problem hiding this comment.
How about setting these in user preferences instead given these are programmatical states and resets on every login
There was a problem hiding this comment.
@shubham1g5 It's easier to handle when the user logs out instead of clearing individual preferences.
There was a problem hiding this comment.
are not we resetting these properties anyway individually on re-login ?
Also you can have a new user specific preference class PersonalIDPreferences that we clear as a whole on user signing out.
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Just to confirm, this plan is not going to be merged into master right?
3 participants
Product Description
Users are presented with a screen to add / validate their email during the sign-up and recovery flow. These screens are optional, and users can skip them. For the legacy users, they are prompt to add the emails.
Technical Summary
### CCCT-2204
This task was taken during AI week to have a tech spec and implementation plan for adding email during the user sign-up / recovery flow.
Design Doc here.
This plan was created with Claude code after working with it in iterative mode. Enough context was provided to ensure the implementation plan is realistic, and if wanted, we can ask Claude to write code to directly implement it without the dev writing any code.
This plan has also created the sub-tickets to break down the task. We can also ask Claude to work on each ticket and give a corresponding PR.
The server-side endpoints
/users/send_email_otpand/users/verify_email_otpneed to be confirmed with the backend team before finalising.Safety story
Unit tests are included in the implementation plan
Labels and Review