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

[bug] Upload images is broken #1063

Closed
karlcow opened this issue May 24, 2016 · 6 comments
Closed

[bug] Upload images is broken #1063

karlcow opened this issue May 24, 2016 · 6 comments
Assignees

Comments

@karlcow
Copy link
Member

karlcow commented May 24, 2016

Try to upload an image in the comments of a bug. For example

POST /upload/ HTTP/1.1
Host: webcompat.com
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0
Accept: */*
Accept-Language: en,fr;q=0.7,en-US;q=0.3
Accept-Encoding: gzip, deflate, br
DNT: 1
Referer: https://webcompat.com/issues/1930
x-requested-with: XMLHttpRequest
Content-Length: 63126
Content-Type: multipart/form-data; boundary=---------------------------1931572924435841969129253066
Cookie: session=.…cut…
Connection: keep-alive

And receiving from the server.

HTTP/1.1 501 NOT IMPLEMENTED
Server: nginx/1.1.19
Date: Tue, 24 May 2016 06:04:49 GMT
Content-Type: text/html
Content-Length: 188
Connection: keep-alive
Set-Cookie: session=… cut…

with a flash message.

Ping @miketaylr :)

@karlcow
Copy link
Member Author

karlcow commented May 24, 2016

Did we forget to fix the way the upload in the comments?
So we go into 501.
https://github.com/webcompat/webcompat.com/blob/master/webcompat/api/uploads.py#L109-L113

<div class="wc-Form-group js-ImageUploadView">
        <form>
          <label class="wc-UploadForm wc-UploadForm--new" for="image">
            <div class="wc-UploadForm-wrapper">
              <label class="wc-UploadForm-label wc-UploadForm-label--uploadForm">Attach images by <span class="wc-UploadForm-label--link">selecting them</span></label>
              <input accept=".jpe,.jpg,.jpeg,.gif,.png,.bmp" class="wc-ButtonUpload js-buttonUpload" id="image" name="image" type="file">
            </div>


      </label></form></div>

Should we get a real form too ;) aka action="/upload/" method="post"

  • validateAndUpload: function(e) {
    if (this.checkImageTypeValidity(e.target)) {
    // The assumption here is that FormData is supported, otherwise
    // the upload view is not shown to the user.
    var formdata = new FormData($('form').get(0));
    this._loaderImage.show();
    $.ajax({
    // File upload will fail if we pass contentType: multipart/form-data
    // to jQuery (because it won't have the boundary string and then all
    // hell breaks loose and you're like 10 stackoverflow posts deep).
    contentType: false,
    processData: false,
    data: formdata,
    method: 'POST',
    url: '/upload/',
    success: _.bind(function(response) {
    this.addImageUploadComment(response);
    this._loaderImage.hide();
    }, this),
    error: function() {
    var msg = 'There was an error trying to upload the image.';
    wcEvents.trigger('flash:error', {message: msg, timeout: 4000});
    }
    });
    }

Do we have twice the feature at two different places?

@miketaylr
Copy link
Member

Yeah, I broke this. 💣
Hoping to get it fixed today!

@miketaylr miketaylr self-assigned this May 25, 2016
@miketaylr
Copy link
Member

Do we have twice the feature at two different places?

Yeah, sort of. We have the homepage image upload stuff, which works with postMessage'd base64 dataURIs (which come from add-ons) and regular file inputs and then issue page, which just has file inputs.

When I "fixed" the homepage, I made the assumption that all image uploads would be POST's sending base64 data... forgetting about the issue page.

@miketaylr
Copy link
Member

It would be nice to abstract out the common upload methods into a mixin, maybe better as a refactoring bug once I fix this.

@miketaylr
Copy link
Member

miketaylr commented May 26, 2016

  • swap out old spinner with new small one
  • use filereader to convert file to base64
  • make sure backend and frontend expect the same things
  • bonus points for tests

@miketaylr
Copy link
Member

Rather than convert the image to base64 on the client (which is what we do on the homepage to show the preview), then convert back to image on the server, let's just re-enable file uploads on the server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants