Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Geoshapes from wikidata #29

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Geoshapes from wikidata #29

wants to merge 9 commits into from

Conversation

GeoWill
Copy link
Contributor

@GeoWill GeoWill commented Jan 17, 2019

No description provided.

@sagepe sagepe temporarily deployed to democratic-commons-demo-pr-29 January 17, 2019 12:46 Inactive
@sagepe sagepe temporarily deployed to democratic-commons-demo-pr-29 January 17, 2019 14:44 Inactive
@sagepe sagepe temporarily deployed to democratic-commons-demo-pr-29 January 18, 2019 08:42 Inactive
@sagepe sagepe temporarily deployed to democratic-commons-demo-pr-29 January 18, 2019 09:28 Inactive
@sagepe sagepe temporarily deployed to democratic-commons-demo-pr-29 January 18, 2019 09:29 Inactive
@sagepe sagepe temporarily deployed to democratic-commons-demo-pr-29 January 18, 2019 10:00 Inactive
@GeoWill GeoWill changed the title [WiP] Geoshapes from wikidata Geoshapes from wikidata Jan 18, 2019
@GeoWill GeoWill self-assigned this Jan 18, 2019
@GeoWill GeoWill added the review label Jan 18, 2019
@GeoWill GeoWill changed the title Geoshapes from wikidata [WiP] Geoshapes from wikidata Jan 18, 2019
@GeoWill
Copy link
Contributor Author

GeoWill commented Jan 18, 2019

This only works for electoral districts, it still needs a bit of untangling for the countries.

@sagepe sagepe temporarily deployed to democratic-commons-demo-pr-29 January 22, 2019 15:16 Inactive
@GeoWill
Copy link
Contributor Author

GeoWill commented Jan 22, 2019

Some feed back on 3ff4d1e would be helpful. The desired behaviour is to look for a geoshape if no p-c-repo exists, and not get stuck in a loop of endless geoshape fetches with each new save. I wonder if a similar etag solution would work here?

@alexdutton
Copy link
Contributor

Some feed back on 3ff4d1e would be helpful. The desired behaviour is to look for a geoshape if no p-c-repo exists, and not get stuck in a loop of endless geoshape fetches with each new save. I wonder if a similar etag solution would work here?

I'd've hoped that if this were e.g.:

    boundary = instance.boundary = Boundary.objects.get(
        set__slug=slug, external_id=instance.id
    )
    if instance.boundary != boundary:
        instance.boundary = boundary
        instance.save()

Then it would only call instance.save() if it's changed. Except I believe that represent-boundaries wipes out and recreates all the BoundarySet and Boundary objects each time, so that foreign key needs updating regardless.

For now, shall we just add a if created and … guard on the post_save handler, so it only gets pulled in at creation time? We can manually kick them off for all existing items, and then we can create an issue to add periodic fetching? (which is a wider issue we ought to solve)

@sagepe sagepe temporarily deployed to democratic-commons-demo-pr-29 January 23, 2019 08:13 Inactive
@GeoWill GeoWill changed the title [WiP] Geoshapes from wikidata Geoshapes from wikidata Jan 23, 2019
@sagepe sagepe temporarily deployed to democratic-commons-demo-pr-29 January 25, 2019 07:49 Inactive
@codecov
Copy link

codecov bot commented Jan 25, 2019 •
edited
Loading

Codecov Report

Merging #29 into master will increase coverage by 2.7%.
The diff coverage is 86.89%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #29     +/-   ##
=========================================
+ Coverage   69.31%   72.02%   +2.7%     
=========================================
  Files          48       51      +3     
  Lines        1395     1537    +142     
