-
Notifications
You must be signed in to change notification settings - Fork 375
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
Implement attachment_api and serving of attachments #4541
Conversation
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.
A few comments to consider.
Also, I see that you have API tests that:
- assert the POST behavior
- assert serving from an entity manually added to the database.
You should add a test that does the POST and serves the entity afterwards. This can come later in a playwright test when we have the UI. But I just wanted to bring that up now.
api/attachments_api.py
Outdated
|
||
if is_thumb and attachment.thumbnail: | ||
content = attachment.thumbnail | ||
headers = self.get_headers() | ||
headers['Content-Type'] = 'image/png' | ||
else: | ||
content = attachment.content | ||
headers = self.get_headers() | ||
headers['Content-Type'] = attachment.mime_type |
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.
Can raise headers = self.get_headers()
out before the conditional statements
if is_thumb and attachment.thumbnail: | |
content = attachment.thumbnail | |
headers = self.get_headers() | |
headers['Content-Type'] = 'image/png' | |
else: | |
content = attachment.content | |
headers = self.get_headers() | |
headers['Content-Type'] = attachment.mime_type | |
headers = self.get_headers() | |
if is_thumb and attachment.thumbnail: | |
content = attachment.thumbnail | |
headers['Content-Type'] = 'image/png' | |
else: | |
content = attachment.content | |
headers['Content-Type'] = attachment.mime_type |
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.
Done.
api/attachments_api.py
Outdated
|
||
|
||
class AttachmentsAPI(basehandlers.EntitiesAPIHandler): | ||
"""Features are the the main records that we track.""" |
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.
Nit: I think this comment was a copy-paste from features_api
chromium-dashboard/api/features_api.py
Line 53 in 92c888d
"""Features are the the main records that we track.""" |
Instead, you may want to mention how the attachments relate to features
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.
Fixed.
api/attachments_api.py
Outdated
if redirect_resp: | ||
self.abort(403, msg='User lacks permission to edit') | ||
|
||
files = kwargs.get('mock_files', self.request.files) |
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.
We should not have this logic that looks for mock_files in the keywords in the live code. Instead, we should adjust the tests. More about this in the test file.
files = kwargs.get('mock_files', self.request.files) | |
files = self.request.files |
api/attachments_api_test.py
Outdated
mock_files = {'uploaded-file': testing_config.Blank( | ||
filename='hello_attach.txt', | ||
read=lambda: b'hello attachments!', | ||
mimetype='text/plain')} | ||
with test_app.test_request_context(self.request_path): |
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.
mock_files = {'uploaded-file': testing_config.Blank( | |
filename='hello_attach.txt', | |
read=lambda: b'hello attachments!', | |
mimetype='text/plain')} | |
with test_app.test_request_context(self.request_path): | |
mock_file = (io.BytesIO(b'hello attachments!'), 'test.txt') | |
with test_app.test_request_context(path=self.request_path, data={'uploaded-file': mock_file}): |
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.
Thanks for that suggestion. I had spent half the day struggling with passing in a complete POST body as a binary string.
if settings.DEV_MODE or settings.UNIT_TEST_MODE: | ||
origin = settings.SITE_URL | ||
else: | ||
digits = attachment.key.integer_id() % 1000 |
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 a comment as to why we are doing modulo 1000?
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.
Done.
api/attachments_api.py
Outdated
|
||
@permissions.require_create_feature | ||
def do_post(self, **kwargs) -> dict[str, str]: | ||
"""Handle POST requests to create a single feature.""" |
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.
Also, I think you meant to adjust this comment too
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.
Done.
I went ahead and added a round-trip unit test now. |
This PR implements the server side logic for uploading and serving attachments. Specifically,
Also in the PR,