From 5fd10622912fb98c08e6206eec64ba78cae64b89 Mon Sep 17 00:00:00 2001 From: abandoned-prototype <41744410+abandoned-prototype@users.noreply.github.com> Date: Sun, 28 Feb 2021 15:46:00 -0600 Subject: [PATCH] simplify and streamline officer filtering (#858) --- OpenOversight/app/config.py | 1 + OpenOversight/app/main/views.py | 19 ++-- OpenOversight/app/models.py | 20 ++--- OpenOversight/app/templates/list_officer.html | 2 +- .../app/templates/tagger_gallery.html | 4 +- OpenOversight/app/utils.py | 88 +++++++++---------- .../routes/test_officer_and_department.py | 12 +-- OpenOversight/tests/test_utils.py | 4 +- 8 files changed, 77 insertions(+), 73 deletions(-) diff --git a/OpenOversight/app/config.py b/OpenOversight/app/config.py index 1e2077b20..8e2636fb5 100644 --- a/OpenOversight/app/config.py +++ b/OpenOversight/app/config.py @@ -50,6 +50,7 @@ def init_app(app): class DevelopmentConfig(BaseConfig): DEBUG = True + SQLALCHEMY_ECHO = True SQLALCHEMY_DATABASE_URI = os.environ.get('SQLALCHEMY_DATABASE_URI') NUM_OFFICERS = 15000 SITEMAP_URL_SCHEME = 'http' diff --git a/OpenOversight/app/main/views.py b/OpenOversight/app/main/views.py index 43fdf0e05..90d95eca6 100644 --- a/OpenOversight/app/main/views.py +++ b/OpenOversight/app/main/views.py @@ -5,6 +5,7 @@ import re from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound +from sqlalchemy.orm import selectinload import sys from traceback import format_exc @@ -540,15 +541,23 @@ def list_officer(department_id, page=1, race=[], gender=[], rank=[], min_age='16 form_data['gender'] = request.args.getlist('gender') unit_choices = [(unit.id, unit.descrip) for unit in Unit.query.filter_by(department_id=department_id).order_by(Unit.descrip.asc()).all()] - rank_choices = [jc[0] for jc in db.session.query(Job.job_title, Job.order).filter_by(department_id=department_id, is_sworn_officer=True).order_by(Job.order).all()] + rank_choices = [jc[0] for jc in db.session.query(Job.job_title, Job.order).filter_by(department_id=department_id).order_by(Job.order).all()] if request.args.get('rank') and all(rank in rank_choices for rank in request.args.getlist('rank')): form_data['rank'] = request.args.getlist('rank') - officers = filter_by_form(form_data, Officer.query, department_id).filter(Officer.department_id == department_id).order_by(Officer.last_name, Officer.first_name, Officer.id).paginate(page, OFFICERS_PER_PAGE, False) + officers = filter_by_form( + form_data, Officer.query, department_id + ).filter(Officer.department_id == department_id) + officers = officers.options(selectinload(Officer.face)) + officers = officers.order_by(Officer.last_name, Officer.first_name, Officer.id) + officers = officers.paginate(page, OFFICERS_PER_PAGE, False) for officer in officers.items: - officer_face = officer.face.order_by(Face.featured.desc()).first() - if officer_face: - officer.image = officer_face.image.filepath + officer_face = sorted(officer.face, key=lambda x: x.featured) + + # could do some extra work to not lazy load images but load them all together + # but we would want to ensure to only load the first picture of each officer + if officer_face and officer_face[0].image: + officer.image = officer_face[0].image.filepath choices = { 'race': RACE_CHOICES, diff --git a/OpenOversight/app/models.py b/OpenOversight/app/models.py index 67ca37c18..3b1731530 100644 --- a/OpenOversight/app/models.py +++ b/OpenOversight/app/models.py @@ -1,4 +1,5 @@ import re +from datetime import date from flask_sqlalchemy import SQLAlchemy from flask_sqlalchemy.model import DefaultMeta @@ -103,15 +104,14 @@ class Officer(BaseModel): birth_year = db.Column(db.Integer, index=True, unique=False, nullable=True) assignments = db.relationship('Assignment', backref='officer', lazy='dynamic') assignments_lazy = db.relationship('Assignment') - face = db.relationship('Face', backref='officer', lazy='dynamic') + face = db.relationship('Face', backref='officer') department_id = db.Column(db.Integer, db.ForeignKey('departments.id')) department = db.relationship('Department', backref='officers') unique_internal_identifier = db.Column(db.String(50), index=True, unique=True, nullable=True) - # we don't expect to pull up officers via link often so we make it lazy. + links = db.relationship( 'Link', secondary=officer_links, - lazy='subquery', backref=db.backref('officers', lazy=True)) notes = db.relationship('Note', back_populates='officer', order_by='Note.date_created') descriptions = db.relationship('Description', back_populates='officer', order_by='Description.date_created') @@ -147,18 +147,12 @@ def gender_label(self): return label def job_title(self): - if self.assignments.all(): - return self.assignments\ - .order_by(self.assignments[0].__table__.c.star_date.desc())\ - .first()\ - .job.job_title + if self.assignments_lazy: + return max(self.assignments_lazy, key=lambda x: x.star_date or date.min).job.job_title def badge_number(self): - if self.assignments.all(): - return self.assignments\ - .order_by(self.assignments[0].__table__.c.star_date.desc())\ - .first()\ - .star_no + if self.assignments_lazy: + return max(self.assignments_lazy, key=lambda x: x.star_date or date.min).star_no def __repr__(self): if self.unique_internal_identifier: diff --git a/OpenOversight/app/templates/list_officer.html b/OpenOversight/app/templates/list_officer.html index cbcbf039b..5d11e1a28 100644 --- a/OpenOversight/app/templates/list_officer.html +++ b/OpenOversight/app/templates/list_officer.html @@ -163,7 +163,7 @@

Gender
{{ officer.gender_label()|default('Unknown') }}
Number of Photos
-
{{ officer.face.count() }}
+
{{ officer.face | count }}
diff --git a/OpenOversight/app/templates/tagger_gallery.html b/OpenOversight/app/templates/tagger_gallery.html index c98b2475d..76136b522 100644 --- a/OpenOversight/app/templates/tagger_gallery.html +++ b/OpenOversight/app/templates/tagger_gallery.html @@ -23,10 +23,10 @@

Matching Officers

{% for officer in officers.items %} - {% if officer.face.first() is none %} + {% if officer.face | count == 0 %} {% set officer_image = '/static/images/placeholder.png' %} {% else %} - {% set officer_image = officer.face.first().image.filepath %} + {% set officer_image = officer.face[0].image.filepath %} {% endif %} {% if loop.index == 1 %} diff --git a/OpenOversight/app/utils.py b/OpenOversight/app/utils.py index 524cb99b1..20ec709f8 100644 --- a/OpenOversight/app/utils.py +++ b/OpenOversight/app/utils.py @@ -18,6 +18,7 @@ from sqlalchemy import func, or_ from sqlalchemy.sql.expression import cast +from sqlalchemy.orm import selectinload import imghdr as imghdr from flask import current_app, url_for from flask_login import current_user @@ -251,66 +252,65 @@ def upload_obj_to_s3(file_obj, dest_filename): return url -def filter_by_form(form, officer_query, department_id=None): - # Some SQL acrobatics to left join only the most recent assignment per officer - row_num_col = func.row_number().over( - partition_by=Assignment.officer_id, order_by=Assignment.star_date.desc() - ).label('row_num') - subq = db.session.query( - Assignment.officer_id, - Assignment.job_id, - Assignment.star_date, - Assignment.star_no, - Assignment.unit_id - ).add_columns(row_num_col).from_self().filter(row_num_col == 1).subquery() - officer_query = officer_query.outerjoin(subq) - - if form.get('name'): +def filter_by_form(form_data, officer_query, department_id=None): + if form_data.get('name'): officer_query = officer_query.filter( - Officer.last_name.ilike('%%{}%%'.format(form['name'])) + Officer.last_name.ilike('%%{}%%'.format(form_data['name'])) ) - if not department_id and form.get('dept'): - department_id = form['dept'].id + if not department_id and form_data.get('dept'): + department_id = form_data['dept'].id officer_query = officer_query.filter( Officer.department_id == department_id ) - if form.get('badge'): - officer_query = officer_query.filter( - subq.c.assignments_star_no.like('%%{}%%'.format(form['badge'])) - ) - if form.get('unit'): - officer_query = officer_query.filter( - subq.c.assignments_unit_id == form['unit'] - ) - if form.get('unique_internal_identifier'): + if form_data.get('unique_internal_identifier'): officer_query = officer_query.filter( - Officer.unique_internal_identifier.ilike('%%{}%%'.format(form['unique_internal_identifier'])) + Officer.unique_internal_identifier.ilike('%%{}%%'.format(form_data['unique_internal_identifier'])) ) + race_values = [x for x, _ in RACE_CHOICES] - if form.get('race') and all(race in race_values for race in form['race']): - if 'Not Sure' in form['race']: - form['race'].append(None) - officer_query = officer_query.filter(Officer.race.in_(form['race'])) + if form_data.get('race') and all(race in race_values for race in form_data['race']): + if 'Not Sure' in form_data['race']: + form_data['race'].append(None) + officer_query = officer_query.filter(Officer.race.in_(form_data['race'])) + gender_values = [x for x, _ in GENDER_CHOICES] - if form.get('gender') and all(gender in gender_values for gender in form['gender']): - if 'Not Sure' not in form['gender']: - officer_query = officer_query.filter(or_(Officer.gender.in_(form['gender']), Officer.gender.is_(None))) + if form_data.get('gender') and all(gender in gender_values for gender in form_data['gender']): + if 'Not Sure' not in form_data['gender']: + officer_query = officer_query.filter(or_(Officer.gender.in_(form_data['gender']), Officer.gender.is_(None))) - if form.get('min_age') and form.get('max_age'): + if form_data.get('min_age') and form_data.get('max_age'): current_year = datetime.datetime.now().year - min_birth_year = current_year - int(form['min_age']) - max_birth_year = current_year - int(form['max_age']) + min_birth_year = current_year - int(form_data['min_age']) + max_birth_year = current_year - int(form_data['max_age']) officer_query = officer_query.filter(db.or_(db.and_(Officer.birth_year <= min_birth_year, Officer.birth_year >= max_birth_year), Officer.birth_year == None)) # noqa - officer_query = officer_query.outerjoin(Job, Assignment.job) - rank_values = [x[0] for x in db.session.query(Job.job_title).filter_by(department_id=department_id, is_sworn_officer=True).all()] - if form.get('rank') and all(rank in rank_values for rank in form['rank']): - if 'Not Sure' in form['rank']: - form['rank'].append(None) - officer_query = officer_query.filter(Job.job_title.in_(form['rank'])) + job_ids = [] + if form_data.get('rank'): + job_ids = [job.id for job in Job.query.filter_by(department_id=department_id).filter(Job.job_title.in_(form_data.get("rank"))).all()] + + print(form_data) + if 'Not Sure' in form_data['rank']: + form_data['rank'].append(None) + + if form_data.get('badge') or form_data.get('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 form_data.get('unit'): + officer_query = officer_query.filter( + Assignment.unit_id == form_data['unit'] + ) + if job_ids: + officer_query = officer_query.filter(Assignment.job_id.in_(job_ids)) + officer_query = ( + officer_query + .options(selectinload(Officer.assignments_lazy)) + ) return officer_query diff --git a/OpenOversight/tests/routes/test_officer_and_department.py b/OpenOversight/tests/routes/test_officer_and_department.py index 714ef00c4..c9bf05a46 100644 --- a/OpenOversight/tests/routes/test_officer_and_department.py +++ b/OpenOversight/tests/routes/test_officer_and_department.py @@ -1527,7 +1527,7 @@ def test_admin_can_upload_photos_of_dept_officers(mockdata, client, session, tes department = Department.query.filter_by(id=AC_DEPT).first() officer = department.officers[3] - officer_face_count = officer.face.count() + officer_face_count = len(officer.face) crop_mock = MagicMock(return_value=Image.query.first()) upload_mock = MagicMock(return_value=Image.query.first()) @@ -1541,7 +1541,7 @@ def test_admin_can_upload_photos_of_dept_officers(mockdata, client, session, tes assert rv.status_code == 200 assert b'Success' in rv.data # check that Face was added to database - assert officer.face.count() == officer_face_count + 1 + assert len(officer.face) == officer_face_count + 1 def test_upload_photo_sends_500_on_s3_error(mockdata, client, session, test_png_BytesIO): @@ -1553,7 +1553,7 @@ def test_upload_photo_sends_500_on_s3_error(mockdata, client, session, test_png_ department = Department.query.filter_by(id=AC_DEPT).first() mock = MagicMock(return_value=None) officer = department.officers[0] - officer_face_count = officer.face.count() + officer_face_count = len(officer.face) with patch('OpenOversight.app.main.views.upload_image_to_s3_and_store_in_db', mock): rv = client.post( url_for('main.upload', department_id=department.id, officer_id=officer.id), @@ -1563,7 +1563,7 @@ def test_upload_photo_sends_500_on_s3_error(mockdata, client, session, test_png_ assert rv.status_code == 500 assert b'error' in rv.data # check that Face was not added to database - assert officer.face.count() == officer_face_count + assert len(officer.face) == officer_face_count def test_upload_photo_sends_415_for_bad_file_type(mockdata, client, session): @@ -1604,7 +1604,7 @@ def test_ac_can_upload_photos_of_dept_officers(mockdata, client, session, test_p data = dict(file=(test_png_BytesIO, '204Cat.png'),) department = Department.query.filter_by(id=AC_DEPT).first() officer = department.officers[4] - officer_face_count = officer.face.count() + officer_face_count = len(officer.face) crop_mock = MagicMock(return_value=Image.query.first()) upload_mock = MagicMock(return_value=Image.query.first()) @@ -1618,7 +1618,7 @@ def test_ac_can_upload_photos_of_dept_officers(mockdata, client, session, test_p assert rv.status_code == 200 assert b'Success' in rv.data # check that Face was added to database - assert officer.face.count() == officer_face_count + 1 + assert len(officer.face) == officer_face_count + 1 def test_edit_officers_with_blank_uids(mockdata, client, session): diff --git a/OpenOversight/tests/test_utils.py b/OpenOversight/tests/test_utils.py index 3d7d4450d..a7d8dfabe 100644 --- a/OpenOversight/tests/test_utils.py +++ b/OpenOversight/tests/test_utils.py @@ -110,7 +110,7 @@ def test_filter_by_full_unique_internal_identifier_returns_officers(mockdata): department = OpenOversight.app.models.Department.query.first() target_unique_internal_id = OpenOversight.app.models.Officer.query.first().unique_internal_identifier results = OpenOversight.app.utils.grab_officers( - {'race': 'Not Sure', 'gender': 'Not Sure', 'rank': 'Not Sure', + {'race': ['Not Sure'], 'gender': ['Not Sure'], 'rank': ['Not Sure'], 'min_age': 16, 'max_age': 85, 'name': '', 'badge': '', 'dept': department, 'unique_internal_identifier': target_unique_internal_id} ) @@ -124,7 +124,7 @@ def test_filter_by_partial_unique_internal_identifier_returns_officers(mockdata) identifier = OpenOversight.app.models.Officer.query.first().unique_internal_identifier partial_identifier = identifier[:len(identifier) // 2] results = OpenOversight.app.utils.grab_officers( - {'race': 'Not Sure', 'gender': 'Not Sure', 'rank': 'Not Sure', + {'race': ['Not Sure'], 'gender': ['Not Sure'], 'rank': ['Not Sure'], 'min_age': 16, 'max_age': 85, 'name': '', 'badge': '', 'dept': department, 'unique_internal_identifier': partial_identifier} )