feat: reader registration on donate #1655

miguelpeixe · 2022-05-19T23:13:31Z

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:

Implements reader registration functionality to the donation block.

Auth intention

If the reader registration detects an existing user, a cookie stores the auth intention that is accessible through the Reader_Activation::get_auth_intention() method. This API can be used across other plugins to improve UX over resolving the reader authentication, subscription, donation, etc.

This PR currently implements the auth intention to the wp_login_form() defaults, which is used by the core's Login block. Unfortunately this hook is not applied to the WooCommerce's My Account page login form.

The reader

The PR also proposes the definition of a reader user, which can be any user with the np_reader meta or within the roles subscriber or customer. Identifying a reader helps us build flows tied specifically to this type of user, such as the magic link functionality.

Email verification

Reader registration aims for a quick user registration and doesn't impose any validation for a new reader to be created and authenticated. The Reader_Activation::verify_reader_email() method is public but also executed on the reset password form action hook (resetpass_form). Once a registered reader clicks on the reset password link received on their email, it'll trigger the method to make the reader verified.

Experimental flag

This PR also proposes that all experimental features related to reader activation should be placed behind NEWSPACK_EXPERIMENTAL_READER_ACTIVATION, or the Reader_Activation::is_enabled() method for easier deprecation of the EXPERIMENTAL flag in the future.

How to test the changes in this Pull Request:

Check out this branch and make sure you have the AMP Plus flag and Reader Revenue setup with a test Stripe account
Add the experimental flag to your config: define( 'NEWSPACK_EXPERIMENTAL_READER_ACTIVATION', true );
While logged out, visit a page with the Donate block and send a test donation with an email that does not exist
Visit WooCommerce's my-account/orders page and confirm you are logged in and you see the donation order
Log out, donate again with the same email
Attempt to visit my-account/orders and confirm you are not authenticated
Check the browser cookies and confirm the np_auth_intention cookie with the email as the value
Logged-in as admin, visit the Orders dashboard and confirm the last donation is not connected to the existing customer

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?

yogeshbeniwal · 2022-05-20T07:51:16Z

@miguelpeixe Depending on what is the next/roadmap for reader revenue, shall there be an option to change title of button from donate.

miguelpeixe · 2022-05-20T11:33:58Z

Hi, @yogeshbeniwal! I don't think we have that in our roadmap. I like that suggestion, do you mind filing an issue so we can have a dedicated place for this discussion?

adekbadek

Some stuff to iron out, but for the general approach is good!

Unfortunately [login form] hook is not applied to the WooCommerce's My Account page login form.

Is there no WooCommerce hook that could be used here?

includes/class-reader-activation.php

includes/reader-revenue/class-stripe-connection.php

adekbadek · 2022-05-24T08:43:40Z

includes/reader-revenue/class-woocommerce-connection.php

