-
Notifications
You must be signed in to change notification settings - Fork 49
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
fix(ras): only sync spend total and last payment amounts for completed orders #2886
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.
Thanks @dkoo!
I re-reviewed a couple things I found while this PR was still in draft mode:
Failed transactions weren't syncing most fields, like name: Running a failed transaction as a new account, I now get: email address, first and last name, NP_Account, NP_Registration Date, Method & Page, and an NP_Last Payment Amount and NP Total Paid of 0. So this all looks good!
I also ran a successful transaction as a new account, and then followed up with a failed one-time transaction, and things look right on the MC side (the data remains the same as after the successful transaction, with no change to last donation amount or NP_Membership Status).
Manually trigger renewal of a recurring donation: running this again with this PR, the NP_Membership Status remained as 'Monthly Donor', and doesn't switch to 'Ex-Monthly Donor'.
I then tried cancelling that subscription, and confirmed NP_Membership Status did correctly change to Ex-Monthly Donor.
I also re-ran these tests just to make sure:
- Confirmed I could log into my failed transaction user from the first step.
- Confirmed updating the credit card for the failed user and re-running the donation updated the information successfully in MC to a Monthly Donor.
- Confirmed that I could successfully make a one-time donation.
- Confirmed I could log in as that successful one-time donor.
This gets a 👍 from me on the functional side, but I'm adding this as just a comment to give @chickenn00dle a chance to look as well 🙂
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 @dkoo, left a few nitpicks but they are not blockers
$order_date_paid = $order->get_date_paid(); | ||
if ( null !== $order_date_paid ) { | ||
$metadata[ Newspack_Newsletters::get_metadata_key( 'last_payment_date' ) ] = $order_date_paid->date( Newspack_Newsletters::METADATA_DATE_FORMAT ); | ||
if ( $payment_received && null !== $order_date_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.
Nitpick: might be better to use something like ! empty
here rather than null !==
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 idea—updated in 02b0da0.
return false; | ||
} | ||
// Only update last payment data if new payment has been received. | ||
$payment_received = $is_new && in_array( $order->get_status(), [ 'processing', 'completed' ], true ); |
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.
Might be able to use has_status()
here and a few other places in this PR for readability.
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.
Didn't know this was a thing! Updated this instance and another one below in 02b0da0.
…2895) * feat(ci): add epic/* release workflow and rename `master` to `trunk` * chore: point to dev version of newspack-scripts (REVERT BEFORE MERGING) * docs: update branch names in README * fix: revert unintentional branch name update for third-party repo * fix: update newsletter scroll appearance in Sign Up modal (#2897) * fix(ras): only sync spend total and last payment amounts for completed orders (#2886) * fix(ras): only sync spend total and last payment amounts for completed orders * fix: no need to manually add last order amount to total * fix: logic for updating upon subscription status change * fix: apply ex- status only for cancelled or expired subs * refactor: syntax fixes * fix: update newspack-scripts to release version --------- Co-authored-by: Laurel <laurel.fulford@automattic.com>
🎉 This PR is included in version 3.0.0-epic-ras-acc.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [3.1.0-alpha.1](v3.0.3...v3.1.0-alpha.1) (2024-02-08) ### Bug Fixes * **engagement-wizard:** handle error when retrieving subscription lists ([e85c108](e85c108)) * **ras:** only sync spend total and last payment amounts for completed orders ([#2886](#2886)) ([68aaf39](68aaf39)) * redirect to origin from magic link ([9f41947](9f41947)) * typescript errors ([dc27973](dc27973)) * TypeScript usage; add to CI ([#2884](#2884)) ([6f5e7a6](6f5e7a6)) * update newsletter scroll appearance in Sign Up modal ([#2897](#2897)) ([496723a](496723a)) ### Features * **ci:** add epic/* release workflow and rename `master` to `trunk` ([#2895](#2895)) ([ea02075](ea02075)), closes [#2897](#2897) [#2886](#2886) * **reader-revenue:** make NYP and Stripe Gateway optional ([#2866](#2866)) ([fcfa88c](fcfa88c)) * remove new tab default on image credits ([#2880](#2880)) ([3c996b1](3c996b1)) * **wc:** override cart, checkout, and my-account page templates ([#2893](#2893)) ([68b1836](68b1836))
🎉 This PR is included in version 3.1.0-alpha.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 3.1.0-epic-ras-acc.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [3.1.0](v3.0.5...v3.1.0) (2024-02-20) ### Bug Fixes * add frequency tab options for donations, even when tiers are disabled ([#2930](#2930)) ([cb7eb7b](cb7eb7b)) * **categories:** fix pager urls ([#2913](#2913)) ([bb7e534](bb7e534)) * **categories:** fix pager urls ([#2913](#2913)) ([c851bb6](c851bb6)) * **engagement-wizard:** handle error when retrieving subscription lists ([e85c108](e85c108)) * **ras:** only sync spend total and last payment amounts for completed orders ([#2886](#2886)) ([68aaf39](68aaf39)) * redirect to origin from magic link ([9f41947](9f41947)) * typescript errors ([dc27973](dc27973)) * TypeScript usage; add to CI ([#2884](#2884)) ([6f5e7a6](6f5e7a6)) * update newsletter scroll appearance in Sign Up modal ([#2897](#2897)) ([496723a](496723a)) * update path to wide template file ([#2918](#2918)) ([fdd6b69](fdd6b69)) ### Features * **ci:** add epic/* release workflow and rename `master` to `trunk` ([#2895](#2895)) ([ea02075](ea02075)), closes [#2897](#2897) [#2886](#2886) * **reader-revenue:** make NYP and Stripe Gateway optional ([#2866](#2866)) ([fcfa88c](fcfa88c)) * remove new tab default on image credits ([#2880](#2880)) ([3c996b1](3c996b1)) * **wc:** override cart, checkout, and my-account page templates ([#2893](#2893)) ([68b1836](68b1836))
🎉 This PR is included in version 3.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
All Submissions:
Changes proposed in this Pull Request:
Fixes a logical error in our reader data when syncing "last payment" amounts for a contact. Currently, when a reader logs into an existing account, we resync their contact data, including info about their last order. But the
get_last_order
method Woo provides to get a customer's last order gets any order, even failed ones. This means that for any reader whose most recent transaction resulted in the creation of a new order which subsequently failed (which could happen if the credit card they used is declined by the bank after the point of service in Stripe), the last payment amount will reflect this failed payment, instead of the most recent successful payment amount. (Amusing anecdote: we discovered this bug after a customer made a probably fraudulent $1,000,000 donation that got declined despite the order completing successfully—maybe because the card itself was valid, but the amount was not?)This PR adds some additional logic to ensure that we're only syncing data about successful orders going forward. It also adds some measures to ensure that the correct total spend and last payment amount values are synced to any existing reader account upon login.
How to test the changes in this Pull Request:
master
, as a new reader, complete a donation on a site with Stripe in test mode. Use this test credit card number to simulate a card that successfully creates the order in Woo, but then fails after when the charge is subsequently declined by the bank:4000 0000 0000 0341
4242 4242 4242 4242
. Confirm that both the Last Payment Amount and Total Paid fields get synced to the amount of this successful order.Other information: