Field changes to the territorial entity. #127

ataalik · 2019-05-11T20:31:47Z

Adds start and end dates and a label for names to be displayed. Fixes #99

quorth0n · 2019-05-12T01:34:58Z

project/api/models.py

@@ -33,8 +33,12 @@ class TerritorialEntity(models.Model):
    """

    wikidata_id = models.PositiveIntegerField()  # Excluding the Q
+    label = models.TextField(max_length=90)


Should be a nullable PointField similar to visual_center, and updated as a post create hook

Actually, on second thought we should probably keep this field for #21. We will still need the PointField though.

We have Point Field for the visual center in STV's table

Right, my comment was before the Slack post. Originally I was thinking a field here could correspond to P625 for a wikdiata entity but it doesn't seem that property does us much good in any situation.

MiklerGM · 2019-05-12T21:14:20Z

project/api/models.py

@@ -33,8 +33,12 @@ class TerritorialEntity(models.Model):
    """

    wikidata_id = models.PositiveIntegerField()  # Excluding the Q
+    label = models.TextField(max_length=90)


We have Point Field for the visual center in STV's table

MiklerGM · 2019-05-12T21:19:48Z

project/api/models.py

    color = ColorField()
    admin_level = models.PositiveIntegerField()
+    inception_date = models.DecimalField(decimal_places=1, max_digits=10)
+    dissolution_date = models.DecimalField(decimal_places=1, max_digits=10)


I assume that inception or dissolution dates can be unknown. For example, countries that are existing right now does not have a dissolution date.

Probably we should add blank=True, null=True

MiklerGM · 2019-05-15T15:48:49Z

Hey, probably it's time to add successor or predecessor field to the TE? (for #5)
Or it is a bad idea and I should forget about it.
What do you think?

ataalik · 2019-05-17T04:59:55Z

Hey, probably it's time to add successor or predecessor field to the TE? (for #5)
Or it is a bad idea and I should forget about it.
What do you think?

I don't see why not. Many-to-Many relationships? It could be a PoliticalRelation too maybe.

MiklerGM · 2019-05-17T17:08:21Z

We have a range of dates in Political Relations, probably it is not the best place to store this.

Many-to-Many will be the most accurate description for this field.

MiklerGM · 2019-05-23T15:03:29Z

Well, I've noticed that we have lots of duplicate wikidata_id entities, it might be better to add information about total STVs under TE control to the search output.
As far as I understand we've got duplicate ids mostly because of disputed territories, probably some of them from processing errors. And for sure, some of them there because of incorrect labeling (entities without corresponding wikidata id). I think @v1kun should know more about this.

Can we do this? Do you find it valuable?

Thanks

ataalik · 2019-06-08T05:35:04Z

Should we split the tests for api and models. The linter is giving too-many-lines errors

quorth0n · 2019-06-08T21:05:50Z

@ataalik I noticed that too, we definitely should

MiklerGM

Can we change the behavior of stv_count?

MiklerGM · 2019-06-11T13:26:26Z

project/api/models.py

@@ -37,7 +39,8 @@ class TerritorialEntity(models.Model):
    color = ColorField()
    admin_level = models.PositiveIntegerField()
    inception_date = models.DecimalField(decimal_places=1, max_digits=10)
-    dissolution_date = models.DecimalField(decimal_places=1, max_digits=10)
+    dissolution_date = models.DecimalField(decimal_places=1, max_digits=10, blank=True, null=True)
+    stv_count = models.IntegerField(default=0, null=True, blank=True)


I thought this would be done via aggregate function, like it was done for the votes in #102

backend/project/api/serializers.py

Lines 200 to 208 in 7b75b9d

def get_votes(self, obj): # pylint: disable=R0201

"""

Returns dict of upvotes and downvotes

"""

qs = NarrativeVote.objects.filter(narrative=obj)

upvotes = qs.aggregate(upvotes=Count(Case(When(vote=True, then=1))))

