-
Notifications
You must be signed in to change notification settings - Fork 180
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(app, app-shell, app-shell-odd): remove analytics modals and migrate analytics to default to on #15505
Conversation
expect(result).toEqual(MOCK_CONFIG_V22) | ||
}) | ||
it('should keep version 22', () => { | ||
const v21Config = MOCK_CONFIG_V22 |
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.
const v21Config = MOCK_CONFIG_V22 | |
const v22Config = MOCK_CONFIG_V22 |
}) | ||
it('should keep version 22', () => { | ||
const v21Config = MOCK_CONFIG_V22 | ||
const result = migrate(v21Config) |
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.
const result = migrate(v21Config) | |
const result = migrate(v22Config) |
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.
oops i forgot to push these changes before i merged... i will do this in another PR
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.
the changes look good to me.
Overview
This PR migrates the desktop app and ODD's analytics config to default to on. It also removes the
seenOptIn
config value since it is no longer used. Finally, it removes the app analytics modals that show up when you open the desktop app/ODD for the first time.closes AUTH-529, AUTH-530, AUTH-532, AUTH-533
Test Plan
This one is sort of hard to test — make sure the code looks good and that the migration does what you'd expect. We'll just have to make sure to battle test this one in QA cc @nusrat813 and @y3rsh
Changelog
Review requests
See test plan
Risk assessment
Medium