-
Notifications
You must be signed in to change notification settings - Fork 3
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
Migrate entity search to Haystack #711
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.
much better! what a great improvement. let's take the opportunity to address some smaller perplexities
@@ -28,7 +28,8 @@ update_db : import_directory import_db auth_models.json flush_db link_locations | |||
python manage.py loaddata auth_models.json | |||
python manage.py make_materialized_views --recreate | |||
python manage.py update_countries_plus | |||
python manage.py make_search_index --recreate | |||
python manage.py rebuild_index --noinput |
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.
do we need a command to build the schema?
also, it would seem like these search index commands maybe belong under a different PHONY target?
@@ -75,7 +83,8 @@ Build and start the Docker image for the Solr server: | |||
|
|||
Open up another shell and create the search index: | |||
|
|||
docker-compose run --rm app ./manage.py make_search_index | |||
docker-compose run --rm app ./manage.py update_index |
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.
you do not have to do address this in this PR, but we should use the makefile commands here, imo.
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'm not sure what the application of this Makefile is, to be honest. There's one reference to the import_google_docs
target in the README, but I don't see other references to the recipes in the code base (including deployment scripts). I think I'd like to update the Makefile in our import refactor so it has some useful targets for local development and eventual deployment on Heroku. I'll plan to make those changes in a separate PR.
organization/search_indexes.py
Outdated
|
||
class OrganizationIndex(SearchEntity, indexes.Indexable): | ||
|
||
''' |
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.
what is this comment doing for us?
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.
Ah, I included these as a reference for what needs to go into each index. I think it's safe to remove!
organization/search_indexes.py
Outdated
countries = set() | ||
|
||
for division in prepared_data['division_ids']: | ||
countries.update([country_name(division)]) |
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 understand this is copy paste, but this seems like it should be
countries.add(country_name(division))
or even better
countries = {country_name(division) for division in prepared_data['division_ids']}
person/search_indexes.py
Outdated
countries = set() | ||
|
||
for division in prepared_data['division_ids']: | ||
countries.update([country_name(division)]) |
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.
another weird use of update
person/search_indexes.py
Outdated
person_division_id = object.division_id.get_value() | ||
|
||
if person_division_id: | ||
division_ids.update([person_division_id.value]) |
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.
another weird use of update
person/search_indexes.py
Outdated
org_division_id = org.division_id.get_value() | ||
|
||
if org_division_id: | ||
division_ids.update([org_division_id.value]) |
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.
another weird use of update
person/search_indexes.py
Outdated
|
||
memberships = [mem.object_ref for mem in object.memberships] | ||
|
||
if memberships: |
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.
don't need this conditional
name = organization.name.get_value() | ||
|
||
parents = organization.parent_organization.all() | ||
published = all([p.value.published for p in parents]) |
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 can be all(p.value...)
, don't need the list comprehension
Okey doke, @fgregg, I think I responded to and/or addressed all of your comments. Do you want to take another look? |
TODO:
|
Superseded by #746 (rebase, plus additional work to patch bugs). |
Overview
This PR:
Connects #674.
Demo
Notes
There are a few classes of changes here:
${ENTITY_MODULE}.search_indexes
for each search entity: person, unit, violation, and source.schema.xml
to include Haystack fields. N.b., the template into which these fields are generated lives intemplates/search_configs/schema.xml
and is a lightly edited version of the original schema.FacetedSearchView
and adjusts the search templates to work with the new context.make_search_index
toupdate_composition_index
and removes code to add documents related to the base search entities.HAYSTACK_*
variables (as exemplified inlocal_settings.example.py
).RealtimeSignalProcessor
for this behavior, i.e., we do not need to test third-party code.)Testing Instructions
docker-compose run --rm app python manage.py update_index -v 3
docker-compose run --rm app python manage.py make_search_index
docker-compose run --rm app python manage.py createsuperuser