Allow non-account-owner admin users to submit the payment activity survey #8739
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ount-owner users to submit survey
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
…is the Jetpack connection owner
Jinksi
commented
May 1, 2024
| @@ -49,7 +49,7 @@ public function init_hooks() { | |||
| * @param array $args - The arguments to passed to Jetpack. | |||
| * @param string $body - The body passed on to the HTTP request. | |||
| * @param bool $is_site_specific - If true, the site ID will be included in the request url. Defaults to true. | |||
| * @param bool $use_user_token - If true, the request will be signed with the user token rather than blog token. Defaults to false. | |||
| * @param bool $use_user_token - If true, the request will be signed with the Jetpack connection owner user token rather than blog token. Defaults to false. | |||
There was a problem hiding this comment.
I've added this distinction here, which would have helped me resolve this issue.
There was a problem hiding this comment.
Should we change the name of this param? user_token seems misleading.
shendy-a8c
approved these changes
May 1, 2024
There was a problem hiding this comment.
Great work @Jinksi. It tested well.
It's a bit hacky to submit the survey as the user who established the Jetpack connection. I was expecting that we can send the request as a blog instead of as a user but maybe the marketing/survey end-point expects the request to be user-specific.
@shendy-a8c I'm guessing by your approval that you're not too concerned about the hacky-ness of this approach. Do you think we should explore an alternative? I don't see this as a concern, since we are not observing user-specific data (only blogid). |
That's correct. Just an observation that the |
haszari
reviewed
Aug 19, 2024
| @@ -63,33 +63,42 @@ public function test_empty_rating_returns_400_status_code() { | |||
| $this->assertEquals( 400, $response->get_status() ); | |||
| } | |||
|
|
|||
|
|
|||
| public function test_valid_request_forwards_data_to_jetpack() { | |||
There was a problem hiding this comment.
How important and useful is this e2e test, considering that the survey has a limited lifespan?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #8710
Changes proposed in this Pull Request
This PR allows non-account-owner WP admin users to submit the Payments activity card survey by sending the POST request as the account-owner user.
Account-owner in this context means the WP admin user who set up the WooPayments account and therefore the Jetpack connection.
The (soft) limit for submitting the survey remains once per site (soft, since deleting the WP option
wcpay_survey_payment_overview_submittedresets this limitation).Testing instructions
wp option update _wcpay_feature_payment_overview_widget 1in CLI (npm run cliif docker)wp option delete wcpay_survey_payment_overview_submittedto clear your local submission state (if survey submitted already).Payments > Overview.🙌 We appreciate your feedback!✅Payments > Overviewpage and ensure that the survey is not rendered, since it has been submitted.Payments > Overview.wp option delete wcpay_survey_payment_overview_submittedto clear your local submission state.Payments > Overviewpage and ensure the survey is rendered.🙌 We appreciate your feedback!✅npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge