-
Notifications
You must be signed in to change notification settings - Fork 115
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
Enable failed receipt sending for Stripe Gateway #14864
Enable failed receipt sending for Stripe Gateway #14864
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.
LGTM
As before, the email didn't work again for me 😓 I tried two different personal emails (and triple checked I didn't mispell), maybe it's because the admin email is an A8C email. I haven't seen the SPAM email in the A8C spam folder yet. I couldn't find the transactional-emails
log in this self-hosted Pressable store either. If you've received the email from testing, I don't think this is blocking.
async let isWooCommerceSupported = isPluginSupported(Constants.wcPluginName, | ||
minimumVersion: Constants.ReceiptAfterPayment.wcPluginMinimumVersion) |
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: since both payment gateways check the WC version the same way, how about having a function like isWCPluginSupported
to encapsulate the plugin name & version?
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.
Yes, I can do that!
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.
Made improvements in 994ad8b
let wooCommerceResult = await isWooCommerceSupported | ||
let wooPaymentsResult = await isWooPaymentsSupported | ||
let isSupported = wooCommerceResult && wooPaymentsResult |
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: Could these be shortened to let isSupported = await isWooCommerceSupported && isWooPaymentsSupported
?
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 wish or at least I couldn't find a way. It could be shortened to get a tuple:
let supported = await (isWooCommerceSupported, isWooPaymentsSupported)
I'll look for more examples of how to structure such code better.
Mmm... this is strange why it doesn't work for you 🤔 The email did arrive for me, I use Pressable as well. When you have time, @jaclync, could you add me to the sites for testing purposes? |
Sure I've sent an invitation link to your A8C email for the self-hosted Pressable test store. I think you already have access to the atomic test store "fun testing" from the wp-admin users. |
@jaclync, the emails didn't work for me for this particular site as well. However, replacing the A8C email with an email to contain a subdomain in WooCommerce -> Settings -> Emails it started working. ![]() |
Oh wow, thanks for looking into the email issue. TIL about the email setting and will make this change for other sites with email issues. |
Closes: #14494
Description
After WooCommerce Stripe Gateway update, failed payment receipts are now supported for Stripe Gateway.
Enabling it for versions >=9.1.0
Steps to reproduce
Eligible + Sending Receipt After
Eligible + Attach Customer Before Payment
Non-eligible
Testing information
There are unit tests written for different cases. Also confirmed WooPayments continues to work.
RELEASE-NOTES.txt
if necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: