search

datamade / django-councilmatic

:heartpulse: Django app providing core functions for *.councilmatic.org
http://councilmatic.org
MIT License
26 stars 16 forks source link

Add option to avoid using unstable OCD_CITY_COUNCIL_ID #44

Closed patcon closed 8 years ago

patcon commented 8 years ago

In the Canadian scrapers in scrapers-ca repo (hosted at http://scrapers.herokuapp.com/), we've made the choice to blow away the db regularly, and so OCD ids change. This is arguably something that we should eventually rethink, but for now, it's not an issue. Except in one place :)

https://github.com/datamade/django-councilmatic/blob/6d81da28435a31eff1e7f35662515a20aa935417/councilmatic_core/models.py#L67

https://github.com/datamade/chi-councilmatic/blob/master/councilmatic/settings_jurisdiction.py#L7

Would it be possible to instead use _organization__name instead of organization__ocd_id as the filter, as this is consistent and won't lead to odd behaviour whenever we re-run loaddata.

cc: @jpmckinney

patcon commented 8 years ago

Also here: https://github.com/datamade/django-councilmatic/blob/6d81da28435a31eff1e7f35662515a20aa935417/councilmatic_core/views.py#L137

EDIT: And some in here https://github.com/datamade/django-councilmatic/blob/6d81da28435a31eff1e7f35662515a20aa935417/councilmatic_core/management/commands/loaddata.py

cathydeng commented 8 years ago

this is a nice idea. related to #38

patcon commented 8 years ago

Haven't tested this yet, but this was the gist

patcon commented 8 years ago

Confirmed that this works during regular running of instance, but loaddata needs fixing

cathydeng commented 8 years ago

kk - lemme know when you've got loaddata working, & I'll review

patcon commented 8 years ago

Ok, this should be good for review, currently using it for tor-councilmatic :)

@cathydeng Def lemme know if anything needs fix-ups to make things pythonic!

patcon commented 8 years ago

Oh hey, this is your call, but should I use CITY_COUNCIL_NAME instead of a new OCD_CITY_COUNCIL_NAME? The former was for copy and kinda cosmetic, and the latter is new and must match the name in the OCD API. I used OCD_CITY_COUNCIL_NAME as it's consistent and clear that it relates to OCD_CITY_COUNCIL_ID when xomeone comes across it.

Fwiw, whenever the OCD_CITY_COUNCIL_ID is present, it takes precedent over OCD_CITY_COUNCIL_NAME.

Any thoughts on those choices?

cathydeng commented 8 years ago

I like having a clear separation between display settings & data loading settings, so let's keep OCD_CITY_COUNCIL_NAME

cathydeng commented 8 years ago

looks good! this is great, thank you 👍