Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add invalid ID tests for main.views #1124

Merged
merged 22 commits into from
Oct 1, 2024
Merged

Add invalid ID tests for main.views #1124

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a39a823
Add invalid ID check
michplunkett Sep 27, 2024
0b054ec
Update test_officer_and_department.py
michplunkett Sep 27, 2024
ebb5155
Update test_officer_and_department.py
michplunkett Sep 27, 2024
6762875
Update views.py
michplunkett Sep 27, 2024
0084af3
Add missing officer validation
michplunkett Sep 27, 2024
82c2223
Use constant
michplunkett Sep 27, 2024
2306f30
Update test_officer_and_department.py
michplunkett Sep 27, 2024
4010e67
Update test_image_tagging.py
michplunkett Sep 27, 2024
26ba9f1
Update views.py
michplunkett Sep 27, 2024
5294b31
Update test_officer_and_department.py
michplunkett Sep 27, 2024
f4b5cd6
Update views.py
michplunkett Sep 28, 2024
6b1cce4
Update test_image_tagging.py
michplunkett Sep 28, 2024
1e482ef
Update test_image_tagging.py
michplunkett Sep 28, 2024
65a61a5
Update test_image_tagging.py
michplunkett Sep 28, 2024
4505618
Add test for bad officer id
michplunkett Sep 28, 2024
f73d55c
Update views.py
michplunkett Sep 28, 2024
2544c81
Update views.py
michplunkett Sep 28, 2024
da722e7
Update views.py
michplunkett Sep 28, 2024
452f6c5
Update test_officer_and_department.py
michplunkett Sep 28, 2024
fabf793
Update test_image_tagging.py
michplunkett Sep 28, 2024
e199cb5
Update test_officer_and_department.py
michplunkett Sep 28, 2024
5451777
Update views.py
michplunkett Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion OpenOversight/app/main/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,9 +263,14 @@ def redirect_sort_images(department_id: int):
)
@login_required
def sort_images(department_id: int):
try:
department = Department.query.filter_by(id=department_id).one()
except NoResultFound:
abort(HTTPStatus.NOT_FOUND)
Comment on lines +266 to +269
Copy link
Collaborator Author

@michplunkett michplunkett Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though it is equivalent logic, but I feel more comfortable using the id field of the Department object to query the Images downstream.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming this will lead to one additional query so not exactly equivalent, but makes sense to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, one more additional query. I think there's more value to the sureness around it than the minor performance hit.


# Select a random unsorted image from the database
image_query = Image.query.filter_by(contains_cops=None).filter_by(
department_id=department_id
department_id=department.id
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would either not change this line, or also update the department_id on line 282

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update line 282, good 👁️!

)
image = get_random_image(image_query)

Expand Down Expand Up @@ -320,6 +325,7 @@ def officer_profile(officer_id: int):
exception_type, value, full_traceback = sys.exc_info()
error_str = " ".join([str(exception_type), str(value), format_exc()])
current_app.logger.error(f"Error finding officer: {error_str}")
abort(HTTPStatus.NOT_FOUND)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little unsure of how to test this. Any advice would be appreciated, @sea-kelp @abandoned-prototype.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you're saying - it doesn't seem like this code is reachable since (as far as I can tell) the only other exception that could be thrown in the try block is MultipleResultsFound, but officer id is configured as a primary key. I wonder if we should just remove it.

If you do want to test it, you could patch Query.one() with a side effect to raise MultipleResultsFound

Copy link
Collaborator Author

@michplunkett michplunkett Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we are searching by the primary key, I am comfortable removing this exception section. Thanks for the 👁️ , @sea-kelp!

form.job_title.query = (
Job.query.filter_by(department_id=officer.department_id)
.order_by(Job.order.asc())
Expand Down
8 changes: 8 additions & 0 deletions OpenOversight/tests/routes/test_image_tagging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ def test_route_login_required(route, client, mockdata):
assert rv.status_code == HTTPStatus.FOUND


def test_invalid_department_image_sorting(client, session):
with current_app.test_request_context():
login_user(client)

rv = client.get(url_for("main.sort_images", department_id=44000))
assert rv.status_code == HTTPStatus.NOT_FOUND


# POST-only routes
@pytest.mark.parametrize(
"route",
Expand Down
6 changes: 6 additions & 0 deletions OpenOversight/tests/routes/test_officer_and_department.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ def test_route_post_only(route, client, mockdata):
assert rv.status_code == HTTPStatus.METHOD_NOT_ALLOWED


def test_invalid_id_officer_profile(mockdata, client, session):
with current_app.test_request_context():
rv = client.get(url_for("main.officer_profile", officer_id=400000))
assert rv.status_code == HTTPStatus.NOT_FOUND


def test_user_can_access_officer_profile(mockdata, client, session):
with current_app.test_request_context():
rv = client.get(
Expand Down
Loading