-
Notifications
You must be signed in to change notification settings - Fork 58
feat(group-subscriptions): handle group subscription members #4397
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
Conversation
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.
Pull request overview
This PR implements the business logic for managing group subscription members in Newspack. It adds functionality to associate user IDs with group subscriptions, retrieve members/managers for subscriptions, and check user access to group subscriptions.
Changes:
- Added member_ids field to subscription settings and UI for admins to manage group members via autocomplete
- Implemented utility methods for checking group subscription status, retrieving members/managers, and verifying user access
- Added AJAX handler for searching reader users to add as group members
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
includes/plugins/woocommerce-subscriptions/class-group-subscriptions.php
Outdated
Show resolved
Hide resolved
|
@miguelpeixe this should be ready for another look. There are some substantial changes compared to the first review:
|
miguelpeixe
left a comment
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.
Looking great! Thank you for revising the approach. Besides copilot's feedback, I just have a few more comments.
| \add_meta_box( | ||
| 'newspack-group-subscription', | ||
| __( 'Group subscription', 'newspack-plugin' ), | ||
| [ __CLASS__, 'add_group_subscription_options' ], | ||
| $post_type, | ||
| 'normal', | ||
| 'high' | ||
| ); |
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.
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.
Good call, I had dragged it to another location at some point and forgot about the default placement. c00655e adds the metabox on priority 26 so that it appears underneath the "line items" meta box by default:
...s/plugins/woocommerce-subscriptions/group-subscription/class-group-subscription-settings.php
Show resolved
Hide resolved
| $users = array_map( | ||
| function( $user ) { | ||
| return [ | ||
| 'id' => $user->ID, | ||
| 'text' => $user->user_email . ' (#' . $user->ID . ')', | ||
| ]; | ||
| }, | ||
| array_merge( $query1, $query2 ) | ||
| ); |
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.
Indeed, I'm getting duplicated users in my tests.
| /** | ||
| * Update the member IDs for a group subscription. | ||
| * | ||
| * @param WC_Subscription|int $subscription The subscription object or ID. | ||
| * @param int[] $members_to_add Group member user IDs to add the subscription. | ||
| * @param int[] $members_to_remove Group member user IDs to remove from the subscription. | ||
| * | ||
| * @return object|WP_Error Added/removed results. | ||
| */ | ||
| public static function update_members( $subscription, $members_to_add, $members_to_remove = [] ) { |
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.
Non-blocking NIT, but do we need an API designed for bulk operations like that? A simpler CRUD approach is more convenient and easier to maintain.
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 initially wrote this as a bulk update method because of the original UI as a multi-select. I kept it because I thought it might be useful if we decide to implement a CLI command to bulk add/remove members for convenience in the future. Maybe this can be improved to abstract the add/remove actions into separate methods, though.
| /** | ||
| * Check if a user has access to a group subscription. | ||
| * | ||
| * @param int $user_id The user ID. | ||
| * @param WC_Subscription|int $subscription The subscription object or ID. | ||
| * | ||
| * @return bool|null Whether the user has access to the group subscription, or null if not a group subscription. | ||
| */ | ||
| public static function user_can_access( $user_id, $subscription ) { |
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.
This is also non-blocking: I'm not sure user_can_access is the best name here. "Access" in the context of the subscription alone makes me think of access to something (like content), which is not a concern here.
Even though we include members and "managers", I find user_is_member() to be more straightforward.
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.
Updated in 6f43ff8
| if ( ! self::is_group_subscription( $subscription ) ) { | ||
| return null; | ||
| } | ||
| $can_access = in_array( $user_id, self::get_managers( $subscription ), true ) || in_array( $user_id, self::get_members( $subscription ), true ); |
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 query inside get_members() is not optimal for this.
We could go straight to the meta item with select 1 from {$usermeta} where user_id = '{$user_id}' and meta_key = '{$meta_key}' and meta_value = '{$subscription_id}' limit 1.
Or, to benefit from object caching, filter through get_user_meta( $user_id, self::GROUP_SUBSCRIPTION_USER_META_KEY, false ).
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.
Ah, I never updated this method after we switched the data to user meta. f194e06 updates it so that we use get_group_subscriptions_for_user() (which itself calls get_user_meta() instead of running a user meta query.
|
@thomasguillot as discussed on the content gating call, tagging you for a design review of the core UI proposed in this PR. If you think we should go in a very different direction for the UI in either the product or subscription admin pages (or both), then let's consider tackling that in a separate PR after this one lands. |
|
@miguelpeixe pushed some other fixes to address your feedback:
|
miguelpeixe
left a comment
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.
Thank you for the revisions! Changes look great and test well.
Just a minor question/suggestion that I missed in my previous review.
| foreach ( $subscription_ids as $subscription_id ) { | ||
| $subscription = \wcs_get_subscription( $subscription_id ); | ||
| if ( $subscription && $subscription->has_status( $subscription_statuses ) ) { | ||
| $subscriptions[] = $ids_only ? $subscription_id : $subscription; | ||
| } | ||
| } |
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.
Apologies for not noticing this sooner, but should this be a concern here? It adds complexity and additional queries to the method. Regardless of the subscription status, the user is still a member of it.
IMO, status validation should happen by the feature that uses it.
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.
Agreed, 4e265e7 simplifies that method so that it takes just one arg ($user_id) and always returns an array of WC_Subscription objects.
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 liked ids_only, though. It avoid running wcs_get_subscription() for every item if the caller knows it wants to match a particular subscription (like user_is_member())
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.
Gotcha, de139cd restores that arg
|
Thanks for the review and feedback, @miguelpeixe! |
|
Hey @dkoo, good job getting this PR merged! 🎉 Now, the Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label. If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label. Thank you! ❤️ |
|
Whoops, sorry, @thomasguillot! I forgot I tagged you on this before merging. If you do have any feedback on the core admin UI stuff here, let's handle changes in a new PR. You can comment on this PR with any feedback you have. Thanks! |

All Submissions:
Changes proposed in this Pull Request:
This PR adds the business logic for associating user IDs with a group subscription as group members, as well as getting the members for a given subscription and the group subscriptions for a given user ID. It also adds a feature missing from NPPD-1037, to allow admins to directly edit the members of any group subscription, which should aid in testing for this PR.
The UI in the subscription admin pages now perform operations via REST API instead of requiring
The logic to control content restriction by group subscription membership, as well as UI for group owners to invite and manage group members, will come in future PRs.
Closes NPPD-1119 and NPPD-1040.
How to test the changes in this Pull Request:
Group_Subscription::update_members()to support the admin flow of enabling the checkbox and immediately adding new members.)wp shell, test the new utility methods which will lay the groundwork for future Group Subscription features:Other information: