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

GH-2072: Add subscription interval to ping params and change UpgradePlan CTA link params #586

Merged
merged 8 commits into from Jul 27, 2020

Conversation

@benstrumeyer
Copy link
Contributor

@benstrumeyer benstrumeyer commented Jul 27, 2020

  • 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?
  • Add subscription interval parameter to all pings, taken from the account conf variable
  • 0 if the user is a free user or not signed in
  • 1 if monthly
  • 2 if yearly

The login ping will send a 0 regardless of subscription status. All other pings without refreshing the panel will send the updated value. Let me know if there is a way to fix this

  • Change CTA param in UpgradePlanView from subscription_interval to interval to match the query param in checkout-web

Ticket: https://ghostery.atlassian.net/browse/GH-2072

@benstrumeyer benstrumeyer added this to the 8.5.2 milestone Jul 27, 2020
@benstrumeyer benstrumeyer requested a review from wlycdgr Jul 27, 2020
@benstrumeyer benstrumeyer requested a review from jsignanini as a code owner Jul 27, 2020
@benstrumeyer benstrumeyer self-assigned this Jul 27, 2020
Copy link
Member

@wlycdgr wlycdgr left a comment

Re: the login pin always sending 0 for si - looks like we are sending the sign_in ping in the account.login message handler in background before we actually call account.login. Moving the ping into a finally handler might do the trick.

app/hub/Views/UpgradePlanView/UpgradePlanView.jsx Outdated Show resolved Hide resolved
app/hub/Views/UpgradePlanView/UpgradePlanView.jsx Outdated Show resolved Hide resolved
app/hub/Views/UpgradePlanView/UpgradePlanView.jsx Outdated Show resolved Hide resolved
src/classes/Metrics.js Outdated Show resolved Hide resolved
@benstrumeyer benstrumeyer requested a review from zarembsky as a code owner Jul 27, 2020
@christophertino christophertino merged commit 40ba358 into develop Jul 27, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@christophertino christophertino deleted the metrics branch Jul 27, 2020
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

3 participants