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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 17, 2022
Merged

fix(auth): pass Product/Plan Metadata to Email Sender #12835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/fxa-auth-server/lib/payments/subscription-reminders.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ export class SubscriptionReminders {
// Using invoice prefix instead of plan to accomodate `yarn write-emails`.
invoiceTotalInCents: amount,
invoiceTotalCurrency: currency,
productMetadata: formattedSubscription.productMetadata,
}
);
await this.updateSentEmail(uid, emailParams);
Expand Down
3 changes: 2 additions & 1 deletion packages/fxa-auth-server/lib/senders/email.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2789,7 +2789,8 @@ module.exports = function (log, config, bounces) {
privacyNoticeDownloadURL = this.privacyUrl,
} = productDetailsFromPlan(
{
product_metadata: message.productMetadata,
product_metadata:
message.productMetadata || message.subscription?.productMetadata,
Comment on lines +2792 to +2793
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

},
determineLocale(message.acceptLanguage)
);
Expand Down
18 changes: 15 additions & 3 deletions packages/fxa-auth-server/test/local/payments/subscription-reminders.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,11 @@ describe('SubscriptionReminders', () => {
},
};
reminder.alreadySentEmail = sandbox.fake.resolves(false);
const account = { emails: [], email: 'testo@test.test', locale: 'NZ' };
const account = {
emails: [],
email: 'testo@test.test',
locale: 'NZ',
};
reminder.db.account = sandbox.fake.resolves(account);
mockLog.info = sandbox.fake.returns({});
mockStripeHelper.formatSubscriptionForEmail = sandbox.fake.resolves({});
Expand All @@ -264,8 +268,15 @@ describe('SubscriptionReminders', () => {
interval_count: longPlan1.interval_count,
interval: longPlan1.interval,
});
const formattedSubscription = {
id: 'subscriptionId',
productMetadata: {
privacyUrl: 'http://privacy',
termsOfServiceUrl: 'http://tos',
},
};
reminder.mailer.sendSubscriptionRenewalReminderEmail =
sandbox.fake.resolves({});
sandbox.fake.resolves(formattedSubscription);
reminder.updateSentEmail = sandbox.fake.resolves({});
const realDateNow = Date.now.bind(global.Date);
Date.now = sinon.fake(() => MOCK_DATETIME_MS);
Expand Down Expand Up @@ -305,12 +316,13 @@ describe('SubscriptionReminders', () => {
acceptLanguage: account.locale,
uid: 'uid',
email: 'testo@test.test',
subscription: {},
subscription: formattedSubscription,
reminderLength: 14,
planIntervalCount: 1,
planInterval: 'month',
invoiceTotalInCents: 499,
invoiceTotalCurrency: 'usd',
productMetadata: formattedSubscription.productMetadata,
}
);
sinon.assert.calledOnceWithExactly(
Expand Down