Skip to content

CCCT-2886 Reword The Skip-Email Confirmation Dialog - #3935

Merged
shubham1g5 merged 2 commits into
masterfrom
ccct-2886_skip_dialog_email
Oct 2, 2026
Merged

shubham1g5 merged 2 commits into
masterfrom
ccct-2886_skip_dialog_email

Conversation

@shubham1g5

@shubham1g5 shubham1g5 commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

CCCT-2886

Product Description

The skip-email confirmation dialog now explains why an email protects the account, with "Skip" and "Add Email" buttons replacing Yes/No.

WhatsApp Image 2026-10-01 at 16 17 59

Safety Assurance

Safety story

Text updates only
Locally Tested

The dialog now explains why an email protects the account, with Skip /
Add Email buttons.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@shubham1g5 shubham1g5 self-assigned this Oct 1, 2026
@shubham1g5

Copy link
Copy Markdown
Contributor Author

Suggested Review Order

  • app/res/values/strings.xml — new English copy and button labels
  • app/src/org/commcare/fragments/personalId/PersonalIdEmailFragment.kt — dialog switches to the new button strings
  • app/unit-tests/src/org/commcare/fragments/personalId/PersonalIdEmailFragmentTest.kt — covers dialog copy and both button outcomes
  • app/res/values-*/strings.xml — translations of the same strings

@shubham1g5
shubham1g5 marked this pull request as ready for review October 1, 2026 09:36
@shubham1g5
shubham1g5 requested review from a team and Jignesh-dimagi and removed request for a team October 1, 2026 09:37
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: dbbef91d-0545-41b8-ae36-447a525a9a6b

📥 Commits

Reviewing files that changed from the base of the PR and between 2105da9 and 33ab706.

📒 Files selected for processing (12)
  • app/res/values-es/strings.xml
  • app/res/values-fr/strings.xml
  • app/res/values-ha/strings.xml
  • app/res/values-hi/strings.xml
  • app/res/values-lt/strings.xml
  • app/res/values-no/strings.xml
  • app/res/values-pt/strings.xml
  • app/res/values-sw/strings.xml
  • app/res/values-ti/strings.xml
  • app/res/values/strings.xml
  • app/src/org/commcare/fragments/personalId/PersonalIdEmailFragment.kt
  • app/unit-tests/src/org/commcare/fragments/personalId/PersonalIdEmailFragmentTest.kt

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The email-skip confirmation now explains account-security and recovery benefits and uses separate skip and add-email labels. The dialog callbacks remain unchanged. Tests verify the dialog content, the add-email action, and skip navigation in the registration workflow.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Suggested reviewers: jignesh-dimagi

Merge Risk: ⚪ Minimal · up to 33ab7

The dialog explains email benefits and offers clearer Skip and Add Email choices without changing navigation behavior. No actionable merge-blocking issue remains; merge after normal checks.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (10 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description explains the user-facing change and includes a brief safety story, but it omits the Technical Summary, automated test coverage, and Labels and Review sections required by the template. Add a Technical Summary with the rationale and design decisions. Document the automated tests and the conditions they cover. Complete the Labels and Review checklist. Expand the safety story to address existing data impact and the change’s …
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: rewording the skip-email confirmation dialog.
Full details: Docstring Coverage

Explanation

Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (10 skipped: 10 unsupported.)

Full details: Description check

Resolution

Add a Technical Summary with the rationale and design decisions. Document the automated tests and the conditions they cover. Complete the Labels and Review checklist. Expand the safety story to address existing data impact and the change’s risk.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Jignesh-dimagi
Jignesh-dimagi previously approved these changes Oct 1, 2026
@shubham1g5 shubham1g5 added the skip-integration-tests Skip android tests. label Oct 1, 2026
@Jignesh-dimagi

Copy link
Copy Markdown
Contributor

@shubham1g5 I have a follow-up question regarding the UI: Should the 'Add Email' button be highlighted as the primary CTA instead of the 'Skip' button?

@codecov

codecov Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 35.39%. Comparing base (4be305a) to head (bf84d9c).
⚠️ Report is 55 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3935      +/-   ##
============================================
+ Coverage     35.38%   35.39%   +0.01%     
- Complexity     6499     6503       +4     
============================================
  Files          1035     1036       +1     
  Lines         61398    61467      +69     
  Branches       7453     7459       +6     
============================================
+ Hits          21726    21758      +32     
- Misses        37156    37191      +35     
- Partials       2516     2518       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

… dialog [AI]

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@shubham1g5

Copy link
Copy Markdown
Contributor Author

@Jignesh-dimagi Agree, made the change

@shubham1g5
shubham1g5 merged commit 687f1af into master Oct 2, 2026
10 checks passed
@shubham1g5
shubham1g5 deleted the ccct-2886_skip_dialog_email branch October 2, 2026 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-integration-tests Skip android tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants