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

Fix for improving user search. #3407

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

johnpinto1
Copy link
Contributor

@johnpinto1 johnpinto1 commented Apr 10, 2024

Currently Searches of users using search "firstname_<empty_spaces>_surname" will only get valid matches if the <empty_spaces> is a single empty space.

Change:
The search term string is squished to remove extra empty spaces. As a
result search terms like "Jill Bloggs" (one space between) and "Jill Bloggs" (more than one space between) will both return the same results.

Reference:
https://api.rubyonrails.org/classes/String.html#method-i-squish

@johnpinto1
Copy link
Contributor Author

johnpinto1 commented Apr 10, 2024

Will rebase with development once @benjaminfaure has released v4.2. That should fix test failures.

@johnpinto1 johnpinto1 force-pushed the fix-for-improving-user-search branch from 6808847 to ec8f36b Compare April 10, 2024 15:59
Currently Searches of users using search "firstname_<empty_spaces>_surname"
will only get valid matches if the _<empty_spaces>_ is a single empty space.

Change:
The search term string is squished to remove extra empty spaces. As a
result search terms like "Jill Bloggs" (one space between) and "Jill    Bloggs" (more than one space between) will both return the same results.
@johnpinto1 johnpinto1 force-pushed the fix-for-improving-user-search branch from ec8f36b to 4e6995e Compare April 16, 2024 12:03
Copy link
Contributor

@aaronskiba aaronskiba left a comment

Choose a reason for hiding this comment

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

Looks like it's working as it should. I wonder if we should apply this to other searches? For example, the same issue exists within the "Plans" search box:
Screenshot from 2024-06-17 12-20-51
Screenshot from 2024-06-17 12-20-56

@briri
Copy link
Contributor

briri commented Jun 17, 2024

Yes, if you see it elsewhere then it should also be patched. I am not going to have time to do that in the next day or so, so feel free to update in this branch if you want. :)

@aaronskiba
Copy link
Contributor

Yes, if you see it elsewhere then it should also be patched. I am not going to have time to do that in the next day or so, so feel free to update in this branch if you want. :)

Cool. @johnpinto1 would you want me to do that? Or would you prefer to take care of it yourself?

@johnpinto1
Copy link
Contributor Author

Yes, if you see it elsewhere then it should also be patched. I am not going to have time to do that in the next day or so, so feel free to update in this branch if you want. :)

Cool. @johnpinto1 would you want me to do that? Or would you prefer to take care of it yourself?

Please do @aaronskiba. Cheers!

@aaronskiba
Copy link
Contributor

Yes, if you see it elsewhere then it should also be patched. I am not going to have time to do that in the next day or so, so feel free to update in this branch if you want. :)

Cool. @johnpinto1 would you want me to do that? Or would you prefer to take care of it yourself?

Please do @aaronskiba. Cheers!

I was working on this and discovered a bit of an edge-case that complicates this PR a bit:

The issue is when the string being searched (User.firstname + User.surname, Org.name, etc.) actually has double spaces built-in.

SELECT firstname, surname
FROM users
WHERE firstname LIKE '% '
OR surname LIKE ' %';

When I execute the above query on a recent DMP Assistant DB dump from production, I get 307 returned results.

As a single example, there is a User with first name "Susan " and last name "Brown".

Searches for both "Susan Brown" (one space) or "Susan Brown" (two spaces) both return no results:
Screenshot from 2024-06-26 10-44-19
Screenshot from 2024-06-26 10-44-28

However, when I revert the changes from this PR, the User is returned when I search "Susan Brown" (two spaces):
Screenshot from 2024-06-26 10-45-00

I like the idea behind this PR, but to avoid this edge-case, I'm thinking that in addition to the search string, we'll have to apply the same .squish to the records being queried.

@johnpinto1
Copy link
Contributor Author

I like the idea behind this PR, but to avoid this edge-case, I'm thinking that in addition to the search string, we'll have to apply the same .squish to the records being queried.

@aaronskiba Well spotted, sounds good to me.

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.

3 participants