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.

Sign up for GitHub

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

Jump to bottom

EZP-30567: Impl. ez_query:pagingQueryAction to handle pagination #2651

Merged

alongosz merged 2 commits into ezsystems:master from jlchassaing:EZP-30567

Oct 24, 2019

Contributor

jlchassaing commented May 20, 2019 •

edited by micszo

Loading

Question	Answer
JIRA issue	EZP-30567
Improvement	yes
New feature	yes
Target version	`master (8.0@dev)` for eZ Platform `v3.0`.
BC breaks	no
Tests pass	yes
Doc needed	yes

The ez_query:pagingQueryAction will return a pagerFanta object set to the assign_to_result parameter.

alongside with the assign_to_result param as described a limit parameter can be added to set the page number of items.

QA

Configure and test results of ez_query:pagingQueryAction Query controller (very well described by the Behat tests in the PR).

TODO:

Implement ez_query:pagingQueryAction.
Implement tests.
Fix new code according to Coding Standards ($ composer fix-cs).
Ask for Code Review.

andrerom reviewed

View reviewed changes

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

andrerom reviewed

View reviewed changes

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

andrerom reviewed

View reviewed changes

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

andrerom reviewed

View reviewed changes

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

Contributor

andrerom commented May 21, 2019

ping @bdunogier

andrerom reviewed

View reviewed changes

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

alongosz changed the title ~~add pagingQueryAction to ez_query controler to return a pagerFanta result using a query_type~~ EZP-30567: Impl. ez_query:pagingQueryAction to handle pagination

alongosz requested changes

View reviewed changes

Member

alongosz left a comment

Target version 7.x

Current master is 8.0. Feature development process for 7.x has been closed. Just mentioning this so there's no surprise once the feature is merged.

I see that you've skipped most of the points from the checklist. They would need to be addressed before we can process the PR.

Besides:

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

Contributor Author

jlchassaing commented May 29, 2019

@alongosz thank you for your comments. I fixed the comments, ran fix-cs and did code changes. This feature is really useful and I think it could be merged to 8.x release.

alongosz reviewed

View reviewed changes

Member

alongosz left a comment •

edited

Loading

There's one outstanding item from the checklist - implement tests. AFAICS for Query Controller we have BDD tests so something similar would be the best choice. Test also allows you to clearly state what your feature does, so it's good for more than one reason.

I also looked deeper at the implementation and have one more remark:

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

jlchassaing force-pushed the EZP-30567 branch from d6bca97 to 2450daf Compare

June 1, 2019 08:05

andrerom approved these changes

View reviewed changes

Contributor

andrerom commented Jun 3, 2019

Nice work on the tests @jlchassaing 👍

Contributor

SylvainGuittard commented Jun 3, 2019

Thanks for your contribution @jlchassaing

alongosz reviewed

View reviewed changes

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

crevillo reviewed

View reviewed changes

eZ/Publish/Core/MVC/Symfony/Controller/Content/QueryController.php Outdated Show resolved Hide resolved

Contributor Author

jlchassaing commented Jun 6, 2019

Does someone have a clue why I'm getting this travis fail ?

bdunogier approved these changes

View reviewed changes

Member

bdunogier left a comment •

edited

Loading

Looks good to me, I like the feature !

Added a suggestion for a simplification of the behat test, but it's not a must-have. It would shorten the file a lot though.

Also, I was wondering if handling an option in the existing controller would have been a better choice than a separate controller.

eZ/Bundle/EzPublishCoreBundle/Features/QueryController/query_controller.feature Outdated Show resolved Hide resolved

jlchassaing force-pushed the EZP-30567 branch from 47cbfbd to 42decc8 Compare

June 11, 2019 12:35

ViniTou requested a review from alongosz

October 17, 2019 14:11

alongosz approved these changes

View reviewed changes

alongosz added Feature Ready for QA labels

micszo self-assigned this

alongosz force-pushed the EZP-30567 branch from 42decc8 to fc2ed5a Compare

October 18, 2019 12:54

alongosz mentioned this pull request

EZP-30689: Registered Query Types for automatic configuration #2834

Merged

8 tasks

Member

alongosz commented Oct 18, 2019 •

edited

Loading

Status: ~~blocked by CI, partially solved via #2834, trying to fix failing new tests.~~

alongosz force-pushed the EZP-30567 branch 2 times, most recently from e575cbb to 0cd65dc Compare

October 22, 2019 14:37


          Implemented QueryController:pagingQueryAction to handle pagination

d717b38

QueryController pagingQueryAction returns query results as a
Pagerfanta instance

Co-Authored-By: André R. <[email protected]>

alongosz force-pushed the EZP-30567 branch from 0cd65dc to 0f66354 Compare

October 22, 2019 14:51


          Added Behat tests coverage for QueryController:pagingQueryAction

420e8c2

Co-Authored-By: Andrew Longosz <[email protected]>

alongosz force-pushed the EZP-30567 branch from 0f66354 to 420e8c2 Compare

October 23, 2019 07:55

Member

alongosz commented Oct 23, 2019

@micszo you can proceed with QA here.

micszo approved these changes

View reviewed changes

Member

micszo left a comment

QA Approved on eZ Platform EE master.

micszo added QA approved and removed Ready for QA labels

micszo removed their assignment

alongosz merged commit 83b375d into ezsystems:master

Member

alongosz commented Oct 24, 2019

Thank you @jlchassaing 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bdunogier bdunogier approved these changes

crevillo crevillo left review comments

andrerom andrerom approved these changes

alongosz alongosz approved these changes

micszo micszo approved these changes

Labels

Feature QA approved