fix(subscribe): optionally include metadata on subscribe

[dkoo]

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:

With Automattic/newspack-plugin#3779, adds an optional param to allow contact metadata fields to be synced upon a subscribe() call. The cases we'd want this to happen are upon newsletter signup + account registration via Newsletter Subscription Form and Reader Registration blocks, where there may be account registration, referrer, and UTM meta fields captured during the signup, but not synced via the Newspack Plugin's Sync classes.

How to test the changes in this Pull Request:

Follow testing instructions in Automattic/newspack-plugin#3779.

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?

[leogermani]

Do you remember which case we were handling when we decided to remove the metadata from here?

If we are setting this param to true everywhere we call this method there will likely be a regression... or is there anywhere this will still be called as false?

[dkoo]

I think we were accounting for when the reader updates their newsletter subscriptions via the My Account > Newsletters UI. In that scenario, we shouldn't be sending any additional metadata as part of the subscribe request.

This PR doesn't set the param to true everywhere it's called—it only sets it to true when calling Newspack_Newsletters_Contacts::subscribe() following a form submission via the Newsletter Subscription Form block or Registration block. All other instances of Newspack_Newsletters_Contacts::subscribe() should be unchanged.

[leogermani]

I don't feel confident... It seems we are circling back to something we intentionally tried to avoid.

WDYT of simply adding the other metadata that are part of newsletter subscription to the list of accepted metadata in the subscribe method. Currently it reads

$accepted_metadata = [ 'status', 'name' ];

Makes sense?

@miguelpeixe , if you can help us remember the rationale behind our choices here as well it would be awesome

[dkoo]

Hmm, but for these particular cases we'd want any valid metadata fields to be synced. Maybe we should be calling upsert() instead of subscribe() for signup requests from the blocks?

[dkoo]

Looking at this a bit more, it seems like the only real differences between subscribe and upsert methods are that the former allows for async execution, and it strips all metadata except for status and name keys. After that, subscribe passes its data through to the upsert method anyway.

[dkoo]

Wracking my memory for the reason behind these decisions...I think we wanted to retain the async option for certain newsletter signup flows so we could show an optimistic UI to make it feel faster, and show any errors to the user at a later point in their My Account dashboard. These are all of the flows where the async subscribe method is currently used:

• Registration block
• Newsletter Subscription Form block
• RAS auth modal registration
• Post-modal-checkout newsletter signup modal
• Newsletter signup changes via the My Account dashboard
• A CLI command to resync premium newsletter memberships to premium newsletter signups (wp newspack-newsletters sync-membership-tied-subscribers)

I think for all of these except for the CLI command and the My Account page, we actually want all of the contact metadata to be synced to the ESP.

[leogermani]

it seems like the only real differences between subscribe and upsert methods are that the former allows for async execution, and it strips all metadata except for status and name keys.

A key difference is that it triggers a different action, that will trigger a different data event

idk, maybe we don't need to strip metadata out? I can't remember why this specific piece is there... would love to know if @miguelpeixe remembers

[miguelpeixe]

IIRC, the rationale was mostly regarding the separation of concerns and better control of the data flow. subscribe() is a user action to subscribe to list(s).

We're mixing names here. upsert() is the lower-level method, used by both subscribe() and sync().

sync() is our async/background strategy for metadata updates.

I don't have a strong opinion on whether we should allow the metadata to be sent via subscribe() or trigger a sync() after the subscribe(). I lean more to let sync() be the sole owner of that task.

I'm not a fan of restoring the normalize contact data filter as proposed in Automattic/newspack-plugin#3779. It creates room for metadata coming from everywhere and debugging more difficult, as it was before the refactors.

[dkoo]

Looks like we're already firing a newspack_newsletters_contact_subscribed action and newsletter_subscribed with the unfiltered metadata after subscribing and syncing on the data event, but not including the metadata from the event payload. So if we include the metadata from this data event in the sync, then the changes in this PR are no longer necessary. See changes in 6b571e9 for details.

After these changes, we can remove the normalize contact data filter and rely on the syncs triggered by data events, since contact metadata from these events will be included in the triggered syncs, and metadata from queued syncs will be carried through to the single sync executed on shutdown.

[leogermani]

Ah, good call! I think that makes much more sense!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 23.33%. Comparing base (69ed769) to head (69c4f11).
Report is 55 commits behind head on release.

Additional details and impacted files

@@              Coverage Diff              @@
##             release    #1763      +/-   ##
=============================================
+ Coverage      23.29%   23.33%   +0.04%     
- Complexity      2710     2724      +14     
=============================================
  Files             49       49              
  Lines          10754    10827      +73     
=============================================
+ Hits            2505     2527      +22     
- Misses          8249     8300      +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.