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
feat(auth-server): Modify transactional emails to accomodate PayPal payments #7653
Conversation
00c4c8e
to
f144d5f
Compare
b6d8612
to
fe38349
Compare
: 'stripe'; | ||
} | ||
return 'not_chosen'; | ||
} |
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.
nit: this function doesn't really need to be in a class.
return options.fn(this); | ||
} | ||
return options.inverse(this); | ||
} |
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.
Consistency suggestion: using the pattern from orHelper
above we can write
function isEqualHelper() {
return reduceOp(arguments, (a, b) => a === b);
}
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 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.
🤦 Sorry about that. In that case, please delete that orHelper
dead code.
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.
ok so - I was mistaken! This helper is actually used here. I had been looking for {{#or}}
but the usage was just a bit different. I've updated ifEqualHelper to match.
packages/fxa-auth-server/lib/senders/templates/partials/paymentProvider.html
Outdated
Show resolved
Hide resolved
packages/fxa-auth-server/lib/senders/templates/subscriptionPaymentFailed.html
Show resolved
Hide resolved
@lesleyjanenorton I tried this locally and didn't get any emails when I paid with PayPal. It did work when I paid with Stripe. Is it working for you? I don't think it's directly related to this PR; I'm confused as to why I'm not even getting the welcome email though. |
@chenba Emails triggered by PayPal checkouts should be working now |
@@ -76,8 +76,8 @@ export class StripeWebhookHandler extends StripeHandler { | |||
await this.handleInvoiceOpenEvent(request, event); | |||
} | |||
break; | |||
case 'invoice.payment_succeeded': | |||
await this.handleInvoicePaymentSucceededEvent(request, event); | |||
case 'invoice.paid': |
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 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.
Looks good to me!
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.
WFM
Because
This pull request
extractInvoiceDetailsForEmail()
and relevant email handlers so thatpayment_provider
is available to the email templates.subscriptionPaymentFailed
: Changes "3 days" to "24 hours". Maybe we should say "1 day" instead? I don't know. How do we ensure string changes bubble up to l10n?Issue that this pull request solves
Closes: #7264 #7101
Checklist
Put an
x
in the boxes that applyScreenshots (Optional)
Proposed language change to reactivation email
Proposed language change in subscriptionPaymentFailed.
Needs UX: Should we say "1 day" instead of "24 hours" ?
subscriptionFirstInvoice when payment_method === "paypal"
subscriptionSubsequentInvoice when payment_method === "paypal"
Other information (Optional)
How do we get updated strings to l10n? These emails are localized but the plumbing is an area of great mystery to me.