Skip to content

Migration to django-popolo part 1#1193

Merged
mhl merged 5 commits intoreviewed-migrate-to-django-popolofrom
migrate-to-django-popolo-part-1
Sep 23, 2016
Merged

Migration to django-popolo part 1#1193
mhl merged 5 commits intoreviewed-migrate-to-django-popolofrom
migrate-to-django-popolo-part-1

Conversation

@mhl
Copy link
Collaborator

@mhl mhl commented Sep 23, 2016

This is the first part of: #1181 for review. See that pull request for the context for this pull request.

It adds django-popolo models in parallel with the popit-django models that we're intending to replace.

@mhl mhl changed the title Migrate to django popolo part 1 Migration to django-popolo part 1 Sep 23, 2016
@coveralls
Copy link

coveralls commented Sep 23, 2016

Coverage Status

Coverage decreased (-97.6%) to 0.859% when pulling ed804f5 on migrate-to-django-popolo-part-1 into 7b1b984 on reviewed-migrate-to-django-popolo.

Copy link
Contributor

@chrismytton chrismytton left a comment

Choose a reason for hiding this comment

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

Even after removing the popolo_source = models.ForeignKey(PopoloSource, null=True, blank=True) line I'm still getting some errors when running the tests, is that related to the two lines that call PopoloPerson.objects?

)
writeitinstance = models.ForeignKey(WriteItInstance)
popitapiinstance = models.ForeignKey(ApiInstance)
popolo_source = models.ForeignKey(PopoloSource, null=True, blank=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting an error on this line when I run the tests:

(writeit-virtualenv)vagrant@precise64:/vagrant$ ./manage.py test nuntium contactos mailit
Traceback (most recent call last):
  File "./manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 385, in execute_from_command_line
    utility.execute()
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 354, in execute
    django.setup()
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/__init__.py", line 21, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/apps/registry.py", line 108, in populate
    app_config.import_models(all_models)
  File "/home/vagrant/writeit-virtualenv/local/lib/python2.7/site-packages/django/apps/config.py", line 202, in import_models
    self.models_module = import_module(models_module_name)
  File "/usr/lib/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
  File "/vagrant/instance/models.py", line 149, in <module>
    class WriteitInstancePopitInstanceRecord(models.Model):
  File "/vagrant/instance/models.py", line 159, in WriteitInstancePopitInstanceRecord
    popolo_source = models.ForeignKey(PopoloSource, null=True, blank=True)
NameError: name 'PopoloSource' is not defined

I'm guessing this line has snuck into this pull request unintentionally? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact it's just missing the import - we need this foreign key to be there for the datamigration which will populate it with the equivalent PopoloSource to the old ApiInstance. I've pushed a fixup for this and the other accidental s/Person/PopoloPerson/ changes that you mentioned.

self.do_something_with_a_vanished_popit_api_instance(popit_api_instance)
return (False, e)
persons = Person.objects.filter(api_instance=popit_api_instance)
persons = PopoloPerson.objects.filter(api_instance=popit_api_instance)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this line should probably not be changed until after the data has been migrated in the later commits?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, sorry, that should be in a later PR.

@property
def people(self):
people = Person.objects.filter(
people = PopoloPerson.objects.filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this line, I think this should probably only be changed once the data has been migrated in later commits?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, right again.

@coveralls
Copy link

coveralls commented Sep 23, 2016

Coverage Status

Coverage increased (+0.0009%) to 98.451% when pulling 8088b5b on migrate-to-django-popolo-part-1 into 7b1b984 on reviewed-migrate-to-django-popolo.

Copy link
Contributor

@chrismytton chrismytton left a comment

Choose a reason for hiding this comment

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

Woo! 🎉

@mhl mhl force-pushed the migrate-to-django-popolo-part-1 branch from 8088b5b to bc4d6c6 Compare September 23, 2016 14:05
@mhl mhl merged commit bc4d6c6 into reviewed-migrate-to-django-popolo Sep 23, 2016
@mhl mhl removed the in progress label Sep 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants