-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: adds "data collection for marketing" toggles #24605
Conversation
…sk/metamask-extension into jb-opt-in-metrics
…k-extension into jb-opt-in-metrics
…mask-extension into jb-opt-in-metrics
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NidhiKJha you will need to render the new onboarding screen, right now it's hidden through the privacy policy date. Just make the |
@@ -162,6 +163,12 @@ export default function reduceMetamask(state = initialState, action) { | |||
participateInMetaMetrics: action.value, | |||
}; | |||
|
|||
case actionConstants.SET_DATA_COLLECTION_FOR_MARKETING: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for? Generally we shouldn't adding anything to this Redux slice, it's meant as a mirror of the background state. The actions here other than the background state update action are all examples of technical debt that we have not cleaned up yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to mirror the implementation of SET_PARTICIPATE_IN_METAMETRICS. We are adding the marketing toggle flag to the state as well (dataCollectionForMarketing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a misunderstanding. SET_PARTICIPATE_IN_METAMETRICS
shouldn't be here either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are good points, and these changes will be postponed for a later date given our time constraints. Both SET_PARTICIPATE_IN_METAMETRICS and SET_DATA_COLLECTION_FOR_MARKETING should be deleted from the redux state.
Builds ready [e27aef2]
Page Load Metrics (49 ± 4 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #24605 +/- ##
===========================================
- Coverage 65.64% 65.60% -0.04%
===========================================
Files 1362 1362
Lines 54189 54265 +76
Branches 14112 14127 +15
===========================================
+ Hits 35572 35600 +28
- Misses 18617 18665 +48 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking as RC until Mark's item is addressed: #24605 (review)
Builds ready [0dc9878]
Page Load Metrics (46 ± 5 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I tested all combinations successfully.
- New user:
- metrics off, marketing off
- metrics on, marketing off
- metrics on, marketing on
- existing user:
- metrics off (don't see modal)
- metrics on, marketing off
- metrics on, marketing on
Just make sure we resolve this slack thread to be sure nothing is broken:
https://consensys.slack.com/archives/C06VBTL358A/p1718150795110559
Missing release label release-11.16.12 on PR. Adding release label release-11.16.12 on PR and removing other release labels(release-12.1.0), as PR was cherry-picked in branch 11.16.12. |
Adds data collection for marketing toggles (and toasts/warnings) on:
Description
Related issues
Fixes:
https://github.com/MetaMask/MetaMask-planning/issues/2437
https://github.com/MetaMask/MetaMask-planning/issues/2438
https://github.com/MetaMask/MetaMask-planning/issues/2526
Manual testing steps
Onboarding checkbox:
Make the
metametrics.js
to returnrenderOnboarding
instead ofrenderLegacyOnboarding
Security tab:
true
, the "Participate in MetaMetrics" toggle should turn totrue
false
, "Data collection for marketing" should be set tofalse
true
and "Data collection for marketing" istrue
, and the latter is set tofalse
, a warning message should appear.Toast:
An already onboarded user will have the "dataCollectionForMarketing" value as
null
(neithertrue
orfalse
). This will trigger the toast.true
.false
.All of these actions should trigger subsequent Segment events.
Screenshots/Recordings
Before
After
demo2.mp4
Pre-merge author checklist
Pre-merge reviewer checklist