Skip to content
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

setup_number ping parameter #669

Merged
merged 16 commits into from Jan 29, 2021
Merged

Conversation

@benstrumeyer
Copy link
Contributor

@benstrumeyer benstrumeyer commented Jan 27, 2021

  • Have you followed the guidelines in CONTRIBUTING.md?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do?
  • Does your submission pass tests?
  • Did you lint your code prior to submission?
@benstrumeyer benstrumeyer added this to the 8.5.5 milestone Jan 27, 2021
@benstrumeyer benstrumeyer requested a review from wlycdgr Jan 27, 2021
@benstrumeyer benstrumeyer self-assigned this Jan 27, 2021
@benstrumeyer benstrumeyer requested review from christophertino and ghostery/extension as code owners Jan 27, 2021
Copy link
Member

@wlycdgr wlycdgr left a comment

Lemme know if you have any questions!

@@ -524,6 +524,7 @@ function handleGhosteryHub(name, message, callback) {
const origin = message.origin || '';
if (origin === 'onboarding') {
conf.setup_step = message.setup_step;
conf.setup_number = message.setup_number;

This comment has been minimized.

@wlycdgr

wlycdgr Jan 28, 2021
Member

Even though we use the same Metrics qs param for both, can we store this setup_number in a new and separate conf prop to make the whole thing a bit less dirty? Maybe dawn_setup_number. Then in Metrics we would buildQueryPair with one or the other depending on which ping we were sending

This comment has been minimized.

@benstrumeyer

benstrumeyer Jan 28, 2021
Author Contributor

Good idea, changed to dawn_setup_number and made a function that cases on gb_onboarding

src/classes/Metrics.js Show resolved Hide resolved
@benstrumeyer benstrumeyer requested a review from wlycdgr Jan 29, 2021
Copy link
Member

@wlycdgr wlycdgr left a comment

Looks good

@wlycdgr wlycdgr merged commit 13b92f5 into ghostery-browser-intro-hub Jan 29, 2021
@benstrumeyer benstrumeyer deleted the setup-number-ping-param branch Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants