simplify and streamline officer filtering (#858)

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'

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,

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:

OpenOversight/app/templates/list_officer.html

@@ -163,7 +163,7 @@ <h2>
                       <dt>Gender</dt>
                       <dd>{{ officer.gender_label()|default('Unknown') }}</dd>
                       <dt>Number of Photos</dt>
-                      <dd>{{ officer.face.count() }}</dd>
+                      <dd>{{ officer.face | count }}</dd>
                     </dl>
                   </div>
                 </div>

OpenOversight/app/templates/tagger_gallery.html

@@ -23,10 +23,10 @@ <h1 class="page-header">Matching Officers</h1>
         <div class="carousel-inner" role="listbox">
         {% 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 %}

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
 

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):

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}
     )