-
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(auth): pass Product/Plan Metadata to Email Sender #12835
Conversation
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.
R+WC (with changes). Thanks Ivo. This looks like it will fix the specific issue reported by QA. I see it's marked as a draft, but I reviewed it anyway just in case given we're tagging shortly and you requested fxa-devs review.
Before merging, please:
- squash these into a single commit,
- revert one of the changes (I have a comment in line),
- Check the applicable boxes on the PR checklist (none are checked currently)
It'd be nice to prevent this type of error from happening in the future for any email, but I think that more comprehensive fix would be covered by (#10110).
packages/fxa-auth-server/test/local/payments/subscription-reminders.js
Outdated
Show resolved
Hide resolved
product_metadata: | ||
message.productMetadata || message.subscription?.productMetadata, |
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.
Since we're now explicitly passing a message.productMetadata
for this email, why not leave this line unchanged?
Admittedly we're passing in the same information as part of the subscription.productMetadata
in this one email's case at least as well, which isn't ideal.
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.
There are a few other instances where we call a method to format the subscription for email which also calls a helper method that merges the product and plan metadata - i figured adding this would also cover those other cases were we aren't explicitly pulling out the productMetadata
property and passing it the email sender method. If you think that is outside the scope of this I can remove it.
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.
Thanks for clarifying; if it's broken, might as well fix it here if it's easy to do.
Though now that I look at this again, perhaps a more comprehensive approach would be to existence check termsOfServiceDownloadURL
and privacyNoticeDownloadURL
and throw an error if they don't exist in Mailer.prototype._generateLinks
.
This would cover any scenario where we send an email and neglect to pass in those required values. What do you think?
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.
Note: these links are for the 'subscriptions'
template email footers specifically, so I suppose we'd have to conditionally throw to go this route.
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.
I'd like to avoid any conditional logic around throwing an error if we would have to look at the calling method name or template name - with how we have all the methods wrapped, I suspect that is how we would have to go about it. This seems like a bit of a smell where in order to be as DRY as possible we are creating bugs that we then have to add checks to catch.
I think the ticket you linked above to remove the default product config might be the place to take a look at how to enforce that the URLs are provided and remove the default values since they are no longer useful. We should also take a look at this line again then since we could coalesce around a single convention for how this information should be provided to the email sending method (i.e. do we always just look for message.subscription.productMetadata
or does the calling method figure out what should be provided for productMetadata
?)
If possible, I'd rather leave this line in since it just adds one more check before we go with the default values. Let me know if we can proceed as is.
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.
Thanks for your thoughts; sure sounds good to me. Yes we can go for a more comprehensive solution in the ticket to remove the defaults entirely.
5b3b078
to
a666436
Compare
Because: * The email sender defaults to the VPN's privacy and ToS links in the email footer, we need to pass the product/plan specific metadata This commit: * passes the metadata from the formattedSubscription object to the message received by the email sender Closes #12655
a666436
to
2cae023
Compare
Because:
This commit:
Closes #12655
Checklist
Put an
x
in the boxes that apply