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.

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-30988: Implemented Section filtering URL Query Criterion #2783

Merged
merged 1 commit into from
Oct 22, 2019
Merged

EZP-30988: Implemented Section filtering URL Query Criterion #2783

merged 1 commit into from
Oct 22, 2019

Conversation

ITernovtsii
Copy link
Contributor

@ITernovtsii ITernovtsii commented Sep 30, 2019 •
edited by alongosz
Loading

Question Answer
JIRA issue EZP-30988
Bug/Improvement yes
New feature no
Target version 7.5.x for eZ Platform v2.5.x LTS
BC breaks no
Tests pass yes
Doc needed yes
  • Implement Section criteria for URL lookup
  • Fix slow query when filter by VisibleOnly criteria

// Edited by @alongosz:

QA

  • You can play with the test command to find URLs filtered by Content Section:
    ezsystems/ezplatform@2.5...alongosz:for-qa-ezp-30988-find-urls-cmd.
    Usage: php ./bin/console test:find-urls [<url1> <url2> ...]. No arguments returns all registered URLs. An URL is registered when e.g. added to Content RichText field or set via URL Field. By manipulating Content item Section you can test the feature.
  • Sanity checks for LinkManager (might be an overkill, because new code is not used there).

Doc

I can't find doc about API to find URLs. Either it's too late and I'm not seeing something obvious or indeed we need to document the whole thing (more than the scope of this feature).

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@ITernovtsii
Copy link
Contributor Author

tests failed in untouched code, all related tests pass

@adamwojs
Copy link
Member

adamwojs commented Oct 1, 2019

Hi @ITernovtsiy! Thank you for contribution, I will review changes later.

tests failed in untouched code, all related tests pass

tests failed in untouched code, all related tests pass

Solr tests should be fixed by #2784 (rebase is required)

@ITernovtsii ITernovtsii changed the base branch from master to 7.5 October 1, 2019 12:00
@ITernovtsii
Copy link
Contributor Author

rebased

adamwojs
adamwojs reviewed Oct 8, 2019
View reviewed changes
Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

Nice contribution🍺 Do you need this feature in the v2.5? I'm asking because according to the book new features should be targeted to master 😉

eZ/Publish/API/Repository/Values/URL/Query/Criterion/Section.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/Values/URL/Query/Criterion/SectionTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/Tests/URL/Query/CriterionHandler/SectionTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/URL/Query/CriterionHandler/Base.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/URL/Query/CriterionHandler/Base.php Outdated Show resolved Hide resolved
@adamwojs adamwojs changed the title EZP-30988 - As a Developer I want PHP API allow fetching URLs filtered by Section EZP-30988: As a Developer I want PHP API allow fetching URLs filtered by Section Oct 8, 2019
@ITernovtsii
Copy link
Contributor Author

@adamwojs Yes, we're using this with v2.5 and will be able to clean-up project code a little bit (if this can be merged into 2.5).
If not, v3 is okay. While we have this on one project only, it's not so critical :)
Let me know if I should rebase it with master.
Thanks.

@lserwatka
Copy link
Member

We can bring this to 2.5.

@alongosz alongosz self-assigned this Oct 18, 2019
@alongosz alongosz changed the title EZP-30988: As a Developer I want PHP API allow fetching URLs filtered by Section EZP-30988: Implemented Section filtering URL Query Criterion Oct 18, 2019
alongosz
alongosz requested changes Oct 18, 2019
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your contribution @ITernovtsiy 🎉

I'm sorry for the long delay, my queue is quite long...

Besides smaller issues, the solution needs usage & naming DX improvements. I've elaborated on this in the diffs for eZ/Publish/API/Repository/Values/URL/Query/Criterion/Section.php

eZ/Publish/API/Repository/Tests/Values/URL/Query/Criterion/SectionTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/Tests/URL/Query/CriterionHandler/SectionTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/URL/Gateway/DoctrineDatabase.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/URLServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Values/URL/Query/Criterion/Section.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/URL/Query/CriterionHandler/Base.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/URL/Query/CriterionHandler/Base.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/URL/Query/CriterionHandler/Section.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/URL/Query/CriterionHandler/VisibleOnly.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/URL/Query/CriterionHandler/VisibleOnly.php Outdated Show resolved Hide resolved
@ITernovtsii
Copy link
Contributor Author

Thanks for review @alongosz
I fixed code style, removed unit tests, renamed Section to be SectionId and introduced SectionIdentifier criteria. And, added more integration tests to cover them.
Please review once again and let me know if I missed anything.

@ITernovtsii ITernovtsii requested a review from alongosz October 18, 2019 17:20
alongosz
alongosz approved these changes Oct 18, 2019
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Great work @ITernovtsiy!

I'm sending this to our QA for tests.

@lserwatka
Copy link
Member

We will ship it in 2.5.7💪👌🎉well done to all involved!

@micszo micszo self-assigned this Oct 21, 2019
@micszo
Copy link
Member

micszo commented Oct 21, 2019 •
edited
Loading

Testing discovered: https://jira.ez.no/browse/EZP-31059.
That issue affects this feature happens with the test command also (but waiting for root cause).

EZP-31059 2

micszo
micszo approved these changes Oct 22, 2019
View reviewed changes
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

QA Approved on eZ Platform v2.5.6 with test command.

@micszo micszo removed their assignment Oct 22, 2019
@lserwatka lserwatka merged commit 888411b into ezsystems:7.5 Oct 22, 2019
@lserwatka
Copy link
Member

@alongosz could you merge it up?

@alongosz
Copy link
Member

@lserwatka Done.

Thank you @ITernovtsiy 🎉

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

@mikadamczyk mikadamczyk mikadamczyk left review comments

@SerheyDolgushev SerheyDolgushev SerheyDolgushev left review comments

@micszo micszo micszo approved these changes

@alongosz alongosz alongosz approved these changes

@adamwojs adamwojs adamwojs approved these changes

Assignees

@alongosz alongosz

Labels
Feature QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ITernovtsii @adamwojs @lserwatka @micszo @alongosz @SerheyDolgushev @mikadamczyk