diff --git a/OpenOversight/app/main/views.py b/OpenOversight/app/main/views.py index 6901a34f0..b6a0152b5 100644 --- a/OpenOversight/app/main/views.py +++ b/OpenOversight/app/main/views.py @@ -675,7 +675,7 @@ def list_officer( ): form_data["gender"] = genders - unit_choices = [ + unit_choices = ["Not Sure"] + [ uc[0] for uc in db.session.query(Unit.descrip) .filter_by(department_id=department_id) @@ -717,7 +717,7 @@ def list_officer( "race": RACE_CHOICES, "gender": GENDER_CHOICES, "rank": [(rc, rc) for rc in rank_choices], - "unit": [("Not Sure", "Not Sure")] + [(uc, uc) for uc in unit_choices], + "unit": [(uc, uc) for uc in unit_choices], } next_url = url_for( diff --git a/OpenOversight/app/models.py b/OpenOversight/app/models.py index 3dbd97ad2..110f9f94b 100644 --- a/OpenOversight/app/models.py +++ b/OpenOversight/app/models.py @@ -233,6 +233,7 @@ class Currency(db.TypeDecorator): """ impl = db.Numeric + cache_ok = True def load_dialect_impl(self, dialect): typ = db.Numeric() diff --git a/OpenOversight/app/utils.py b/OpenOversight/app/utils.py index 6e9361838..382b991e7 100644 --- a/OpenOversight/app/utils.py +++ b/OpenOversight/app/utils.py @@ -352,6 +352,7 @@ def filter_by_form(form_data, officer_query, department_id=None): form_data["rank"].append(None) unit_ids = [] + include_null_unit = False if form_data.get("unit"): unit_ids = [ unit.id @@ -361,17 +362,24 @@ def filter_by_form(form_data, officer_query, department_id=None): ] if "Not Sure" in form_data["unit"]: - # Convert "Not Sure" to None, so the resulting SQL query allows NULL values - form_data["unit"].append(None) + include_null_unit = True - if form_data.get("badge") or unit_ids or job_ids: + if form_data.get("badge") or unit_ids or include_null_unit or job_ids: officer_query = officer_query.join(Officer.assignments) if form_data.get("badge"): officer_query = officer_query.filter( Assignment.star_no.like("%%{}%%".format(form_data["badge"])) ) - if unit_ids: - officer_query = officer_query.filter(Assignment.unit_id.in_(unit_ids)) + + if unit_ids or include_null_unit: + # Split into 2 expressions because the SQL IN keyword does not match NULLs + unit_filters = [] + if unit_ids: + unit_filters.append(Assignment.unit_id.in_(unit_ids)) + if include_null_unit: + unit_filters.append(Assignment.unit_id.is_(None)) + officer_query = officer_query.filter(or_(*unit_filters)) + if job_ids: officer_query = officer_query.filter(Assignment.job_id.in_(job_ids)) officer_query = officer_query.options(selectinload(Officer.assignments_lazy)) diff --git a/OpenOversight/tests/conftest.py b/OpenOversight/tests/conftest.py index 48be8f914..83a202408 100644 --- a/OpenOversight/tests/conftest.py +++ b/OpenOversight/tests/conftest.py @@ -9,7 +9,7 @@ from decimal import Decimal from io import BytesIO from pathlib import Path -from typing import List +from typing import List, Optional import pytest from faker import Faker @@ -131,12 +131,14 @@ def generate_officer(): ) -def build_assignment(officer: Officer, units: List[Unit], jobs: Job): +def build_assignment(officer: Officer, units: List[Optional[Unit]], jobs: Job): + unit = random.choice(units) + unit_id = unit.id if unit else None return models.Assignment( star_no=pick_star(), job_id=random.choice(jobs).id, officer=officer, - unit_id=random.choice(units).id, + unit_id=unit_id, star_date=pick_date(officer.full_name().encode("utf-8")), resign_date=pick_date(officer.full_name().encode("utf-8")), ) @@ -308,6 +310,7 @@ def add_mockdata(session): ] session.add_all(test_units) session.commit() + test_units.append(None) test_images = [ models.Image( diff --git a/OpenOversight/tests/routes/test_officer_and_department.py b/OpenOversight/tests/routes/test_officer_and_department.py index 8edecfa3d..568fdc99e 100644 --- a/OpenOversight/tests/routes/test_officer_and_department.py +++ b/OpenOversight/tests/routes/test_officer_and_department.py @@ -31,6 +31,7 @@ from OpenOversight.app.models import ( Assignment, Department, + Face, Image, Incident, Job, @@ -1728,8 +1729,14 @@ def test_admin_can_upload_photos_of_dept_officers( officer = department.officers[3] officer_face_count = len(officer.face) - crop_mock = MagicMock(return_value=Image.query.first()) - upload_mock = MagicMock(return_value=Image.query.first()) + # Filter out images that the officer is already tagged in + officer_faces = Face.query.filter_by(officer_id=officer.id).all() + image = Image.query.filter( + Image.id.not_in([face.img_id for face in officer_faces]) + ).first() + + crop_mock = MagicMock(return_value=image) + upload_mock = MagicMock(return_value=image) with patch( "OpenOversight.app.main.views.upload_image_to_s3_and_store_in_db", upload_mock, @@ -1830,8 +1837,14 @@ def test_ac_can_upload_photos_of_dept_officers( officer = department.officers[4] officer_face_count = len(officer.face) - crop_mock = MagicMock(return_value=Image.query.first()) - upload_mock = MagicMock(return_value=Image.query.first()) + # Filter out images that the officer is already tagged in + officer_faces = Face.query.filter_by(officer_id=officer.id).all() + image = Image.query.filter( + Image.id.not_in([face.img_id for face in officer_faces]) + ).first() + + crop_mock = MagicMock(return_value=image) + upload_mock = MagicMock(return_value=image) with patch( "OpenOversight.app.main.views.upload_image_to_s3_and_store_in_db", upload_mock, diff --git a/OpenOversight/tests/test_utils.py b/OpenOversight/tests/test_utils.py index 5812d733e..00938e314 100644 --- a/OpenOversight/tests/test_utils.py +++ b/OpenOversight/tests/test_utils.py @@ -6,8 +6,12 @@ from mock import MagicMock, Mock, patch import OpenOversight -from OpenOversight.app.models import Image -from OpenOversight.app.utils import crop_image, upload_image_to_s3_and_store_in_db +from OpenOversight.app.models import Department, Image, Officer, Unit +from OpenOversight.app.utils import ( + crop_image, + filter_by_form, + upload_image_to_s3_and_store_in_db, +) from OpenOversight.tests.routes.route_helpers import login_user @@ -305,3 +309,29 @@ def test_crop_image_calls_upload_image_to_s3_and_store_in_db_with_user_id( assert ( current_user.get_id() in upload_image_to_s3_and_store_in_db.call_args[0] ) + + +@pytest.mark.parametrize( + "units, has_officers_with_unit, has_officers_with_no_unit", + [ + (["Not Sure"], False, True), + (["Donut Devourers"], True, False), + (["Not Sure", "Donut Devourers"], True, True), + ], +) +def test_filter_by_form_filter_unit( + mockdata, units, has_officers_with_unit, has_officers_with_no_unit +): + form_data = dict(unit=units) + unit_id = Unit.query.filter_by(descrip="Donut Devourers").one().id + department_id = Department.query.first().id + + officers = filter_by_form(form_data, Officer.query, department_id).all() + + for officer in officers: + found = False + if has_officers_with_unit: + found = found or any([a.unit_id == unit_id for a in officer.assignments]) + if has_officers_with_no_unit: + found = found or any([a.unit_id is None for a in officer.assignments]) + assert found