Skip to content

Fixes #24: Allow images to be uploaded when creating a new bug report.#679

Merged
karlcow merged 29 commits intomasterfrom
issues/24/1
Aug 25, 2015
Merged

Fixes #24: Allow images to be uploaded when creating a new bug report.#679
karlcow merged 29 commits intomasterfrom
issues/24/1

Conversation

@miketaylr
Copy link
Copy Markdown
Member

There's a lot going on here, including:

New upload blueprint route (we'll make more use of this in the future when we add comment uploads and screenshot uploads).
Adding a <input type=file> for our bug report form.
Image uploading to the server.
Use Flask-Uploads for image uploading.
Use Flask-WTF for form handling and image type validation.
Clientside validation for image types.
Functional tests for clientside file validation.
Unit tests for new upload rote.

Thx to @magsout for his help with frontend, as always.

r? @karlcow

Mike Taylor and others added 21 commits August 14, 2015 13:29

This comment was marked as abuse.

This comment was marked as abuse.

@karlcow
Copy link
Copy Markdown
Member

karlcow commented Aug 19, 2015

I would love if @magsout could review the JavaScript. upload seems to me a bit too dangerous to just me thinking the JS is fine. I'll focus on the python code.

@miketaylr
Copy link
Copy Markdown
Member Author

I would love if @magsout could review the JavaScript. upload seems to me a bit too dangerous to just me thinking the JS is fine.

Sounds good. The JS doesn't really do much with respect to uploads. Basically it's just duplicating the file validation on the client side that Flask-WTF is doing on the server side--so people don't have to submit an issue and get an error back (if they have JS enabled). But a sanity-check on checkImageTypeValidity in bugform.js and the tests would be welcome.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@karlcow
Copy link
Copy Markdown
Member

karlcow commented Aug 20, 2015

@miketaylr
I imported locally to test a couple of things.
Test at webcompat/webcompat-tests#272

I noticed something. First of all there are two nested uploads folder. Is it what you want?

→ find uploads
uploads
uploads/uploads
uploads/uploads/751a802ba7b56cd1f5d1170fda195a06_1433859324.jpg

Second, I would encourage to set a kind of dated space for the images inside upload. Something like

uploads/2015/08/image_hash_key.jpg

The reason is that the image are not stored into a database but in the filesystem. Filesystems have a tendency to become slow when the number of individual items grow into one unique directory. Having a dated space helps to create a partition that will reduce the number of images in one directory. It makes also less necessary to have a long hash_key, but that's secondary.

This comment was marked as abuse.

This comment was marked as abuse.

@miketaylr
Copy link
Copy Markdown
Member Author

I noticed something. First of all there are two nested uploads folder. Is it what you want?

Oh, we can fix that. That's now actually how it's laid out on the server - just a goof up for the localhost stuff.

@miketaylr
Copy link
Copy Markdown
Member Author

Second, I would encourage to set a kind of dated space for the images inside upload.

Agreed, let me work on that today.

@karlcow
Copy link
Copy Markdown
Member

karlcow commented Aug 20, 2015

r+ from me for the python side.
I don't know if you want to wait for @magsout r+ on JS.

Thanks a lot @miketaylr for your patience and the code. 💯
This is a very nice feature. I think there's an additional item which might come from this, a separate issue. Design work for the bug submitting form.

@miketaylr
Copy link
Copy Markdown
Member Author

Thanks! Let's wait a day or two to see if @magsout can take a look. No real rush for this feature.

I think there's an additional item which might come from this, a separate issue. Design work for the bug submitting form.

Cool. Also I need to file bugs (or find the ones that exist) for: adding upload form to issues/comments, drag & drop of images, API upload support for an arbitrary issue (then the stuff around screenshots).

@magsout
Copy link
Copy Markdown
Member

magsout commented Aug 25, 2015

lgtm

@karlcow
Copy link
Copy Markdown
Member

karlcow commented Aug 25, 2015

Thanks @magsout !

And let's pull it with love.

karlcow added a commit that referenced this pull request Aug 25, 2015
Fixes #24: Allow images to be uploaded when creating a new bug report.
@karlcow karlcow merged commit 2936db3 into master Aug 25, 2015
@magsout
Copy link
Copy Markdown
Member

magsout commented Aug 25, 2015

Good job everyone ❤️

@magsout magsout deleted the issues/24/1 branch August 25, 2015 07:45
@miketaylr
Copy link
Copy Markdown
Member Author

Thanks for review @magsout and @karlcow! Will deploy at some point today.

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.

3 participants