downvotes = qs.aggregate(downvotes=Count(Case(When(vote=False, then=1))))

return {**upvotes, **downvotes}

MiklerGM

Well, stv_count is working fine, but I forgot about #125 and nested STVs as an array.
I am sorry that I've asked you to add the counter when it can be relatively easy to calculate stv_count on the frontend.
We can keep the field or remove it, and it's up to you.

However, I would advise changing the restrictions for inception_date. I would go with the full restrictions in case we can guarantee that we've got the clean data from the Wikidata

MiklerGM · 2019-06-12T09:21:00Z

project/api/models.py

    color = ColorField()
    admin_level = models.PositiveIntegerField()
+    inception_date = models.DecimalField(decimal_places=1, max_digits=10)
+    dissolution_date = models.DecimalField(
+        decimal_places=1, max_digits=10, blank=True, null=True


Are you sure that we should do this strict validation on the inception date?

Especially when you are validating it for not null

backend/project/api/models.py

Lines 55 to 60 in 88e5fc7

def clean(self, *args, **kwargs): # pylint: disable=W0221

if not self.inception_date is None and not self.dissolution_date is None:

if self.inception_date > self.dissolution_date:

raise ValidationError(

"Inception date cannot be later than dissolution date"

)

postgres=# INSERT INTO api_territorialentity VALUES (1, 30, '10', 2, NULL, NULL, 'USA'); ERROR: null value in column "inception_date" violates not-null constraint DETAIL: Failing row contains (1, 30, 10, 2, null, null, USA). postgres=# INSERT INTO api_territorialentity (wikidata_id, color, admin_level, label) VALUES (30, '10', 2, 'USA'); ERROR: null value in column "inception_date" violates not-null constraint DETAIL: Failing row contains (1, 30, 10, 2, null, null, USA). postgres=# INSERT INTO api_territorialentity VALUES (1, 30, '10', 2, 0, 0, 'USA'); INSERT 0 1 postgres=# INSERT INTO api_territorialentity VALUES (2, 30, '10', 2, NULL, 0, 'USA2'); INSERT 0 1 postgres=# select * from api_territorialentity; id | wikidata_id | color | admin_level | dissolution_date | inception_date | label ----+-------------+-------+-------------+------------------+----------------+------- 1 | 30 | 10 | 2 | 0.0 | 0.0 | USA 2 | 30 | 10 | 2 | | 0.0 | USA2 (2 rows)

One more question, why inception and dissolution date are returned as a string via API request?

{ "id":2, "stvs":[], "stv_count":0, "wikidata_id":30, "label":"USA2", "color":"10", "admin_level":2, "inception_date":"0.0", "dissolution_date":null, "predecessor":[], "relations":[] }

I've noticed that we have one more reserved SQL word as a column - references, #119

The response after adding an STV

{ "id": 1, "stvs": [ // This is an array of all stvs { "id": 1, "start_date": "1.0", "end_date": "0.0", "references": [ "qwe", "qwee" ], "visual_center": { "type": "Point", "coordinates": [ -71.060316, 48.432044 ] }, "entity": 1, "related_events": [] } ], "stv_count": 1, // this is a new count field "wikidata_id": 30, "label": "USA", "color": "10", "admin_level": 2, "inception_date": "0.0", "dissolution_date": "0.0", "predecessor": [], "relations": [] }

I was thinking thinking the dates as a user input but of course since we have gaps in wikidata both date fields should have less restriction. I guess they should be nullable?

I need to convert the dates using the library in the viewset I think. Will take care of it.

What should references be changed as. Citations comes to mind.

MiklerGM · 2019-06-14T09:27:12Z

bors r+

127: Field changes to the territorial entity. r=MiklerGM a=ataalik Adds start and end dates and a label for names to be displayed. Fixes #99 Co-authored-by: ata <[email protected]>

bors · 2019-06-14T09:32:19Z

Build failed

continuous-integration/travis-ci/push

ataalik · 2019-06-15T08:30:42Z

I was expecting the linting errors. Apparently pylint has a bug where you can not disable duplicate-code check on a per file basis with comments etc that they don't fix. We could disable it in the makefile for all the project.

Not sure about the test errors though. It runs fine on my end. I will check that out.

MiklerGM · 2019-06-23T11:09:47Z

Can we disable this rule for whole tests directory?

ataalik · 2019-06-24T17:35:26Z

I think the only thing that works is running the lint command two times, once for tests directory with disable duplicate code check flag and once for the general project.

quorth0n · 2019-06-25T20:38:21Z

Could we try placing a new .pylintrc in the tests directory to disable the duplicate-code check?

quorth0n · 2019-06-25T20:44:54Z

ERROR: unsatisfiable constraints:
  proj4-dev (missing):
    required by: .build-deps-testing-20190623.110637[proj4-dev]

This needs to be fixed, can we fix the apk add lines so that this dependency is accounted for? make clean is helpful in obtaining a clean testing environment to test docker setup with.

ataalik · 2019-06-26T11:08:27Z

Could we try placing a new .pylintrc in the tests directory to disable the duplicate-code check?

I couldn't get that to work, maybe you should take a look

MiklerGM · 2019-06-26T22:22:16Z

As it was discussed on slack:

I would propose to mock requests.get in unit tests.
Additionally, I would recommend mimicking browser behavior by adding the user-agent header. Current UA is set to python-requests/2.21.0.
Wikidata can ban all users by UA or network.

I've got next error page from wikidata

<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
<title>Error 403 You have been banned until 2019-06-26T17:35:05.645Z, please respect throttling and retry-after headers.</title>
</head>
<body><h2>HTTP ERROR 403</h2>
<p>Problem accessing /bigdata/namespace/wdq/sparql. Reason:
<pre>    You have been banned until 2019-06-26T17:35:05.645Z, please respect throttling and retry-after headers.</pre></p><hr><a href="http://eclipse.org/jetty">Powered by Jetty:// 9.4.12.v20180830</a><hr/>

</body>
</html>

MiklerGM · 2019-06-26T22:52:48Z

bors r+

127: Field changes to the territorial entity. r=MiklerGM a=ataalik Adds start and end dates and a label for names to be displayed. Fixes #99 Co-authored-by: ata <[email protected]> Co-authored-by: Michael Orlov <[email protected]>

bors · 2019-06-26T22:58:07Z

Build succeeded

continuous-integration/travis-ci/push

ataalik added 2 commits May 11, 2019 19:25

Added inception and dissolution dates for terretorial entities

78e5e0b

Added label to TE

728a236

quorth0n reviewed May 12, 2019

View reviewed changes

MiklerGM reviewed May 12, 2019

View reviewed changes

MiklerGM closed this May 13, 2019

MiklerGM deleted the ata-99 branch May 13, 2019 13:25

MiklerGM restored the ata-99 branch May 13, 2019 13:28

MiklerGM reopened this May 13, 2019

made dates emptry and nullable

c54418e

ataalik added 3 commits June 8, 2019 07:56

Added stv count to TE

9097cc4

Merge branch 'ata-99' of github.com:chronhq/backend into ata-99

0a37bfb

Lint and simplify migrations

0c26622

MiklerGM requested changes Jun 11, 2019

View reviewed changes

Moved stv_count to views

88e5fc7

MiklerGM reviewed Jun 12, 2019

View reviewed changes

ataalik added 2 commits June 13, 2019 10:07

Made inception_date nullable

b3ae534

Split tests across two files

d0fb730

MiklerGM approved these changes Jun 13, 2019

View reviewed changes

bors bot added a commit that referenced this pull request Jun 14, 2019

Merge #127

f2f9e41

127: Field changes to the territorial entity. r=MiklerGM a=ataalik Adds start and end dates and a label for names to be displayed. Fixes #99 Co-authored-by: ata <[email protected]>

Added new pylintrc for tests module

1a1d03e