-
Notifications
You must be signed in to change notification settings - Fork 800
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
Add form responses v2 endpoint #29043
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run
to get started. More details: p9dueE-5Nn-p2 |
…kage. Mapped through from .com
342844c
to
3e9e495
Compare
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.
Works for me! Code looks clean too, just left one comment but nothing blocking 👍
projects/packages/forms/src/class-wpcom-rest-api-v2-endpoint-forms.php
Outdated
Show resolved
Hide resolved
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.
Nice job, Christian!
Left a couple inline comments. Many thanks for working on this!
projects/packages/forms/src/class-wpcom-rest-api-v2-endpoint-forms.php
Outdated
Show resolved
Hide resolved
projects/packages/forms/src/class-wpcom-rest-api-v2-endpoint-forms.php
Outdated
Show resolved
Hide resolved
projects/packages/forms/src/class-wpcom-rest-api-v2-endpoint-forms.php
Outdated
Show resolved
Hide resolved
projects/packages/forms/src/class-wpcom-rest-api-v2-endpoint-forms.php
Outdated
Show resolved
Hide resolved
projects/packages/forms/src/class-wpcom-rest-api-v2-endpoint-forms.php
Outdated
Show resolved
Hide resolved
projects/packages/forms/src/class-wpcom-rest-api-v2-endpoint-forms.php
Outdated
Show resolved
Hide resolved
projects/packages/forms/src/class-wpcom-rest-api-v2-endpoint-forms.php
Outdated
Show resolved
Hide resolved
@fgiannar all wonderful suggestions, thanks! I think I've addressed them all. I did search for more explicit docs on those |
Excellent work, thank you! I only have two more suggestions:
https://shawnhooper.ca/2017/02/15/wp-rest-secrets-found-reading-core-code/ |
Oh boy, there was a
I've been scouting how to do so, but a lot of the rest testing libs reside only on WPCOM. We're looking to have better/more testing on the package, maybe even bring some of the libs to the package to be able to do so, but I think we can keep at it as continuous effort on further iterations. I'll look on other packages to see how they got it, but for now I'd like to move this forward. Thanks for the reviews! |
return $site_id; | ||
} | ||
|
||
if ( ! current_user_can( 'manage_options' ) || ! is_user_member_of_blog( get_current_user_id(), $site_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.
With current_user_can( 'manage_options' )
in place, then is_user_member_of_blog( get_current_user_id(), $site_id )
becomes redundant, right?
In that case we don't need the $site_id
as well I guess.
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.
User might be member of blog but not have management capabilities? Is that not a possible scenario?
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.
No worries, doesn't hurt anyway :)
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 followed up on this conversation in #35571 after discussion in p1707435111778979-slack-C05PV073SG3
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 for addressing all my comments! Appreciate your effort 👍
* [not verified] add form-responses endpoint to be handled by forms package. Mapped through from .com * [not verified] changelog * fix typo on class name * [not verified] add backend code for v2/v4 endpoint * changelog * fixup project versions * use wpcom/v2 instead of jetpack/v4 * update apiRoot backend config JS * remove unnecessary plugin endpoint file, add api-fetch dependency for testing * rename controller file to reflect forms base path * instance the right class name after change * fix route register handler name * edit comments/docblocks * use simpler validation through args props * use Jetpack\Connection\Manager to get site ID, remove unneeded code and is_wpcom var * get the date from post, not from parsed content * add capability check to the endpoint permission callback
* [not verified] add form-responses endpoint to be handled by forms package. Mapped through from .com * [not verified] changelog * fix typo on class name * [not verified] add backend code for v2/v4 endpoint * changelog * fixup project versions * use wpcom/v2 instead of jetpack/v4 * update apiRoot backend config JS * remove unnecessary plugin endpoint file, add api-fetch dependency for testing * rename controller file to reflect forms base path * instance the right class name after change * fix route register handler name * edit comments/docblocks * use simpler validation through args props * use Jetpack\Connection\Manager to get site ID, remove unneeded code and is_wpcom var * get the date from post, not from parsed content * add capability check to the endpoint permission callback
* [not verified] add form-responses endpoint to be handled by forms package. Mapped through from .com * [not verified] changelog * fix typo on class name * [not verified] add backend code for v2/v4 endpoint * changelog * fixup project versions * use wpcom/v2 instead of jetpack/v4 * update apiRoot backend config JS * remove unnecessary plugin endpoint file, add api-fetch dependency for testing * rename controller file to reflect forms base path * instance the right class name after change * fix route register handler name * edit comments/docblocks * use simpler validation through args props * use Jetpack\Connection\Manager to get site ID, remove unneeded code and is_wpcom var * get the date from post, not from parsed content * add capability check to the endpoint permission callback
This is a follow-up to #29043. When registering the endpoint, we used the wrong parameter to define permissions.
Proposed changes:
This PR adds wpcom/v2/form-responses endpoint as the target for WPCOM's D102076-code
Other information:
Does this pull request change what data or activity we track or use?
Testing instructions:
On docker, add these 2 lines on
plugins/jetpack/modules/contact-form.php
:Then head to http://localhost/wp-admin/admin.php?page=jetpack-forms to get a mock of the new inbox page. Open devtools and type this on the console:
You should get a list of form responses.