Officer unit view

[fritzdavenport]

Status

Ready for review

Description of Changes

Fixes #733 + #786

Changes proposed in this pull request:

• add unit selector to list_officer form
• add make lint and make attach with updated docs, to makefile
• log form validation errors as logging.info

Notes for Deployment

Screenshots (if appropriate)

Tests and linting

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

[fritzdavenport]

Not sure how to add reviewers
@dismantl @brianmwaters - this is a redone version of the other attempt, this time as a part of the department view of officers. Looks much cleaner, thanks for the suggestion @dismantl

Working on a few tests that got weird, but code-complete.

[fritzdavenport]

This was added - there was a weird issue of failing unit tests on the first PR. I updated the test data to have datetime's for start_date + resign_date on assignments. Without this equality check, we would be == comparing datetime(2020, 01, 01) to '2020-01-01', which would fail and we would trigger spurious row updates and fail the tests.

[fritzdavenport]

Added pick_date to get an idempotent but random-enough date for officers. Jumbles it up based on name

[fritzdavenport]

fixing a bug - patch-pulled this in from my s3 PR

[abandoned-prototype]

You are using a SelectField without providing choices. This is what leads to the error with test_user_can_use_form_to_get_to_browse (although this certainly should be surfaced better)
Since the options for unit are not static like race and gender and furthermore depend on the department that was selected, I think the right choice would be to do whatever the solution for rank is, since that field has similar constraints (I am not familiar with how this is done though)

[abandoned-prototype]

For the two failing tests test_ac_can_edit_officer_in_their_dept and test_ac_can_add_new_officer_with_unit_in_their_dept the following patch is a quick-fix for those issues:

diff --git a/OpenOversight/app/utils.py b/OpenOversight/app/utils.py
index 6baaf3f..c8a04da 100644
--- a/OpenOversight/app/utils.py
+++ b/OpenOversight/app/utils.py
@@ -12,6 +12,7 @@ import os
 import random
 import sys
 from traceback import format_exc
+from typing import Optional
 
 from sqlalchemy import func
 from sqlalchemy.sql.expression import cast
@@ -69,7 +70,9 @@ def get_or_create(session, model, defaults=None, **kwargs):
         return instance, True
 
 
-def unit_choices():
+def unit_choices(department_id: Optional[int] = None):
+    if department_id is not None:
+        return db.session.query(Unit).filter_by(department_id=department_id).all()
     return db.session.query(Unit).all()
 
 
diff --git a/OpenOversight/tests/routes/test_officer_and_department.py b/OpenOversight/tests/routes/test_officer_and_department.py
index 2bbad96..15a0f93 100644
--- a/OpenOversight/tests/routes/test_officer_and_department.py
+++ b/OpenOversight/tests/routes/test_officer_and_department.py
@@ -757,7 +757,7 @@ def test_ac_can_add_new_officer_with_unit_in_their_dept(mockdata, client, sessio
     with current_app.test_request_context():
         login_ac(client)
         department = Department.query.filter_by(id=AC_DEPT).first()
-        unit = random.choice(unit_choices())
+        unit = random.choice(unit_choices(department.id))
         first_name = 'Testy'
         last_name = 'OTester'
         middle_initial = 'R'
@@ -922,7 +922,7 @@ def test_ac_can_edit_officer_in_their_dept(mockdata, client, session):
     with current_app.test_request_context():
         login_ac(client)
         department = Department.query.filter_by(id=AC_DEPT).first()
-        unit = random.choice(unit_choices())
+        unit = random.choice(unit_choices(department.id))
         first_name = 'Testier'
         last_name = 'OTester'
         middle_initial = 'R'

The problem was that the changes in the PR add some new units to the test-data, which results in the random pick selecting a unit that does not belong to the department of the area coordinator, which makes the validation fail.

This points to several issues, specifically how we surface failures in validation (we don't at all) and the problems with choosing randomly from a pool where some choices make the test fail.

[fritzdavenport]

alright. Fixed some of that other issue of form errors dying silently. I skipped adding the logging in a few places that had the form validation jumbled with a bunch of other conditionals.

Tests passing, lint passing. Thanks a whole heap @abandoned-prototype - you definitely unstuck me. Much appreciated.

[dismantl]

Works perfectly as advertised, great job on this! I also appreciate the addition Make targets. Just one comment I'll add is that the extra logging doesn't give much useful information. I tried hitting several form errors on the site and they mostly just logged the message INFO in views: {}. So in the future it would probably be good to either include more context in those log messages or instead refrain from logging them.

[fritzdavenport]

@dismantl - that's interesting and unfortunate. The logging was at least popping up on the failed unit tests but I didn't test in-depth and didn't read enough on WTForms to figure out why forms.errors would or wouldn't be filled. Maybe I should expand those conditionals to also not print on {}? I'm not a fan of excessive logging, either way :/