-
Notifications
You must be signed in to change notification settings - Fork 79
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.
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
Fix upload bug #1095
Fix upload bug #1095
Conversation
The failing test seems to pass locally for me once I add a S3 bucket - not sure if that's set up in the CI tests. |
@@ -1342,7 +1338,7 @@ def leaderboard(): | |||
|
|||
|
|||
@main.route( | |||
"/cop_face/department/<int:department_id>/images/<int:image_id>", | |||
"/cop_face/department/<int:department_id>/image/<int:image_id>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find!
def get_dept_units(department_id: int = 0): | ||
def get_dept_units(department_id: Optional[int] = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add methods
for this function and the redirect one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app.route
methods defaults to ["GET"]
. Is there any reason we need to specify methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer minimal assumptions when possible, but it's not a deal breaker.
flash(FLASH_MSG_PERMANENT_REDIRECT) | ||
return redirect( | ||
url_for("main.get_dept_ranks", department_id=department_id), | ||
url_for("main.get_dept_units", department_id=department_id), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find.
|
||
|
||
@pytest.mark.xdist_group | ||
def test_anonymous_user_can_upload_image(mockdata, browser, server_port): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noice.
It fails because file uploads are not supported in the CI tests. The other test that makes use of the upload is marked with this decorator: |
@@ -1855,9 +1852,8 @@ def redirect_upload(department_id: int = 0, officer_id: int = 0): | |||
"/upload/departments/<int:department_id>/officers/<int:officer_id>", | |||
methods=[HTTPMethod.POST], | |||
) | |||
@login_required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the line that was causing issues
We can deploy this tomorrow afternoon, should you be good to merge it before then. |
I don't seem to have merge permissions so feel free to merge when you get a chance! |
Description of Changes
Cherry-pick of OrcaCollective#434
@login_required
decoratorNotes for Deployment
None!
Screenshots (if appropriate)
N/A
Tests and Linting
develop
branch.pytest
passes on my local development environment.pre-commit
passes on my local development environment.