Merge commit from fork

* Raise (and display) error when icon or preview upload hash is invalid

* Add a couple comments explaining where the hash is coming from

Author: diox

src/olympia/devhub/forms.py

@@ -7,7 +7,7 @@
 
 from django import forms
 from django.conf import settings
-from django.core.validators import MinLengthValidator
+from django.core.validators import MinLengthValidator, RegexValidator
 from django.db.models import Q
 from django.forms.models import BaseModelFormSet, modelformset_factory
 from django.forms.widgets import RadioSelect
@@ -238,7 +238,15 @@ class AddonFormMedia(AddonFormBase):
     icon_type = forms.CharField(
         widget=IconTypeSelect(choices=ICON_TYPES), required=False
     )
-    icon_upload_hash = forms.CharField(required=False)
+    icon_upload_hash = forms.CharField(
+        required=False,
+        validators=[
+            # Should look like an uuid, it's generated by upload_image()
+            RegexValidator(
+                r'^[a-f0-9]{32}$', _('There was an error uploading your image.')
+            )
+        ],
+    )
 
     class Meta:
         model = Addon
@@ -1344,7 +1352,15 @@ class DescribeFormUnlistedContentOptimization(
 class PreviewForm(TranslationFormMixin, AMOModelForm):
     caption = TransField(widget=TransTextarea, required=False)
     file_upload = forms.FileField(required=False)
-    upload_hash = forms.CharField(required=False)
+    upload_hash = forms.CharField(
+        required=False,
+        validators=[
+            # Should look like an uuid, it's generated by upload_image()
+            RegexValidator(
+                r'^[a-f0-9]{32}$', _('There was an error uploading your image.')
+            )
+        ],
+    )
 
     def save(self, addon, commit=True):
         if self.cleaned_data:

src/olympia/devhub/templates/devhub/addons/forms_shared/media.html

@@ -28,6 +28,7 @@
             </label>
             {% endif %}
             {% if editable %}
+              {{ main_form.icon_upload_hash.errors }}
               <div id="icon_preview">
                 <div class="icon_preview" id="icon_preview_32">
                   <img src="{{ addon.get_icon_url(32) }}" alt="">
@@ -76,9 +77,6 @@
                 {% endif %}
                 {{ _('Icons will be resized to 128x128 pixels if larger.') }}
               </div>
-              <ul class="errorlist">
-                <li id="edit-icon-error"></li>
-              </ul>
             {% else %}
               <div id="icon_preview_readonly">
                   <img src="{{ addon.get_icon_url(128) }}" alt="">
@@ -110,6 +108,7 @@
                 {{ preview_form.non_form_errors() }}
                 <div id="file-list">
                   {% for form in preview_form.forms %}
+                    {{ form.errors }}
                     <div class="preview">
                       <span class="handle">&nbsp;</span>
                       {% if form.instance.id %}
@@ -133,7 +132,6 @@
                       <div class="preview_extra">
                         {{ form.upload_hash }}
                       </div>
-                      {{ form.errors }}
                     </div>
                   {% endfor %}
                 </div>

src/olympia/devhub/tests/test_forms.py

@@ -1,6 +1,7 @@
 import os
 import shutil
 import tempfile
+import uuid
 from datetime import datetime, timedelta
 from unittest import mock
 
@@ -531,19 +532,28 @@ def setUp(self):
     def test_preview_modified(self, update_mock):
         addon = Addon.objects.get(pk=3615)
         name = 'transparent.png'
+        upload_hash = uuid.uuid4().hex
         form = forms.PreviewForm(
-            {'caption': 'test', 'upload_hash': name, 'position': 1}
+            {'caption': 'test', 'upload_hash': upload_hash, 'position': 1}
         )
-        with storage.open(os.path.join(self.dest, name), 'wb') as f:
+        with storage.open(os.path.join(self.dest, upload_hash), 'wb') as f:
             shutil.copyfileobj(open(get_image_path(name), 'rb'), f)
         assert form.is_valid()
         form.save(addon)
         assert update_mock.called
 
+    def test_preview_invalid_hash(self):
+        form = forms.PreviewForm(
+            {'caption': 'test', 'upload_hash': 'something.png', 'position': 1}
+        )
+        assert not form.is_valid()
+        assert form.errors == {
+            'upload_hash': ['There was an error uploading your image.']
+        }
+
     def test_caption_too_long(self):
-        name = 'transparent.png'
         form = forms.PreviewForm(
-            {'caption': 'û' * 281, 'upload_hash': name, 'position': 1}
+            {'caption': 'û' * 281, 'upload_hash': uuid.uuid4().hex, 'position': 1}
         )
         assert form.fields['caption'].max_length == 280
         assert form.fields['caption'].widget.attrs['maxlength'] == '280'
@@ -555,11 +565,11 @@ def test_caption_too_long(self):
     def test_preview_transparency(self):
         addon = Addon.objects.get(pk=3615)
         name = 'transparent-cotton'
-        hash = '12345678abcd'
+        upload_hash = uuid.uuid4().hex
         form = forms.PreviewForm(
-            {'caption': 'test', 'upload_hash': hash, 'position': 1}
+            {'caption': 'test', 'upload_hash': upload_hash, 'position': 1}
         )
-        with storage.open(os.path.join(self.dest, hash), 'wb') as f:
+        with storage.open(os.path.join(self.dest, upload_hash), 'wb') as f:
             shutil.copyfileobj(open(get_image_path(name + '.png'), 'rb'), f)
         assert form.is_valid()
         form.save(addon)
@@ -575,10 +585,11 @@ def test_preview_transparency(self):
     def test_preview_size(self, pngcrush_image_mock):
         addon = Addon.objects.get(pk=3615)
         name = 'teamaddons.jpg'
+        upload_hash = uuid.uuid4().hex
         form = forms.PreviewForm(
-            {'caption': 'test', 'upload_hash': name, 'position': 1}
+            {'caption': 'test', 'upload_hash': upload_hash, 'position': 1}
         )
-        with storage.open(os.path.join(self.dest, name), 'wb') as f:
+        with storage.open(os.path.join(self.dest, upload_hash), 'wb') as f:
             shutil.copyfileobj(open(get_image_path(name), 'rb'), f)
         assert form.is_valid()
         form.save(addon)
@@ -1236,17 +1247,30 @@ def test_default_icons(self):
     @mock.patch('olympia.amo.models.ModelBase.update')
     def test_icon_modified(self, update_mock):
         name = 'transparent.png'
+        icon_upload_hash = uuid.uuid4().hex
         form = forms.AddonFormMedia(
-            {'icon_upload_hash': name}, request=self.request, instance=self.addon
+            {'icon_upload_hash': icon_upload_hash},
+            request=self.request,
+            instance=self.addon,
         )
-
-        dest = os.path.join(self.icon_path, name)
+        dest = os.path.join(self.icon_path, icon_upload_hash)
         with storage.open(dest, 'wb') as f:
             shutil.copyfileobj(open(get_image_path(name), 'rb'), f)
         assert form.is_valid()
         form.save(addon=self.addon)
         assert update_mock.called
 
+    def test_icon_invalid_hash(self):
+        form = forms.AddonFormMedia(
+            {'icon_upload_hash': 'some-icon.png'},
+            request=self.request,
+            instance=self.addon,
+        )
+        assert not form.is_valid()
+        assert form.errors == {
+            'icon_upload_hash': ['There was an error uploading your image.']
+        }
+
 
 class TestCategoryForm(TestCase):
     def test_only_one_possible_category_for_dicts(self):

static/js/zamboni/devhub.js

@@ -287,7 +287,7 @@ function addonFormSubmit() {
       $document.scrollTop($document.height() - scrollBottom);
       truncateFields();
       annotateLocalizedErrors(parent_div);
-      if (parent_div.is('#edit-addon-media')) {
+      if (!hasErrors && parent_div.is('#edit-addon-media')) {
         imageStatus.start();
         hideSameSizedIcons();
       }
@@ -1201,6 +1201,9 @@ let imageStatus = {
   },
   newurl: function (orig) {
     let bst = new Date().getTime();
+    if (!orig) {
+      orig = '';
+    }
     orig += (orig.indexOf('?') > 1 ? '&' : '?') + bst;
     return orig;
   },