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

Els duty station lookup #166968918 #2369

Merged
merged 7 commits into from Jul 12, 2019

Conversation

3 participants
@tinyels
Copy link
Contributor

commented Jul 10, 2019

Description

This pull request addresses performance issues with the duty station lookup by

  • adding an index on name to the duty_stations table
  • making the lookup less fuzzy because the current algorithm of add '%' between every typed character performs poorly and was not what design asked for originally:
    image
  • changes the duty station lookup so that it won't display "No Options" until after it has requested matching options from the server to prevent a regression of the work to resolve User Feedback

Reviewer Notes

Given that the current endpoint could kill the server, this is meant to be potentially temporary change until we can get more feedback from design about how this lookup should really work.

Code Review Verification Steps

  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #dp3-engineering
  • There are no aXe warnings for UI.
  • 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

On edit, the dropdown shows an empty message
image

Instead of showing a "no options" message :
image

Typing "do" now returns this:
image

instead of this:
image

@tinyels tinyels changed the title [WIP] Els duty station lookup #166968918 Els duty station lookup #166968918 Jul 10, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

🚀 - I still wish we wouldn't send queries until the third character, but this seems to be a good intermediate step to reduce the load I'd been seeing. For future I'd like to replace ILIKE with fuzzy string matching or possibly just preload duty stations in the client.

@garrettqmartin8
Copy link
Contributor

left a comment

Looks great. I do also like the idea of preloading duty stations on the client in the future.

@tinyels

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

Looks great. I do also like the idea of preloading duty stations on the client in the future.

I'm not sure we would want to preload that on to service members phones, but seems like a good idea to consider for the office.

@tinyels tinyels force-pushed the els-duty-station-lookup-#166968918 branch from e9d3484 to d379bbb Jul 12, 2019

@codecov

This comment has been minimized.

Copy link

commented Jul 12, 2019

Codecov Report

Merging #2369 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2369      +/-   ##
==========================================
- Coverage   59.68%   59.64%   -0.04%     
==========================================
  Files         267      266       -1     
  Lines       14887    14871      -16     
==========================================
- Hits         8885     8869      -16     
  Misses       4970     4970              
  Partials     1032     1032

@tinyels tinyels merged commit 5d79c29 into master Jul 12, 2019

19 checks passed

ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 59.68%)
Details
codecov/project/go Absolute coverage decreased by -0.04% but relative coverage increased by +40.48% compared to d8584e8
Details

@tinyels tinyels deleted the els-duty-station-lookup-#166968918 branch Jul 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.