Make url_for work with all endpoints (#1070)

## Description of Changes
Some of the `ModelView` based endpoints were missing a distinct name
that could be used for `url_for`, and instead string concatenation was
used (mostly in templates). I also moved some logic out of templates
(e.g. calculation of total pay), added helper methods and made some
other changed to reduce the code needed in the templates and improve
readability a little bit.

I also added an anonymous user class that is useful to call things like
`.is_admin_or_coordinator(department)` on any `current_user` object,
without first making sure the user is not anonymous.

## 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.

Author: abandoned-prototype

OpenOversight/app/__init__.py

@@ -17,6 +17,7 @@
 from OpenOversight.app.filters import instantiate_filters
 from OpenOversight.app.models.config import config
 from OpenOversight.app.models.database import db
+from OpenOversight.app.models.users import AnonymousUser
 from OpenOversight.app.utils.constants import MEGABYTE
 
 
@@ -25,6 +26,7 @@
 
 login_manager = LoginManager()
 login_manager.session_protection = "strong"
+login_manager.anonymous_user = AnonymousUser
 login_manager.login_view = "auth.login"
 
 limiter = Limiter(

OpenOversight/app/filters.py

@@ -92,6 +92,10 @@ def thousands_separator(value: int) -> str:
     return f"{value:,}"
 
 
+def display_currency(value: float) -> str:
+    return f"${value:,.2f}"
+
+
 def instantiate_filters(app: Flask):
     """Instantiate all template filters"""
     app.template_filter("capfirst")(capfirst_filter)
@@ -104,3 +108,4 @@ def instantiate_filters(app: Flask):
     app.template_filter("display_time")(display_time)
     app.template_filter("local_time")(local_time)
     app.template_filter("thousands_separator")(thousands_separator)
+    app.template_filter("display_currency")(display_currency)

OpenOversight/app/main/views.py

@@ -332,12 +332,10 @@ def officer_profile(officer_id: int):
             .all()
         )
         assignments = Assignment.query.filter_by(officer_id=officer_id).all()
-        face_paths = []
-        for face in faces:
-            face_paths.append(serve_image(face.image.filepath))
+        face_paths = [(face, serve_image(face.image.filepath)) for face in faces]
         if not face_paths:
             # Add in the placeholder image if no faces are found
-            face_paths = [url_for("static", filename="images/placeholder.png")]
+            face_paths = [(None, url_for("static", filename="images/placeholder.png"))]
     except:  # noqa: E722
         exception_type, value, full_traceback = sys.exc_info()
         error_str = " ".join([str(exception_type), str(value), format_exc()])
@@ -355,8 +353,7 @@ def officer_profile(officer_id: int):
     return render_template(
         "officer.html",
         officer=officer,
-        paths=face_paths,
-        faces=faces,
+        face_paths=face_paths,
         assignments=assignments,
         form=form,
     )
@@ -2085,24 +2082,31 @@ def populate_obj(self, form: FlaskForm, obj: Incident):
 main.add_url_rule(
     "/incidents/",
     defaults={"obj_id": None},
+    endpoint="incident_api",
     view_func=incident_view,
     methods=[HTTPMethod.GET],
 )
 main.add_url_rule(
     "/incidents/new",
+    endpoint="incident_api_new",
     view_func=incident_view,
     methods=[HTTPMethod.GET, HTTPMethod.POST],
 )
 main.add_url_rule(
-    "/incidents/<int:obj_id>", view_func=incident_view, methods=[HTTPMethod.GET]
+    "/incidents/<int:obj_id>",
+    endpoint="incident_api",
+    view_func=incident_view,
+    methods=[HTTPMethod.GET],
 )
 main.add_url_rule(
     "/incidents/<int:obj_id>/edit",
+    endpoint="incident_api_edit",
     view_func=incident_view,
     methods=[HTTPMethod.GET, HTTPMethod.POST],
 )
 main.add_url_rule(
     "/incidents/<int:obj_id>/delete",
+    endpoint="incident_api_delete",
     view_func=incident_view,
     methods=[HTTPMethod.GET, HTTPMethod.POST],
 )
@@ -2189,7 +2193,7 @@ def redirect_get_notes(officer_id: int, obj_id=None):
 def redirect_edit_note(officer_id: int, obj_id=None):
     flash(FLASH_MSG_PERMANENT_REDIRECT)
     return redirect(
-        f"{url_for('main.note_api', officer_id=officer_id, obj_id=obj_id)}/edit",
+        url_for("main.note_api_edit", officer_id=officer_id, obj_id=obj_id),
         code=HTTPStatus.PERMANENT_REDIRECT,
     )
 
@@ -2199,14 +2203,15 @@ def redirect_edit_note(officer_id: int, obj_id=None):
 def redirect_delete_note(officer_id: int, obj_id=None):
     flash(FLASH_MSG_PERMANENT_REDIRECT)
     return redirect(
-        f"{url_for('main.note_api', officer_id=officer_id, obj_id=obj_id)}/delete",
+        url_for("main.note_api_delete", officer_id=officer_id, obj_id=obj_id),
         code=HTTPStatus.PERMANENT_REDIRECT,
     )
 
 
 note_view = NoteApi.as_view("note_api")
 main.add_url_rule(
     "/officers/<int:officer_id>/notes/new",
+    endpoint="note_api",
     view_func=note_view,
     methods=[HTTPMethod.GET, HTTPMethod.POST],
 )
@@ -2217,6 +2222,7 @@ def redirect_delete_note(officer_id: int, obj_id=None):
 )
 main.add_url_rule(
     "/officers/<int:officer_id>/notes/<int:obj_id>",
+    endpoint="note_api",
     view_func=note_view,
     methods=[HTTPMethod.GET],
 )
