Fixes #1049. Upload images before form submission.

tests/test_uploads.py

@@ -79,15 +79,20 @@ def testGet(self):
         rv = self.test_client.get('/upload/')
         self.assertEqual(rv.status_code, 404)
 
-    def testBadFileUploads(self):
+    def testRegularUploads(self):
         # Loop over some files and the status codes that we are expecting
+        # Basically it should never be possible to upload a "regular" file.
         for filename, status_code in (
-                ('evil.py', 415),
-                ('evil', 415),
-                ('evil.png', 415),
-                ('green_square.webp', 415)):
-
-            # The reason why we are definisng it in here and not outside
+                ('evil.py', 501),
+                ('evil', 501),
+                ('evil.png', 501),
+                ('green_square.webp', 501),
+                ('green_square.png', 501),
+                ('green_square.jpg', 501),
+                ('green_square.gif', 501),
+                ('green_square.bmp', 501)):
+
+            # The reason why we are defining it in here and not outside
             # this method is that we are setting the filename of the
             # TestingFileStorage to be the one in the for loop. This way
             # we can ensure that the filename that we are "uploading"
@@ -110,32 +115,6 @@ def files(self):
             rv = test_client.post('/upload/', data=dict())
             self.assertEqual(rv.status_code, status_code)
 
-    def testGoodFileUploads(self):
-        # Loop over some files and the URLs that we are expecting back
-        for filename, status_code in (
-                ('green_square.png', 201),
-                ('green_square.jpg', 201),
-                ('green_square.gif', 201),
-                ('green_square.bmp', 201)):
-
-            class TestingRequest(Request):
-                """A testing request to use that will return a
-                TestingFileStorage to test the uploading."""
-                @property
-                def files(self):
-                    d = MultiDict()
-                    f = open(os.path.join('tests', 'fixtures', filename), 'r')
-                    d['image'] = TestingFileStorage(stream=StringIO(f.read()),
-                                                    filename=filename)
-                    f.close()
-                    return d
-
-            self.app.request_class = TestingRequest
-            test_client = self.app.test_client()
-
-            rv = test_client.post('/upload/', data=dict())
-            self.assertEqual(rv.status_code, status_code)
-
     def testBase64ScreenshotUploads(self):
         BASE64_PNG = u'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAAAAAA6fptVAAAACklEQVQYV2P4DwABAQEAWk1v8QAAAABJRU5ErkJggg=='  # nopep8
         BASE64_PNG_GARBAGE = u'data:image/png;base64,garbage!'
@@ -155,7 +134,7 @@ class TestingRequest(Request):
                 @property
                 def form(self):
                     d = MultiDict()
-                    d['screenshot'] = filedata
+                    d['image'] = filedata
                     return d
 
             self.app.request_class = TestingRequest

webcompat/__init__.py

@@ -15,8 +15,10 @@
 app = Flask(__name__, static_url_path='')
 app.config.from_object('config')
 
-# set limit of 4MB for file uploads
-app.config['MAX_CONTENT_LENGTH'] = 4 * 1024 * 1024
+# set limit of 5.5MB for file uploads
+# in practice, this is ~4MB (5.5 / 1.37)
+# after the data URI is saved to disk
+app.config['MAX_CONTENT_LENGTH'] = 5.5 * 1024 * 1024
 
 github = GitHub(app)
 limiter = Limiter(app)

webcompat/api/uploads.py

@@ -17,7 +17,6 @@
 from flask import request
 from io import BytesIO
 from PIL import Image
-from werkzeug.datastructures import FileStorage
 from werkzeug.exceptions import RequestEntityTooLarge
 from uuid import uuid4
 
@@ -44,10 +43,7 @@ def __init__(self, imagedata):
     def to_image_object(self, imagedata):
         '''Method to return a Pillow Image object from the raw imagedata.'''
         try:
-            # Is this a file uploaded via <input type=file>?
-            if isinstance(imagedata, FileStorage):
-                return Image.open(imagedata)
-            # Is this base64 encoded screenshot?
+            # Make sure we're being sent a base64 encoded image
             if (isinstance(imagedata, unicode) and
                     imagedata.startswith('data:image/')):
                 # Chop off 'data:image/.+;base64,' before decoding
@@ -59,7 +55,12 @@ def to_image_object(self, imagedata):
             abort(415)
 
     def get_file_ext(self):
-        '''Method to return the file extension, as determined by Pillow.'''
+        '''Method to return the file extension, as determined by Pillow.
+
+        (But, we return jpg for png images, since we convert them always.)
+        '''
+        if self.image_object.format.lower() == 'png':
+            return 'jpg'
         return self.image_object.format.lower()
 
     def get_filename(self):
