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.

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

Merged
merged 3 commits into from
Apr 8, 2024
Merged

Fix upload bug #1095

merged 3 commits into from
Apr 8, 2024

Conversation

sea-kelp
Copy link
Collaborator

@sea-kelp sea-kelp commented Apr 7, 2024

Description of Changes

Cherry-pick of OrcaCollective#434

  • Fixed bug introduced in Pluralize REST routes #1018 where anonymous uploads were silently failing due to misplaced @login_required decorator
  • Audited all other redirect routes
  • Fixed note_detail.html and description_detail.html

Notes for Deployment

None!

Screenshots (if appropriate)

N/A

Tests and Linting

  • This branch is up-to-date with the develop branch.
  • pytest passes on my local development environment.
  • pre-commit passes on my local development environment.

@sea-kelp
Copy link
Collaborator Author

sea-kelp commented Apr 7, 2024

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>",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good find!

Comment on lines -1111 to +1108
def get_dept_units(department_id: int = 0):
def get_dept_units(department_id: Optional[int] = None):
Copy link
Collaborator

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?

Copy link
Collaborator Author

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?

Copy link
Collaborator

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),
Copy link
Collaborator

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):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Noice.

@michplunkett
Copy link
Collaborator

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: @pytest.mark.skip("Enable once real file upload in tests is supported."). It may have to be a manual test for now.

@@ -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
Copy link
Collaborator Author

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

@michplunkett michplunkett self-requested a review April 8, 2024 00:58
@michplunkett
Copy link
Collaborator

We can deploy this tomorrow afternoon, should you be good to merge it before then.

@sea-kelp
Copy link
Collaborator Author

sea-kelp commented Apr 8, 2024

I don't seem to have merge permissions so feel free to merge when you get a chance!

@michplunkett michplunkett merged commit 429869c into develop Apr 8, 2024
3 checks passed
@michplunkett michplunkett deleted the fix/upload-bug-2 branch April 8, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants