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

feat: network donation events as reader data #8

Merged
merged 15 commits into from
Dec 7, 2023

Conversation

miguelpeixe
Copy link
Member

@miguelpeixe miguelpeixe commented Sep 4, 2023

Implements donation events as reader data. Any time a new donation is placed or a donation subscription is cancelled, a reader data item will update with the timestamp and the type of action (either the recurrence or "subscription_cancelled").

Example of the data item in localStorage:

{
  "np_reader_1_network_donor": {
    "https://node1.com": {
      "1234567890": "once",
      "1234567891": "monthly",
      "1234567892": "subscription_cancelled"
    },
    "https://node2.com": {
      "1234567890": "monthly"
    }
  }
}

Testing instructions are in the TESTING_SCENARIOS.md file.

@miguelpeixe miguelpeixe changed the title Add/donation events feat: network donation events as reader data Sep 5, 2023
@miguelpeixe miguelpeixe self-assigned this Sep 5, 2023
@miguelpeixe miguelpeixe marked this pull request as ready for review September 5, 2023 19:43
@miguelpeixe miguelpeixe requested a review from a team September 5, 2023 19:43
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.

We also need to update the reader in the Hub, not only in other nodes. Something similar with what we do with the reader_registered event (just do the same thing on post_process_in_hub() method.

I'm happy to make the changes if you want

@leogermani
Copy link
Contributor

@miguelpeixe I updated the PR to also process the event on the Hub. If a reader makes a donation in a node and later logs into the Hub, they should be segmented there as well.

Everything works on my test, but I'm seeing one issue and I have one question:

The issue is that not only I'm seeing the donation events, but the user is also being marked as a newsletter subscriber, even with the list ID... but it's a brand new user who only made a donation on another site...

I'm then logging into the other site, sending the OTP and verifying the user

image

What seems to be happening is that when I verify the user, I'm being subscribed to the Newsletters. I think that's an issue with the registration/login flow, but wanted to confirm with you.

The question is:

donation_new will also be triggered for renewals. Do we want to add every renewal to the user actions there? Do we do it for donors in a stand alone site?

@leogermani
Copy link
Contributor

Update: Just realized there is an issue. When the donation is done on the Hub, the site seems to be empty and then it creates an array on the top level? Need more testing

@leogermani
Copy link
Contributor

2962201 should fix the empty site. Not sure why I didn't do this before, but it looks safe

@leogermani
Copy link
Contributor

@miguelpeixe ecb226d adds a little refactor, to make sure we always insert user the same way, and with the same meta.

I'm gonna need this in #17 as well... I needed to add this to one of the PRs, added to this one because it will use it 3 times :)

@leogermani
Copy link
Contributor

Ah, another question.

Do we want to register a segmentation criteria here? Are we doing this in another PR? Have you thought about it?

@miguelpeixe
Copy link
Member Author

The issue is that not only I'm seeing the donation events, but the user is also being marked as a newsletter subscriber, even with the list ID... but it's a brand new user who only made a donation on another site...

Are you under RAS, connected to ActiveCampaign, and with a configured "master list"? That's the only case where a subscription should happen (without the reader opting in).

donation_new will also be triggered for renewals. Do we want to add every renewal to the user actions there? Do we do it for donors in a stand alone site?

Since the data event now holds the "is_renewal" property, we can store it as "{frequency}_renewal". WDYT?

Do we want to register a segmentation criteria here? Are we doing this in another PR? Have you thought about it?

Didn't think of anything like that. This schema is pretty flexible and can support pretty creative criteria but I wouldn't rush it. WDYT?

@leogermani
Copy link
Contributor

Are you under RAS, connected to ActiveCampaign, and with a configured "master list"? That's the only case where a subscription should happen (without the reader opting in).

No, didn't it happen to you?

Since the data event Automattic/newspack-plugin#2756, we can store it as "{frequency}_renewal". WDYT?

It look pretty complete, but aren't there any downsides of keeping so much data ?

Didn't think of anything like that. This schema is pretty flexible and can support pretty creative criteria but I wouldn't rush it. WDYT?

I think people are starting to play with this plugin and some of its feature are pretty abstract. If we could come up with a very simple criteria to start with people would have a better time understanding the possibilities. For example just "is network donor"

@miguelpeixe
Copy link
Member Author

It look pretty complete, but aren't there any downsides of keeping so much data ?

I guess it's fine to ignore renewals for this store.

For example just "is network donor"

I can set it up in a separate PR so we can have a discussion there.

@miguelpeixe
Copy link
Member Author

Are you under RAS, connected to ActiveCampaign, and with a configured "master list"? That's the only case where a subscription should happen (without the reader opting in).

No, didn't it happen to you?

No. My other guess would be that this localStorage entry was already there from a previous reader account you were logged in to. Switching accounts doesn't clear the storage.

@miguelpeixe
Copy link
Member Author

Pushed 581c88e to ignore renewals.

@leogermani
Copy link
Contributor

Thank you! I'm running a few more tests on the subscription data thing

@leogermani
Copy link
Contributor

The newsletters subscription thing happens consistently with me all the time... on a brand anonymous window, with a brand new user... not using Active Campaign..

This is most certainly not related to this PR, but I need to understand what's going on

@leogermani
Copy link
Contributor

Ok, figured it out. User was being automatically subscribed to a premium newsletter on one site, and I didn't know our data library was smart enough to detect that

@leogermani leogermani merged commit 9a114a4 into master Dec 7, 2023
3 checks passed
@leogermani leogermani deleted the add/donation_events branch December 7, 2023 12:20
matticbot pushed a commit that referenced this pull request Dec 22, 2023
# [1.0.0-alpha.2](v1.0.0-alpha.1...v1.0.0-alpha.2) (2023-12-22)

### Bug Fixes

* dont sync authors terms ([#26](#26)) ([a4a788b](a4a788b))
* **newspack-ads:** parse targeting site url ([#22](#22)) ([acb84c1](acb84c1))

### Features

* allow editors to pull content ([#28](#28)) ([3021f7e](3021f7e))
* network donation events as reader data ([#8](#8)) ([9a114a4](9a114a4))
* sync user avatar ([#21](#21)) ([70e4c83](70e4c83))
* update authors on post update ([#29](#29)) ([25cb65f](25cb65f))
* update menu icon and position ([#18](#18)) ([11638b2](11638b2))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.0.0-alpha.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Dec 22, 2023
# 1.0.0 (2023-12-22)

### Bug Fixes

* adjust node_id check logic ([9ceb09f](9ceb09f))
* canonical url processing ([c9ee2c9](c9ee2c9))
* dont sync authors terms ([#26](#26)) ([a4a788b](a4a788b))
* failing CI jobs ([#11](#11)) ([c62248b](c62248b))
* **newspack-ads:** parse targeting site url ([#22](#22)) ([acb84c1](acb84c1))
* remove gam ad targeting ([#16](#16)) ([c47cb15](c47cb15))

### Features

* add circle ci config ([3f9d752](3f9d752))
* Add site role selection and unify menus ([bbc8eb5](bbc8eb5))
* add support to local Woo events ([1273ee1](1273ee1))
* **ads:** support network targeting key-val ([#13](#13)) ([37f677a](37f677a))
* allow editors to pull content ([#28](#28)) ([3021f7e](3021f7e))
* bump limit of events pulled by nodes from to 2 to 20 ([#7](#7)) ([c5fbdc5](c5fbdc5))
* network donation events as reader data ([#8](#8)) ([9a114a4](9a114a4))
* **node:** debug tools ([#12](#12)) ([07b5470](07b5470))
* refactor to symetric crypto ([1f55551](1f55551))
* sync author bio and meta ([#15](#15)) ([f60236d](f60236d))
* sync user avatar ([#21](#21)) ([70e4c83](70e4c83))
* update authors on post update ([#29](#29)) ([25cb65f](25cb65f))
* update menu icon and position ([#18](#18)) ([11638b2](11638b2))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants