Add invalid ID tests for main.views
#1124
Conversation
OpenOversight/app/main/views.py
Outdated
| 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) |
There was a problem hiding this comment.
I'm a little unsure of how to test this. Any advice would be appreciated, @sea-kelp @abandoned-prototype.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Given that we are searching by the primary key, I am comfortable removing this exception section. Thanks for the 👁️ , @sea-kelp!
michplunkett
changed the title
main.views
| try: | ||
| department = Department.query.filter_by(id=department_id).one() | ||
| except NoResultFound: | ||
| abort(HTTPStatus.NOT_FOUND) |
There was a problem hiding this comment.
Though it is equivalent logic, but I feel more comfortable using the id field of the Department object to query the Images downstream.
There was a problem hiding this comment.
I am assuming this will lead to one additional query so not exactly equivalent, but makes sense to me
There was a problem hiding this comment.
True, one more additional query. I think there's more value to the sureness around it than the minor performance hit.
| except: # noqa: E722 | ||
| 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}") |
There was a problem hiding this comment.
This logic was unreachable.
OpenOversight/app/main/views.py
Outdated
| @@ -896,6 +898,10 @@ def list_officer( | |||
| current_job=None, | |||
| require_photo: Optional[bool] = None, | |||
| ): | |||
| department = db.session.get(Department, department_id) | |||
There was a problem hiding this comment.
If there isn't a valid Department there is no need to perform any of the other queries.
OpenOversight/app/main/views.py
Outdated
| if not officer: | ||
| flash("Officer not found") | ||
| abort(HTTPStatus.NOT_FOUND) |
There was a problem hiding this comment.
Similar logic to: https://github.com/lucyparsons/OpenOversight/pull/1124/files#r1779369649
| @@ -379,15 +380,17 @@ def redirect_add_assignment(officer_id: int): | |||
| @ac_or_admin_required | |||
| def add_assignment(officer_id: int): | |||
| form = AssignmentForm() | |||
| officer = db.session.get(Officer, officer_id) | |||
| try: | |||
There was a problem hiding this comment.
Made object queries more uniform to the rest of the application.
michplunkett
requested review from
b-meson,
abandoned-prototype and
sea-kelp
| try: | ||
| department = Department.query.filter_by(id=department_id).one() | ||
| except NoResultFound: | ||
| abort(HTTPStatus.NOT_FOUND) |
There was a problem hiding this comment.
I am assuming this will lead to one additional query so not exactly equivalent, but makes sense to me
| # 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 |
There was a problem hiding this comment.
I would either not change this line, or also update the department_id on line 282
There was a problem hiding this comment.
I'll update line 282, good 👁️!
Description of Changes
There were a handful of routes where we did not validate the initial ID parameters. Additionally, I standardized how object querying was done within the
main.viewsfile.Tests and Linting
developbranch.pytestpasses on my local development environment.pre-commitpasses on my local development environment.