+			} else {
+				Logger::log( 'This order will result in a membership, creating account for user.' );
+				$user_login = sanitize_title( $full_name );
+				$user_id    = wc_create_new_customer( $email_address, $user_login, '', [ 'display_name' => $full_name ] );


The user creation is duplicated – here and in the Reader Activation module. The module should expose a method for handling user creation, and the is_enabled only used for the reader-activation-flow specific tasks: currently adding the meta fields. Or, the user creation should be delegated to the main Newspack class.

I wouldn't call duplicated, the original intended behavior is preserved while reader activation is experimental.

WP core already provides a straightforward API for generic user creation that should be used if not making use of reader activation logic. I don't see why reader activation would provide methods outside of its concept.

I was thinking about a method that would either wp_insert_user or wc_create_new_customer – but on a second thought it should not be used here because at this point we only wanna handle customer creation, not user creation. I was overthinking it.

Still, I think it'd be more readable if Reader_Activation - in this code block - only set metadata on the created user, not take over the user creation and flow. Something like:

$user_id = wc_create_new_customer … if ( Reader_Activation::is_enabled() ) { Reader_Activation::setup_reader( $user_id ); } wp_set_current_user …

But it's not a blocker.

A "reader" is a fairly flexible concept:

https://github.com/Automattic/newspack-plugin/blob/feat/reader-activation-donate-block/includes/class-reader-activation.php#L131-L154

Currently any subscriber or customer is eligible to use the reader-related tools, including register_reader( $email ). What happens when we call setup_reader( $user_id )?

To clarify on why we have the user meta to also determine the same thing: the user roles that determine a reader are filterable. If by any chance this is filtered to an empty array, we must still be able to know readers that were created through this flow.

What happens when we call setup_reader( $user_id )?

The meta fields are set up. Actually, it can be register_reader – it can be called with an existing user ID.

That's an interesting idea, we should probably do it in a separate method to not necessarily trigger all the side effects of a reader registration if we just want to set the user as a reader.

If you don't mind, I think we could work on that on a separate PR, once we have more practical use on other cases.

includes/class-reader-activation.php

miguelpeixe · 2022-05-25T12:26:03Z

634c385 (with a typo fix at b0e8696) adds an additional test to assert that a regular subscriber is also a reader.

miguelpeixe · 2022-05-25T14:01:09Z

Another small change with c052066. Use wp_set_auth_cookie() $remember by default. We want that auth to last for a better overall reader experience.

adekbadek

One renaming was neglected, otherwise it's looking great! Good to see some tests, too.

includes/class-reader-activation.php

adekbadek · 2022-05-27T10:26:42Z

includes/reader-revenue/class-woocommerce-connection.php

+			} else {
+				Logger::log( 'This order will result in a membership, creating account for user.' );
+				$user_login = sanitize_title( $full_name );
+				$user_id    = wc_create_new_customer( $email_address, $user_login, '', [ 'display_name' => $full_name ] );


I was thinking about a method that would either wp_insert_user or wc_create_new_customer – but on a second thought it should not be used here because at this point we only wanna handle customer creation, not user creation. I was overthinking it.

Still, I think it'd be more readable if Reader_Activation - in this code block - only set metadata on the created user, not take over the user creation and flow. Something like:

$user_id = wc_create_new_customer … if ( Reader_Activation::is_enabled() ) { Reader_Activation::setup_reader( $user_id ); } wp_set_current_user …

But it's not a blocker.

miguelpeixe · 2022-05-27T15:30:58Z

Thank you for reviewing, @adekbadek! 🙇

# [1.84.0-alpha.1](v1.83.1...v1.84.0-alpha.1) (2022-06-02) ### Bug Fixes * **reader-revenue:** initial order state with total of 0 ([7c30b09](7c30b09)) ### Features * **ads:** handle gam default ad units ([#1654](#1654)) ([321b98e](321b98e)) * reader registration on donate ([#1655](#1655)) ([5821b57](5821b57)) * remove theme selection from setup wizard ([#1656](#1656)) ([94e4580](94e4580))

matticbot · 2022-06-02T13:49:57Z

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

# [1.84.0](v1.83.3...v1.84.0) (2022-06-13) ### Bug Fixes * **ads:** resolve conflicts from hotfix merge ([#1685](#1685)) ([8ce12cd](8ce12cd)) * **reader-revenue:** initial order state with total of 0 ([7c30b09](7c30b09)) ### Features * **ads:** handle gam default ad units ([#1654](#1654)) ([321b98e](321b98e)) * **analytics:** automatically link GA4 with Site Kit ([#1698](#1698)) ([266135f](266135f)) * reader registration on donate ([#1655](#1655)) ([5821b57](5821b57)) * remove theme selection from setup wizard ([#1656](#1656)) ([94e4580](94e4580))

matticbot · 2022-06-13T19:57:46Z

🎉 This PR is included in version 1.84.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

miguelpeixe and others added 11 commits May 19, 2022 16:40

feat: reader activation on donate integration

84d276d

fix: allow prevent auth cookie to be sent

08c2800

fix: tests

065b3f4

fix: tests

5bd3870

fix: tests

7f395f5

fix: tests

d664a15

fix: email

6a916ea

fix: is reader verified method

4ee673f

fix: tests

54fd5e2

fix: tests

61f047a

fix: tests

f15f752

miguelpeixe requested a review from a team as a code owner May 19, 2022 23:13

miguelpeixe self-assigned this May 19, 2022

miguelpeixe added the [Status] Needs Review The issue or pull request needs to be reviewed label May 19, 2022

chore: cleanup

26c1af9

adekbadek self-requested a review May 20, 2022 15:22

adekbadek requested changes May 24, 2022

View reviewed changes

miguelpeixe added 6 commits May 24, 2022 11:53

fix: clear auth intention on auth cookie

7242930

fix: remove $notify from reader registration

8bb6997

fix: verify reader on password_reset action

b310d17

fix: remove email verification option from auth

c7fcc8d

fix: update test method

2e90fae

fix: preserve incoming userId

b78f646

miguelpeixe requested a review from adekbadek May 24, 2022 18:52

miguelpeixe added 2 commits May 25, 2022 09:23

test: subscriber is also a reader

634c385

fix: typo

b0e8696

feat: use auth remember by default

c052066

adekbadek requested changes May 27, 2022

View reviewed changes

miguelpeixe requested a review from adekbadek May 27, 2022 12:14

fix: rename get_auth_intention_value() for verbosity

09dc78b

adekbadek approved these changes May 27, 2022

View reviewed changes

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 May 27, 2022

miguelpeixe merged commit 5821b57 into master May 27, 2022

miguelpeixe deleted the feat/reader-activation-donate-block branch May 27, 2022 15:31

miguelpeixe mentioned this pull request May 27, 2022

feat: magic links #1668

Merged

6 tasks

matticbot added the released on @alpha label Jun 2, 2022

matticbot added the released label Jun 13, 2022

miguelpeixe mentioned this pull request Jun 23, 2022

feat: reader activation and registration block #1643

Closed

6 tasks

adekbadek mentioned this pull request Jun 30, 2022

Segmentation: handle pre-authentication Automattic/newspack-popups#903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: reader registration on donate #1655

feat: reader registration on donate #1655

miguelpeixe commented May 19, 2022 •

edited

Loading

yogeshbeniwal commented May 20, 2022

miguelpeixe commented May 20, 2022

adekbadek left a comment

adekbadek May 24, 2022

miguelpeixe May 24, 2022

adekbadek May 27, 2022

miguelpeixe May 27, 2022

miguelpeixe May 27, 2022

adekbadek May 27, 2022

miguelpeixe May 27, 2022

miguelpeixe commented May 25, 2022

miguelpeixe commented May 25, 2022

adekbadek left a comment

adekbadek May 27, 2022

miguelpeixe commented May 27, 2022

matticbot commented Jun 2, 2022

matticbot commented Jun 13, 2022

feat: reader registration on donate #1655

feat: reader registration on donate #1655

Conversation

miguelpeixe commented May 19, 2022 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

Auth intention

The reader

Email verification

Experimental flag

How to test the changes in this Pull Request:

Other information:

yogeshbeniwal commented May 20, 2022

miguelpeixe commented May 20, 2022

adekbadek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelpeixe commented May 25, 2022

miguelpeixe commented May 25, 2022

adekbadek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelpeixe commented May 27, 2022

matticbot commented Jun 2, 2022

matticbot commented Jun 13, 2022

miguelpeixe commented May 19, 2022 •

edited

Loading