Merge pull request #1262 from ngokevin/legacytheme

fix reuploading of legacy getpersonas themes (bug 921214)

apps/addons/models.py

@@ -1787,6 +1787,14 @@ class Persona(caching.CachingMixin, models.Model):
         else:
             return self._image_url('preview.jpg')
 
+    @amo.cached_property
+    def thumb_path(self):
+        """Path to Persona's thumbnail preview."""
+        if self.is_new():
+            return self._image_path('preview.png')
+        else:
+            return self._image_path('preview.jpg')
+
     @amo.cached_property
     def icon_url(self):
         """URL to personas square preview."""
@@ -1795,6 +1803,14 @@ class Persona(caching.CachingMixin, models.Model):
         else:
             return self._image_url('preview_small.jpg')
 
+    @amo.cached_property
+    def icon_path(self):
+        """Path to personas square preview."""
+        if self.is_new():
+            return self._image_path('icon.png')
+        else:
+            return self._image_path('preview_small.jpg')
+
     @amo.cached_property
     def preview_url(self):
         """URL to Persona's big, 680px, preview."""
@@ -1803,6 +1819,14 @@ class Persona(caching.CachingMixin, models.Model):
         else:
             return self._image_url('preview_large.jpg')
 
+    @amo.cached_property
+    def preview_path(self):
+        """Path to Persona's big, 680px, preview."""
+        if self.is_new():
+            return self._image_path('preview.png')
+        else:
+            return self._image_path('preview_large.jpg')
+
     @amo.cached_property
     def header_url(self):
         return self._image_url(self.header)

apps/addons/tasks.py

@@ -335,8 +335,8 @@ def save_theme_reupload(header, footer, addon, **kw):
 
     if header_dst or footer_dst:
         theme = addon.persona
-        header = 'pending_header.png' if header_dst else 'header.png'
-        footer = 'pending_footer.png' if footer_dst else 'footer.png'
+        header = 'pending_header.png' if header_dst else theme.header
+        footer = 'pending_footer.png' if footer_dst else theme.footer
 
         # Store pending header and/or footer file paths for review.
         RereviewQueueTheme.objects.filter(theme=theme).delete()

apps/devhub/tests/test_forms.py

@@ -396,7 +396,7 @@ class TestEditThemeForm(amo.tests.TestCase):
             type=amo.ADDON_PERSONA, name='xxxx')
         self.instance.addoncategory_set.create(category=self.cat)
         self.license = amo.LICENSE_CC_BY.id