=========================================
+ Hits          967     1107    +140     
- Misses        428      430      +2
Impacted Files Coverage Δ
commons_api/wikidata/tasks/legislature.py 13.17% <0%> (-0.21%) ⬇️
commons_api/wikidata/tasks/country.py 36.36% <0%> (-1.74%) ⬇️
commons_api/wikidata/tasks/geoshape.py 100% <100%> (ø)
commons_api/wikidata/tests/__init__.py 100% <100%> (ø) ⬆️
commons_api/celery.py 100% <100%> (ø) ⬆️
commons_api/wikidata/views.py 52.25% <100%> (+7.59%) ⬆️
commons_api/wikidata/tasks/__init__.py 100% <100%> (ø) ⬆️
commons_api/wikidata/models.py 89.26% <100%> (+2.13%) ⬆️
commons_api/wikidata/tests/geoshape.py 100% <100%> (ø)
...pi/wikidata/migrations/0010_add_geoshape_fields.py 100% <100%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update daa0a51...e585e6e. Read the comment docs.

@sagepe sagepe temporarily deployed to democratic-commons-demo-pr-29 January 25, 2019 09:16 Inactive
@sagepe sagepe temporarily deployed to democratic-commons-demo-pr-29 January 25, 2019 09:50 Inactive
codeclimate[bot]
codeclimate bot reviewed Jan 31, 2019
View reviewed changes
commons_api/wikidata/tasks/legislature.py Outdated
@celery.shared_task
def refresh_legislature_list(country_id):
country = models.Country.objects.get(id=country_id)
def refresh_legislatures(id, queued_at):
Copy link

Choose a reason for hiding this comment

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

Function refresh_legislatures has a Cognitive Complexity of 16 (exceeds 5 allowed). Consider refactoring.

commons_api/wikidata/tasks/legislature.py Outdated
@celery.shared_task
def refresh_legislature_members(legislature_id):
house = models.LegislativeHouse.objects.get(id=legislature_id)
def refresh_members(id, queued_at):
Copy link

Choose a reason for hiding this comment

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

Function refresh_members has a Cognitive Complexity of 39 (exceeds 5 allowed). Consider refactoring.

@alexdutton alexdutton changed the base branch from master to periodic-updates January 31, 2019 16:02
@alexdutton alexdutton changed the base branch from periodic-updates to master February 1, 2019 17:04
codeclimate[bot]
codeclimate bot reviewed Feb 1, 2019
View reviewed changes
commons_api/wikidata/tests/geoshape.py
'sources': 'sources',
'data': {
'type': 'FeatureCollection',
'features': [{
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

GeoWill and others added 9 commits February 4, 2019 09:46
@GeoWill @alexdutton
Add geoshape_url field to spatial model
f9e65c4
@GeoWill @alexdutton
Update queries to fetch geoshape urls
9fca364
@GeoWill @alexdutton
Make country and district tasks pick up geoshape urls
fe5c607
@alexdutton
Add queryset_filter param to periodic queuing
3d8b22e 
Some tasks (e.g. `refresh_geoshape`) only need to happen on a particular subset
of items. This change means that `with_periodic_queuing_task` will only queue
tasks for items that meet a particular condition, specified though a
`queryset_filter=lambda qs: qs.filter(…)` parameter when constructing the
decorator.
@GeoWill @alexdutton
Task to fetch and load geoshape into boundaries
5a9171d
@GeoWill @alexdutton
Make check for proto-commons repo fail quitely when repo missing.
c7a3fe8
@GeoWill @alexdutton
Add signal to get geoshape on creation of spatial instance.
fe23c20
@GeoWill @alexdutton
Add button to country to fetch geoshape from wikidata.
4499a82
@alexdutton
Check whether to refresh geoshapes regularly
e585e6e
alexdutton
alexdutton reviewed Feb 4, 2019
View reviewed changes
Copy link
Contributor

@alexdutton alexdutton left a comment

Choose a reason for hiding this comment

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

I've done some more work on this, so I'm passing the review on to Nick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate[bot] left review comments

@alexdutton alexdutton alexdutton left review comments

@jacksonj04 jacksonj04 Awaiting requested review from jacksonj04

Assignees

@GeoWill GeoWill

Labels
review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GeoWill @alexdutton @sagepe