-
Notifications
You must be signed in to change notification settings - Fork 619
Add more details to order email #8173
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Yopi
left a comment
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.
A few small comments, but otherwise nice work!
| const formattedAddress = | ||
| addressParts.length > 0 ? addressParts.join(', ') : null | ||
|
|
||
| const orderUrl = `https://polar.sh/dashboard/${organization_slug}/sales/${order_id}` |
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 think we send emails in both environments, in which case this needs to handle sandbox / prod
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.
Nice catch!
| class MaintainerNewProductSaleNotificationPayload(NotificationPayloadBase): | ||
| customer_name: str | ||
| customer_email: str | ||
| customer_name: str | None = None |
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.
Won't we still always have a customer_name? So this should just be : str
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.
Ah, or was the previous guarantee based on that we were passing in the customer_email?
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.
Ah, or was the previous guarantee based on that we were passing in the customer_email?
Exactly!
| order_date=order.created_at.isoformat(), | ||
| organization_name=organization.name, | ||
| organization_slug=organization.slug, | ||
| billing_reason=order.billing_reason, |
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 think this is typed as a non-str (OrderBillingReasonInternal), so you should be able to pass through the enum instead of a string
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.
Good call!
|
|
||
| @computed_field | ||
| def formatted_billing_reason(self) -> str: | ||
| reasons = { |
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.
Passing through the enum (as per the other comment) will make this a bit more complete. I think you can even get an exhaustive switch in that case (if you opt to not use a dict)
5d181b9 to
491018f
Compare
📦 Next.js Bundle Analysis for webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
491018f to
457b5f2
Compare
📦 Next.js Bundle Analysis for webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
457b5f2 to
e2dc6c8
Compare
📦 Next.js Bundle Analysis for webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
This reverts commit 6d26294.
📋 Summary
Related Issue: Fixes #7984
🎯 What
This PR will add some more data to the order confirmation email to give our users a quick glance of the purchase that has been made:
I also took the opportunity to redesign the email a bit.
🖼️ Screenshots/Recordings