Skip to content
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

Pattern Directory API: Add support for pagination parameters #45293

Merged
merged 10 commits into from
Nov 8, 2022

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Oct 25, 2022

This PR enables passing pagination parameters through to the pattern directory API request. In meta-12147, the hard-coded per_page and order values were removed, allowing paginated queries. But the connecting API in WordPress/Gutenberg doesn't support those - yet.

The pattern directory explorer here #45237 will need these parameters to support pagination and ordering patterns using the same sort as wordpress.org/patterns.

This PR sets up an allowed-list of query parameters that are passed through to api.wordpress.org. If/when more are needed, they can be added to the list.

Testing Instructions

  1. Ensure the remote patterns are still loading in the editor — the Featured query, the core patterns, and any theme-added patterns.

Test the API endpoint in a REST client.

  1. Try pagination, ex: /wp-json/wp/v2/pattern-directory/patterns/?per_page=6&page=2 — It should show the middle 6 patterns on wordpress.org/patterns/.
  2. Try order: /wp-json/wp/v2/pattern-directory/patterns/?category=37&per_page=3&orderby=date&order=asc — should show the last 3 items on wordpress.org/patterns/categories/footer/page/2/
  3. Make sure search still works /wp-json/wp/v2/pattern-directory/patterns/?search=plants — should match wordpress.org/patterns/search/plants/
  4. Make sure slug still works /wp-json/wp/v2/pattern-directory/patterns/?slug=centered-text-title-with-two-columns — should return a single pattern

@ryelle ryelle added the [Feature] Pattern Directory The Pattern Directory, a place to find patterns label Oct 25, 2022
@ryelle ryelle self-assigned this Oct 25, 2022
Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @ryelle! Just noting that with php changes we will also need some tests.

@ryelle ryelle force-pushed the update/pattern-directory-api-params branch from 6bfc372 to d3204a6 Compare October 28, 2022 16:34
Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ryelle, this is really close! We still need to add php tests for these changes.

lib/compat/wordpress-6.2/rest-api.php Outdated Show resolved Hide resolved
lib/compat/wordpress-6.2/rest-api.php Outdated Show resolved Hide resolved
@ryelle
Copy link
Contributor Author

ryelle commented Oct 31, 2022

We still need to add php tests for these changes.

Is there an example or documentation somewhere about what tests exactly are needed?

@ntsekouras
Copy link
Contributor

We still need to add php tests for these changes.

Is there an example or documentation somewhere about what tests exactly are needed?

I'm not sure about where the docs could be, but you can see the current tests in core here: https://github.com/WordPress/wordpress-develop/blob/trunk/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php., and a GB example of overriding just a few tests is: https://github.com/WordPress/gutenberg/blob/trunk/phpunit/class-gutenberg-rest-block-patterns-controller-test.php.

So you create a new file for the controller in phpunit folder and add the tests for the new/updated things in your PR. You'll need similar tests to test_get_items_search - for example for per_page you'll have to set that prop to the request and then test the returned results.

I'm not sure we need tests for get_collection_params change here. I couldn't find related tests in core, but I think there is something like a snapshot that will need updating when porting.. --cc @spacedmonkey

@ryelle
Copy link
Contributor Author

ryelle commented Nov 2, 2022

I've added tests, but I don't know if I'm being too clever about it — instead of setting up a bunch of mocked responses, I'm capturing the requested wp.org API URL and confirming that the parameters passed in match those requested from wp.org. The other way (the way the current get_items_* tests are) would require a lot of fixtures and also seems more like testing the tests, not checking that the request is correct.

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the tests and your work @ryelle!

instead of setting up a bunch of mocked responses, I'm capturing the requested wp.org API URL and confirming that the parameters passed in match those requested from wp.org.

I think we should do it like core does and add some fixtures. It should be possible to just update the search fixture(copy from core first and append some items if needed). I don't think we should need to add many more data or new fixtures(if any).

@ryelle
Copy link
Contributor Author

ryelle commented Nov 3, 2022

