-
Notifications
You must be signed in to change notification settings - Fork 212
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
fix(fxa-payments-server): capture productId with PayPal payments #12566
fix(fxa-payments-server): capture productId with PayPal payments #12566
Conversation
FWIW The failed check is unrelated:
|
@caugner Thank you for the change. I had a quick look at the code and it looks good. There are just a few small things I was hoping you could address. I've listed them below.
|
@StaberindeZA How do I get push access to this repository?
Edit: Probably it would be good to update CONTRIBUTING.md with the new contribution instructions. That document has some dead links, mentions npm and grunt as well as forking, but not squashing. |
Because: * PayPal payment events were missing the productId values. This commit: * Passes the selected plan to Amplitude via the PayPalButton. Closes #12432
@StaberindeZA I have now squashed, and updated the commit message according to the contribution guidelines. |
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.
LGTM and CI passed thanks to Barry's DNM PR. Thank you Claas for the patch.
@annasob Have these changes already landed on production? Asking, because I'm not yet seeing a productId on PayPal subscription events (https://mozilla.cloud.looker.com/dashboards/596). |
Train 231 will be deployed tomorrow starting at 1pm EST |
Because
This pull request
Issue that this pull request solves
Closes: #12432
Checklist
Put an
x
in the boxes that applyScreenshots (Optional)
n/a
Other information (Optional)
n/a