Simplify report controller parameters #1826
Merged
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
github-actions
bot
added
the
changelog: tweak
tomalec
reviewed
Dec 26, 2022
| @@ -39,7 +39,7 @@ public function __construct( ContainerInterface $container ) { | |||
| * @return array | |||
| */ | |||
| public function get_collection_params(): array { | |||
| $params = [ | |||
There was a problem hiding this comment.
❓
Shouldn't we call the parent here as well to get context, page, per_page, and search from there?
There was a problem hiding this comment.
I don't think we really can because while we try to reuse as much as possible from the parent controller, we get our data from the Google API instead of from a local DB. Which means some of those query parameters are different.
- context: we redefine this one, although we don't really use context much internally
- page: we can't reuse this because the Google API expects a next page token so we can't query a specific page
- per_page: we redefine this since the Google API sets some different limitations on how many records you can retrieve per request
- search: for the parent controller this is for searching for posts that match a particular search string (again this isn't really appropriate for our controllers)
tomalec
approved these changes
Dec 26, 2022
There was a problem hiding this comment.
Reviewed the code, smoke tested locally, and checked by fiddling with the debugger.
Left on a note, for a potential improvement #1826 (comment)
Merged
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.
Changes proposed in this Pull Request:
Based on the discussion here, this PR cleans up the report controllers to make the code reusable and take advantage of the existing function WP_REST_Request::get_default_params
Detailed test instructions:
Unit testing is still not implemented for the Report Controllers (I started an attempt, but it's going to take a substantial effort to unit test these classes so I left it out for now).
The reports can be tested manually as outlined in PR #251, although this PR doesn't change the report data so we would only want to confirm that the parameter defaults still get mapped in the same way.
The easiest way to test that is to set a break point in
prepare_query_argumentsand confirm that the arguments are as expected:A GET request without query parameters can be sent to
https://domain.test/wp-json/wc/gla/ads/reports/programs.Changelog entry