fix(ras): only sync spend total and last payment amounts for completed orders #2886

dkoo · 2024-01-27T01:22:40Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

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:

On 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
Note the amount of money of the failed order, then in a new session log in as that reader. In the contact data synced to the connected ESP, observe that the "Last Payment Amount" gets set to this failed order amount.
Check out this branch and log in as the same reader in a new session again. After the resync, confirm that the "Last Payment Amount" gets reset to zero.
In another new session, complete a successful donation with the same reader using a "successful" test card number: 4242 4242 4242 4242. Confirm that both the Last Payment Amount and Total Paid fields get synced to the amount of this successful order.
In yet another new session, complete another successful donation with a different amount. Make sure this one is a subscription. Confirm that the Last Payment Amount gets set to this amount, and the Total Paid amount gets set to the cumulative spend (it should match the "Total spend" figure in the WooCommerce > Customers table in WP admin).
In WP admin, trigger a renewal for the subscription purchased in step 5. Again, confirm that the Payment Amount and Total Paid fields get updated correctly in the ESP.
In WP admin, cancel the subscription. Confirm that the contact data in the ESP is updated to reflect "Ex-" donor status.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully ran tests with your changes locally?

…d orders

laurelfulford

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 🙂

chickenn00dle

Looks good @dkoo, left a few nitpicks but they are not blockers

chickenn00dle · 2024-01-31T20:48:05Z

includes/reader-revenue/woocommerce/class-woocommerce-connection.php

 			$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 ) {


Nitpick: might be better to use something like ! empty here rather than null !==

Good idea—updated in 02b0da0.

chickenn00dle · 2024-01-31T20:53:30Z

includes/reader-revenue/woocommerce/class-woocommerce-connection.php

-			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 );


Might be able to use has_status() here and a few other places in this PR for readability.

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>

matticbot · 2024-02-06T00:52:56Z

🎉 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))

matticbot · 2024-02-08T19:34:49Z

🎉 This PR is included in version 3.1.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot · 2024-02-15T08:21:56Z

🎉 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))

matticbot · 2024-02-20T19:26:08Z

🎉 This PR is included in version 3.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

fix(ras): only sync spend total and last payment amounts for complete…

bcede2f

…d orders

dkoo self-assigned this Jan 29, 2024

dkoo added the [Status] Needs Review The issue or pull request needs to be reviewed label Jan 29, 2024

dkoo marked this pull request as ready for review January 29, 2024 21:13

dkoo requested a review from a team as a code owner January 29, 2024 21:13

dkoo changed the base branch from master to release January 29, 2024 21:13

dkoo changed the base branch from release to master January 29, 2024 21:13

fix: no need to manually add last order amount to total

6f0842f

dkoo marked this pull request as draft January 29, 2024 21:51

fix: logic for updating upon subscription status change

3d8341b

dkoo marked this pull request as ready for review January 29, 2024 22:18

fix: apply ex- status only for cancelled or expired subs

ec278ea

laurelfulford reviewed Jan 31, 2024

View reviewed changes

chickenn00dle approved these changes Jan 31, 2024

View reviewed changes

github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Jan 31, 2024

refactor: syntax fixes

02b0da0

dkoo merged commit 68aaf39 into master Jan 31, 2024
7 checks passed

dkoo deleted the fix/payment-amount-sync branch January 31, 2024 21:43

matticbot added the released on @epic/ras-acc label Feb 6, 2024

matticbot added the released on @alpha label Feb 8, 2024

matticbot added the released label Feb 20, 2024

leogermani mentioned this pull request Oct 8, 2024

fix: cancelled subscriptions sync #3466

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #2886

dkoo commented Jan 27, 2024 •

edited

Loading

laurelfulford left a comment

chickenn00dle left a comment

chickenn00dle Jan 31, 2024

dkoo Jan 31, 2024

chickenn00dle Jan 31, 2024

dkoo Jan 31, 2024

matticbot commented Feb 6, 2024

matticbot commented Feb 8, 2024

matticbot commented Feb 15, 2024

matticbot commented Feb 20, 2024

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 #2886

Conversation

dkoo commented Jan 27, 2024 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

laurelfulford left a comment

Choose a reason for hiding this comment

chickenn00dle left a comment

Choose a reason for hiding this comment

chickenn00dle Jan 31, 2024

Choose a reason for hiding this comment

dkoo Jan 31, 2024

Choose a reason for hiding this comment

chickenn00dle Jan 31, 2024

Choose a reason for hiding this comment

dkoo Jan 31, 2024

Choose a reason for hiding this comment

matticbot commented Feb 6, 2024

matticbot commented Feb 8, 2024

matticbot commented Feb 15, 2024

matticbot commented Feb 20, 2024

dkoo commented Jan 27, 2024 •

edited

Loading