-        Persona.objects.create(
+        self.theme = Persona.objects.create(
             persona_id=0, addon_id=self.instance.id, license=self.license,
             accentcolor='C0FFEE', textcolor='EFFFFF')
         Tag(tag_text='sw').save_tag(self.instance)
@@ -522,12 +522,10 @@ class TestEditThemeForm(amo.tests.TestCase):
         eq_(rqt[0].footer, 'pending_footer.png')
         assert not rqt[0].dupe_persona
 
+    @mock.patch('addons.tasks.create_persona_preview_images', new=mock.Mock)
+    @mock.patch('addons.tasks.save_persona_image', new=mock.Mock)
     @mock.patch('addons.tasks.make_checksum')
-    @mock.patch('addons.tasks.create_persona_preview_images')
-    @mock.patch('addons.tasks.save_persona_image')
-    def test_reupload_duplicate(self, save_persona_image_mock,
-                                create_persona_preview_images_mock,
-                                make_checksum_mock):
+    def test_reupload_duplicate(self, make_checksum_mock):
         make_checksum_mock.return_value = 'checksumbeforeyouwrecksome'
 
         theme = amo.tests.addon_factory(type=amo.ADDON_PERSONA)
@@ -543,6 +541,34 @@ class TestEditThemeForm(amo.tests.TestCase):
         rqt = RereviewQueueTheme.objects.get(theme=self.instance.persona)
         eq_(rqt.dupe_persona, theme.persona)
 
+    @mock.patch('addons.tasks.make_checksum', new=mock.Mock)
+    @mock.patch('addons.tasks.create_persona_preview_images', new=mock.Mock)
+    @mock.patch('addons.tasks.save_persona_image', new=mock.Mock)
+    def test_reupload_legacy_header_only(self):
+        """
+        STR the bug this test fixes:
+
+        - Reupload a legacy theme (/w footer == leg.png) legacy, header only.
+        - The header would get saved as 'pending_header.png'.
+        - The footer would get saved as 'footer.png'.
+        - On approving, it would see 'footer.png' !== 'leg.png'
+        - It run move_stored_file('footer.png', 'leg.png').
+        - But footer.png does not exist. BAM BUG.
+        """
+        self.theme.header = 'Legacy-header3H.png'
+        self.theme.footer = 'Legacy-footer3H-Copy.jpg'
+        self.theme.save()
+
+        data = self.get_dict(header_hash='arthro')
+        self.form = EditThemeForm(data, request=self.request,
+                                  instance=self.instance)
+        eq_(self.form.is_valid(), True)
+        self.form.save()
+
+        rqt = RereviewQueueTheme.objects.get()
+        eq_(rqt.header, 'pending_header.png')
+        eq_(rqt.footer, 'Legacy-footer3H-Copy.jpg')
+
 
 class TestEditThemeOwnerForm(amo.tests.TestCase):
     fixtures = ['base/apps', 'base/users']

mkt/reviewers/tasks.py

@@ -76,6 +76,13 @@ def approve_rereview(theme):
     reupload = rereview[0]
 
     if reupload.header_path != reupload.theme.header_path:
+        create_persona_preview_images(
+            src=reupload.header_path,
+            full_dst=[
+                reupload.theme.preview_path,
+                reupload.theme.icon_path],
+            set_modified_on=[reupload.theme.addon])
+
         move_stored_file(
             reupload.header_path, reupload.theme.header_path,
             storage=storage)
@@ -83,12 +90,6 @@ def approve_rereview(theme):
         move_stored_file(
             reupload.footer_path, reupload.theme.footer_path,
             storage=storage)
-    create_persona_preview_images(
-        src=reupload.theme.header_path,
-        full_dst=[
-            reupload.theme.header_path.replace('header', 'preview'),
-            reupload.theme.header_path.replace('header', 'icon')],
-        set_modified_on=[reupload.theme.addon])
     rereview.delete()
 
     theme.addon.increment_version()

mkt/reviewers/tests/test_views_themes.py

@@ -107,7 +107,7 @@ class ThemeReviewTestMixin(object):
     @mock.patch('mkt.reviewers.tasks.send_mail_jinja')
     @mock.patch('mkt.reviewers.tasks.create_persona_preview_images')
     @mock.patch('amo.storage_utils.copy_stored_file')
-    def test_commit(self, copy_file_mock, create_preview_mock,
+    def test_commit(self, copy_mock, create_preview_mock,
                     send_mail_jinja_mock, version_changed_mock,
                     approve_rereview_mock, reject_rereview_mock):
         if self.flagged:
@@ -160,10 +160,10 @@ class ThemeReviewTestMixin(object):
                 eq_(themes[i].header, 'header')
                 eq_(themes[i].footer, 'footer')
 
-            assert '/pending_header' in copy_file_mock.call_args_list[0][0][0]
-            assert '/header' in copy_file_mock.call_args_list[0][0][1]
-            assert '/pending_footer' in copy_file_mock.call_args_list[1][0][0]
-            assert '/footer' in copy_file_mock.call_args_list[1][0][1]
+            assert copy_mock.call_args_list[0][0][0].endswith('pending_header')
+            assert copy_mock.call_args_list[0][0][1].endswith('header')
+            assert copy_mock.call_args_list[1][0][0].endswith('pending_footer')
+            assert copy_mock.call_args_list[1][0][1].endswith('footer')
 
             create_preview_args = create_preview_mock.call_args_list[0][1]
             assert '/header' in create_preview_args['src']
@@ -536,6 +536,66 @@ class TestThemeQueueRereview(ThemeReviewTestMixin, amo.tests.TestCase):
         eq_(doc('.theme').length, 1)
         eq_(RereviewQueueTheme.with_deleted.count(), 1)
 
+    @mock.patch.object(settings, 'LOCAL_MIRROR_URL', '')
+    @mock.patch('mkt.reviewers.tasks.send_mail_jinja')
+    @mock.patch('mkt.reviewers.tasks.create_persona_preview_images')
+    @mock.patch('amo.storage_utils.copy_stored_file')
+    def test_update_legacy_theme(self, copy_mock, prev_mock, noop3):
+        """
+        Test updating themes that were submitted from GetPersonas.
+        STR the bug this test fixes:
+
+        - Reupload a legacy theme and approve it.
+        - On approving, it would make a preview image with the destination as
+         'preview.png' and 'icon.png', but legacy themes use
+         'preview_large.jpg' and 'preview_small.png'.
+        - Thus the preview images were not being updated, but the header/footer
+          images were.
+        """
+        theme = self.theme_factory(status=amo.STATUS_PUBLIC).persona
+        theme.header = 'Legacy-header3H.png'
+        theme.footer = 'Legacy-footer3H-Copy.jpg'
+        theme.persona_id = 5
+        theme.save()
+        form_data = amo.tests.formset(initial_count=5, total_count=6)
+
+        RereviewQueueTheme.objects.create(
+            theme=theme, header='pending_header.png',
+            footer='pending_footer.png')
+
+        # Create lock.
+        reviewer = self.create_and_become_reviewer()
+        ThemeLock.objects.create(
+            theme=theme, reviewer=reviewer, expiry=self.days_ago(-1))
+        form_data['form-0-theme'] = str(theme.id)
+
+        # Build formset.
+        form_data['form-0-action'] = str(rvw.ACTION_APPROVE)
+
+        # Commit.
+        self.client.post(reverse('reviewers.themes.commit'), form_data)
+
+        # Check nothing has changed.
+        eq_(theme.header, 'Legacy-header3H.png')
+        eq_(theme.footer, 'Legacy-footer3H-Copy.jpg')
+        theme.thumb_path.endswith('preview.jpg')
+        theme.icon_path.endswith('preview_small.jpg')
+        theme.preview_path.endswith('preview_large.jpg')
+
+        # Test calling create_persona_preview_images.
+        eq_(prev_mock.call_args_list[0][1]['full_dst'][0], theme.preview_path)
+        eq_(prev_mock.call_args_list[0][1]['full_dst'][1], theme.icon_path)
+
+        # pending_header should be mv'ed to Legacy-header3H.png.
+        assert copy_mock.call_args_list[0][0][0].endswith('pending_header')
+        assert (copy_mock.call_args_list[0][0][1]
+                .endswith('Legacy-header3H.png'))
+        # pending_footer should be mv'ed to Legacy-footer-Copy3H.png.
+        assert (copy_mock.call_args_list[1][0][0]
+                .endswith('pending_footer'))
+        assert (copy_mock.call_args_list[1][0][1]
+                .endswith('Legacy-footer3H-Copy.jpg'))
+
 
 class TestDeletedThemeLookup(amo.tests.TestCase):
     fixtures = fixture('group_admin', 'user_admin', 'user_admin_group',