WIP: photo conversion (Supercedes #404) #412
Conversation
ssempervirens
changed the title
| @@ -95,6 +96,18 @@ def allowed_file(filename): | |||
| filename.rsplit('.', 1)[1].lower() in current_app.config['ALLOWED_EXTENSIONS'] | |||
|
|
|||
|
|
|||
| def requires_conversion(filename): | |||
There was a problem hiding this comment.
It would be dandy (I'm bringing this word back) to have a unit test for requires_conversion. It could be modeled off test_user_can_submit_allowed_file below - the asserts would verify that the correct result is returned by the function for files that do and do not require conversion.
| @@ -450,6 +451,8 @@ def upload(department_id): | |||
| file_to_upload = request.files['file'] | |||
| if not allowed_file(file_to_upload.filename): | |||
| return jsonify(error="File type not allowed!"), 415 | |||
| if requires_conversion(file_to_upload.filename): | |||
| convert_image() | |||
There was a problem hiding this comment.
It looks like the image is getting converted and saved on the filesystem but then we don't use the result and instead continue to use the image data stored in file_to_upload.filename (also in convert_image we actually remove the contents of file_to_upload.filename). I think we'll want to return the filename in convert_image and then use that image for the remainder of the code in upload.
| filename.split('.', 1)[1].lower() in current_app.config['REQUIRES_CONV'] | ||
|
|
||
|
|
||
| def convert_image(file_to_upload): |
There was a problem hiding this comment.
A test here would also be 💯 - it could assert:
- The original image is deleted
- The modified image is created
- The name or path of the modified file is returned (see comment above)
|
Hey @redshiftzero I am working on this, and I am confused about how to write a unit test for this without mock s3 bucket capabilities. Do you have any thoughts? |
|
Oh! I have thoughts on this, and actually am planning on refactoring the
current photo upload view to make it easier, after #459 is merged.
Basically, I'm going to use the code I wrote in that PR for all uploads,
making it really easy to mock views that have uploading. Hopefully I'm
doing that work on Wednesday.
On Jul 1, 2018 4:16 PM, "camille" <[email protected]> wrote:
Hey @redshiftzero <https://github.com/redshiftzero> I am working on this,
and I am confused about how to write a unit test for this without mock s3
bucket capabilities. Do you have any thoughts?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#412 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ANRw2Iqfb33cHsMCaZUWyc3b2WA0YAitks5uCS4DgaJpZM4S55LN>
.
|
|
But if you need it right away, Boto will mock an S3 bucket for you.
On Sun, Jul 1, 2018, 5:08 PM Rachel Stevens <[email protected]>
wrote:
… Oh! I have thoughts on this, and actually am planning on refactoring the
current photo upload view to make it easier, after #459 is merged.
Basically, I'm going to use the code I wrote in that PR for all uploads,
making it really easy to mock views that have uploading. Hopefully I'm
doing that work on Wednesday.
On Jul 1, 2018 4:16 PM, "camille" ***@***.***> wrote:
Hey @redshiftzero <https://github.com/redshiftzero> I am working on this,
and I am confused about how to write a unit test for this without mock s3
bucket capabilities. Do you have any thoughts?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#412 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ANRw2Iqfb33cHsMCaZUWyc3b2WA0YAitks5uCS4DgaJpZM4S55LN>
.
|
|
@raq929 sweet, totally missed that, sounds great! will play with this later this week in that case. :) |
|
|
||
|
|
||
| def convert_image(file_to_upload): | ||
| changeimage = Image.open(file_to_upload.filename) |
There was a problem hiding this comment.
Since we're calling Image.open() on untrusted content here, can you also hoist the decompression-bomb-safety warning into an error as documented in PIL's docs? AIUI we've been mostly free of risk-to-the-server for the files we're uploading since we were only reading headers out of them before, but now we're doing parsing and things that risk actual attack.
ghost
mentioned this pull request
|
Closing in favor of #691 |
Status
In progress, still need to resolve tests and write unit tests before merge
Description of Changes
Fixes #369. Supersedes PR #404
Changes proposed in this pull request:
Notes for Deployment
Screenshots (if appropriate)
Tests and linting
[x] I have rebased my changes on current
develop[] pytests pass in the development environment on my local machine
[]
flake8checks pass