@@ -2227,6 +2233,7 @@ def redirect_delete_note(officer_id: int, obj_id=None):
 )
 main.add_url_rule(
     "/officers/<int:officer_id>/notes/<int:obj_id>/edit",
+    endpoint="note_api_edit",
     view_func=note_view,
     methods=[HTTPMethod.GET, HTTPMethod.POST],
 )
@@ -2237,6 +2244,7 @@ def redirect_delete_note(officer_id: int, obj_id=None):
 )
 main.add_url_rule(
     "/officers/<int:officer_id>/notes/<int:obj_id>/delete",
+    endpoint="note_api_delete",
     view_func=note_view,
     methods=[HTTPMethod.GET, HTTPMethod.POST],
 )
@@ -2252,7 +2260,7 @@ def redirect_delete_note(officer_id: int, obj_id=None):
 def redirect_new_description(officer_id: int):
     flash(FLASH_MSG_PERMANENT_REDIRECT)
     return redirect(
-        url_for("main.description_api", officer_id=officer_id),
+        url_for("main.description_api_new", officer_id=officer_id),
         code=HTTPStatus.PERMANENT_REDIRECT,
     )
 
@@ -2270,7 +2278,7 @@ def redirect_get_description(officer_id: int, obj_id=None):
 def redirect_edit_description(officer_id: int, obj_id=None):
     flash(FLASH_MSG_PERMANENT_REDIRECT)
     return redirect(
-        f"{url_for('main.description_api', officer_id=officer_id, obj_id=obj_id)}/edit",
+        url_for("main.description_api_edit", officer_id=officer_id, obj_id=obj_id),
         code=HTTPStatus.PERMANENT_REDIRECT,
     )
 
@@ -2280,14 +2288,15 @@ def redirect_edit_description(officer_id: int, obj_id=None):
 def redirect_delete_description(officer_id: int, obj_id=None):
     flash(FLASH_MSG_PERMANENT_REDIRECT)
     return redirect(
-        f"{url_for('main.description_api', officer_id=officer_id, obj_id=obj_id)}/delete",
+        url_for("main.description_api_delete", officer_id=officer_id, obj_id=obj_id),
         code=HTTPStatus.PERMANENT_REDIRECT,
     )
 
 
 description_view = DescriptionApi.as_view("description_api")
 main.add_url_rule(
     "/officers/<int:officer_id>/descriptions/new",
+    endpoint="description_api_new",
     view_func=description_view,
     methods=[HTTPMethod.GET, HTTPMethod.POST],
 )
@@ -2298,6 +2307,7 @@ def redirect_delete_description(officer_id: int, obj_id=None):
 )
 main.add_url_rule(
     "/officers/<int:officer_id>/descriptions/<int:obj_id>",
+    endpoint="description_api",
     view_func=description_view,
     methods=[HTTPMethod.GET],
 )
@@ -2308,6 +2318,7 @@ def redirect_delete_description(officer_id: int, obj_id=None):
 )
 main.add_url_rule(
     "/officers/<int:officer_id>/descriptions/<int:obj_id>/edit",
+    endpoint="description_api_edit",
     view_func=description_view,
     methods=[HTTPMethod.GET, HTTPMethod.POST],
 )
@@ -2318,6 +2329,7 @@ def redirect_delete_description(officer_id: int, obj_id=None):
 )
 main.add_url_rule(
     "/officers/<int:officer_id>/descriptions/<int:obj_id>/delete",
+    endpoint="description_api_delete",
     view_func=description_view,
     methods=[HTTPMethod.GET, HTTPMethod.POST],
 )

OpenOversight/app/models/database.py

@@ -1,8 +1,9 @@
+import operator
 import re
 import time
 import uuid
 from datetime import date, datetime
-from typing import List
+from typing import List, Optional
 
 from authlib.jose import JoseError, JsonWebToken
 from cachetools import cached
@@ -294,21 +295,27 @@ def gender_label(self):
     def job_title(self):
         if self.assignments:
             return max(
-                self.assignments, key=lambda x: x.start_date or date.min
+                self.assignments, key=operator.attrgetter("start_date_or_min")
             ).job.job_title
 
     def unit_description(self):
         if self.assignments:
-            unit = max(self.assignments, key=lambda x: x.start_date or date.min).unit
+            unit = max(
+                self.assignments, key=operator.attrgetter("start_date_or_min")
+            ).unit
             return unit.description if unit else None
 
     def badge_number(self):
         if self.assignments:
-            return max(self.assignments, key=lambda x: x.start_date or date.min).star_no
+            return max(
+                self.assignments, key=operator.attrgetter("start_date_or_min")
+            ).star_no
 
     def currently_on_force(self):
         if self.assignments:
-            most_recent = max(self.assignments, key=lambda x: x.start_date or date.min)
+            most_recent = max(
+                self.assignments, key=operator.attrgetter("start_date_or_min")
+            )
             return "Yes" if most_recent.resign_date is None else "No"
         return "Uncertain"
 
@@ -340,6 +347,16 @@ class Salary(BaseModel, TrackUpdates):
     def __repr__(self):
         return f"<Salary: ID {self.officer_id} : {self.salary}"
 
+    @property
+    def total_pay(self) -> float:
+        return self.salary + self.overtime_pay
+
+    @property
+    def year_repr(self) -> str:
+        if self.is_fiscal_year:
+            return f"FY{self.year}"
+        return str(self.year)
+
 
 class Assignment(BaseModel, TrackUpdates):
     __tablename__ = "assignments"
@@ -371,6 +388,14 @@ class Assignment(BaseModel, TrackUpdates):
     def __repr__(self):
         return f"<Assignment: ID {self.officer_id} : {self.star_no}>"
 
+    @property
+    def start_date_or_min(self):
+        return self.start_date or date.min
+
+    @property
+    def start_date_or_max(self):
+        return self.start_date or date.max
+
 
 class Unit(BaseModel, TrackUpdates):
     __tablename__ = "unit_types"
@@ -702,6 +727,12 @@ class User(UserMixin, BaseModel):
         unique=False,
     )
 
+    def is_admin_or_coordinator(self, department: Optional[Department]) -> bool:
+        return self.is_administrator or (
+            department is not None
+            and (self.is_area_coordinator and self.ac_department_id == department.id)
+        )
+
     def _jwt_encode(self, payload, expiration):
         secret = current_app.config["SECRET_KEY"]
         header = {"alg": SIGNATURE_ALGORITHM}

OpenOversight/app/models/users.py

@@ -0,0 +1,8 @@
+from flask_login import AnonymousUserMixin
+
+from OpenOversight.app.models.database import Department
+
+
+class AnonymousUser(AnonymousUserMixin):
+    def is_admin_or_coordinator(self, department: Department) -> bool:
+        return False

OpenOversight/app/templates/cop_face.html

@@ -9,7 +9,7 @@
 {% block content %}
   <div class="container theme-showcase" role="main">
     {% if current_user and current_user.is_authenticated %}
