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

search: fix query search with brackets #153

Closed

Conversation

AoNoOokami
Copy link

@AoNoOokami AoNoOokami commented Mar 26, 2020

Co-Authored-by: Alicia Zangger alicia.zangger@rero.ch

Why are you opening this PR?

To close issue mentioned above.

How to test?

See issue description
Use npm link or npm pack to install this version of ng-core in rero-ils-ui (see https://github.com/rero/ng-core/blob/master/projects/rero/ng-core/README.md).

Code review check list

  • Commit message template compliance.
  • Commit message without typos.
  • File names.
  • Functions names.
  • Functions docstrings.
  • Unnecessary commited files?
  • Extracted translations?

* Allows queries with brackets and other specific symbols.
* Closes rero/rero-ils#755.

Co-Authored-by: Alicia Zangger <alicia.zangger@rero.ch>
@AoNoOokami AoNoOokami self-assigned this Mar 26, 2020
@AoNoOokami
Copy link
Author

Reflexion: https://stackoverflow.com/a/332897

Copy link

@sebdeleze sebdeleze left a comment

Choose a reason for hiding this comment

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

The regex seems to be OK, but I juste suggest you to change the commit message for explaining that specials symbols will be replaced by spaces.

Copy link
Contributor

@benerken benerken left a comment

Choose a reason for hiding this comment

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

The fix works well.
But as explained yesterday, IMHO, we should find a more generic way to manage such case.
Like in your "reflexion" link here above :-) But I don't exactly which one to use :-\

@jma
Copy link
Contributor

jma commented Mar 30, 2020

I think it is not the right approach. This remove the possibility to do expert search and I'm not sure that it is really robust. Imagine you have to search explicitly Title[subtitle]. This is a part of a more general US. This needs probably to change the use of the query_string to a simple_query_string in ES. We can imagine to use this PR as a quick fix. But we should not forget to remove this when a clean solution will be found.

@AoNoOokami AoNoOokami closed this Mar 31, 2020
@AoNoOokami
Copy link
Author

I close this PR as discussed during sprint planning. The issue will be solve in another way after the analysis of the results of pilot tests and a reflexion about search query.

@AoNoOokami AoNoOokami deleted the zaa-#1333-fix-search-with-brackets branch April 20, 2020 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants