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-31226: Added new integration test for embedded content #2922

Merged
merged 6 commits into from
Jan 31, 2020
Merged

EZP-31226: Added new integration test for embedded content #2922

merged 6 commits into from
Jan 31, 2020

Conversation

michal-myszka
Copy link
Contributor

@michal-myszka michal-myszka commented Jan 21, 2020 •
edited by alongosz
Loading

Question Answer
JIRA issue EZP-31226
Bug/Improvement yes
New feature no
Target version 7.5
BC breaks no
Tests pass yes
Doc needed no

Solution for reported bug:
https://jira.ez.no/browse/EZP-31226
Fixed missing "depth" parameters and added new test for fulltext search for embed content
Changes also in ezplatform-solr-search-engine (ezsystems/ezplatform-solr-search-engine#165)

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.
  • Make an "integration" between changes from above PR to pass the build tests

@michal-myszka michal-myszka self-assigned this Jan 22, 2020
@michal-myszka michal-myszka mentioned this pull request Jan 22, 2020
Merged
6 tasks
alongosz
alongosz requested changes Jan 22, 2020
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 to see your contributions already 🎉 💪

It's a good chance for me to explain a few things ;)

Naming conventions:
PR title should start with JIRA ticket number (e.g. EZP-0000) followed by colon and a brief summary of what has been changed in the current codebase, in past simple. Copying titles from JIRA is not very useful for git history. They land in Release Notes in such form anyway.

The commits should follow the same convention, but if there's more than one commit, JIRA issue is not necessary there as they get squashed. Multiple commits which repeat the same message are useless. Please state what you did per commit.

In general, providing RichText-based tests here is a maintenance issue, which I've explained in one of the following diff comments.

.gitignore Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
phpunit-integration-legacy-solr.xml Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
@michal-myszka michal-myszka changed the title Ezp 31226 indexing depth doesnt work with location query EZP-31226: Added new integration test for embedded content Jan 22, 2020
@michal-myszka michal-myszka force-pushed the EZP-31226-indexing-depth-doesnt-work-with-location-query branch from a726da4 to 1cb1bdb Compare January 22, 2020 13:47
@michal-myszka
Copy link
Contributor Author

Thank you for your accurate comment @alongosz , it is really helpful !

@michal-myszka michal-myszka force-pushed the EZP-31226-indexing-depth-doesnt-work-with-location-query branch from 1cb1bdb to fa7ecc2 Compare January 22, 2020 15:28
alongosz
alongosz requested changes Jan 29, 2020
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.

More review (github interface is broken, sorry):

eZ/Publish/API/Repository/Tests/SearchService/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchService/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchService/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchService/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchService/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchService/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchService/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchService/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchService/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/SearchService/SearchServiceFulltextEmbedTest.php Outdated Show resolved Hide resolved
@michal-myszka michal-myszka force-pushed the EZP-31226-indexing-depth-doesnt-work-with-location-query branch from 4d28c9e to 9c5c4df Compare January 30, 2020 07:05
alongosz
alongosz requested changes Jan 30, 2020
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.

Looks almost good, one more request:

eZ/Publish/API/Repository/Tests/SearchService/SearchServiceFullTextEmbedTest.php Outdated Show resolved Hide resolved
@michal-myszka michal-myszka force-pushed the EZP-31226-indexing-depth-doesnt-work-with-location-query branch from 9c5c4df to 06fa787 Compare January 30, 2020 10:04
@micszo micszo self-assigned this Jan 30, 2020
@micszo
Copy link
Member

micszo commented Jan 31, 2020

@michal-myszka have you verified the CI failure here as well?

@adamwojs
Copy link
Member

@michal-myszka have you verified the CI failure here as well?

@micszo @michal-myszka Rebase is needed here. See #2928

@michal-myszka michal-myszka force-pushed the EZP-31226-indexing-depth-doesnt-work-with-location-query branch from 06fa787 to f80b953 Compare January 31, 2020 09:49
@michal-myszka
Copy link
Contributor Author

michal-myszka commented Jan 31, 2020 •
edited
Loading

@michal-myszka have you verified the CI failure here as well?

@micszo @michal-myszka Rebase is needed here. See #2928

I've made rebase, but this will take an effect on
ezsystems/ezplatform-solr-search-engine#165 .
After merge above 165 this PR's build should also pass

micszo
micszo approved these changes Jan 31, 2020
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.

Tested together with ezsystems/ezplatform-solr-search-engine#165.

@micszo micszo removed their assignment Jan 31, 2020
@alongosz alongosz merged commit 3cbdfbb into ezsystems:7.5 Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamwojs adamwojs adamwojs approved these changes

@alongosz alongosz alongosz approved these changes

@micszo micszo micszo approved these changes

@ViniTou ViniTou Awaiting requested review from ViniTou

Assignees

@michal-myszka michal-myszka

Labels
Bug QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@michal-myszka @micszo @adamwojs @alongosz @ViniTou