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

EZP-31287: Added FullText search Fields for Email and ISBN #3023

Merged
merged 7 commits into from
Oct 29, 2020
Merged

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Apr 21, 2020

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

Email address and ISBN fieldtypes were not searchable due to lacking FullTextField search field assignment.

Also citing @alongosz from the related PR comment (#2206 (comment)):

{So here's the thing: if I do a FullText search for the entire e-mail, e.g. holmes4@ez.no, I get all Content items containing parts of e-mail: holmes4 or ez or no. It looks like parser treats this as separate words.

{My expectation in this case would be to find one Content item with field matching exactly the given e-mail.
However Solr FullText search gets me the same result, so maybe my expectations are wrong?

The e-mail address was split into parts when indexing, that's why it was behaving like that. In order to prevent such behavior for chosen field types this specific commit has been created: 59859e8 which handles such case and is easily adaptable for other field types.

// @alongosz note:

QA

Besides tests for LSE, do a regression for Solr as well please, because in the past I had some unexpected results there with this task. Should not be affected though because changes are related to LSE only. But... you know ;)
// -- end of the note

// author's note:
Old parts of the e-mail string will still be able to be found when searching for the full e-mail address as they were indexed like that previously, before this change. Example:
eZ Platform - folder
support@ez.no - e-mail

When searching for support@ez.no one will find eZ Platform folder as well.

TODO:

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

@barw4 barw4 added the Bug label Apr 21, 2020
@barw4 barw4 self-assigned this Apr 21, 2020
@adamwojs
Copy link
Member

adamwojs commented Apr 21, 2020

Email address and ISBN fieldtypes were not searchable due to lacking FullTextField search field assignment.

It's not searchable using FullText Criterion to be precise. What is the use case here?

@andrerom
Copy link
Contributor

andrerom commented Apr 22, 2020

Use case: Be able to find the data using search (typically in backend, but in this case frontend might also be relevant).

Similar to the one we discussed in user (login / email), in that case typically for admins in backend, given default permissions.

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.

Regarding e-mail: It's not gonna work as expected across all search engines. See #2206 and EZP-28625 (duplicate).
You're missing tests.

@barw4
Copy link
Member Author

barw4 commented Apr 22, 2020

Regarding e-mail: It's not gonna work as expected across all search engines. See #2206 and EZP-28625 (duplicate).
You're missing tests.

@alongosz yeah, you are right, besides providing whole e-mail and finding invalid Contents with parts of email string, Solr and Legacy search results are also a bit inconsistent with each other. I'll try to dig more into it later on, it's probably much more complicated.

Shouldn't we have a different ticket for this e-mail part searching, though? This issue was occurring even before this PR has been created.

@barw4 barw4 changed the title EZP-31287: Assigned FullTextField indexation for Email and ISBN [WIP]EZP-31287: Assigned FullTextField indexation for Email and ISBN Apr 29, 2020
@barw4
Copy link
Member Author

barw4 commented May 11, 2020

You're missing tests.

@alongosz may I know what kind of tests am I missing? The FullTextField criterion is tested in eZ/Publish/API/Repository/Tests/SearchServiceFulltextTest.php and there is no test which is field type specific for this criterion.

@barw4 barw4 requested a review from alongosz May 11, 2020 15:02
@barw4 barw4 changed the title [WIP]EZP-31287: Assigned FullTextField indexation for Email and ISBN EZP-31287: Assigned FullTextField indexation for Email and ISBN May 11, 2020
@alongosz
Copy link
Member

You're missing tests.

@alongosz may I know what kind of tests am I missing? The FullTextField criterion is tested in eZ/Publish/API/Repository/Tests/SearchServiceFulltextTest.php and there is no test which is field type specific for this criterion.

@barw4 please see #2206

@barw4 barw4 removed the request for review from michal-myszka September 24, 2020 13:56
@barw4 barw4 changed the title EZP-31287: Assigned FullTextField indexation for Email and ISBN EZP-31287: Added FullText search Fields for Email and ISBN Sep 28, 2020
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.

Good direction, needs some changes though.

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.

Almost good, final CS remark for internal impl.

@barw4 barw4 requested a review from alongosz October 27, 2020 08:55
Copy link

@tomaszszopinski tomaszszopinski 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 eZPlatform-ee 2.5 with diff.

@lserwatka lserwatka merged commit 2509f18 into 7.5 Oct 29, 2020
@lserwatka lserwatka deleted the EZP-31287 branch October 29, 2020 11:54
@lserwatka
Copy link
Member

You can merge it up.

@barw4
Copy link
Member Author

barw4 commented Oct 29, 2020

Merged into 1.1: ezsystems/ezplatform-kernel@04bde0e
Merged into master: ezsystems/ezplatform-kernel@215c4ed

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

Successfully merging this pull request may close these issues.

6 participants