@@ -105,10 +106,8 @@ def upload():
 
     Returns a JSON string that contains the filename and url.
     '''
-    if 'image' in request.files and request.files['image'].filename:
-        imagedata = request.files['image']
-    elif 'screenshot' in request.form:
-        imagedata = request.form['screenshot']
+    if 'image' in request.form:
+        imagedata = request.form['image']
     else:
         # We don't know what you're trying to do, but it ain't gonna work.
         abort(501)

webcompat/static/css/development/components/loader.css

@@ -15,3 +15,15 @@
     display:block;
     background-color:rgba(255,255,255,.8);
   }
+
+.wc-Upload-Loader {
+  display:none;
+  position:absolute;
+  right:4%;top: 27%;
+  width: 24px;
+  height: 24px;
+}
+
+.wc-Upload-Loader.is-active {
+  display: block;
+}

webcompat/static/js/lib/bugform.js

@@ -12,6 +12,7 @@ function BugForm() {
   this.loadingIndicator = $('.js-Loader');
   this.reportButton = $('#js-ReportBug');
   this.loaderImage = $('.js-Loader');
+  this.uploadLoader = $('.js-Upload-Loader');
   this.screenshotData = '';
   // by default, submission type is anonymous
   this.submitType = 'github-proxy-report';
@@ -65,76 +66,54 @@ function BugForm() {
 
     // Set up listener for message events from screenshot-enabled add-ons
     window.addEventListener('message', _.bind(function(event) {
-      var img = document.createElement('img');
-      var canvas = document.createElement('canvas');
-      var ctx = canvas.getContext('2d');
       // Make sure the data is coming from ~*inside the house*~!
       // (i.e., our add-on sent it)
       if (location.origin === event.origin) {
-        this.form.on('submit', _.bind(this.submitFormWithScreenshot, this));
         this.screenshotData = event.data;
 
         // The final size of Base64-encoded binary data is ~equal to
         // 1.37 times the original data size + 814 bytes (for headers).
         // so, bytes = (encoded_string.length - 814) / 1.37)
         // see https://en.wikipedia.org/wiki/Base64#MIME
         if ((String(this.screenshotData).length - 814 / 1.37) > this.UPLOAD_LIMIT) {
-          img.onload = _.bind(function() {
-            // scale the tmp canvas to 50%
-            canvas.width = Math.floor(img.width / 2);
-            canvas.height = Math.floor(img.height / 2);
-            ctx.scale(0.5, 0.5);
-            // draw back in the screenshot (at 50% scale)
-            // and re-serialize to data URI
-            ctx.drawImage(img, 0, 0);
-            this.screenshotData = canvas.toDataURL();
-            img = null, canvas = null;
-
-            this.addPreviewBackground(this.screenshotData);
-          }, this);
-
-          img.src = this.screenshotData;
+          this.downsampleImageAndUpload(this.screenshotData);
         } else {
-          this.addPreviewBackground(this.screenshotData);
+          this.addPreviewBackgroundAndUpload(this.screenshotData);
         }
       }
     }, this), false);
   };
 
-  // If we've gotten this far, form validation has happened and
-  // we're reacting to a submit event.
-  this.submitFormWithScreenshot = function(event) {
-    // Stop regular form submission
-    event.preventDefault();
-    // construct a FormData object and append the base64 image to it.
-    var formdata = new FormData(this.form.get(0));
-    // we call this 'screenshot', rather than 'image' because it needs to be
-    // handled differently on the backend.
-    formdata.append('screenshot', this.screenshotData);
-    // add in the submit-type, which won't be included in JS form submission by default
-    formdata.append('submit-type', this.submitType);
-
-    this.loaderImage.show();
-    $.ajax({
-      contentType: false,
-      processData: false,
-      data: formdata,
-      method: 'POST',
-      url: '/issues/new',
-      success: _.bind(function(response) {
-        // navigate to thanks page.
-        this.loaderImage.hide();
-        window.location.href = '/thanks/' + response.number;
-      }, this),
-      error: _.bind(function(response) {
-        var msg = 'There was an error trying to file the bug, try again?.';
-        if (response && response.status === 413) {
-          msg = 'The image is too big! Please choose something smaller than 4MB.';
-        }
-        wcEvents.trigger('flash:error', {message: msg, timeout: 5000});
-        this.loaderImage.hide();
-      }, this)
-    });
+  this.downsampleImageAndUpload = function(dataURI) {
+    var img = document.createElement('img');
+    var canvas = document.createElement('canvas');
+    var ctx = canvas.getContext('2d');
+
+    img.onload = _.bind(function() {
+      // scale the tmp canvas to 50%
+      canvas.width = Math.floor(img.width / 2);
+      canvas.height = Math.floor(img.height / 2);
+      ctx.scale(0.5, 0.5);
+      // draw back in the screenshot (at 50% scale)
+      // and re-serialize to data URI
+      ctx.drawImage(img, 0, 0);
+      // Note: this will convert GIFs to JPEG, which breaks
+      // animated GIFs. However, this only will happen if they
+      // were above the upload limit size. So... sorry?
+      this.screenshotData = canvas.toDataURL('image/jpeg', 0.8);
+
+      // The limit is 4MB (which is crazy big!), so let the user know if their
+      // file is unreasonably large at this point (after 1 round of downsampling)
+      if (this.screenshotData > this.UPLOAD_LIMIT) {
+        this.makeInvalid('img_too_big');
+        return;
+      }
+
+      this.addPreviewBackgroundAndUpload(this.screenshotData);
+      img = null, canvas = null;
+    }, this);
+
+    img.src = dataURI;
   };
 
   this.checkParams = function() {
@@ -298,14 +277,6 @@ function BugForm() {
     // We can just grab the 0th one, because we only allow uploading
     // a single image at a time (for now)
     var img = event.target.files[0];
-    // The limit is 4MB (which is crazy big!), so let the user know if their
-    // file is unreasonably large.
-    if (img.size > this.UPLOAD_LIMIT) {
-      this.makeInvalid('img_too_big');
-      return;
-    } else if (img.size < this.UPLOAD_LIMIT) {
-      this.makeValid('img_too_big');
-    }
 
     // One last image type validation check.
     if (!img.type.match('image.*')) {
@@ -315,12 +286,17 @@ function BugForm() {
 
     var reader = new FileReader();
     reader.onload = _.bind(function(event) {
-      this.addPreviewBackground(event.target.result);
+      var dataURI = event.target.result;
+      if ((String(dataURI).length - 814 / 1.37) > this.UPLOAD_LIMIT) {
+        this.downsampleImageAndUpload(dataURI);
+      } else {
+        this.addPreviewBackgroundAndUpload(dataURI);
+      }
     }, this);
     reader.readAsDataURL(img);
   };
 
-  this.addPreviewBackground = function(dataURI) {
+  this.addPreviewBackgroundAndUpload = function(dataURI) {
     if (!_.startsWith(dataURI, 'data:image/')) {
       return;
     }
@@ -332,6 +308,7 @@ function BugForm() {
     });
 
     this.showRemoveUpload(label);
+    this.getUploadURL(dataURI);
   };
   /*
     Allow users to remove an image from the form upload.
@@ -347,16 +324,69 @@ function BugForm() {
     removeBanner.removeClass('is-hidden');
     uploadWrapper.addClass('is-hidden');
     removeBanner.on('click', _.bind(function() {
-      // clear out the input value or screenshot data
-      this.uploadField.val(this.uploadField.get(0).defaultValue);
-      this.screenshotData = '';
       // remove the preview and hide the banner
       label.css('background', 'none');
       removeBanner.addClass('is-hidden');
       uploadWrapper.removeClass('is-hidden');
       removeBanner.off('click');
+
+      // remove the last embedded image URL
+      // Note: this could fail in weird ways depending on how
+      // the user has edited the descField.
+      this.descField.val(function(idx, value) {
+        return value.replace(/!\[.+\.jpe*g\)$/, '');
+      });
     }, this));
   };
+  /*
+     Upload the image before form submission so we can
+     put an image link in the bug description.
+  */
+  this.getUploadURL = function(dataURI) {
+    this.disableSubmits();
+    this.uploadLoader.addClass('is-active');
+    var formdata = new FormData();
+    formdata.append('image', dataURI);
+
+    $.ajax({
+      contentType: false,
+      processData: false,
+      data: formdata,
+      method: 'POST',
+      url: '/upload/',
+      success: _.bind(function(response) {
+        this.addImageURL(response.url);
+        this.uploadLoader.removeClass('is-active');
+        this.enableSubmits();
+      }, this),
+      error: _.bind(function(response) {
+        var msg;
+        if (response && response.status === 415) {
+          wcEvents.trigger('flash:error',
+            {message: this.inputMap.image.helpText, timeout: 5000});
+        }
+
+        if (response && response.status === 413) {
+          msg = 'The image is too big! Please choose something smaller than 4MB.';
+          wcEvents.trigger('flash:error', {message: msg, timeout: 5000});
+        }
+        this.loaderImage.hide();
+      }, this)
+    });
+
+    // clear out the input[type=file], because we don't need it anymore.
+    this.uploadField.val(this.uploadField.get(0).defaultValue);
+  };
+  /*
+    copy over the URL of a newly uploaded image asset to the bug
+    description textarea.
+  */
+  this.addImageURL = function(url) {
+    var imageURL = ['![Screenshot Description](', url, ')'].join('');
+    this.descField.val(function(idx, value) {
+      return value + '\n\n' + imageURL;
+    });
+  };
   /*
      copy URL from urlField into the first line of the
      description field. early return if the user has deleted
@@ -371,7 +401,6 @@ function BugForm() {
       }
       return value.replace(firstLine, prefix + this.urlField.val() + '\n');
     }, this));
-
   };
 
   return this.init();