-
Notifications
You must be signed in to change notification settings - Fork 23
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
Migration to django-popolo part 3 #1196
Migration to django-popolo part 3 #1196
Conversation
This also involves renaming the celery tasks and many methods that referred to PopIt explicitly so that they no longer do.
This depends on this commit: everypolitician/everypolitician-writeinpublic@ae62cad ... which has been deployed now.
This include a migration that changes some foreign keys to use the PopoloPerson proxy model instead of the base Person model.
Rather than using the /persons collection endpoint of PopIt's API, the code now just uses export.json to extract Popolo JSON from PopIt. This commit has the results of re-recording these fixtures (using roughly the procedure here: https://gist.github.com/mhl/848f7980d256ca38ff603375be4f264e ). The exception was persons_with_null_values.yaml which seems to be missing the source JSON MongoDB fixture, so this was created by hand based on one of the others.
The start_local_popit_api.bash script is designed to make it easy to set up a local version of PopIt for testing against; this is still useful for re-recording the fixtures (see the commit message for 967f842529 and https://gist.github.com/mhl/848f7980d256ca38ff603375be4f264e ). However, this script installed a very old version of PopIt - this commit updates it to use the most recent version. (A key problem with the old version of this script is that it sets up a version of PopIt without the export.json endpoints.)
This doesn't seem to matter, and since there is no Identifier associated with a ContactDetail in django-popolo, it'd be awkward to preserve the ContactDetail identifiers. It doesn't seem to be actually used anyway.
dc98bea
to
0ecc77e
Compare
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 quite a big pull request!
I've had a look through the code changes and run the migration successfully on my local instance and it all appeared to work correctly. It's tricky to know whether I've tested exhaustively, but I've had a good click around and nothing was obviously broken :)
👍 🚀
This is the third part of: #1181 for review. See that pull request for the context for this pull request.
This should update all the code and tests to use the new models with the migrated data, and update the new models from Popolo JSON sources.
I've tried to make sure that the API still works for clients with the same identifiers, but I find tastypie quite confusing, so it's definitely worth double-checking all of that.
Also particularly worth looking at is: "Update the code to sync from Popolo JSON, not a PopIt instance" f9fd9e9.