Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: reader registration on donate #1655
feat: reader registration on donate #1655
Changes from 21 commits
84d276d
08c2800
065b3f4
5bd3870
7f395f5
d664a15
6a916ea
4ee673f
54fd5e2
61f047a
f15f752
26c1af9
7242930
8bb6997
b310d17
c7fcc8d
2e90fae
b78f646
634c385
b0e8696
c052066
09dc78b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 mainNewspack
class.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about a method that would either
wp_insert_user
orwc_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:But it's not a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 callsetup_reader( $user_id )
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The meta fields are set up. Actually, it can be
register_reader
– it can be called with an existing user ID.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.