Enable admin to add new officers and units for any department #310

redshiftzero · 2017-09-24T19:04:10Z

Status

Ready for review

Description of Changes

This PR:

Adds department_id columns to Officer and Unit
Adds manage.py command to link existing officers and units to the first department
Adds admin-only "Add officer" feature (for any department). Closes "Add cop" admin only route #213.
Adds admin-only "Add unit" feature (for any department).

Notes for Deployment

$ pip install -r ../requirements.txt
$ python manage.py db upgrade
$ python manage.py link_officers_to_department

Tests and linting

I have rebased my changes on current develop
pytests pass in the development environment on my local machine
flake8 checks pass

r4v5

Some questions, nothing block-worthy. Nice work!

r4v5 · 2017-11-08T03:08:04Z

OpenOversight/app/db_repository/versions/006_migration.py

+    Column('middle_initial', String(length=120)),
+    Column('race', String(length=120)),
+    Column('gender', String(length=120)),
+    Column('employment_date', DateTime),


this has the potential for weird timezone bugs, but i don’t know how much potential it has.

Specifically i don’t remember if postgres’s datetime support includes zone or assumes the application handles it; there’s a potential for discrepancy when initializing these with dates (at time 00:00) and being returned referencing the day before/after we intended.

Good point @r4v5. Filed this as a followup in #323 (note that this table exists and the column is already populated in the prod database)

r4v5 · 2017-11-08T03:09:44Z

OpenOversight/app/db_repository/versions/006_migration.py

