-
Notifications
You must be signed in to change notification settings - Fork 51
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(ras-acc): add donation cancellation email template #3056
feat(ras-acc): add donation cancellation email template #3056
Conversation
a7c8b2f
to
ef078c6
Compare
includes/reader-revenue/woocommerce/class-woocommerce-connection.php
Outdated
Show resolved
Hide resolved
6e3d8b2
to
e5b4422
Compare
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.
Pre-approving pending a small tweak.
: __( | ||
'This email is not active. The default receipt will be used.', | ||
'newspack-plugin' | ||
); |
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.
This should rather be the other way around – if the type is receipt, use the message about the default receipt. Otherwise, use the generic message.
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.
This should rather be the other way around – if the type is receipt, use the message about the default receipt. Otherwise, use the generic message.
Do you mean this should be changed to:
const notification =
email.type === 'receipt'
? __(
'This email is not active. The default receipt will be used.',
'newspack-plugin'
)
: __( 'This email is not active.', 'newspack-plugin' );
If so, this will have the same result as the current changes. On my end, it looks like the messaging is correct on the frontend as well:
Or did you mean something else?
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.
That's what I meant. The result is the same, but the code is more logical.
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 see. Happy to make the change if you feel that way! Done in 052255f
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.
It's not about feelings, it's logic & readability:
type === 'receipt' ? notification_about_receipt : generic_notification
instead of
type === 'cancellation' ? generic_notification : notification_about_receipt
As long as type can only be 'receipt'
or 'cancellation'
the former version seems fine, but it's not a good assumption to build on (there will be other types).
🎉 This PR is included in version 3.6.0-epic-ras-acc.7 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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 this is an epic branch, I did not submit an issue for this. But let me know if there's a better way.
|
||
$items = $subscription->get_items(); | ||
$item = array_shift( $items ); | ||
$is_donation = Donations::is_donation_product( $item->get_product_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.
When deleting and account which has active subscriptions, this method will throw with
PHP Fatal error: Uncaught Error: Call to a member function get_product_id() on null
because the $item
is null
at this point.
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 catching this! I'll get a fix up for this today 👍
🎉 This PR is included in version 3.9.0-epic-ras-acc.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 4.1.0-epic-ras-acc.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 4.2.0-epic-ras-acc.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 4.4.0-epic-ras-acc.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
All Submissions:
Changes proposed in this Pull Request:
Closes https://app.asana.com/0/1206810965479792/1205743341940640/f
This PR adds a new donation cancellation email:
Note:
I've populated the cancellation email with super basic copy based mostly off of the email template for receipt. Since we are revisiting email design in ras-acc and this is going into epic/ras-acc, I think this is fine for now. If it's alright, I'd like to address updating the email content if necessary in https://app.asana.com/0/1205234045751551/1205668937699542/f where we will be updating all transactional emails.Scratch that, I found the copy for this here https://newspackdesignp2.wordpress.com/2023/12/05/cancellation-emails/. I will still be addressing the design updates in another pr, so this PR only gets the email setup with the old design.How to test the changes in this Pull Request:
Other information: