CCCT-2489 Add Workflow Parameter To PersonalID Continue Analytics#3770
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughA new Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3770 +/- ##
============================================
+ Coverage 25.94% 25.96% +0.01%
- Complexity 4406 4408 +2
============================================
Files 951 953 +2
Lines 57223 57366 +143
Branches 6817 6824 +7
============================================
+ Hits 14849 14893 +44
- Misses 40538 40640 +102
+ Partials 1836 1833 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| import org.junit.Assert.assertEquals | ||
| import org.junit.Test | ||
|
|
||
| class PersonalIdWorkflowTest { |
There was a problem hiding this comment.
nit: flagging that these tests are not achieving much given it's testing just switch case mapping and tests like this would add to overhead of changing this mapping.
There was a problem hiding this comment.
Yeah, it has been added for code coverage calculation purposes.
There was a problem hiding this comment.
Emphasizing that I don't think increasing code coverage by addition of meannigless tests should be our target as a team.
There was a problem hiding this comment.
I agree, but I don't think it is completely meaningless in this case, as the mapping needs to be protected from unintentional modifications across the app. Additionally, if we keep skipping these helper or mapper classes, we might miss testing small pieces of logic. Therefore, there is no harm in adding this test.
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Looks great! Just some nits
| * - [USER_PROMPT]: the user is acting on a prompt surfaced outside the config flow, | ||
| * e.g. the email-collection prompt shown to a logged-in user from | ||
| * {@code StandardHomeActivity} (EmailOfferHelper.checkEmailCollection). | ||
| * - [EDIT_PROFILE]: reserved for the future edit-profile feature. |
There was a problem hiding this comment.
Small nit: can we take the word "future" out of this just in case we forget to update it later
| * - [EDIT_PROFILE]: reserved for the future edit-profile feature. | |
| * - [EDIT_PROFILE]: reserved for the edit-profile feature. |
There was a problem hiding this comment.
Yeah, we can modify this comment once the edit profile feature becomes available.
| static final String PARAM_API_NEW_JOBS = "ccc_api_new_jobs"; | ||
| public static final String PERSONAL_ID_CONTINUE_CLICKED_INFO = | ||
| "personal_id_continue_button_clicked_info"; | ||
| public static final String PERSONAL_ID_WORKFLOW = "personalid_flow"; |
There was a problem hiding this comment.
Nit: personalid_ is redundant and we could use workflow instead
There was a problem hiding this comment.
There may be different workflows across the app, but since this one is for personalid, we appended it to the name.
| import org.junit.Test | ||
|
|
||
| class PersonalIdWorkflowTest { | ||
| // --- fromEmailWorkFlow --- |
There was a problem hiding this comment.
Nit: this comment does not add much value and can be deleted
4 participants
CCCT-2489
Technical Summary
Adds a
personalid_flowparameter to thepersonal_id_continue_clickedanalytics event to track the origin of the Continue button press.PersonalIdWorkflowenum (CONFIGURATION/USER_PROMPT/EDIT_PROFILE, the last reserved for the future edit-profile feature), threaded throughFirebaseAnalyticsUtil.reportPersonalIDContinueClickedas a required argument.CONFIGURATION; the email screens derive the value from their existingEmailWorkFlow(existing-user prompt →USER_PROMPT, registration/recovery →CONFIGURATION).PersonalIdPhoneVerificationFragmentandPersonalIdPhotoCaptureFragmentso they also report the workflow.Safety Assurance
Safety story
What gives me confidence:
fromEmailWorkFlowmapping is covered by unit tests.Automated test coverage
PersonalIdWorkflowTestcovers thefromEmailWorkFlowmapping for all threeEmailWorkFlowvalues (existing-user → user_prompt, registration/recovery → configuration).