diff --git a/apps/addons/models.py b/apps/addons/models.py index 2f50299da5..3d18d0a9c3 100644 --- a/apps/addons/models.py +++ b/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) diff --git a/apps/addons/tasks.py b/apps/addons/tasks.py index 5f32201a47..8c8ed27eb9 100644 --- a/apps/addons/tasks.py +++ b/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() diff --git a/apps/devhub/tests/test_forms.py b/apps/devhub/tests/test_forms.py index 083d04db1b..4e1f1fad5a 100644 --- a/apps/devhub/tests/test_forms.py +++ b/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'] diff --git a/mkt/reviewers/tasks.py b/mkt/reviewers/tasks.py index 6acedcb013..9aef0f95af 100644 --- a/mkt/reviewers/tasks.py +++ b/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() diff --git a/mkt/reviewers/tests/test_views_themes.py b/mkt/reviewers/tests/test_views_themes.py index 39bc18e682..7d145d8e57 100644 --- a/mkt/reviewers/tests/test_views_themes.py +++ b/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',