Skip to content

fix(esp-meta): handle state of the 'Woo Team' meta #3352

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.

Sign up for GitHub

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

Jump to bottom
Merged
adekbadek merged 3 commits into from
Aug 26, 2024
Merged

fix(esp-meta): handle state of the 'Woo Team' meta #3352

adekbadek merged 3 commits into from
Aug 26, 2024

Conversation

@adekbadek
Copy link
Member

@adekbadek adekbadek commented Aug 21, 2024

All Submissions:

Changes proposed in this Pull Request:

Fixes a small issue with #3325.

How to test the changes in this Pull Request:

  1. On trunk, activate the Teams for Memberships plugin
  2. In the Engagement wizard, ensure the "Woo Team" meta field is deselected
  3. Add a user to a team, then log in as them and make a donation
  4. Observe that the synced contact in the ESP has the "NP_Woo Team" field value set
  5. Switch to this branch, repeat step 3 & 4
  6. Observe the ESP contact has the field value not set
  7. Enable the field in the Engagement wizard and re-test – observe the field is now set

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 21, 2024
@adekbadek adekbadek requested a review from a team as a code owner August 21, 2024 07:18
@adekbadek adekbadek force-pushed the fix/woo-team-esp-meta-disabled branch from dbc3604 to f4782b4 Compare August 21, 2024 08:01
@adekbadek adekbadek mentioned this pull request Aug 21, 2024
Merged
6 tasks
leogermani
leogermani approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Approving as it works as expected, but I think this interface could be a little better. Ideas:

  • applicable_fields sounds weird to me. Maybe a better name there
  • Also offer a should_sync_field( $field_name ) that would be an easier method to use if you are just checking for one field

or

  • the newspack_esp_sync_normalize_contact filter could pass the applicable_filters as the second argument, then the callback could use it to do its own check...

wdyt?

@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 23, 2024
@adekbadek
Copy link
Member Author

adekbadek commented Aug 26, 2024

applicable_fields sounds weird to me. Maybe a better name there

Agreed, changed the name to "filtered enabled fields" in c77e150.

Also offer a should_sync_field( $field_name ) that would be an easier method to use if you are just checking for one field

Hm, not a fan of adding a method just to handle a single value. Unnecessary complexity IMO.

the newspack_esp_sync_normalize_contact filter could pass the applicable_filters as the second argument, then the callback could use it to do its own check

That's an alternative, but I'm not sure if one is better than the other ¯_(ツ)_/¯

@adekbadek adekbadek merged commit ba5ea1e into trunk Aug 26, 2024
@adekbadek adekbadek deleted the fix/woo-team-esp-meta-disabled branch August 26, 2024 07:56
matticbot pushed a commit that referenced this pull request Aug 29, 2024
@semantic-release-bot
chore(release): 5.4.0-alpha.1 [skip ci]
67d5663 
# [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

matticbot commented Aug 29, 2024

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

@leogermani leogermani mentioned this pull request Aug 30, 2024
Merged
6 tasks
@leogermani
Copy link
Contributor

leogermani commented Aug 30, 2024 •
edited
Loading

@adekbadek I was looking at this again while reviewing #3360 and I noticed that this is a redundant check...

The metadata will already be excluded from the contact if it is disabled in admin. It happens here.

I've tested this again and I can safely remove the check added in this PR. The meta will only be synced if it's active.

Is there another scenario where this will not be respected?

cc @miguelpeixe

matticbot pushed a commit that referenced this pull request Oct 3, 2024
@semantic-release-bot
chore(release): 5.5.0-alpha.1 [skip ci]
2d3cf87 
# [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

matticbot commented Oct 3, 2024

🎉 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
@semantic-release-bot
chore(release): 5.5.0 [skip ci]
b53dbb6 
# [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

Reviewers

@leogermani leogermani leogermani approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@adekbadek @matticbot @leogermani