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

Update bool operator syntax for Elasticsearch simple_query_string and excludes #6405

Merged
merged 7 commits into from
Aug 26, 2024

Conversation

patphongs
Copy link
Member

@patphongs patphongs commented Jul 31, 2024

Summary (required)

This updates the query string syntax that is used on the web UI as well as what is submitted in the API query string. Refer to the ticket for the syntax.

Required reviewers

1 Developer
1 UX

Impacted areas of the application

General components of the application that this PR will affect:

Screenshot 2024-08-09 at 12 02 00 PM

Related PRs

Related PRs against other branches:

branch PR
feature/add_case_respondents_and_exclude link

How to test

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 82.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 80.59%. Comparing base (e5b7b1d) to head (5323974).
Report is 51 commits behind head on develop.

Files Patch % Lines
fec/legal/views.py 64.00% 9 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6405      +/-   ##
===========================================
+ Coverage    80.54%   80.59%   +0.05%     
===========================================
  Files          233      233              
  Lines         5042     5077      +35     
===========================================
+ Hits          4061     4092      +31     
- Misses         981      985       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@patphongs patphongs changed the title Feature/bool ops simple query string v2 Update bool operator syntax for Elasticsearch simple_query_string and excludes Aug 9, 2024
Copy link
Member

@cnlucas cnlucas left a comment

Choose a reason for hiding this comment

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

Working for me, thank you so much @patphongs!

@patphongs patphongs marked this pull request as ready for review August 21, 2024 19:44
@rfultz
Copy link
Contributor

rfultz commented Aug 22, 2024

Maybe outside the scope of this work, but could we make some changes to the directions for symbols that will work in the main search box?

  • The pipe requires spaces before and after it, right?
  • The plus sign requires spaces before and after?
  • The minus/dash requires a space in front and zero spaces after?
  • If a user actually typed curly quotes, things would break, yes?
  • Using a <pre> would apply a monospace font to what should be typed.
  • Giving the <pre> a background color would visually demonstrate where spaces should and should not be included.

(the code in these: <pre style="background-color: rgba(211,211,211,.5);display:inline">)

image

I don't know if this is a better option than a space between the quotes.
image

@patphongs
Copy link
Member Author

patphongs commented Aug 22, 2024

Maybe outside the scope of this work, but could we make some changes to the directions for symbols that will work in the main search box?

Thanks for your insights @rfultz, I think we can consider these changes more in depth in another ticket. Let me address each of your questions:

  • The pipe requires spaces before and after it, right?

The pipe doesn't require spaces before and after, but I put it in there so the query is easier to read.

  • The plus sign requires spaces before and after?

The plus sign doesn't require spaces before and after, but I put it in there so the query is easier to read also.

  • The minus/dash requires a space in front and zero spaces after?

This is correct, the dash to exclude does require a space in front and 0 spaces after or else it will put in an additional dash in the space directly after.

  • If a user actually typed curly quotes, things would break, yes?

Because we switched over to the standard analyzer and simple_query_string for the API, it's actually more forgiving with random chars like that. It would just ignore the curly quotes entirely. But you make a good point that a user may copy and paste and get confused. We shouldn't sacrifice styling for usability.

  • Using a <pre> would apply a monospace font to what should be typed.
  • Giving the <pre> a background color would visually demonstrate where spaces should and should not be included.

Interesting idea! I'd love to take a look at this further with you.

Other upcoming changes

  • Remove symbol section for the more keyword options box as the user doesn't need it to fill out the inputs
  • We need to add documentation for the symbols somewhere else after we remove it from the options box. It will be in the tooltip for now, but I think we need better documentation coming up.
  • Possibly using commas to do the .split(',') in the future since the standard analyzer ignores most punctuation including commas. So it may be reasonable to ask the user to insert a comma between terms so that it's easier for the webpage to parse.

Copy link
Contributor

@rfultz rfultz left a comment

Choose a reason for hiding this comment

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

Nice! I've been trying to break it and can't seem to!

For this ticket or a future one, could we add tests for gibberish, too, to make sure failures are handled properly? Mismatched parens (e.g. (, )(, ), (…(, )…) ), strange characters, too short, etc

Copy link
Contributor

@JonellaCulmer JonellaCulmer left a comment

Choose a reason for hiding this comment

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

Tested on all effected pages, looks good. Thanks @patphongs.

@JonellaCulmer JonellaCulmer merged commit 0c05c78 into develop Aug 26, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Update bool operator syntax for Elasticsearch simple_query_string and excludes
4 participants