-      {% if image and current_user.is_disabled == False %}
+      {% if image and not current_user.is_disabled %}
         <div class="row">
           <div class="text-center">
             <h1>
@@ -138,18 +138,12 @@ <h2>
             </div>
           </div>
           <div class="col-sm-2 text-center skip-button">
-            {% if department %}
-              <a href="{{ url_for('main.label_data', department_id=department.id) }}"
-                 class="btn btn-lg btn-primary"
-                 role="button"><span class="glyphicon glyphicon-repeat" aria-hidden="true"></span> Next Photo</a>
-            {% else %}
-              <a href="{{ url_for("main.label_data") }}"
-                 class="btn btn-lg btn-primary"
-                 role="button"><span class="glyphicon glyphicon-repeat" aria-hidden="true"></span> Next Photo</a>
-            {% endif %}
+            <a href="{{ url_for('main.label_data', department_id=department.id if department else 0) }}"
+               class="btn btn-lg btn-primary"
+               role="button"><span class="glyphicon glyphicon-repeat" aria-hidden="true"></span> Next Photo</a>
           </div>
           <div class="col-sm-2 text-center done-button">
-            <a href="{{ url_for('main.complete_tagging', image_id=image.id, department_id=department.id, contains_cops=0) }}"
+            <a href="{{ url_for('main.complete_tagging', image_id=image.id, department_id=department.id if department else 0, contains_cops=0) }}"
                class="btn btn-sm btn-success">
               <span class="glyphicon glyphicon glyphicon-ok" aria-hidden="true"></span>
               All officers have been identified!
@@ -172,7 +166,7 @@ <h2>
             </div>
           </div>
         </div>
-      {% elif current_user.is_disabled == True %}
+      {% elif current_user.is_disabled %}
         <h3>Your account has been disabled due to too many incorrect classifications/tags!</h3>
         <p>
           <a href="mailto:info@lucyparsonslabs.com"

OpenOversight/app/templates/department_add_edit.html

@@ -35,7 +35,7 @@ <h5>Enter ranks in hierarchical order, from lowest to highest rank:</h5>
           </div>
           <div class="text-danger">{{ wtf.form_errors(form.jobs, hiddens="only") }}</div>
           {% if form.jobs|length > 1 %}
-            {% for subfield in (form.jobs|rejectattr('data.is_sworn_officer','eq',False)|sort(attribute='data.order')|list) %}
+            {% for subfield in (form.jobs|sort(attribute='data.order')|list) %}
               <fieldset>
                 <div class="input-group {% if subfield.errors %} has-error{% endif -%} {%- if subfield.flags.required %} required{% endif -%}">
                   <div class="input-group-addon">

OpenOversight/app/templates/description_delete.html

@@ -8,7 +8,7 @@ <h1>Delete Description of officer {{ obj.officer_id }}</h1>
     <p class="lead">
       Are you sure you want to delete this description?
       This cannot be undone.
-      <form action="{{ '{}/delete'.format(url_for('main.description_api', obj_id=obj.id, officer_id=obj.officer_id) ) }}"
+      <form action="{{ url_for('main.description_api_delete', obj_id=obj.id, officer_id=obj.officer_id) }}"
             method="post">
         <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
         <button class="btn btn-danger" type="submit">Delete</button>

OpenOversight/app/templates/description_edit.html

@@ -7,7 +7,7 @@
   {% if form.errors %}
     {% set post_url = url_for('main.description_api', officer_id=obj.officer_id, obj_id=obj.id) %}
   {% else %}
-    {% set post_url = "{}/edit".format(url_for('main.description_api', officer_id=obj.officer_id, obj_id=obj.id)) %}
+    {% set post_url = url_for('main.description_api_edit', officer_id=obj.officer_id, obj_id=obj.id) %}
   {% endif %}
   {{ wtf.quick_form(form, action=post_url, method='post', button_map={'submit':'primary'}) }}
   <br>

OpenOversight/app/templates/image.html

@@ -83,8 +83,7 @@ <h3>Classification</h3>
                 </tr>
               </tbody>
             </table>
-            {% if current_user.is_administrator
-              or (current_user.is_area_coordinator and current_user.ac_department_id == image.department.id) %}
+            {% if current_user.is_admin_or_coordinator(image.department) %}
               <h3>
                 Classify <small>Admin only</small>
               </h3>