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

Add duty station search improvements: 167501188 #2760

Merged
merged 14 commits into from Oct 4, 2019

Conversation

lynzt
Copy link
Contributor

@lynzt lynzt commented Oct 2, 2019

Description

Make searching for a duty station more flexible... allow "fuzzy" matching.

Use trigram matching when letting a SM or an Office user search for duty stations. Calculate the similarity between the "search query" and the duty stations and return the top 7 results.

Reviewer Notes

Alternate spellings/abbrs we want to catch are in the pivotal story linked as a spreadsheet.

Setup

Add any steps or code to run in this section to help others prepare to run your code:

make server_test
make server_run
make client_run

Log into SM or Office app and do a duty station search using "alternate" duty station names... or try fuzzy searching for things:

  • ft bragg => fort bragg
  • joint base => recs for JB *
  • etc

Code Review Verification Steps

  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Any new migrations/schema changes:
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • User facing changes have been reviewed by design.
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

Screenshots

Screen Shot 2019-10-02 at 2 17 28 PM

@lynzt lynzt added the ttv label Oct 2, 2019
@lynzt lynzt force-pushed the lt-add-search-duty-station-improvements-167501188 branch from c6302cb to 143a816 Compare October 2, 2019 17:35
@codecov
Copy link

codecov bot commented Oct 2, 2019

Codecov Report

Merging #2760 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #2760     +/-   ##
========================================
+ Coverage    57.6%   57.6%   +0.1%     
========================================
  Files         278     276      -2     
  Lines       12567   12550     -17     
========================================
- Hits         7233    7226      -7     
+ Misses       4586    4578      -8     
+ Partials      748     746      -2
Impacted Files Coverage Δ
pkg/handlers/internalapi/transportation_offices.go 100% <100%> (ø) ⬆️
pkg/handlers/internalapi/duty_stations.go 87% <100%> (-0.5%) ⬇️
pkg/models/duty_station.go 50.7% <100%> (+15.7%) ⬆️
pkg/handlers/adminapi/api.go 0% <0%> (ø) ⬆️
pkg/services/office_user/office_user_fetcher.go
pkg/services/office_user/office_user_updater.go
...g/services/office_user/office_user_list_fetcher.go
pkg/handlers/adminapi/admin_users.go
pkg/services/office_user/office_user_creator.go
pkg/services/admin_user/admin_user_list_fetcher.go
... and 5 more

@lynzt
Copy link
Contributor Author

lynzt commented Oct 2, 2019

There's another pivotal story that deals with the fact we're doing an Eager("address") when looking up duty stations for the user to choose from...

@lynzt lynzt marked this pull request as ready for review October 2, 2019 20:01
@@ -0,0 +1,3 @@
CREATE EXTENSION pg_trgm;

CREATE INDEX duty_stations_name_trgm_idx ON duty_stations USING gin(name gin_trgm_ops);
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

Copy link
Contributor

@chrisgilmerproj chrisgilmerproj left a comment

Choose a reason for hiding this comment

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

Awesome work! Can we test this on experimental with the load testing framework? I can show you how.

with names as (
(select id as duty_station_id, name, similarity(name, $1) as sim
from duty_stations
where similarity(name, $1) > 0.03
Copy link
Contributor

Choose a reason for hiding this comment

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

We might be able to get away with using the % operator with these similarity calls, if we set the pg_trgm.similarity_threshold to the 0.03 value we're using (default is 0.3, so definitely would need to override in that case). Since the operator returns a true/false based on the threshold as opposed to the value, I suspect it might run a bit faster (I wouldn't hold the PR up over this, just something to think about).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't use % in case we want to tweek the similarity threashold and figured it clearer what was happening if things were in the query... and setting the threashold didn't add much/any speed gains since we're not dealing w/ a ton of rows.

However, if/when we add additional duty stations - we should def. keep this in mind for query optimization.

Copy link
Contributor

@Ryan-Koch Ryan-Koch left a comment

Choose a reason for hiding this comment

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

This looks really great! I left a minor comment about the query, but otherwise this seems to do what it's supposed to. I think after the load testing that was mentioned in another comment it's good to go. 🚀

@lynzt
Copy link
Contributor Author

lynzt commented Oct 4, 2019

@chrisgilmerproj results from load testing.

Screen Shot 2019-10-04 at 9 59 32 AM

Copy link
Contributor

@chrisgilmerproj chrisgilmerproj left a comment

Choose a reason for hiding this comment

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

🚀 - Awesome work. Thanks for doing the load testing check on this endpoint because its caused us so much trouble in the past.

Also, don't forget to undo the changes to experimental :)

@lynzt
Copy link
Contributor Author

lynzt commented Oct 4, 2019

@chrisgilmerproj do you mean pushing to experimental again to reset the auth variable?
I've already had @rdhariwal add the rate-limiting back in the other repo...

@chrisgilmerproj
Copy link
Contributor

@chrisgilmerproj do you mean pushing to experimental again to reset the auth variable?
I've already had @rdhariwal add the rate-limiting back in the other repo...

Yep! You'll want to turn off that variable. And then remove the stuff from this PR.

@lynzt lynzt force-pushed the lt-add-search-duty-station-improvements-167501188 branch from a2ac006 to cde02be Compare October 4, 2019 20:49
@lynzt lynzt merged commit 47442ea into master Oct 4, 2019
@lynzt lynzt deleted the lt-add-search-duty-station-improvements-167501188 branch December 18, 2019 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants