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

Full text admin user search #3133

Merged
merged 3 commits into from Aug 6, 2020
Merged

Full text admin user search #3133

merged 3 commits into from Aug 6, 2020

Conversation

dracos
Copy link
Member

@dracos dracos commented Jul 31, 2020

One "bug" fix - forcing postgresql to use the index in situations where it does not want to.

And then adding full text search to the user admin search as well. Plus side - is quicker. Down side - can no longer search for part of name/phone/email only whole words. Perhaps alternative solution would be to search whole fields first (for exact match) and only do ilike search if no results to that. Or just say is okay as is. Review the first bug commit regardless please :)

[skip changelog]

@dracos dracos requested a review from davea July 31, 2020 15:08
@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #3133 into master will increase coverage by 0.00%.
The diff coverage is 92.85%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3133   +/-   ##
=======================================
  Coverage   83.43%   83.44%           
=======================================
  Files         250      250           
  Lines       15593    15600    +7     
  Branches     2910     2910           
=======================================
+ Hits        13010    13017    +7     
  Misses       1659     1659           
  Partials      924      924           
Impacted Files Coverage Δ
perllib/FixMyStreet/Roles/FullTextSearch.pm 92.85% <85.71%> (-7.15%) ⬇️
...erllib/FixMyStreet/App/Controller/Admin/Reports.pm 84.82% <100.00%> (+0.06%) ⬆️
perllib/FixMyStreet/App/Controller/Admin/Users.pm 86.11% <100.00%> (-0.13%) ⬇️
perllib/FixMyStreet/DB/ResultSet/Comment.pm 100.00% <100.00%> (ø)
perllib/FixMyStreet/DB/ResultSet/Problem.pm 98.29% <100.00%> (+0.01%) ⬆️
perllib/FixMyStreet/DB/ResultSet/User.pm 100.00% <100.00%> (ø)
perllib/Utils.pm 98.98% <0.00%> (+1.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd42d6a...967b706. Read the comment docs.

Copy link
Member

@davea davea left a comment

Choose a reason for hiding this comment

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

This seems to remove the ability to search for users by domain, which is something I do from time to time. Is that a deliberate change or just a side effect of the way the index is constructed?

@dracos
Copy link
Member Author

dracos commented Aug 4, 2020

Just a side effect I hadn't noticed, hmm. Could translate @ to space and then it'd index local and domain separately, which I think would work (assuming you did the same on query too)?

Copy link
Member

@davea davea left a comment

Choose a reason for hiding this comment

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

It'd be good to have the domain-only searching back, but happy for this to be merged as-is (with a ticket for the domain searching, maybe) if it needs to go out the door 🚪 👍

@dracos
Copy link
Member Author

dracos commented Aug 5, 2020

Have changed it to treat @ and . as spaces in both indexing and query, so a domain search should work (possible false positives now I guess, e.g. a search for "gmail.com" would match "gmail@example.com" but that doesn't seem too bad. Changed the index to wrap id || ' ' || coalesce(name,'') || ' ' || coalesce(email,'') || ' ' || coalesce(phone,'') in translate(..., '@.', ' '), and changed FullTextSearch.pm to allow variable translation and apply it to the query too (otherwise you'd search for "gmail.com" and it wouldn't find anything because only "gmail" and "com" were indexed).

I guess we could index email parts (with @ and . as space) plus the whole email as well, plus domain and whole local part, and then not do translation on query which would then sort of work? But meh.

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.

None yet

2 participants