From f8b402806f44c3e74c163f9b5939ab532846e538 Mon Sep 17 00:00:00 2001 From: abandoned-prototype <41744410+abandoned-prototype@users.noreply.github.com> Date: Tue, 23 May 2023 00:02:00 -0500 Subject: [PATCH] Packages/test performance (#921) * Make sure adding mockdata is only run once. * Fix tests. --- OpenOversight/app/commands.py | 2 +- OpenOversight/app/config.py | 1 + OpenOversight/tests/conftest.py | 118 ++++++---- .../routes/test_officer_and_department.py | 215 ++++++++---------- OpenOversight/tests/test_commands.py | 177 +++++++------- OpenOversight/tests/test_functional.py | 2 +- 6 files changed, 262 insertions(+), 253 deletions(-) diff --git a/OpenOversight/app/commands.py b/OpenOversight/app/commands.py index d5a70aa5e..0f91d62a5 100644 --- a/OpenOversight/app/commands.py +++ b/OpenOversight/app/commands.py @@ -588,7 +588,7 @@ def advanced_csv_import( links_csv, incidents_csv, force_create, - overwrite_assignments + overwrite_assignments, ) diff --git a/OpenOversight/app/config.py b/OpenOversight/app/config.py index de99c3f39..d7850f5b1 100644 --- a/OpenOversight/app/config.py +++ b/OpenOversight/app/config.py @@ -69,6 +69,7 @@ class TestingConfig(BaseConfig): NUM_OFFICERS = 120 APPROVE_REGISTRATIONS = False SITEMAP_URL_SCHEME = "http" + RATELIMIT_ENABLED = False class ProductionConfig(BaseConfig): diff --git a/OpenOversight/tests/conftest.py b/OpenOversight/tests/conftest.py index 2cc1f1a8c..6ae9d04de 100644 --- a/OpenOversight/tests/conftest.py +++ b/OpenOversight/tests/conftest.py @@ -1,5 +1,6 @@ import csv import datetime +import math import os import random import sys @@ -51,6 +52,10 @@ "Chief", ] +DEPARTMENT_NAME = "Springfield Police Department" +OTHER_DEPARTMENT_NAME = "Chicago Police Department" +DEPARTMENT_WITHOUT_OFFICERS_NAME = "Empty Police Department" + AC_DEPT = 1 @@ -119,7 +124,7 @@ def pick_salary(): return Decimal(random.randint(100, 100000000)) / 100 -def generate_officer(): +def generate_officer(department): year_born = pick_birth_date() f_name, m_initial, l_name = pick_name() return models.Officer( @@ -130,7 +135,7 @@ def generate_officer(): gender=pick_gender(), birth_year=year_born, employment_date=datetime.datetime(year_born + 20, 4, 4, 1, 1, 1), - department_id=pick_department().id, + department_id=department.id, unique_internal_identifier=pick_uid(), ) @@ -215,6 +220,13 @@ def db(app): with app.app_context(): _db.app = app _db.create_all() + connection = _db.engine.connect() + session = scoped_session(session_factory=sessionmaker(bind=connection)) + _db.session = session + add_mockdata(session) + session.commit() + connection.close() + session.remove() yield _db @@ -267,29 +279,48 @@ def test_csv_dir(): def add_mockdata(session): NUM_OFFICERS = current_app.config["NUM_OFFICERS"] + assert NUM_OFFICERS >= 5 department = models.Department( - name="Springfield Police Department", + name=DEPARTMENT_NAME, short_name="SPD", unique_internal_identifier_label="homer_number", ) session.add(department) - department2 = models.Department(name="Chicago Police Department", short_name="CPD") + department2 = models.Department(name=OTHER_DEPARTMENT_NAME, short_name="CPD") session.add(department2) + empty_department = models.Department( + name=DEPARTMENT_WITHOUT_OFFICERS_NAME, short_name="EPD" + ) + session.add(empty_department) session.commit() - i = 0 - for rank in RANK_CHOICES_1: + for i, rank in enumerate(RANK_CHOICES_1): session.add( - models.Job(job_title=rank, order=i, is_sworn_officer=True, department_id=1) + models.Job( + job_title=rank, + order=i, + is_sworn_officer=True, + department_id=department.id, + ) + ) + session.add( + models.Job( + job_title=rank, + order=i, + is_sworn_officer=True, + department_id=empty_department.id, + ) ) - i += 1 - i = 0 - for rank in RANK_CHOICES_2: + for i, rank in enumerate(RANK_CHOICES_2): session.add( - models.Job(job_title=rank, order=i, is_sworn_officer=True, department_id=2) + models.Job( + job_title=rank, + order=i, + is_sworn_officer=True, + department_id=department2.id, + ) ) - i += 1 session.commit() # Ensure test data is deterministic @@ -309,15 +340,18 @@ def add_mockdata(session): test_images = [ models.Image( - filepath="/static/images/test_cop{}.png".format(x + 1), department_id=1 + filepath=f"/static/images/test_cop{x+1}.png", + department_id=department.id, ) for x in range(5) ] + [ models.Image( - filepath="/static/images/test_cop{}.png".format(x + 1), department_id=2 + filepath=f"/static/images/test_cop{x+1}.png", + department_id=department2.id, ) for x in range(5) ] + session.add_all(test_images) test_officer_links = [ models.Link( @@ -334,10 +368,11 @@ def add_mockdata(session): ), ] - officers = [generate_officer() for o in range(NUM_OFFICERS)] + officers = [] + for d in [department, department2]: + officers += [generate_officer(d) for _ in range(NUM_OFFICERS)] officers[0].links = test_officer_links session.add_all(officers) - session.add_all(test_images) session.commit() @@ -347,15 +382,22 @@ def add_mockdata(session): # assures that there are some assigned and unassigned images in each department assigned_images_dept1 = models.Image.query.filter_by(department_id=1).limit(3).all() - assigned_images_dept2 = models.Image.query.filter_by(department_id=2).limit(2).all() + assigned_images_dept2 = models.Image.query.filter_by(department_id=2).limit(3).all() jobs_dept1 = models.Job.query.filter_by(department_id=1).all() jobs_dept2 = models.Job.query.filter_by(department_id=2).all() + + # which percentage of officers have an assignment + assignment_ratio = 0.9 # 90% + num_officers_with_assignments_1 = math.ceil(len(officers_dept1) * assignment_ratio) assignments_dept1 = [ - build_assignment(officer, test_units, jobs_dept1) for officer in officers_dept1 + build_assignment(officer, test_units, jobs_dept1) + for officer in officers_dept1[:num_officers_with_assignments_1] ] + num_officers_with_assignments_2 = math.ceil(len(officers_dept2) * assignment_ratio) assignments_dept2 = [ - build_assignment(officer, test_units, jobs_dept2) for officer in officers_dept2 + build_assignment(officer, test_units, jobs_dept2) + for officer in officers_dept2[:num_officers_with_assignments_2] ] salaries = [build_salary(officer) for officer in all_officers] @@ -548,35 +590,29 @@ def add_mockdata(session): @pytest.fixture def mockdata(session): - return add_mockdata(session) + pass @pytest.fixture def department(session): - department = models.Department( - id=1, - name="Springfield Police Department", - short_name="SPD", - unique_internal_identifier_label="homer_number", - ) - session.add(department) - session.commit() - return department + return models.Department.query.filter_by(name=DEPARTMENT_NAME).one() @pytest.fixture -def department_with_ranks(department, session): - for order, rank in enumerate(RANK_CHOICES_1): - session.add( - models.Job( - job_title=rank, - order=order, - is_sworn_officer=True, - department=department, - ) - ) - session.commit() - return department +def department_without_officers(session): + return models.Department.query.filter_by( + name=DEPARTMENT_WITHOUT_OFFICERS_NAME + ).one() + + +@pytest.fixture +def officer_no_assignments(department): + return ( + Officer.query.filter_by(department_id=department.id) + .outerjoin(Officer.assignments) + .filter(Officer.assignments == None) # noqa: E711 + .first() + ) @pytest.fixture diff --git a/OpenOversight/tests/routes/test_officer_and_department.py b/OpenOversight/tests/routes/test_officer_and_department.py index 3b3984441..45bb55d88 100644 --- a/OpenOversight/tests/routes/test_officer_and_department.py +++ b/OpenOversight/tests/routes/test_officer_and_department.py @@ -43,10 +43,10 @@ User, ) from OpenOversight.app.utils.constants import ENCODING_UTF_8 -from OpenOversight.app.utils.db import dept_choices, unit_choices +from OpenOversight.app.utils.db import unit_choices from OpenOversight.app.utils.forms import add_new_assignment -from ..conftest import AC_DEPT, RANK_CHOICES_1 +from ..conftest import AC_DEPT, DEPARTMENT_NAME, RANK_CHOICES_1 from .route_helpers import login_ac, login_admin, login_user, process_form_data @@ -332,12 +332,13 @@ def test_admin_can_edit_assignment(mockdata, client, session): assert assignment.resign_date == date(2019, 11, 30) -def test_admin_edit_assignment_validation_error(mockdata, client, session): +def test_admin_edit_assignment_validation_error( + mockdata, client, session, officer_no_assignments +): with current_app.test_request_context(): login_admin(client) - # Remove existing assignments - officer = Officer.query.filter_by(id=3).one() + officer = officer_no_assignments job = Job.query.filter_by( department_id=officer.department_id, job_title="Police Officer" ).one() @@ -349,14 +350,14 @@ def test_admin_edit_assignment_validation_error(mockdata, client, session): ) rv = client.post( - url_for("main.add_assignment", officer_id=3), + url_for("main.add_assignment", officer_id=officer.id), data=form.data, follow_redirects=True, ) # Attempt to set resign date to a date before the start date form = AssignmentForm(resign_date=date(2018, 12, 31)) - officer = Officer.query.filter_by(id=3).one() + officer = Officer.query.filter_by(id=officer.id).one() rv = client.post( url_for( @@ -605,12 +606,10 @@ def test_admin_can_edit_police_department(mockdata, client, session): assert edit_short_name_department.short_name == "CPD" -def test_ac_cannot_edit_police_department(mockdata, client, session): +def test_ac_cannot_edit_police_department(mockdata, client, session, department): with current_app.test_request_context(): login_ac(client) - department = Department.query.first() - form = EditDepartmentForm(name="Corrected Police Department", short_name="CPD") rv = client.post( @@ -622,21 +621,19 @@ def test_ac_cannot_edit_police_department(mockdata, client, session): assert rv.status_code == HTTPStatus.FORBIDDEN -def test_admin_can_edit_rank_order(mockdata, client, session): +def test_admin_can_edit_rank_order(mockdata, client, session, department): with current_app.test_request_context(): login_admin(client) - - ranks = ( - Department.query.filter_by(name="Springfield Police Department").one().jobs - ) + department_name = department.name + ranks = department.jobs ranks_update = ranks.copy() original_first_rank = copy.deepcopy(ranks_update[0]) ranks_update[0], ranks_update[1] = ranks_update[1], ranks_update[0] ranks_stringified = [rank.job_title for rank in ranks_update] rank_change_form = EditDepartmentForm( - name="Springfield Police Department", - short_name="SPD", + name=department_name, + short_name=department.short_name, jobs=ranks_stringified, ) processed_data = process_form_data(rank_change_form.data) @@ -647,30 +644,25 @@ def test_admin_can_edit_rank_order(mockdata, client, session): follow_redirects=True, ) - updated_ranks = ( - Department.query.filter_by(name="Springfield Police Department").one().jobs - ) - assert "Department Springfield Police Department edited" in rv.data.decode( - ENCODING_UTF_8 - ) + updated_ranks = Department.query.filter_by(name=department_name).one().jobs + assert f"Department {department_name} edited" in rv.data.decode(ENCODING_UTF_8) assert ( updated_ranks[0].job_title == original_first_rank.job_title and updated_ranks[0].order != original_first_rank.order ) -def test_admin_cannot_delete_rank_in_use(mockdata, client, session): +def test_admin_cannot_delete_rank_in_use(mockdata, client, session, department): with current_app.test_request_context(): login_admin(client) - ranks = ( - Department.query.filter_by(name="Springfield Police Department").one().jobs - ) + ranks = department.jobs + department_name = department.name original_ranks = ranks.copy() ranks_update = RANK_CHOICES_1.copy()[:-1] rank_change_form = EditDepartmentForm( - name="Springfield Police Department", short_name="SPD", jobs=ranks_update + name=department_name, short_name=department.short_name, jobs=ranks_update ) processed_data = process_form_data(rank_change_form.data) @@ -680,9 +672,7 @@ def test_admin_cannot_delete_rank_in_use(mockdata, client, session): follow_redirects=True, ) - updated_ranks = ( - Department.query.filter_by(name="Springfield Police Department").one().jobs - ) + updated_ranks = Department.query.filter_by(name=department_name).one().jobs assert ( "You attempted to delete a rank, Commander, that is still in use" in result.data.decode(ENCODING_UTF_8) @@ -690,10 +680,10 @@ def test_admin_cannot_delete_rank_in_use(mockdata, client, session): assert len(updated_ranks) == len(original_ranks) -def test_admin_can_delete_rank_not_in_use(mockdata, client, session): +def test_admin_can_delete_rank_not_in_use(mockdata, client, session, department): with current_app.test_request_context(): login_admin(client) - + department_name = department.name ranks_update = RANK_CHOICES_1.copy() original_ranks_length = len(ranks_update) ranks_update.append( @@ -706,7 +696,7 @@ def test_admin_can_delete_rank_not_in_use(mockdata, client, session): ) rank_change_form = EditDepartmentForm( - name="Springfield Police Department", short_name="SPD", jobs=ranks_update + name=department_name, short_name=department.short_name, jobs=ranks_update ) processed_data = process_form_data(rank_change_form.data) @@ -719,24 +709,18 @@ def test_admin_can_delete_rank_not_in_use(mockdata, client, session): assert rv.status_code == HTTPStatus.OK assert ( - len( - Department.query.filter_by(name="Springfield Police Department") - .one() - .jobs - ) + len(Department.query.filter_by(name=department_name).one().jobs) == original_ranks_length + 1 ) ranks_update = [ job.job_title - for job in Department.query.filter_by(name="Springfield Police Department") - .one() - .jobs + for job in Department.query.filter_by(name=department_name).one().jobs ] ranks_update = ranks_update[:-1] rank_change_form = EditDepartmentForm( - name="Springfield Police Department", short_name="SPD", jobs=ranks_update + name=department_name, short_name=department.short_name, jobs=ranks_update ) processed_data = process_form_data(rank_change_form.data) @@ -749,16 +733,14 @@ def test_admin_can_delete_rank_not_in_use(mockdata, client, session): assert rv.status_code == HTTPStatus.OK assert ( - len( - Department.query.filter_by(name="Springfield Police Department") - .one() - .jobs - ) + len(Department.query.filter_by(name=department_name).one().jobs) == original_ranks_length ) -def test_admin_can_delete_multiple_ranks_not_in_use(mockdata, client, session): +def test_admin_can_delete_multiple_ranks_not_in_use( + mockdata, client, session, department +): with current_app.test_request_context(): login_admin(client) @@ -767,8 +749,10 @@ def test_admin_can_delete_multiple_ranks_not_in_use(mockdata, client, session): ranks_update.append("Temporary Rank 1") ranks_update.append("Temporary Rank 2") + department_name = department.name + rank_change_form = EditDepartmentForm( - name="Springfield Police Department", short_name="SPD", jobs=ranks_update + name=department_name, short_name=department.short_name, jobs=ranks_update ) processed_data = process_form_data(rank_change_form.data) @@ -781,24 +765,18 @@ def test_admin_can_delete_multiple_ranks_not_in_use(mockdata, client, session): assert rv.status_code == HTTPStatus.OK assert ( - len( - Department.query.filter_by(name="Springfield Police Department") - .one() - .jobs - ) + len(Department.query.filter_by(name=department_name).one().jobs) == original_ranks_length + 2 ) ranks_update = [ job.job_title - for job in Department.query.filter_by(name="Springfield Police Department") - .one() - .jobs + for job in Department.query.filter_by(name=department_name).one().jobs ] ranks_update = ranks_update[:-2] rank_change_form = EditDepartmentForm( - name="Springfield Police Department", short_name="SPD", jobs=ranks_update + name=department_name, short_name=department.short_name, jobs=ranks_update ) processed_data = process_form_data(rank_change_form.data) @@ -811,17 +789,13 @@ def test_admin_can_delete_multiple_ranks_not_in_use(mockdata, client, session): assert rv.status_code == HTTPStatus.OK assert ( - len( - Department.query.filter_by(name="Springfield Police Department") - .one() - .jobs - ) + len(Department.query.filter_by(name=department_name).one().jobs) == original_ranks_length ) def test_admin_cannot_commit_edit_that_deletes_one_rank_in_use_and_one_not_in_use_rank( - mockdata, client, session + mockdata, client, session, department ): with current_app.test_request_context(): login_admin(client) @@ -836,9 +810,9 @@ def test_admin_cannot_commit_edit_that_deletes_one_rank_in_use_and_one_not_in_us department_id=1, ) ) - + department_name = department.name rank_change_form = EditDepartmentForm( - name="Springfield Police Department", short_name="SPD", jobs=ranks_update + name=department_name, short_name=department.short_name, jobs=ranks_update ) processed_data = process_form_data(rank_change_form.data) @@ -851,25 +825,19 @@ def test_admin_cannot_commit_edit_that_deletes_one_rank_in_use_and_one_not_in_us assert rv.status_code == HTTPStatus.OK assert ( - len( - Department.query.filter_by(name="Springfield Police Department") - .one() - .jobs - ) + len(Department.query.filter_by(name=department_name).one().jobs) == original_ranks_length + 1 ) # attempt to delete multiple ranks ranks_update = [ job.job_title - for job in Department.query.filter_by(name="Springfield Police Department") - .one() - .jobs + for job in Department.query.filter_by(name=department_name).one().jobs ] ranks_update = ranks_update[:-2] rank_change_form = EditDepartmentForm( - name="Springfield Police Department", short_name="SPD", jobs=ranks_update + name=department_name, short_name=department.short_name, jobs=ranks_update ) processed_data = process_form_data(rank_change_form.data) @@ -881,11 +849,7 @@ def test_admin_cannot_commit_edit_that_deletes_one_rank_in_use_and_one_not_in_us ) assert ( - len( - Department.query.filter_by(name="Springfield Police Department") - .one() - .jobs - ) + len(Department.query.filter_by(name=department_name).one().jobs) == original_ranks_length + 1 ) @@ -949,13 +913,12 @@ def test_expected_dept_appears_in_submission_dept_selection(mockdata, client, se rv = client.get(url_for("main.submit_data"), follow_redirects=True) - assert "Springfield Police Department" in rv.data.decode(ENCODING_UTF_8) + assert DEPARTMENT_NAME in rv.data.decode(ENCODING_UTF_8) -def test_admin_can_add_new_officer(mockdata, client, session): +def test_admin_can_add_new_officer(mockdata, client, session, department): with current_app.test_request_context(): login_admin(client) - department = random.choice(dept_choices()) links = [ LinkForm(url="http://www.pleasework.com", link_type="link").data, LinkForm(url="http://www.avideo/?v=2345jk", link_type="video").data, @@ -987,10 +950,9 @@ def test_admin_can_add_new_officer(mockdata, client, session): assert officer.gender == "M" -def test_admin_can_add_new_officer_with_unit(mockdata, client, session): +def test_admin_can_add_new_officer_with_unit(mockdata, client, session, department): with current_app.test_request_context(): login_admin(client) - department = random.choice(dept_choices()) unit = random.choice(unit_choices()) links = [ LinkForm(url="http://www.pleasework.com", link_type="link").data, @@ -1148,10 +1110,9 @@ def test_ac_cannot_add_new_officer_not_in_their_dept(mockdata, client, session): assert officer is None -def test_admin_can_edit_existing_officer(mockdata, client, session): +def test_admin_can_edit_existing_officer(mockdata, client, session, department): with current_app.test_request_context(): login_admin(client) - department = random.choice(dept_choices()) unit = random.choice(unit_choices()) link_url0 = "http://pleasework.com" link_url1 = "http://avideo/?v=2345jk" @@ -1298,11 +1259,12 @@ def test_ac_can_edit_officer_in_their_dept(mockdata, client, session): assert officer.last_name == new_last_name -def test_admin_adds_officer_without_middle_initial(mockdata, client, session): +def test_admin_adds_officer_without_middle_initial( + mockdata, client, session, department +): with current_app.test_request_context(): login_admin(client) - department = random.choice(dept_choices()) job = Job.query.filter_by(department_id=department.id).first() form = AddOfficerForm( first_name="Test", @@ -1328,11 +1290,12 @@ def test_admin_adds_officer_without_middle_initial(mockdata, client, session): assert officer.gender == "M" -def test_admin_adds_officer_with_letter_in_badge_no(mockdata, client, session): +def test_admin_adds_officer_with_letter_in_badge_no( + mockdata, client, session, department +): with current_app.test_request_context(): login_admin(client) - department = random.choice(dept_choices()) job = Job.query.filter_by(department_id=department.id).first() form = AddOfficerForm( first_name="Test", @@ -1359,13 +1322,10 @@ def test_admin_adds_officer_with_letter_in_badge_no(mockdata, client, session): assert officer.assignments[0].star_no == "T666" -def test_admin_can_add_new_unit(mockdata, client, session): +def test_admin_can_add_new_unit(mockdata, client, session, department): with current_app.test_request_context(): login_admin(client) - department = Department.query.filter_by( - name="Springfield Police Department" - ).first() form = AddUnitForm(descrip="Test", department=department.id) rv = client.post( @@ -1413,10 +1373,9 @@ def test_ac_cannot_add_new_unit_not_in_their_dept(mockdata, client, session): assert unit is None -def test_admin_can_add_new_officer_with_suffix(mockdata, client, session): +def test_admin_can_add_new_officer_with_suffix(mockdata, client, session, department): with current_app.test_request_context(): login_admin(client) - department = random.choice(dept_choices()) links = [ LinkForm(url="http://www.pleasework.com", link_type="link").data, LinkForm(url="http://www.avideo/?v=2345jk", link_type="video").data, @@ -1468,10 +1427,9 @@ def test_ac_cannot_directly_upload_photos_of_of_non_dept_officers( assert rv.status_code == HTTPStatus.FORBIDDEN -def test_officer_csv(mockdata, client, session): +def test_officer_csv(mockdata, client, session, department): with current_app.test_request_context(): login_admin(client) - department = random.choice(dept_choices()) links = [ LinkForm(url="http://www.pleasework.com", link_type="link").data, ] @@ -1518,9 +1476,8 @@ def test_officer_csv(mockdata, client, session): assert form.star_no.data == added_lines[0]["badge number"] -def test_assignments_csv(mockdata, client, session): +def test_assignments_csv(mockdata, client, session, department): with current_app.test_request_context(): - department = random.choice(dept_choices()) officer = Officer.query.filter_by(department_id=department.id).first() job = ( Job.query.filter_by(department_id=department.id) @@ -1558,10 +1515,9 @@ def test_assignments_csv(mockdata, client, session): assert new_assignment[0]["job title"] == job.job_title -def test_incidents_csv(mockdata, client, session): +def test_incidents_csv(mockdata, client, session, department): with current_app.test_request_context(): login_admin(client) - department = random.choice(dept_choices()) # Delete existing incidents for chosen department Incident.query.filter_by(department_id=department.id).delete() @@ -2164,9 +2120,10 @@ def test_ac_cannot_edit_non_dept_salary(mockdata, client, session): assert officer.salaries[0].salary == Decimal("123456.78") -def test_get_department_ranks_with_specific_department_id(mockdata, client, session): +def test_get_department_ranks_with_specific_department_id( + mockdata, client, session, department +): with current_app.test_request_context(): - department = Department.query.first() rv = client.get( url_for("main.get_dept_ranks", department_id=department.id), follow_redirects=True, @@ -2185,7 +2142,7 @@ def test_get_department_ranks_with_no_department(mockdata, client, session): data = [x[1] for x in data] assert "Commander" in data - assert data.count("Commander") == 2 # Once for each test department + assert data.count("Commander") == 3 # Once for each test department def test_admin_can_add_link_to_officer_profile(mockdata, client, session): @@ -2304,7 +2261,13 @@ def test_ac_can_edit_link_on_officer_profile_in_their_dept(mockdata, client, ses with current_app.test_request_context(): login_ac(client) ac = User.query.filter_by(email="raq929@example.org").first() - officer = Officer.query.filter_by(department_id=AC_DEPT).first() + # Officer from department with id AC_DEPT and no links + officer = ( + Officer.query.filter_by(department_id=AC_DEPT) + .outerjoin(Officer.links) + .filter(Officer.links == None) # noqa: E711 + .first() + ) assert len(officer.links) == 0 @@ -2356,9 +2319,13 @@ def test_ac_cannot_edit_link_on_officer_profile_not_in_their_dept( with current_app.test_request_context(): login_admin(client) admin = User.query.filter_by(email="jen@example.org").first() - officer = Officer.query.except_( - Officer.query.filter_by(department_id=AC_DEPT) - ).all()[10] + # Officer from another department (not id AC_DEPT) and no links + officer = ( + Officer.query.filter(Officer.department_id != AC_DEPT) + .outerjoin(Officer.links) + .filter(Officer.links == None) # noqa: E711 + .first() + ) assert len(officer.links) == 0 @@ -2407,7 +2374,13 @@ def test_ac_cannot_edit_link_on_officer_profile_not_in_their_dept( def test_admin_can_delete_link_from_officer_profile(mockdata, client, session): with current_app.test_request_context(): login_admin(client) - officer = Officer.query.filter_by(id=1).one() + # Officer from department with id AC_DEPT and some links + officer = ( + Officer.query.filter_by(department_id=AC_DEPT) + .outerjoin(Officer.links) + .filter(Officer.links != None) # noqa: E711 + .first() + ) assert len(officer.links) > 0 @@ -2427,7 +2400,13 @@ def test_ac_can_delete_link_from_officer_profile_in_their_dept( with current_app.test_request_context(): login_ac(client) ac = User.query.filter_by(email="raq929@example.org").first() - officer = Officer.query.filter_by(department_id=AC_DEPT).first() + # Officer from department with id AC_DEPT and no links + officer = ( + Officer.query.filter_by(department_id=AC_DEPT) + .outerjoin(Officer.links) + .filter(Officer.links == None) # noqa: E711 + .first() + ) assert len(officer.links) == 0 @@ -2467,9 +2446,13 @@ def test_ac_cannot_delete_link_from_officer_profile_not_in_their_dept( with current_app.test_request_context(): login_admin(client) admin = User.query.filter_by(email="jen@example.org").first() - officer = Officer.query.except_( - Officer.query.filter_by(department_id=AC_DEPT) - ).all()[10] + # Officer from another department (not id AC_DEPT) and no links + officer = ( + Officer.query.filter(Officer.department_id != AC_DEPT) + .outerjoin(Officer.links) + .filter(Officer.links == None) # noqa: E711 + .first() + ) assert len(officer.links) == 0 diff --git a/OpenOversight/tests/test_commands.py b/OpenOversight/tests/test_commands.py index ec99477de..f8818b743 100644 --- a/OpenOversight/tests/test_commands.py +++ b/OpenOversight/tests/test_commands.py @@ -110,9 +110,9 @@ def test_add_department__missing_argument(session): def test_add_job_title__success(session, department): department_id = department.id - job_title = "Police Officer" + job_title = "New Rank" is_sworn = True - order = 1 + order = 15 # run command to add job title result = run_command_print_output( @@ -135,14 +135,14 @@ def test_add_job_title__duplicate(session, department): job_title = "Police Officer" is_sworn = True order = 1 - job = Job( - job_title=job_title, - is_sworn_officer=is_sworn, - order=order, - department=department, + + # make sure Job is already included in db + assert ( + Job.query.filter_by( + job_title=job_title, is_sworn_officer=True, order=1, department=department + ).first() + is not None ) - session.add(job) - session.commit() # adding exact same job again via command result = run_command_print_output( @@ -163,14 +163,13 @@ def test_add_job_title__different_departments(session, department): job_title = "Police Officer" is_sworn = True order = 1 - job = Job( - job_title=job_title, - is_sworn_officer=is_sworn, - order=order, - department=department, + # make sure Job is already included in db + assert ( + Job.query.filter_by( + job_title=job_title, is_sworn_officer=True, order=1, department=department + ).first() + is not None ) - session.add(job) - session.commit() # adding samme job but for different department result = run_command_print_output( @@ -434,14 +433,13 @@ def test_csv_new_salary(csvfile): ) -def test_bulk_add_officers__success(session, department_with_ranks, csv_path): +def test_bulk_add_officers__success(session, department_without_officers, csv_path): # generate two officers with different names - first_officer = generate_officer() - first_officer.department = department_with_ranks + first_officer = generate_officer(department_without_officers) print(Job.query.all()) - print(Job.query.filter_by(department=department_with_ranks).all()) + print(Job.query.filter_by(department=department_without_officers).all()) job = ( - Job.query.filter_by(department=department_with_ranks).filter_by(order=1) + Job.query.filter_by(department=department_without_officers).filter_by(order=1) ).first() fo_fn = "Uniquefirst" first_officer.first_name = fo_fn @@ -451,8 +449,7 @@ def test_bulk_add_officers__success(session, department_with_ranks, csv_path): assignment = Assignment(baseofficer=first_officer, job_id=job.id) session.add(assignment) session.commit() - different_officer = generate_officer() - different_officer.department = department_with_ranks + different_officer = generate_officer(department_without_officers) different_officer.job = job do_fn = different_officer.first_name do_ln = different_officer.last_name @@ -461,7 +458,7 @@ def test_bulk_add_officers__success(session, department_with_ranks, csv_path): session.add(assignment) session.commit() - department_id = department_with_ranks.id + department_id = department_without_officers.id # generate csv to update one existing officer and add one new @@ -532,15 +529,13 @@ def test_bulk_add_officers__duplicate_name(session, department, csv_path): # two officers with the same name first_name = "James" last_name = "Smith" - first_officer = generate_officer() - first_officer.department = department + first_officer = generate_officer(department) first_officer.first_name = first_name first_officer.last_name = last_name session.add(first_officer) session.commit() - different_officer = generate_officer() - different_officer.department = department + different_officer = generate_officer(department) different_officer.first_name = first_name different_officer.last_name = last_name session.add(different_officer) @@ -577,9 +572,8 @@ def test_bulk_add_officers__duplicate_name(session, department, csv_path): def test_bulk_add_officers__write_static_null_field(session, department, csv_path): # start with an officer whose birth_year is missing - officer = generate_officer() + officer = generate_officer(department) officer.birth_year = None - officer.department = department session.add(officer) session.commit() fo_uuid = officer.unique_internal_identifier @@ -621,10 +615,9 @@ def test_bulk_add_officers__write_static_null_field(session, department, csv_pat def test_bulk_add_officers__write_static_field_no_flag(session, department, csv_path): # officer with birth year set - officer = generate_officer() + officer = generate_officer(department) old_birth_year = 1979 officer.birth_year = old_birth_year - officer.department = department session.add(officer) session.commit() fo_uuid = officer.unique_internal_identifier @@ -668,9 +661,8 @@ def test_bulk_add_officers__write_static_field_no_flag(session, department, csv_ def test_bulk_add_officers__write_static_field__flag_set(session, department, csv_path): # officer with birth year set - officer = generate_officer() + officer = generate_officer(department) officer.birth_year = 1979 - officer.department = department session.add(officer) session.commit() officer_uuid = officer.unique_internal_identifier @@ -713,12 +705,13 @@ def test_bulk_add_officers__write_static_field__flag_set(session, department, cs assert officer.birth_year == new_birth_year -def test_bulk_add_officers__no_create_flag(session, department, csv_path): +def test_bulk_add_officers__no_create_flag( + session, department_without_officers, csv_path +): # department with one officer - department_id = department.id - officer = generate_officer() + department_id = department_without_officers.id + officer = generate_officer(department_without_officers) officer.gender = None - officer.department = department session.add(officer) session.commit() officer_uuid = officer.unique_internal_identifier @@ -765,14 +758,15 @@ def test_bulk_add_officers__no_create_flag(session, department, csv_path): assert result.exception is None # confirm that only one officer is in database and information was updated + print(Officer.query.filter_by(department_id=department_id).all()) officer = Officer.query.filter_by(department_id=department_id).one() assert officer.unique_internal_identifier == officer_uuid assert officer.gender == officer_gender_updated -def test_advanced_csv_import__success(session, department_with_ranks, test_csv_dir): +def test_advanced_csv_import__success(session, department, test_csv_dir): # make sure department name aligns with the csv files - assert department_with_ranks.name == "Springfield Police Department" + assert department.name == "Springfield Police Department" # set up existing data officer = Officer( @@ -789,7 +783,7 @@ def test_advanced_csv_import__success(session, department_with_ranks, test_csv_d officer_id=officer.id, star_no="4567", star_date=datetime.date(2020, 1, 1), - job_id=department_with_ranks.jobs[0].id, + job_id=department.jobs[0].id, ) session.add(assignment) @@ -820,7 +814,7 @@ def test_advanced_csv_import__success(session, department_with_ranks, test_csv_d result = run_command_print_output( advanced_csv_import, [ - str(department_with_ranks.name), + str(department.name), "--officers-csv", os.path.join(test_csv_dir, "officers.csv"), "--assignments-csv", @@ -962,10 +956,10 @@ def _create_csv(data, path, csv_file_name): return csv_path -def test_advanced_csv_import__force_create(session, department_with_ranks, tmp_path): +def test_advanced_csv_import__force_create(session, department, tmp_path): tmp_path = str(tmp_path) - department_name = department_with_ranks.name + department_name = department.name other_department = Department(name="Other department", short_name="OPD") session.add(other_department) @@ -1041,7 +1035,7 @@ def test_advanced_csv_import__force_create(session, department_with_ranks, tmp_p result = run_command_print_output( advanced_csv_import, [ - str(department_with_ranks.name), + str(department.name), "--officers-csv", officers_csv, "--assignments-csv", @@ -1082,35 +1076,37 @@ def test_advanced_csv_import__force_create(session, department_with_ranks, tmp_p assert cop1.links[0] == link -def test_advanced_csv_import__overwrite_assignments( - session, department_with_ranks, tmp_path -): +def test_advanced_csv_import__overwrite_assignments(session, department, tmp_path): tmp_path = str(tmp_path) - department_name = department_with_ranks.name + department_name = department.name other_department = Department(name="Other department", short_name="OPD") session.add(other_department) + cop1_id = 999001 + cop2_id = 999002 officer = Officer( - id=99001, - department_id=department_with_ranks.id, + id=cop1_id, + department_id=department.id, first_name="Already", last_name="InDatabase", ) officer2 = Officer( - id=99002, - department_id=department_with_ranks.id, + id=cop2_id, + department_id=department.id, first_name="Also", last_name="InDatabase", ) + a1_id = 999101 + a2_id = 999102 assignment = Assignment( - id=123, + id=a1_id, officer_id=officer.id, job_id=Job.query.filter_by(job_title="Police Officer").first().id, ) assignment2 = Assignment( - id=124, + id=a2_id, officer_id=officer2.id, job_id=Job.query.filter_by(job_title="Police Officer").first().id, ) @@ -1132,17 +1128,19 @@ def test_advanced_csv_import__overwrite_assignments( officers_csv = _create_csv(officers_data, tmp_path, "officers.csv") + b1 = "12345" + b2 = "999" assignments_data = [ { - "officer_id": 99001, + "officer_id": cop1_id, "job title": "Captain", - "badge number": "12345", + "badge number": b1, "start date": "2020-07-24", }, { "officer_id": "#1", "job title": "Police Officer", - "badge number": "999", + "badge number": b2, "start date": "2020-07-21", }, ] @@ -1152,7 +1150,7 @@ def test_advanced_csv_import__overwrite_assignments( result = run_command_print_output( advanced_csv_import, [ - str(department_with_ranks.name), + str(department.name), "--officers-csv", officers_csv, "--assignments-csv", @@ -1166,24 +1164,22 @@ def test_advanced_csv_import__overwrite_assignments( assert result.exit_code == 0 # make sure all the data is imported as expected - cop1 = Officer.query.get(99001) + cop1 = Officer.query.get(cop1_id) assert len(cop1.assignments.all()) == 1 - assert cop1.assignments[0].star_no == "12345" + assert cop1.assignments[0].star_no == b1 - cop2 = Officer.query.get(99002) + cop2 = Officer.query.get(cop2_id) assert len(cop2.assignments.all()) == 1 - assert cop2.assignments[0] == Assignment.query.get(124) + assert cop2.assignments[0] == Assignment.query.get(a2_id) - cop3 = Officer.query.filter_by(first_name="Second").first() + cop3 = Officer.query.filter_by(first_name="Second", last_name="Test").first() assert len(cop3.assignments.all()) == 1 - assert cop3.assignments[0].star_no == "999" + assert cop3.assignments[0].star_no == b2 assert cop3.assignments[0].job.job_title == "Police Officer" -def test_advanced_csv_import__extra_fields_officers( - session, department_with_ranks, tmp_path -): - department_name = department_with_ranks.name +def test_advanced_csv_import__extra_fields_officers(session, department, tmp_path): + department_name = department.name # create csv with invalid field 'name' officers_data = [ {"id": "", "department_name": department_name, "name": "John Smith"}, @@ -1193,7 +1189,7 @@ def test_advanced_csv_import__extra_fields_officers( # run command result = run_command_print_output( advanced_csv_import, - [str(department_with_ranks.name), "--officers-csv", officers_csv], + [str(department.name), "--officers-csv", officers_csv], ) # expect the command to fail because of unexpected field 'name' @@ -1203,9 +1199,9 @@ def test_advanced_csv_import__extra_fields_officers( def test_advanced_csv_import__missing_required_field_officers( - session, department_with_ranks, tmp_path + session, department, tmp_path ): - department_name = department_with_ranks.name + department_name = department.name # create csv with missing field 'id' officers_data = [ { @@ -1219,7 +1215,7 @@ def test_advanced_csv_import__missing_required_field_officers( # run command result = run_command_print_output( advanced_csv_import, - [str(department_with_ranks.name), "--officers-csv", officers_csv], + [str(department.name), "--officers-csv", officers_csv], ) # expect the command to fail because 'id' is missing @@ -1228,10 +1224,8 @@ def test_advanced_csv_import__missing_required_field_officers( assert "id" in str(result.exception) -def test_advanced_csv_import__wrong_department( - session, department_with_ranks, tmp_path -): - department_name = department_with_ranks.name +def test_advanced_csv_import__wrong_department(session, department, tmp_path): + department_name = department.name other_department = Department(name="Other department", short_name="OPD") session.add(other_department) @@ -1259,9 +1253,9 @@ def test_advanced_csv_import__wrong_department( def test_advanced_csv_import__update_officer_different_department( - session, department_with_ranks, tmp_path + session, department, tmp_path ): - department_name = department_with_ranks.name + department_name = department.name # set up data other_department = Department(name="Other department", short_name="OPD") @@ -1285,7 +1279,7 @@ def test_advanced_csv_import__update_officer_different_department( # run command result = run_command_print_output( advanced_csv_import, - [str(department_with_ranks.name), "--officers-csv", officers_csv], + [str(department.name), "--officers-csv", officers_csv], ) # command fails because the officer is assigned to a different department @@ -1295,19 +1289,15 @@ def test_advanced_csv_import__update_officer_different_department( def test_advanced_csv_import__unit_other_department( - session, department_with_ranks, tmp_path + session, department, department_without_officers, tmp_path ): - department_id = department_with_ranks.id - # set up data - officer = generate_officer() - officer.department_id = department_id + officer = generate_officer(department) session.add(officer) session.flush() - other_department = Department(name="Other department", short_name="OPD") - session.add(other_department) + session.add(department_without_officers) session.flush() - unit = Unit(department_id=other_department.id) + unit = Unit(department_id=department_without_officers.id) session.add(unit) session.flush() @@ -1323,7 +1313,7 @@ def test_advanced_csv_import__unit_other_department( assignments_csv = _create_csv(assignments_data, tmp_path, "assignments.csv") result = run_command_print_output( advanced_csv_import, - [department_with_ranks.name, "--assignments-csv", assignments_csv], + [department.name, "--assignments-csv", assignments_csv], ) # command fails because the unit does not belong to the department @@ -1331,11 +1321,10 @@ def test_advanced_csv_import__unit_other_department( assert result.exit_code != 0 -def test_create_officer_from_row_adds_new_officer_and_normalizes_gender(app, session): +def test_create_officer_from_row_adds_new_officer_and_normalizes_gender( + app, session, department_without_officers +): with app.app_context(): - department = Department(name="Cityname Police Department", short_name="CNPD") - session.add(department) - session.commit() lookup_officer = Officer.query.filter_by( first_name="NewOfficerFromRow" ).one_or_none() @@ -1348,7 +1337,7 @@ def test_create_officer_from_row_adds_new_officer_and_normalizes_gender(app, ses "employment_date": "1980-12-01", "unique_internal_identifier": "officer-jones-unique-id", } - create_officer_from_row(row, department.id) + create_officer_from_row(row, department_without_officers.id) lookup_officer = Officer.query.filter_by( first_name="NewOfficerFromRow" diff --git a/OpenOversight/tests/test_functional.py b/OpenOversight/tests/test_functional.py index 0ba3cc02f..9c76e7ba9 100644 --- a/OpenOversight/tests/test_functional.py +++ b/OpenOversight/tests/test_functional.py @@ -159,7 +159,7 @@ def test_find_officer_cannot_see_uii_question_for_depts_without_uiis(mockdata, b dept_without_uii = Department.query.filter_by( unique_internal_identifier_label=None - ).one_or_none() + ).first() dept_id = str(dept_without_uii.id) dept_selector = Select(browser.find_element("id", "dept"))