Skip to content

Conversation

@adekbadek
Copy link
Member

@adekbadek adekbadek commented Aug 23, 2024

All Submissions:

Changes proposed in this Pull Request:

Fixes the handling of of "last payment" ESP metadata (NP_Last Payment Date, NP_Last Payment Amount).

How to test the changes in this Pull Request:

  1. On trunk, make a recurring donation as a new reader
  2. Call the \Newspack\WooCommerce_Connection::get_contact_from_customer(new WC_Customer(<user-id>)) in wp shell – observe the NP_Last Payment Date and NP_Last Payment Amount fields are empty
  3. Switch to this branch, call the code again, observe the expected fields are there

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?

@adekbadek adekbadek added the [Status] Needs Review The issue or pull request needs to be reviewed label Aug 23, 2024
@adekbadek adekbadek requested a review from a team as a code owner August 23, 2024 10:09
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested well, but I have some concerns regarding how we're treating the "latest" order:

* @return array Contact order metadata.
*/
public static function get_contact_order_metadata( $order, $payment_page_url = false, $is_new = false ) {
private static function get_contact_order_metadata( $order, $payment_page_url = false ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that we're deprecating this parameter, but that seems to make "Last Payment Amount" and "Last Payment Date" values whichever order passed to get_contact_from_customer(). It's best to query it instead of relying on the passed order.

Copy link
Member Author

@adekbadek adekbadek Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, good call! I was wondering if any of the contact metadata could rely on an order different than the last successful order (or the active subscription), but I don't think so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It used to but, from what I can tell, not since #3086. Turns out the method ended up with a superfluous param.

$order_metadata = self::get_contact_order_metadata( $order, $payment_page_url );
} else {
// If the customer has no successful orders, clear out subscription-related fields.
// If the customer has no successful orders or if no order is provided???, clear out subscription-related fields.
Copy link
Member

@miguelpeixe miguelpeixe Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like an unreasonable amount of question marks 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was more of a note to self :D Fixed in 9342849

@adekbadek adekbadek requested a review from miguelpeixe August 26, 2024 08:37
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods are looking much cleaner now!

@github-actions 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 Aug 26, 2024
@adekbadek adekbadek merged commit d1abbfe into trunk Aug 27, 2024
@adekbadek adekbadek deleted the fix/esp-contact-last-payment-fields branch August 27, 2024 08:05
matticbot pushed a commit that referenced this pull request Aug 29, 2024
# [5.4.0-alpha.1](v5.3.3...v5.4.0-alpha.1) (2024-08-29)

### Bug Fixes

* **esp-meta:** handle state of the 'Woo Team' meta ([#3352](#3352)) ([ba5ea1e](ba5ea1e))
* **esp-wc-metadata:** last payment date & amount handling ([#3363](#3363)) ([d1abbfe](d1abbfe))
* **guest-author:** enqueue the guest author admin script selectively ([0bf37af](0bf37af))
* make email template fetching deterministic ([#3341](#3341)) ([ace91aa](ace91aa))
* woocommerce connection tests ([#3372](#3372)) ([5cea128](5cea128))
* **woocommerce-connection:** handle explicit UTM meta fields meta ([#3371](#3371)) ([bf9e997](bf9e997))

### Features

* **esp-sync:** sync membership data regardless of subscription ([#3353](#3353)) ([9f7d1de](9f7d1de))
* **ga:** disable tracking for editors regardless of RA status ([81323c3](81323c3))
* **memberships:** add memberships-related body classes ([b56b9d8](b56b9d8))
* **ras:** esp sync tools ([#3359](#3359)) ([d7dd754](d7dd754))
* **ras:** helper method for ESP master list ([#3355](#3355)) ([ec56d5b](ec56d5b))
@matticbot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

dkoo added a commit that referenced this pull request Sep 10, 2024
)

Co-authored-by: Adam Cassis <adam@adamcassis.com>
matticbot pushed a commit that referenced this pull request Sep 10, 2024
## [5.3.7](v5.3.6...v5.3.7) (2024-09-10)

### Bug Fixes

* **esp-wc-metadata:** last payment date & amount handling ([#3363](#3363)) ([#3409](#3409)) ([155fd65](155fd65))
matticbot pushed a commit that referenced this pull request Sep 10, 2024
# [5.4.0-alpha.3](v5.4.0-alpha.2...v5.4.0-alpha.3) (2024-09-10)

### Bug Fixes

* **esp-wc-metadata:** last payment date & amount handling ([#3363](#3363)) ([#3409](#3409)) ([155fd65](155fd65))
matticbot pushed a commit that referenced this pull request Oct 3, 2024
# [5.5.0-alpha.1](v5.4.0...v5.5.0-alpha.1) (2024-10-03)

### Bug Fixes

* change the current product criteria for sync ([#3416](#3416)) ([28a84bc](28a84bc))
* **esp-meta:** handle state of the 'Woo Team' meta ([#3352](#3352)) ([ba5ea1e](ba5ea1e))
* **esp-sync:** sync Connected Account field ([#3414](#3414)) ([61c02bc](61c02bc))
* **esp-wc-metadata:** last payment date & amount handling ([#3363](#3363)) ([d1abbfe](d1abbfe))
* **guest-author:** enqueue the guest author admin script selectively ([0bf37af](0bf37af))
* hide My Account links if not relevant ([#3394](#3394)) ([0d039d8](0d039d8))
* make email template fetching deterministic ([#3341](#3341)) ([ace91aa](ace91aa))
* **phpcs:** specify path in custom ruleset ref ([#3384](#3384)) ([b143e74](b143e74))
* prevent PHP notice while checking my-account page ([#3435](#3435)) ([146a26f](146a26f))
* **ras-sync:** deprecate redundant Signup_Page meta field ([#3439](#3439)) ([61d6de8](61d6de8))
* **reader-registration-block:** fix initial newsletter checkbox state ([1890efe](1890efe))
* replace `newspack_image_credits_placeholder` default value ([#3433](#3433)) ([c754fc2](c754fc2))
* **sync:** method name for `membership_saved` handler ([#3399](#3399)) ([2c0bf26](2c0bf26))
* **sync:** place esp sync admin features behind a constant ([#3438](#3438)) ([20a0970](20a0970))
* **sync:** remove localized number format ([#3434](#3434)) ([2243a5d](2243a5d))
* wizards - update type check conditional for `custom_logo` ([#3442](#3442)) ([125f756](125f756))
* woocommerce connection tests ([#3372](#3372)) ([5cea128](5cea128))
* **woocommerce-connection:** handle explicit UTM meta fields meta ([#3371](#3371)) ([bf9e997](bf9e997))

### Features

* add a new action to when a ras setting is updated ([#3357](#3357)) ([35d3492](35d3492))
* **esp-sync:** sync membership data regardless of subscription ([#3353](#3353)) ([9f7d1de](9f7d1de))
* **ga4:** detect gate interaction blocks ([#3408](#3408)) ([e14913c](e14913c))
* **ga:** disable tracking for editors regardless of RA status ([81323c3](81323c3))
* media kit page handling ([#3358](#3358)) ([4454850](4454850))
* **memberships:** add memberships-related body classes ([b56b9d8](b56b9d8))
* **ras:** esp sync tools ([#3359](#3359)) ([d7dd754](d7dd754))
* **ras:** helper method for ESP master list ([#3355](#3355)) ([ec56d5b](ec56d5b))
* **ras:** sync class ([#3362](#3362)) ([88acbee](88acbee))
* **ras:** unify ESP connector strategy for data events ([#3360](#3360)) ([7080864](7080864))
* **reader-activation:** ESP-related tweaks ([#3381](#3381)) ([ac68b67](ac68b67))
* remove Woo Membersip sync fields ([#3411](#3411)) ([28052e8](28052e8))
* **sync:** add ESP sync notice to RAS wizard ([#3400](#3400)) ([f9acd56](f9acd56))
@matticbot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Oct 8, 2024
# [5.5.0](v5.4.1...v5.5.0) (2024-10-08)

### Bug Fixes

* cancelled subscriptions sync ([#3466](#3466)) ([b605a7f](b605a7f))
* change the current product criteria for sync ([#3416](#3416)) ([28a84bc](28a84bc))
* **esp-meta:** handle state of the 'Woo Team' meta ([#3352](#3352)) ([ba5ea1e](ba5ea1e))
* **esp-sync:** sync Connected Account field ([#3414](#3414)) ([61c02bc](61c02bc))
* **esp-wc-metadata:** last payment date & amount handling ([#3363](#3363)) ([d1abbfe](d1abbfe))
* **guest-author:** enqueue the guest author admin script selectively ([0bf37af](0bf37af))
* hide My Account links if not relevant ([#3394](#3394)) ([0d039d8](0d039d8))
* make email template fetching deterministic ([#3341](#3341)) ([ace91aa](ace91aa))
* **phpcs:** specify path in custom ruleset ref ([#3384](#3384)) ([b143e74](b143e74))
* prevent PHP notice while checking my-account page ([#3435](#3435)) ([146a26f](146a26f))
* **ras-sync:** deprecate redundant Signup_Page meta field ([#3439](#3439)) ([61d6de8](61d6de8))
* **reader-registration-block:** fix initial newsletter checkbox state ([1890efe](1890efe))
* replace `newspack_image_credits_placeholder` default value ([#3433](#3433)) ([c754fc2](c754fc2))
* **sync:** method name for `membership_saved` handler ([#3399](#3399)) ([2c0bf26](2c0bf26))
* **sync:** place esp sync admin features behind a constant ([#3438](#3438)) ([20a0970](20a0970))
* **sync:** remove localized number format ([#3434](#3434)) ([2243a5d](2243a5d))
* wizards - update type check conditional for `custom_logo` ([#3442](#3442)) ([125f756](125f756))
* woocommerce connection tests ([#3372](#3372)) ([5cea128](5cea128))
* **woocommerce-connection:** handle explicit UTM meta fields meta ([#3371](#3371)) ([bf9e997](bf9e997))

### Features

* add a new action to when a ras setting is updated ([#3357](#3357)) ([35d3492](35d3492))
* **esp-sync:** sync membership data regardless of subscription ([#3353](#3353)) ([9f7d1de](9f7d1de))
* **ga4:** detect gate interaction blocks ([#3408](#3408)) ([e14913c](e14913c))
* **ga:** disable tracking for editors regardless of RA status ([81323c3](81323c3))
* media kit page handling ([#3358](#3358)) ([4454850](4454850))
* **memberships:** add memberships-related body classes ([b56b9d8](b56b9d8))
* **ras:** esp sync tools ([#3359](#3359)) ([d7dd754](d7dd754))
* **ras:** helper method for ESP master list ([#3355](#3355)) ([ec56d5b](ec56d5b))
* **ras:** sync class ([#3362](#3362)) ([88acbee](88acbee))
* **ras:** unify ESP connector strategy for data events ([#3360](#3360)) ([7080864](7080864))
* **reader-activation:** ESP-related tweaks ([#3381](#3381)) ([ac68b67](ac68b67))
* remove Woo Membersip sync fields ([#3411](#3411)) ([28052e8](28052e8))
* **sync:** add ESP sync notice to RAS wizard ([#3400](#3400)) ([f9acd56](f9acd56))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released on @alpha [Status] Approved The pull request has been reviewed and is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants