-
Notifications
You must be signed in to change notification settings - Fork 110
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 Elasticsearch 7 support to paying_for_college search behind a flag #6075
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor comments.
Still want to run more code.
The current use of `os.path.join` in base settings results in a malformed host value (below), in which the port signifier ':' is surrounded by slashes: ``` ELASTICSEARCH_DSL: {'default': {'hosts': 'http://localhost/:/9200'}} ``` Elasticsearch then reads the ':' as an index name and raises a naming error: ``` "Invalid index name [:], must not contain ':'") ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, but I think we need to make changes to exclude schools that have closed, and to fix autocomplete to enable searches by nickname and school ID.
…y uses an autocomplete search box
I recommend having a test to confirm a school that has closed is excluded. I hope that I have fixed autocomplete to enable searches by school_id and nickname. |
All the schools in test_fixture.json are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're close. Just some related tweaks, for documents.py, models.search.py, and views.py
…oolDocument Removed str from nicknames in school_autocomplete because list-as-string is not friendly in the API response Modified autocomplete to return the default top 10 hits instead of the entire response set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
App's working well now. Great work!
Places ES7 paying_for_college search functionality behind a flag
Additions
test_search.py
withSchoolSearchTest
classUpdates
test_school_search
fromtest_views.py
intotest_search.py
How to test this PR
ELASTICSEARCH_DSL_PFC
flagcfgov/manage.py search_index --rebuild -f --parallel
https://cfgov-pr-6075.demo.cfpb.gov/paying-for-college2/understanding-your-financial-aid-offer/api/search-schools.json?q=Kansas
Screenshots
Notes and todos
Checklist
/docs
folder) – for basic, close-to-the-code docs on working with this repo