+from migrate.changeset import schema
+pre_meta = MetaData()
+post_meta = MetaData()
+unit_types = Table('unit_types', post_meta,


what exactly is this table holding? units or unit types? unit type to me implies a general type with a one-to-many relationship from unit type to department but we make department id an int column, implying 1-to-1.

also, ‘descrip’ feels like a weird choice of column name; is there a reason to not just use name here?
(asking these questions early while refactoring/migrations are still relatively easy)

hmm so this table holds the unit names / descriptions, I linked them to department as I think it's nice to store it this way because when a user adds an officer assignment, the webapp could show them only the units that are associated with that police department

r4v5 · 2017-11-08T03:23:30Z

OpenOversight/app/db_repository/versions/006_migration.py

+    Column('gender', String(length=120)),
+    Column('employment_date', DateTime),
+    Column('birth_year', Integer),
+    Column('pd_id', Integer),


what is pd_id and how does it differ from department_id?

true - pd_id is not used, so removed in e68da55

r4v5 · 2017-11-08T03:25:39Z

OpenOversight/app/main/views.py

+                          department_id=form.department.data.id)
+        db.session.add(officer)
+        db.session.commit()
+        assignment = Assignment(officer_id=officer.id,


worth noting that because we commit the session for officer before assignment, it’s possible to fail to create an officer+assignment duo due to things like db constraints or connectivity loss and have actually created the officer underlying it, which sounds like a way to accidentally get duplicate officers.

Modified this to commit the two together in a91f3f7

r4v5 · 2017-11-08T03:27:12Z

OpenOversight/app/models.py

@@ -33,6 +33,7 @@ class Officer(db.Model):
    pd_id = db.Column(db.Integer, index=True, unique=False)
    assignments = db.relationship('Assignment', backref='officer', lazy='dynamic')
    face = db.relationship('Face', backref='officer', lazy='dynamic')
+    department_id = db.Column(db.Integer)


do we not want to explicitly state these are foreign keys here and in the migration?

also what happens if someone changes departments, say from UCPD to OPD?

r4v5 · 2017-11-08T03:29:22Z

OpenOversight/app/models.py

@@ -62,6 +63,7 @@ class Unit(db.Model):

    id = db.Column(db.Integer, primary_key=True)
    descrip = db.Column(db.String(120), index=True, unique=False)
+    department_id = db.Column(db.Integer)


same FK comment

Fixed in 495df5d

We no longer need these: all these migrations have been applied in master

in officers and units tables

And update migration

redshiftzero · 2017-12-10T05:54:33Z

Well, I tried to squash the migrations in this PR, got annoyed at how finicky sqlalchemy-migrate is, so I ripped it out and replaced it with alembic (via Flask-Migrate). I've upgrade tested this and all went well. Deployment instructions in the PR are updated. I'm going to merge this now since I'm about to push more changes on top of the changes in this PR

Bumps [sass](https://github.com/sass/dart-sass) from 1.61.0 to 1.62.1. - [Release notes](https://github.com/sass/dart-sass/releases) - [Changelog](https://github.com/sass/dart-sass/blob/main/CHANGELOG.md) - [Commits](sass/dart-sass@1.61.0...1.62.1) --- updated-dependencies: - dependency-name: sass dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

r4v5 mentioned this pull request Sep 26, 2017

Show police department on the image and tag detail pages #307

Merged

3 tasks

redshiftzero force-pushed the add-officers-units-multi-city branch from a7e29d4 to 90bb847 Compare October 9, 2017 08:45

r4v5 reviewed Nov 8, 2017

View reviewed changes

redshiftzero added the in progress label Dec 10, 2017

redshiftzero added 22 commits December 9, 2017 20:10

Remove deprecated db_repository directory

e208af1

We no longer need these: all these migrations have been applied in master

Hello alembic, goodbye sqlalchemy-migrate

223e3a8

Rework manage.py to use Flask-Migrate's db command

ad8463e

manage.py db init

fb0f921

Update contributing guide

7098659

Update deployment script

17611f0

Link departments with officers and units

7af2360

Add database migration for new department_id columns

4a62d4c

in officers and units tables

Add management command to link officers and units to a department

8d8e725

Add department_id to Officer, Unit in test data

4e59ac0

Add AddOfficerForm and AddUnitForm to forms.py

c0c2681

Add /officer/new and /unit/new routes

19cd6be

Add unit tests for basic new officer, unit cases

c280b1d

Add dept_choices function for query selector

cdab143

Add templates for add officer, unit forms

e56d098

Add buttons to "Departments" page for adding officers, units

edeeddd

Remove pd_id as it is unused

8c277a6

And update migration

Add FK relationship to departments table

d5a866e

Use a relationship to avoid commiting officer and assn separately

837395a

Improve exception handling

81b0dcb

Update test_data.py

1103720

Add Alembic migration for department_id migrations

8c09314

redshiftzero force-pushed the add-officers-units-multi-city branch from d11d449 to 8c09314 Compare December 10, 2017 05:26

Appease linter

5d9e569

redshiftzero merged commit 76ffaad into develop Dec 10, 2017

r4v5 deleted the add-officers-units-multi-city branch July 1, 2018 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable admin to add new officers and units for any department #310

Enable admin to add new officers and units for any department #310

redshiftzero commented Sep 24, 2017 •

edited

Loading

r4v5 left a comment

r4v5 Nov 8, 2017

redshiftzero Nov 13, 2017

r4v5 Nov 8, 2017

r4v5 Nov 8, 2017

redshiftzero Nov 13, 2017

r4v5 Nov 8, 2017

redshiftzero Nov 13, 2017

r4v5 Nov 8, 2017

redshiftzero Nov 13, 2017

r4v5 Nov 8, 2017

r4v5 Nov 8, 2017

r4v5 Nov 8, 2017

redshiftzero Nov 13, 2017

redshiftzero commented Dec 10, 2017

Enable admin to add new officers and units for any department #310

Enable admin to add new officers and units for any department #310

Conversation

redshiftzero commented Sep 24, 2017 • edited Loading

Status

Description of Changes

Notes for Deployment

Tests and linting

r4v5 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redshiftzero commented Dec 10, 2017

redshiftzero commented Sep 24, 2017 •

edited

Loading