From 6f03a7849e44e3a468c18bd08cb0811b9d246f77 Mon Sep 17 00:00:00 2001 From: Michael Plunkett <5885605+michplunkett@users.noreply.github.com> Date: Wed, 12 Jul 2023 11:59:37 -0500 Subject: [PATCH] Add filter by photo ability in officer query (#965) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Fixes issue https://github.com/lucyparsons/OpenOversight/issues/798 ## Description of Changes Add the ability for the user to filter officers by the availability of a photo, format the `list_officer.html` file using [`djlint`](https://www.djlint.com/), correct carrot behavior on officer search, center displayed officer photos, and update relevant test case. ## Screenshots (if appropriate) Images on the search page are now centered (laptop screen): Screenshot 2023-07-11 at 12 18 34 PM Images on the search page are now centered (phone screen): Screenshot 2023-07-11 at 12 18 45 PM The carrots 🥕 now respond correctly when their respective menus are opened: URL: `http://localhost:3000/department/1?last_name=&first_name=&badge=&min_age=16&max_age=85&unique_internal_identifier=&require_photo=True&submit=Submit` Screenshot 2023-07-11 at 2 21 50 PM ## Tests and linting - [x] This branch is up-to-date with the `develop` branch. - [x] `pytest` passes on my local development environment. - [x] `pre-commit` passes on my local development environment. --- OpenOversight/app/main/forms.py | 12 +- OpenOversight/app/main/views.py | 77 ++- .../app/static/css/openoversight.css | 11 +- OpenOversight/app/templates/list_officer.html | 472 +++++++++++------- OpenOversight/app/utils/forms.py | 3 +- .../routes/test_officer_and_department.py | 27 +- 6 files changed, 386 insertions(+), 216 deletions(-) diff --git a/OpenOversight/app/main/forms.py b/OpenOversight/app/main/forms.py index d7e497793..19a2ff205 100644 --- a/OpenOversight/app/main/forms.py +++ b/OpenOversight/app/main/forms.py @@ -125,6 +125,9 @@ class FindOfficerForm(Form): max_age = IntegerField( "max_age", default=85, validators=[NumberRange(min=16, max=100)] ) + require_photo = BooleanField( + "require_photo", default=False, validators=[Optional()] + ) class FaceTag(Form): @@ -547,18 +550,20 @@ class IncidentForm(DateFieldForm): class BrowseForm(Form): # Any fields added to this form should generally also be added to FindOfficerForm + # query set in view function rank = QuerySelectField( "rank", validators=[Optional()], get_label="job_title", get_pk=lambda job: job.job_title, - ) # query set in view function + ) + # query set in view function unit = QuerySelectField( "unit", validators=[Optional()], get_label="descrip", get_pk=lambda unit: unit.descrip, - ) # query set in view function + ) current_job = BooleanField("current_job", default=None, validators=[Optional()]) name = StringField("Last name") badge = StringField("Badge number") @@ -587,6 +592,9 @@ class BrowseForm(Form): choices=AGE_CHOICES, validators=[AnyOf(allowed_values(AGE_CHOICES))], ) + require_photo = BooleanField( + "require_photo", default=False, validators=[Optional()] + ) submit = SubmitField(label="Submit") diff --git a/OpenOversight/app/main/views.py b/OpenOversight/app/main/views.py index ee6b8c30a..898e44713 100644 --- a/OpenOversight/app/main/views.py +++ b/OpenOversight/app/main/views.py @@ -147,7 +147,7 @@ def get_officer(): jsloads = ["js/find_officer.js"] form = FindOfficerForm() - depts_dict = [dept_choice.toCustomDict() for dept_choice in dept_choices()] + departments_dict = [dept_choice.toCustomDict() for dept_choice in dept_choices()] if getattr(current_user, "dept_pref_rel", None): set_dynamic_default(form.dept, current_user.dept_pref_rel) @@ -177,7 +177,10 @@ def get_officer(): else: current_app.logger.info(form.errors) return render_template( - "input_find_officer.html", form=form, depts_dict=depts_dict, jsloads=jsloads + "input_find_officer.html", + form=form, + depts_dict=departments_dict, + jsloads=jsloads, ) @@ -246,7 +249,7 @@ def officer_profile(officer_id): except NoResultFound: abort(HTTPStatus.NOT_FOUND) except: # noqa: E722 - exception_type, value, full_tback = sys.exc_info() + exception_type, value, full_traceback = sys.exc_info() current_app.logger.error( "Error finding officer: {}".format( " ".join([str(exception_type), str(value), format_exc()]) @@ -272,7 +275,7 @@ def officer_profile(officer_id): # Add in the placeholder image if no faces are found face_paths = [url_for("static", filename="images/placeholder.png")] except: # noqa: E722 - exception_type, value, full_tback = sys.exc_info() + exception_type, value, full_traceback = sys.exc_info() current_app.logger.error( "Error loading officer profile: {}".format( " ".join([str(exception_type), str(value), format_exc()]) @@ -491,7 +494,7 @@ def classify_submission(image_id, contains_cops): flash("Updated image classification") except: # noqa: E722 flash("Unknown error occurred") - exception_type, value, full_tback = sys.exc_info() + exception_type, value, full_traceback = sys.exc_info() current_app.logger.error( "Error classifying image: {}".format( " ".join([str(exception_type), str(value), format_exc()]) @@ -649,8 +652,9 @@ def list_officer( unique_internal_identifier=None, unit=None, current_job=None, + require_photo=False, ): - jsloads = ["js/select2.min.js", "js/list_officer.js"] + js_loads = ["js/select2.min.js", "js/list_officer.js"] form = BrowseForm() form.rank.query = ( @@ -670,6 +674,7 @@ def list_officer( form_data["unit"] = unit or [] form_data["current_job"] = current_job form_data["unique_internal_identifier"] = unique_internal_identifier + form_data["require_photo"] = require_photo department = Department.query.filter_by(id=department_id).first() if not department: @@ -702,15 +707,17 @@ def list_officer( for gender in genders ): form_data["gender"] = genders + if require_photo_arg := request.args.get("require_photo"): + form_data["require_photo"] = require_photo_arg - unit_choices = ["Not Sure"] + [ + unit_selections = ["Not Sure"] + [ uc[0] for uc in db.session.query(Unit.descrip) .filter_by(department_id=department_id) .order_by(Unit.descrip.asc()) .all() ] - rank_choices = [ + rank_selections = [ jc[0] for jc in db.session.query(Job.job_title, Job.order) .filter_by(department_id=department_id) @@ -718,11 +725,11 @@ def list_officer( .all() ] if (units := request.args.getlist("unit")) and all( - unit in unit_choices for unit in units + unit in unit_selections for unit in units ): form_data["unit"] = units if (ranks := request.args.getlist("rank")) and all( - rank in rank_choices for rank in ranks + rank in rank_selections for rank in ranks ): form_data["rank"] = ranks if current_job_arg := request.args.get("current_job"): @@ -731,24 +738,33 @@ def list_officer( officers = filter_by_form(form_data, Officer.query, department_id).filter( Officer.department_id == department_id ) - officers = officers.options(selectinload(Officer.face)) + + # Filter officers by presence of a photo + if form_data["require_photo"]: + officers = officers.join(Face) + else: + officers = officers.options(selectinload(Officer.face)) + officers = officers.order_by(Officer.last_name, Officer.first_name, Officer.id) + officers = officers.paginate( page=page, per_page=current_app.config["OFFICERS_PER_PAGE"], error_out=False ) + for officer in officers.items: officer_face = sorted(officer.face, key=lambda x: x.featured, reverse=True) - # 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 + # Could do some extra work to not lazy load images but load them all together. + # To do that properly 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, "gender": GENDER_CHOICES, - "rank": [(rc, rc) for rc in rank_choices], - "unit": [(uc, uc) for uc in unit_choices], + "rank": [(rc, rc) for rc in rank_selections], + "unit": [(uc, uc) for uc in unit_selections], } next_url = url_for( @@ -766,6 +782,7 @@ def list_officer( unique_internal_identifier=form_data["unique_internal_identifier"], unit=form_data["unit"], current_job=form_data["current_job"], + require_photo=form_data["require_photo"], ) prev_url = url_for( "main.list_officer", @@ -782,6 +799,7 @@ def list_officer( unique_internal_identifier=form_data["unique_internal_identifier"], unit=form_data["unit"], current_job=form_data["current_job"], + require_photo=form_data["require_photo"], ) return render_template( @@ -793,7 +811,7 @@ def list_officer( choices=choices, next_url=next_url, prev_url=prev_url, - jsloads=jsloads, + jsloads=js_loads, ) @@ -812,7 +830,8 @@ def get_dept_ranks(department_id=None, is_sworn_officer=None): ranks = ranks.order_by(Job.job_title).all() rank_list = [(rank.id, rank.job_title) for rank in ranks] else: - ranks = Job.query.all() # Not filtering by is_sworn_officer + # Not filtering by is_sworn_officer + ranks = Job.query.all() # Prevent duplicate ranks rank_list = sorted( set((rank.id, rank.job_title) for rank in ranks), @@ -863,11 +882,11 @@ def add_officer(): abort(HTTPStatus.FORBIDDEN) if form.validate_on_submit(): # Work around for WTForms limitation with boolean fields in FieldList - new_formdata = request.form.copy() - for key in new_formdata.keys(): + new_form_data = request.form.copy() + for key in new_form_data.keys(): if re.fullmatch(r"salaries-\d+-is_fiscal_year", key): - new_formdata[key] = "y" - form = AddOfficerForm(new_formdata) + new_form_data[key] = "y" + form = AddOfficerForm(new_form_data) officer = add_officer_profile(form, current_user) flash("New Officer {} added to OpenOversight".format(officer.last_name)) return redirect(url_for("main.submit_officer_images", officer_id=officer.id)) @@ -1025,7 +1044,8 @@ def label_data(department_id=None, image_id=None): department = None if image_id: image = Image.query.filter_by(id=image_id).one() - else: # Select a random untagged image from the entire database + else: + # Select a random untagged image from the entire database image_query = Image.query.filter_by(contains_cops=True).filter_by( is_tagged=False ) @@ -1052,9 +1072,8 @@ def label_data(department_id=None, image_id=None): flash("Invalid officer ID. Please select a valid OpenOversight ID!") elif department and officer_exists.department_id != department_id: flash( - "The officer is not in {}. Are you sure that is the correct OpenOversight ID?".format( - department.name - ) + "The officer is not in {}. Are you sure that is the correct " + "OpenOversight ID?".format(department.name) ) elif not existing_tag: left = form.dataX.data @@ -1389,7 +1408,8 @@ def upload(department_id, officer_id=None): image.contains_cops = True face = Face( officer_id=officer_id, - # Assuming photos uploaded with an officer ID are already cropped, so we set both images to the uploaded one + # Assuming photos uploaded with an officer ID are already cropped, + # we set both images to the uploaded one img_id=image.id, original_image_id=image.id, user_id=current_user.get_id(), @@ -1712,7 +1732,10 @@ def dispatch_request(self, *args, **kwargs): class OfficerLinkApi(ModelView): - """This API only applies to links attached to officer profiles, not links attached to incidents""" + """ + This API only applies to links attached to officer profiles, not links attached to + incidents. + """ model = Link model_name = "link" diff --git a/OpenOversight/app/static/css/openoversight.css b/OpenOversight/app/static/css/openoversight.css index 66582f205..91fd01e0d 100644 --- a/OpenOversight/app/static/css/openoversight.css +++ b/OpenOversight/app/static/css/openoversight.css @@ -119,6 +119,7 @@ a > .tutorial{ min-height: 700px; margin-bottom: 50px; } + /* Since positioning the image, we need to help out the caption */ .carousel-caption { display:block; @@ -500,6 +501,12 @@ tr:hover .row-actions { content: "\e253"; } +@media all and (min-width: 768px) and (max-width: 991px) { + .filter-sidebar > .form > .btn.btn-primary:first-of-type { + margin-bottom: 5px; + } +} + .search-results .list-group-item { border: 0; } @@ -578,8 +585,8 @@ tr:hover .row-actions { } .officer-face { - min-width: 200px; - min-height: 200px; + height: 300px; + margin: auto; } .face-wrap { diff --git a/OpenOversight/app/templates/list_officer.html b/OpenOversight/app/templates/list_officer.html index 9bdaad382..7b01a4dbb 100644 --- a/OpenOversight/app/templates/list_officer.html +++ b/OpenOversight/app/templates/list_officer.html @@ -1,211 +1,331 @@ {% extends "base.html" %} {% import "bootstrap/wtf.html" as wtf %} -{% block title %}Browse {{ department.name|title }} officers - OpenOversight{% endblock %} -{% block meta %}{% endblock %} -{% block head %}{% endblock %} +{% block title %} + Browse {{ department.name | title }} officers - OpenOversight +{% endblock title %} +{% block meta %} + +{% endblock meta %} +{% block head %} + +{% endblock head %} {% block content %} -
-

{{ department.name|title }} Officers

-
-
-

Filter officers

-
-
-
-

Name

-
-
-
-
-
- - +
+

{{ department.name | title }} Officers

+
+
+

Filter officers

+ +
+
+

Name

+
+
+
+
+
+ + +
-
- -
- -
-
- - +
+
+
+ + +
-
-
-
-

Badge

-
-
-
-
- +
+
+

Badge

+
+
+
+
+ +
-
-
-
-

Race

-
-
-
-
- {% for choice in choices['race'] %} - - {% endfor %} +
+
+

Race

+
+
+
+
+ {% for choice in choices['race'] %} + + {% endfor %} +
-
-
-
-

Gender

-
-
-
-
- {% for choice in choices['gender'] %} - - {% endfor %} +
+
+

Gender

+
+
+
+
+ {% for choice in choices['gender'] %} + + {% endfor %} +
-
-
-
-

Job

-
-
-
-
-
- - (in this rank and/or unit, if specified) +
+
+

Job

+
+
+
+
+
+ + (in this rank and/or unit, if specified) +
-
-
- -
-
- - +
+
+
+ + +
-
-
- -
-
- - +
+
+
+ + +
-
-
-
-

Age range

-
-
-
-
-
- - -
-
- - +
+
+

Age range

+
+
+
+
+
+ + +
+
+ + +
-
-
-
-

Unique ID

-
-
-
-
- +
+
+

Unique ID

+
+
+
+
+ +
-
- {{ wtf.form_field(form.submit, id="submit", button_map={'submit':'primary'}) }} - Clear Filters - -
-
- {% with paginate=officers, location='top' %} - {% include "partials/paginate_nav.html" %} - {% endwith %} -
    - {% for officer in officers.items %} -
  • -
    -
    - - {{ officer.full_name() }} - -
    -
    -

    - {{ officer.full_name() }} - {{ officer.badge_number()|default('') }} -

    -
    -
    -
    -
    Rank
    -
    {{ officer.job_title()|default('Unknown') }}
    -
    Unit
    -
    {{ officer.unit_descrip()|default('Unknown') }}
    -
    Currently on the Force
    -
    {{ officer.currently_on_force() }}
    -
    Known incidents
    -
    {{ officer.incidents | length }}
    -
    -
    -
    -
    -
    Race
    -
    {{ officer.race_label()|default('Unknown')|lower|title }}
    -
    Gender
    -
    {{ officer.gender_label()|default('Unknown') }}
    -
    Number of Photos
    -
    {{ officer.face | count }}
    -
    +
    +
    +

    Photo

    +
    +
    +
    +
    +
    +
    -
    • - {% endfor %} -
- {% with paginate=officers, location='bottom' %} - {% include "partials/paginate_nav.html" %} - {% endwith %} -
-
-
+
+ {{ wtf.form_field(form.submit, id="submit", button_map={'submit':'primary'}) }} + Clear Filters + +
+
+ {% with paginate=officers, location="top" %} + {% include "partials/paginate_nav.html" %} + {% endwith %} +
    + {% for officer in officers.items %} +
  • +
    +
    + + {{ officer.full_name() }} + +
    +
    +

    + {{ officer.full_name() }} + {{ officer.badge_number() | default('') }} +

    +
    +
    +
    +
    Rank
    +
    + {{ officer.job_title() | default('Unknown') }} +
    +
    Unit
    +
    + {{ officer.unit_descrip() | default('Unknown') }} +
    +
    Currently on the Force
    +
    + {{ officer.currently_on_force() }} +
    +
    Known incidents
    +
    + {{ officer.incidents | length }} +
    +
    +
    +
    +
    +
    Race
    +
    + {{ officer.race_label() | default('Unknown') | lower | title }} +
    +
    Gender
    +
    + {{ officer.gender_label() | default('Unknown') }} +
    +
    Number of Photos
    +
    + {{ officer.face | count }} +
    +
    +
    +
    +
    +
    +
    • + {% endfor %} +
+ {% with paginate=officers, location="bottom" %} + {% include "partials/paginate_nav.html" %} + {% endwith %} +
+ +
+ +
+ {% endblock content %} diff --git a/OpenOversight/app/utils/forms.py b/OpenOversight/app/utils/forms.py index 5c917f13b..1eeba9ed8 100644 --- a/OpenOversight/app/utils/forms.py +++ b/OpenOversight/app/utils/forms.py @@ -345,12 +345,11 @@ def filter_roster(form, officer_query): if "dept" in form and form["dept"]: officer_query = officer_query.filter(Officer.department_id == form["dept"].id) - officer_query = ( + return ( officer_query.outerjoin(Face) .order_by(Face.officer_id.asc()) .order_by(Officer.id.desc()) ) - return officer_query def grab_officers(form): diff --git a/OpenOversight/tests/routes/test_officer_and_department.py b/OpenOversight/tests/routes/test_officer_and_department.py index bf7d93452..31dac39ed 100644 --- a/OpenOversight/tests/routes/test_officer_and_department.py +++ b/OpenOversight/tests/routes/test_officer_and_department.py @@ -3,6 +3,7 @@ import csv import json import random +import re from datetime import date, datetime from http import HTTPStatus from io import BytesIO @@ -1620,7 +1621,7 @@ def test_browse_filtering_allows_good(client, mockdata, session): with current_app.test_request_context(): department_id = Department.query.first().id - # Add a officer with a specific race, gender, rank and age to the first page + # Add an officer with a specific race, gender, rank and age to the first page login_admin(client) links = [ LinkForm(url="http://www.pleasework.com", link_type="link").data, @@ -1655,7 +1656,8 @@ def test_browse_filtering_allows_good(client, mockdata, session): assert officer.race == "WHITE" assert officer.gender == "M" - # Check that added officer appears when filtering for this race, gender, rank and age + # Check that added officer appears when filtering for this race, gender, rank + # and age form = BrowseForm( race="WHITE", gender="M", @@ -1673,18 +1675,28 @@ def test_browse_filtering_allows_good(client, mockdata, session): follow_redirects=True, ) - filter_list = rv.data.decode(ENCODING_UTF_8).split("
Rank
")[1:] + def normalize_tokens_for_comparison(html_str: str, split_str: str): + """Remove new lines, leading, and closing spaces between
elements in + formatted HTML.""" + parsed_list = html_str.data.decode(ENCODING_UTF_8).split(split_str)[1:] + parsed_list = [re.sub(r"
\n\s+", "
", token) for token in parsed_list] + parsed_list = [ + re.sub(r"\n\s+
", "", token) for token in parsed_list + ] + return parsed_list + + filter_list = normalize_tokens_for_comparison(rv, "
Rank
") assert any( "
{}
".format(job.job_title) in token for token in filter_list ) - filter_list = rv.data.decode(ENCODING_UTF_8).split("
Unit
")[1:] + filter_list = normalize_tokens_for_comparison(rv, "
Unit
") assert any("
{}
".format(unit.descrip) in token for token in filter_list) - filter_list = rv.data.decode(ENCODING_UTF_8).split("
Race
")[1:] + filter_list = normalize_tokens_for_comparison(rv, "
Race
") assert any("
White
" in token for token in filter_list) - filter_list = rv.data.decode(ENCODING_UTF_8).split("
Gender
")[1:] + filter_list = normalize_tokens_for_comparison(rv, "
Gender
") assert any("
Male
" in token for token in filter_list) @@ -1696,7 +1708,8 @@ def test_find_officer_redirect(client, mockdata, session): min_age = datetime.now().year - 1991 max_age = datetime.now().year - 1989 - # Check that added officer appears when filtering for this race, gender, rank and age + # Check that added officer appears when filtering for this race, gender, rank + # and age form = FindOfficerForm( dept=department_id, first_name="A",