Skip to content
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

Add/sync newsletters subscriptions #23

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

leogermani
Copy link
Contributor

@leogermani leogermani commented Dec 12, 2023

Syncs events related to newsletters subscription to populate the Reader Data library.

This will create the network_newsletter_subscriber entry in the local storage.

Testing

  • In a site in the network, subscribe to a newsletter with a brand new user
  • See the newletter_subscribed event appear in the Event Log
  • In another site, see the new network_newsletter_subscriber entry appearing in the local storage once you log in with the same email

image

The item should containt the url of the site where the subscription happened, the timestamp, and then the lists they were subscribed to

  • Verify the user and go to My Account
  • Change your chosen lists
  • See the newsletter_updated appear in the Event log
  • When the event is propagated to the other site, see a new entry in the network_newsletter_subscriber, now with a list of subscribed and unsubscribed lists

Is it a good way to store these events? It's a bit tricky to determine whether the user is currently a subscriber in a site based on thsese, especially if all the sites are connected to the same ESP account. But I'm not sure there's a way around it, unless we also sync the full list of the current lists they are subscribed to. But what would trigger this?

I noticed that our events don't cover all cases. For example, it catches new subscribes and it catches My account updates. But if you got to a site where you are already a member and subscribe to a newsletter though a Newsletters subscription block, this won't fire any events.


@leogermani leogermani self-assigned this Dec 12, 2023
@leogermani leogermani requested a review from a team as a code owner December 12, 2023 20:07
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.

But what would trigger this?

The reader data library already has logic to keep up with the subscription status on authentication that we can use as a reference.

There's no need to rely on the federation to keep up with this information. There can be a similar trigger implemented to queue an update on other reader activity, like pageview with an interval, so it's not only on authentication.

I'm not sure about the strategy proposed here. It creates a lot of historical data in the browser, which the use is not clear to me.

But if you got to a site where you are already a member and subscribe to a newsletter though a Newsletters subscription block, this won't fire any events.

If that's the case, it's a bug. The Newsletter Subscription Form block triggers Newspack_Newsletters_Subscription::add_contact(), which fires the hook for the data event. It won't trigger for other sites, though. Not sure if that's what you mean.

@leogermani
Copy link
Contributor Author

I'm not sure about the strategy proposed here. It creates a lot of historical data in the browser, which the use is not clear to me.

I tried to follow what you suggested in the Asana task.

But I think this needs further discovery and discussion, and with everything that's going on, I think we can leave this for phase 2. Let's focus on the authorship tasks for this phase

@miguelpeixe
Copy link
Member

I tried to follow what you suggested in the Asana task.

I was thinking of something like this:

"np_reader_1_network_newsletter_subscribed_lists": {
  "https://site1.com": [ "list1", "list2" ],
  "https://site2.com": [ "list2" ],
}

It still doesn't sound too useful to me, but it's less data than keeping track historically with a timestamp.

@leogermani
Copy link
Contributor Author

right, but when I looked at the event, we don't actually have the current list of subscriptions there, we have what happened at that moment.

So maybe it's a different event we should listen to, like the one you mention that triggers on authentication. It makes sense to move away from a event based to a status based property.

I guess I also had in mind what we have on donations, that have the "former donor" filter... I agree we don't need that here

Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Since @miguelpeixe already reviewed I won't give an approval or otherwise. But joining in the discussion:

I noticed that our events don't cover all cases. For example, it catches new subscribes and it catches My account updates. But if you got to a site where you are already a member and subscribe to a newsletter though a Newsletters subscription block, this won't fire any events.

I tested with all my Hub + Node sites connected to the same Mailchimp account. When I did the flow you describe (sign up for a new newsletter list on a node site via the Newsletter Subscription Form block using an email address that was already registered for that node) I saw this familiar error in my logs:

[NEWSPACK-NEWSLETTERS][Newspack_Newsletters_Subscription::process_subscription_intents]: Error adding contact: Missing list id.

Which makes me think that the newspack_newsletters_add_contact action is never fired. I verified that the list subscriptions were actually updated for the email address. I think what's happening is that this scenario is firing the add_contact method in an async context, which ends up triggering this condition and bypasses the add_contact_to_provider method, which is what fires the newspack_newsletters_add_contact action hook.

What is the exact data that all the hub + nodes need to know about a reader's newsletter signups across sites?

  • Are we assuming that the hub + nodes will always be connected to the same ESP account? If so, the check_newsletter_subscription method on account login should be enough to fetch all list subscriptions for a given email address.
  • If not, we can probably still rely on this method, but we'll need to combine data from all the different sites/ESP accounts in that case so we know which lists correspond to which sites/accounts.

I agree with @miguelpeixe that we probaby don't need to add totally new reader data items for this data or to listen to events other than reader login to sync list data across the network.

*/
public function process_subscription() {
$email = $this->get_email();
Debugger::log( 'Processing newsleteter_subscribed with email: ' . $email );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Debugger::log( 'Processing newsleteter_subscribed with email: ' . $email );
Debugger::log( 'Processing newsletter_subscribed with email: ' . $email );

*/
public function process_subscription() {
$email = $this->get_email();
Debugger::log( 'Processing newsleteter_updated with email: ' . $email );
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Debugger::log( 'Processing newsleteter_updated with email: ' . $email );
Debugger::log( 'Processing newsletter_updated with email: ' . $email );

@leogermani
Copy link
Contributor Author

Which makes me think that the newspack_newsletters_add_contact action is never fired. I verified that the list subscriptions were actually updated for the email address. I think what's happening is that this scenario is firing the add_contact method in an async context, which ends up triggering this condition and bypasses the add_contact_to_provider method, which is what fires the newspack_newsletters_add_contact action hook.

Thanks for investigating @dkoo !

I think we need to revisit the requirements here. I agree we don't need all the data I'm adding in this PR. I was led to do it by the nature of the event I thought we should listen to, but I think we need to listen to a different event.

The event I'm listening to fires when a user update their subscriptions, but it doesn't give us the current status of their subscriptions, only a list of the lists that were added and removed.

We need something simliar to check_newsletter_subscription, but I'm not sure checking this only at authentication is enough, especially because we want authentication cookies to last as much as they can!

I was making an effort to push this into this phase 1, but I think we should push this to phase 2.

@miguelpeixe
Copy link
Member

but it doesn't give us the current status of their subscriptions, only a list of the lists that were added and removed.

You can use Newspack_Newsletters_Subscription::get_contact_lists() for that. The event doesn't include that information because it's not relevant to the event.

@miguelpeixe
Copy link
Member

bypasses the add_contact_to_provider method, which is what fires the newspack_newsletters_add_contact action hook.

It skips the execution in that context, but it's executed once the async intent is processed.

@leogermani
Copy link
Contributor Author

It still doesn't sound too useful to me, but it's less data than keeping track historically with a timestamp.

I think the value of it is to be able to segment users that are newsletters subscribers in other sites. "If they are subscribed to Newsletters in another site of the network, they like newsletters, let me offer them some". Or even, if they are subscribed to a specific newsletters in another site, offer them this other one here.

You can use Newspack_Newsletters_Subscription::get_contact_lists() for that. The event doesn't include that information because it's not relevant to the event.

This makes me think. Especially because we are still unsure about the usefulness of this feature. This will add one additional request to all these events. I know we do them async now, but still adds an overhead.

Thinking about pausing this for now for further exploration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants