Create Remaining Initial Django Models #82

rachelekm · 2022-09-29T02:35:48Z

Overview

This creates the initial Django models needed for the different states of submission as well as for the starting user interactions on the frontend, as discussed in the issue comments.

Closes #69

Demo

An ADR is available as part of the PR.

Notes

The State default instance, North Carolina, is created through a data migration that references a GEOJSON file downloaded during scripts/fetch-data. I found the state boundary url here, but not sure if there's a better resource for state boundary geojson files? I also included scripts/fetch-data as part of the setup script so that the GEOJSON gets setup before running migrations. I didn't see any downsides to this, but not sure if I'm missing context on fetching the municipality data (the original script purpose)?
I added __str__ methods to some of the models where it felt appropriate, but not all. I wasn't sure what language to use to best describe instances like the "approval of the submission of a boundary of a utility" when there wasn't a unique identifier besides the utility, and it seemed to better fit a future task once we better know use case anyways.
I decided to group together Submission, Approval, Review, and Annotation models into one file. These felt like all related pieces of the submission process that could benefit from being organized together, but curious what others think here.
~~I made the max_length of the Submission.upload_filename and ReferenceImage.filename fields match the max_length of the Django FileField since that seemed like an appropriate reference.~~ These fields' max_length are now 255.

Testing Instructions

./scripts/fetch-data
./scripts/update
./scripts/server
Visit the below Django admin pages for each newly created object:
- State
- Boundary
- Submission
- ReferenceImage
- Review
- Annotation
- Approval
Confirm the correct field and field types for each object
Confirm correct required field validation on save
Confirm North Carolina is a created State instance and fields are as expected
Confirm User and Utility have new state fields
Confirm Utility's state field defaults to North Carolina

Checklist

fixup! commits have been squashed
CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines
README.md updated if necessary to reflect the changes
CI passes after rebase

rajadain · 2022-09-29T13:37:10Z

Taking a look

rajadain · 2022-09-29T14:13:15Z

Here's a visualization of this model setup made using:

./scripts/manage graph_models api > graph.dot

And using https://dreampuf.github.io/GraphvizOnline/ to convert that to SVG:

scripts/fetch-data

src/django/api/models/utility.py

src/django/api/models/submission.py

rajadain

This looks great! Nice work.

There may be some State design we can borrow from other projects. Only minor comments here and there. Will take another look once addressed.

rajadain · 2022-09-29T14:16:38Z

scripts/fetch-data

+                rm "$DJANGO_DATA_DIR/$NC_GEOJSON_FILENAME"
+                fetchDefaultDBData
+            else
+                echo "Skipping default django data fetching: file exists."


Nice attention to developer experience here

rajadain · 2022-09-29T14:24:40Z

src/django/api/migrations/0007_add_default_state.py

+    # We should populate db with a State so we have ability to
+    # add upcoming state field on Utility with default FK id
+    State = apps.get_model('api', 'State')
+    NC_GEOJSON = json.load(open(os.path.join(settings.BASE_DIR, 'data/NC_BOUNDARY.geojson'), 'r'))


In general, prefer to open files using with so the file is closed after use:

with open(os.path.join(settings.BASE_DIR, 'data/NC_BOUNDARY.geojson'), 'r') as f: NC_GEOSJON = json.load(f) ...

rajadain · 2022-09-29T14:28:09Z

src/django/api/models/state.py

+    id = models.CharField(max_length=2, primary_key=True, unique=True)
+    name = models.CharField(max_length=127)
+    boundary = gis_models.PolygonField(geography=True)
+    options = models.JSONField(blank=True, null=True)


Instead of django.db.models.JSONField, prefer django.contrib.postgres.fields.JSONField, to take advantage of Postgres-native JSON support.

Also consider adding help_text explaining that this JSON dictionary will contain state-specific configuration.

Documenting here since this was a separate slack discussion, the postgres JSONField is deprecated and will lose support in Django 4.0 so we'll move forward with the recommended django.db.models.JSONField. I'll still update to add help_text to this field, though.

rajadain · 2022-09-29T14:29:27Z

src/django/api/models/state.py

+class State(models.Model):
+    id = models.CharField(max_length=2, primary_key=True, unique=True)
+    name = models.CharField(max_length=127)
+    boundary = gis_models.PolygonField(geography=True)


Since "Boundary" is an entity, with "shape" as the polygon field name, let's reuse that here for consistency.

Suggested change

boundary = gis_models.PolygonField(geography=True)

shape = gis_models.PolygonField(geography=True)

Also, let's use a MultiPolygonField since many states have disconnected polygons in their shape (e.g. Michigan)

rajadain · 2022-09-29T14:35:38Z

src/django/api/models/state.py

+
+
+class State(models.Model):
+    id = models.CharField(max_length=2, primary_key=True, unique=True)


Prefer using django.contrib.postgres.fields.CICharField for a case-insensitive field that will match "NC", "nc", and any other combination.

Nice, will implement! Will also get rid of the clean() method to uppercase that field since that will no longer be needed

rajadain · 2022-09-29T14:39:36Z

src/django/api/models/reference_image.py

+
+
+class ReferenceImage(models.Model):
+    filename = models.CharField(max_length=100, blank=True)


Prefer 255 as the filename length which is more standard.

rajadain · 2022-09-29T14:40:58Z

src/django/api/models/reference_image.py

+
+class ReferenceImage(models.Model):
+    filename = models.CharField(max_length=100, blank=True)
+    boundary = models.ForeignKey(Boundary, on_delete=models.CASCADE)


For on_delete, prefer models.PROTECT in most cases, except those where accidental deletion of related data is tolerable. I think in this app, we should use models.PROTECT in all cases.

src/django/api/models/submission.py

rajadain · 2022-09-29T20:27:54Z

Let's also add a updated_at timestamp column to Submission. The UI wireframes need it here:

jacobtylerwalls · 2022-09-30T00:19:18Z

src/django/api/migrations/0007_add_default_state.py

+    ]
+
+    operations = [
+        migrations.RunPython(add_default_state_nc),


Another team discussion topic, but I try to always add a reverse operation for RunPython--at the very least RunPython.noop--because it makes unapplying migrations easier (which does come up in development, when switching branches).

Would second a team discussion on this one! I popped RunPython.noop in for myself when I was reverting some of these migrations for edits but noticed some of our other projects didn't have one so I took it out....but maybe worth bringing back in regardless for our own ease of use?

rajadain · 2022-09-30T14:39:51Z

Taking another look

rajadain

👏 Excellent work! This is looking great. Should be good to go after a few small tweaks.

src/django/api/models/state.py

src/django/api/models/utility.py

src/django/api/models/submission.py

doc/arch/adr-001-data-models.md

Since Utility and State have a many-to-one relationship, with state being a non-nullable FK field on Utility, a default State needed to exist before the creation of the state FK field on Utility. A data migration adding in the NC State row occurs after the State model migration and before the Utility update migration resolves this.

rachelekm · 2022-09-30T18:47:49Z

Thanks, all! Those last changes are all implemented for this next review.

rajadain

+1 tested, this looks great! Nice work carrying this through the different iterations. There may be some more small tweaks in the future, but this solid base sets us up very well. Great job!

rachelekm force-pushed the rm/add-submissions-data-models branch from a31364c to 3cd3a27 Compare September 29, 2022 02:44

rachelekm marked this pull request as ready for review September 29, 2022 02:49

rachelekm requested a review from rajadain September 29, 2022 02:50

rachelekm assigned rajadain Sep 29, 2022

jacobtylerwalls reviewed Sep 29, 2022

View reviewed changes

scripts/fetch-data Show resolved Hide resolved

src/django/api/models/utility.py Outdated Show resolved Hide resolved

jacobtylerwalls reviewed Sep 29, 2022

View reviewed changes

src/django/api/models/submission.py Outdated Show resolved Hide resolved

rajadain requested changes Sep 29, 2022

View reviewed changes

rachelekm force-pushed the rm/add-submissions-data-models branch 2 times, most recently from 92d1397 to 790ea0a Compare September 30, 2022 00:10

rachelekm requested a review from rajadain September 30, 2022 00:13

jacobtylerwalls reviewed Sep 30, 2022

View reviewed changes

rajadain reviewed Sep 30, 2022

View reviewed changes

src/django/api/models/state.py Outdated Show resolved Hide resolved

src/django/api/models/utility.py Outdated Show resolved Hide resolved

src/django/api/models/submission.py Outdated Show resolved Hide resolved

doc/arch/adr-001-data-models.md Outdated Show resolved Hide resolved

rachelekm force-pushed the rm/add-submissions-data-models branch from 790ea0a to 5406b69 Compare September 30, 2022 18:39

rachelekm added 4 commits September 30, 2022 14:42

Add data models for submission states

bfbc09a

Add final ERD to doc dir

847c6bb

Update CHANGELOG

2b771db

rachelekm force-pushed the rm/add-submissions-data-models branch from 5406b69 to 2b771db Compare September 30, 2022 18:42

rachelekm requested a review from rajadain September 30, 2022 18:47

rajadain approved these changes Sep 30, 2022

View reviewed changes

rajadain assigned rachelekm and unassigned rajadain Sep 30, 2022

rachelekm merged commit bb2faf0 into develop Sep 30, 2022

rachelekm deleted the rm/add-submissions-data-models branch September 30, 2022 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Remaining Initial Django Models #82

Create Remaining Initial Django Models #82

rachelekm commented Sep 29, 2022 •

edited

rajadain commented Sep 29, 2022

rajadain commented Sep 29, 2022

rajadain left a comment

rajadain Sep 29, 2022

rajadain Sep 29, 2022

rajadain Sep 29, 2022

rajadain Sep 29, 2022

rachelekm Sep 29, 2022

rajadain Sep 29, 2022

rajadain Sep 29, 2022

rajadain Sep 29, 2022

rachelekm Sep 29, 2022

rajadain Sep 29, 2022

rajadain Sep 29, 2022

rajadain commented Sep 29, 2022

jacobtylerwalls Sep 30, 2022

rachelekm Sep 30, 2022

rajadain commented Sep 30, 2022

rajadain left a comment

rachelekm commented Sep 30, 2022

rajadain left a comment

	boundary = gis_models.PolygonField(geography=True)
	shape = gis_models.PolygonField(geography=True)



		class State(models.Model):
		id = models.CharField(max_length=2, primary_key=True, unique=True)



		class ReferenceImage(models.Model):
		filename = models.CharField(max_length=100, blank=True)

Create Remaining Initial Django Models #82

Create Remaining Initial Django Models #82

Conversation

rachelekm commented Sep 29, 2022 • edited

Overview

Demo

Notes

Testing Instructions

Checklist

rajadain commented Sep 29, 2022

rajadain commented Sep 29, 2022

rajadain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajadain commented Sep 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajadain commented Sep 30, 2022

rajadain left a comment

Choose a reason for hiding this comment

rachelekm commented Sep 30, 2022

rajadain left a comment

Choose a reason for hiding this comment

rachelekm commented Sep 29, 2022 •

edited