I think we should do it like core does and add some fixtures. It should be possible to just update the search fixture(copy from core first and append some items if needed). I don't think we should need to add many more data or new fixtures(if any).

Maybe I'm misunderstanding how the tests work, but I thought it would be a fixture per test-case? At least, it seems that way from the get_raw_response function, because each mocked response matches the endpoint requested from wp.org.

This is also why the core tests feel like testing the tests — it's mocking the response, then making sure the response is what is expected, not making sure the request is the expected request. Making sure the response matches the schema makes sense in test_get_items, but feels redundant in the other tests. It's possible I'm thinking about it wrong, though. @iandunn, you wrote these tests initially, do you have any insight here? (it was quite a while ago…)

@ntsekouras
Copy link
Contributor

This is also why the core tests feel like testing the tests — it's mocking the response, then making sure the response is what is expected, not making sure the request is the expected request. Making sure the response matches the schema makes sense in test_get_items, but feels redundant in the other tests.

TBH I didn't look extensively the other tests because I'm travelling right now, but what you say makes sense. I'll take a closer look as soon as I can.

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much @ryelle, looks good!

markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Jan 24, 2023
…ctory_Controller`.

Adds pagination and ordering support to `WP_REST_Pattern_Directory_Controller` by allow listing `'per_page'`, `'page'`, `'offset'`, `'order'`, and `'orderby'` query parameters. This change enables pagination and ordering features in the pattern directory explorer by using the same sort as wordpress.org/patterns.

Reference:
* [WordPress/gutenberg#45293 Gutenberg PR 45293]

Follow-up to [55098], [51206], [51021].

Props ntsekouras, ryelle, arrasel403, hellofromTonya, ironprogrammer, mukesh27, robinwpdeveloper.
Fixes #57501.
Built from https://develop.svn.wordpress.org/trunk@55132


git-svn-id: http://core.svn.wordpress.org/trunk@54665 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Jan 24, 2023
…ctory_Controller`.

Adds pagination and ordering support to `WP_REST_Pattern_Directory_Controller` by allow listing `'per_page'`, `'page'`, `'offset'`, `'order'`, and `'orderby'` query parameters. This change enables pagination and ordering features in the pattern directory explorer by using the same sort as wordpress.org/patterns.

Reference:
* [WordPress/gutenberg#45293 Gutenberg PR 45293]

Follow-up to [55098], [51206], [51021].

Props ntsekouras, ryelle, arrasel403, hellofromTonya, ironprogrammer, mukesh27, robinwpdeveloper.
Fixes #57501.
Built from https://develop.svn.wordpress.org/trunk@55132


git-svn-id: https://core.svn.wordpress.org/trunk@54665 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@bph bph added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Feb 6, 2023
@bph
Copy link
Contributor

bph commented Feb 6, 2023

Flagged it for Needs Dev Note to have a blurb in the 'Miscellaneous Dev Note', maybe.

@bph bph mentioned this pull request Feb 6, 2023
47 tasks
VenusPR added a commit to VenusPR/Wordpress_Richard that referenced this pull request Mar 9, 2023
…ctory_Controller`.

Adds pagination and ordering support to `WP_REST_Pattern_Directory_Controller` by allow listing `'per_page'`, `'page'`, `'offset'`, `'order'`, and `'orderby'` query parameters. This change enables pagination and ordering features in the pattern directory explorer by using the same sort as wordpress.org/patterns.

Reference:
* [WordPress/gutenberg#45293 Gutenberg PR 45293]

Follow-up to [55098], [51206], [51021].

Props ntsekouras, ryelle, arrasel403, hellofromTonya, ironprogrammer, mukesh27, robinwpdeveloper.
Fixes #57501.
Built from https://develop.svn.wordpress.org/trunk@55132


git-svn-id: http://core.svn.wordpress.org/trunk@54665 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Pattern Directory The Pattern Directory, a place to find patterns Needs Dev Note Requires a developer note for a major WordPress release cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants