-
Notifications
You must be signed in to change notification settings - Fork 3
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(membership-plans): improvements #110
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.
Yes, sorry! I've update the testing step. What's missing is the |
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.
Thanks @adekbadek!
Tested again and it looks to be working as expected 👍
I left a few small bits of feedback and questions, then this should be good to go.
public static function add_data_to_wc_user_membership_response( $response, $user, $request ) { | ||
if ( $request && isset( $request->get_headers()['x_np_network_signature'] ) ) { | ||
// Add network plan ID to the response. | ||
$plan = wc_memberships_get_membership_plan( $response->data['plan_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.
Looks like wc_memberships_get_membership_plan
can return false
, so we will probably need to check the value of $plan
before adding to response data.
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.
Fixed in a4ab6df
if ( \Newspack_Network\Admin::use_experimental_auditing_features() ) { | ||
$query_args['include_active_members_emails'] = 1; | ||
} | ||
$node_plans = self::fetch_collection_from_api( $node, 'wc/v2/memberships/plans', 'membership-plans', $query_args ); | ||
foreach ( $node_plans as $plan ) { |
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.
Getting the following warning on this line:
PHP Warning: foreach() argument must be of type array|object, null given in /Users/raz/Sites/newspack/network/includes/hub/admin/class-membership-plans.php on line 129
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.
Fixed in a4ab6df
$response = wp_remote_get( // phpcs:ignore | ||
$endpoint, | ||
[ | ||
'headers' => $node->get_authorization_headers( 'get-woo-' . $collection_endpoint_id ), | ||
'timeout' => 60, // phpcs:ignore |
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.
nonblocking nit: Does the default not work here?
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.
What do you mean? Default timeout is 5
.
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. Whoops. For some reason I thought this was 30
. Nevermind!
$order = $_REQUEST['order'] ?? false; // phpcs:ignore WordPress.Security.NonceVerification.Missing, WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | ||
$orderby = $_REQUEST['orderby'] ?? false; // phpcs:ignore WordPress.Security.NonceVerification.Missing, WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized |
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.
Can we sanitize these to address the WordPress.Security.ValidatedSanitizedInput.InputNotSanitized
warning?
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.
Done in a4ab6df
🎉 This PR is included in version 1.10.0-alpha.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.10.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
All Submissions:
Changes proposed in this Pull Request:
Some tweaks to the Membership Plans table:
(Note: this is a part of integrating the changes from the
data-integrity-improvements
branch (#89))How to test the changes in this Pull Request:
NEWSPACK_NETWORK_EXPERIMENTAL_AUDITING_FEATURES
env variable totrue
Other information: