-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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(882): Optimize metrics function usage during onboarding flow #20101
Conversation
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. |
*/ | ||
const onboardingChooseMetametricsOption = async (driver, optin = false) => { |
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.
Typo fix
* @returns {import('mockttp/dist/pluggable-admin').MockttpClientResponse[]} | ||
*/ | ||
async function getEventPayloads(driver, mockedEndpoints) { | ||
async function getEventPayloads(driver, mockedEndpoints, hasRequest = true) { |
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.
Expand method to test no request scenario as well
@@ -43,28 +44,6 @@ async function mockSegment(mockServer) { | |||
]; | |||
} | |||
|
|||
/** |
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.
Duplicated in helper.js
const FixtureBuilder = require('../fixture-builder'); | ||
|
||
describe('Segment metrics', function () { | ||
describe('Unlock wallet', function () { |
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.
Move to metrics
folder
}, | ||
); | ||
}); | ||
}); | ||
|
||
function assertBatchValue(mockRequest, assertedTitle, assertedPath) { |
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.
Refactor to make it more readable
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.
Nice work!
LGTM! |
Explanation
Only start
trackEvent
method onui/pages/onboarding-flow/welcome/welcome.js
when user opts in metrics track on onboarding.Screenshots/Screencaps
Before
Send metric events
before.mov
After
Will not send metric events if not opting in
after.mov
Manual Testing Steps
batch
request to send metricsPre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Board
label.In this case, a QA Engineer approval will be be required.