Sync EveryPolitician UUID to Person model

[chrismytton]

Add a management command add EveryPolitician UUIDs to the Person model.

Blocked waiting for EveryPolitician South Africa import to be fixed

This is currently using the south-africa-update-peoples-assembly branch of everypolitician-data, which has the required UUIDs but is also losing some data, so can't be merged in its current state. So before this can be merged we need to fix the problems described in everypolitician/everypolitician-data#42096.

To work around this problem I've made the git ref that we use to fetch countries.json configurable. This means we can point to the branch that has the data we need for now, then when things are fixed upstream we can use a different ref to point to the fixed data.

I've also split the bit that adds a cronjob for this into a separate pull request #2342 so we can hold off merging that until we've got the data in EveryPolitician up-to-date, but we can still merge this in the meantime.

Fixes #2330

[coveralls]

Coverage decreased (-0.02%) to 54.171% when pulling 15739cc on 2330-add-everypolitician-uuid-to-person into 63942cc on master.

[coveralls]

Coverage decreased (-0.04%) to 54.156% when pulling 744fffd on 2330-add-everypolitician-uuid-to-person into a433adc on master.

[chrismytton]

I think it would be good to get this merged ASAP rather than waiting for the EP/DC problems to be resolved. The best way to do this is probably to make the SHA configurable where there's currently a TODO comment in this change:

pombola/pombola/core/management/commands/core_sync_everypolitician_uuid.py

Lines 18 to 20 in 4ea415a

	# TODO: Make this SHA configurable.
	url = ('https://cdn.rawgit.com/everypolitician/everypolitician-data/'
	'eaa170a15d61b024b49e22527fd1fdf6eba18ee6/countries.json')

Then this script will be usable with whatever EveryPolitician data we want to use.

[chrismytton]

Have extracted the crontab bits out to #2342 so that the code for the management command can be merged now, and then we can add the cronjob once we're happy with the EP data.

[mhl]

I've left a few minor comments inline. The only other thing that occurs to me is that it might helpful if the argument was optional rather than required, and have it default to master.

[mhl]

I think these comments can be removed now :)

[chrismytton]

Oops, yep!

[mhl]

I think you can use popolo_person.identifier_value('peoples_assembly') to simplify this: https://github.com/everypolitician/everypolitician-popolo-python/blob/master/popolo_data/base.py#L99 (this isn't documented, sorry)

[chrismytton]

Ah didn't know about that, thanks, have changed it to use that.

[mhl]

This is partly just "fun with Python's .format" but, you could make this:

u"No EveryPolitician UUID found for {0.id} {0.name} https://www.pa.org.za/person/{0.slug}/\n"

... and then do error_msg.format(person). It's a tiny thing, but it'd make changing the format string later less prone to error.

[chrismytton]

😮 Wow, didn't know .format could do that!! You're quite right, that's less error prone. Have switched to using that syntax.

[mhl]

I'd probably collapse these two conditions into a single condition - perhaps makes it slightly easier to see that there's only one branch here?

[chrismytton]

The continue on the line below is actually in the top level if statement, then the logging is part of the inner if statement. I've changed things around in 663e8dc so that it's all at a single level of if statement to make things hopefully a bit clearer.

[mhl]

"Person.everypolitician_uuid field" - I suspect this is an old comment?

[chrismytton]

👍

[mhl]

It's another very minor thing, but I'd probably make these use {name}: {identifier} and then .format(name=person.name, identifier=identifier.identifier)

[chrismytton]

👍

[chrismytton]

The only other thing that occurs to me is that it might helpful if the argument was optional rather than required, and have it default to master.

Ah yep, very sensible suggestion! Have done that in e7e2e96.

[chrismytton]

@mhl I've addressed your very helpful comments, would you mind re-reviewing this please?

[mhl]

@chrismytton Looks good 👍 - just one last thing; since this is currently South Africa specific, could you make the filename pombola/core/management/commands/core_sync_everypolitician_uuid.py instead?

[chrismytton]

@chrismytton Looks good 👍 - just one last thing; since this is currently South Africa specific, could you make the filename pombola/core/management/commands/core_sync_everypolitician_uuid.py instead?

I'm guessing that should have been pombola/south_africa/management/commands/south_africa_sync_everypolitician_uuid.py? If so I've done that in 2e63d66.

[mhl]

Yep, that's what I meant, sorry :)