Merge pull request #1812 from ngokevin/commbadger4

create comm attachments from reviewer tools on reviews (bug 977296)

mkt/comm/tests/test_api.py

@@ -59,6 +59,14 @@ def _note_factory(self, thread, perms=None, no_perms=None, **kw):
 
 class AttachmentManagementMixin(object):
 
+    def _attachment_management_form(self, num=1):
+        """
+        Generate and return data for a management form for `num` attachments
+        """
+        return {'form-TOTAL_FORMS': max(1, num),
+                'form-INITIAL_FORMS': 0,
+                'form-MAX_NUM_FORMS': 1000}
+
     def _attachments(self, num):
         """Generate and return data for `num` attachments """
         data = {}

mkt/comm/tests/test_utils_.py

@@ -1,6 +1,7 @@
 import os.path
 
 from django.conf import settings
+from django.core.files.uploadedfile import SimpleUploadedFile
 
 import mock
 from nose.tools import eq_
@@ -9,7 +10,9 @@
 from amo.tests import app_factory, TestCase, user_factory
 from users.models import UserProfile
 
+from mkt.comm.forms import CommAttachmentFormSet
 from mkt.comm.models import CommunicationThread, CommunicationThreadToken
+from mkt.comm.tests.test_api import AttachmentManagementMixin
 from mkt.comm.utils import (CommEmailParser, create_comm_note,
                             save_from_email_reply)
 from mkt.constants import comm
@@ -73,7 +76,7 @@ def test_body(self):
         eq_(self.parser.get_body(), 'test note 5\n')
 
 
-class TestCreateCommNote(TestCase):
+class TestCreateCommNote(TestCase, AttachmentManagementMixin):
 
     def setUp(self):
         self.create_switch('comm-dashboard')
@@ -147,3 +150,22 @@ def test_custom_perms(self):
             'senior_reviewer': True, 'mozilla_contact': True, 'staff': True}
         for perm, has_perm in expected.items():
             eq_(getattr(thread, 'read_permission_%s' % perm), has_perm, perm)
+
+    @mock.patch('mkt.comm.utils.post_create_comm_note', new=mock.Mock)
+    def test_attachments(self):
+        attach_formdata = self._attachment_management_form(num=2)
+        attach_formdata.update(self._attachments(num=2))
+        attach_formset = CommAttachmentFormSet(
+            attach_formdata,
+            {'form-0-attachment':
+                SimpleUploadedFile(
+                    'lol', attach_formdata['form-0-attachment'].read()),
+             'form-1-attachment':
+                SimpleUploadedFile(
+                    'lol2', attach_formdata['form-1-attachment'].read())})
+
+        thread, note = create_comm_note(
+            self.app, self.app.current_version, self.user, 'lol',
+            note_type=comm.APPROVAL, attachments=attach_formset)
+
+        eq_(note.attachments.count(), 2)

mkt/comm/utils.py

@@ -13,8 +13,8 @@
 from access.models import Group
 from users.models import UserProfile
 
-from mkt.comm.models import (CommunicationNoteRead, CommunicationThreadToken,
-                             user_has_perm_thread)
+from mkt.comm.models import (CommunicationNote, CommunicationNoteRead,
+                             CommunicationThreadToken, user_has_perm_thread)
 from mkt.constants import comm
 
 
@@ -198,7 +198,7 @@ def send_mail_comm(note):
 
 
 def create_comm_note(app, version, author, body, note_type=comm.NO_ACTION,
-                     perms=None, no_switch=False):
+                     perms=None, no_switch=False, attachments=None):
     """
     Creates a note on an app version's thread.
     Creates a thread if a thread doesn't already exist.
@@ -212,6 +212,9 @@ def create_comm_note(app, version, author, body, note_type=comm.NO_ACTION,
                  (e.g. comm.APPROVAL, comm.REJECTION, comm.NO_ACTION).
     perms -- object of groups to grant permission to, will set flags on Thread.
              (e.g. {'developer': False, 'staff': True}).
+    no_switch -- whether to ignore comm switch, needed after we migrate
+                 reviewer tools from ActivityLog to notes.
+    attachments -- formset of attachment files
 
     """
     if not no_switch and not waffle.switch_is_active('comm-dashboard'):
@@ -233,6 +236,9 @@ def create_comm_note(app, version, author, body, note_type=comm.NO_ACTION,
     note = thread.notes.create(
         note_type=note_type, body=body, author=author, **create_perms)
 
+    if attachments:
+        create_attachments(note, attachments)
+
     post_create_comm_note(note)
 
     return thread, note
@@ -277,6 +283,9 @@ def create_attachments(note, formset):
             continue
 
         data = form.cleaned_data
+        if not data:
+            continue
+
         attachment = data['attachment']
         attachment_name = _save_attachment(
             storage, attachment,

mkt/reviewers/tests/test_views.py

@@ -2056,19 +2056,10 @@ def test_no_attachments(self, save_mock):
     @override_settings(REVIEWER_ATTACHMENTS_PATH=ATTACHMENTS_DIR)
     @mock.patch('amo.utils.LocalFileStorage.save')
     def test_attachment(self, save_mock):
-        """ Test addition of 1 attachment """
+        """ Test addition of an attachment """
         self._attachment_post(1)
-        eq_(save_mock.call_args_list,
-            [mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.txt'), mock.ANY)])
-
-    @override_settings(REVIEWER_ATTACHMENTS_PATH=ATTACHMENTS_DIR)
-    @mock.patch('amo.utils.LocalFileStorage.save')
-    def test_multiple_attachments(self, save_mock):
-        """ Test addition of multiple attachments """
-        self._attachment_post(2)
-        eq_(save_mock.call_args_list,
-            [mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.txt'), mock.ANY),
-             mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.jpg'), mock.ANY)])
+        eq_(save_mock.call_args_list[0],
+            mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.txt'), mock.ANY))
 
     @override_settings(REVIEWER_ATTACHMENTS_PATH=ATTACHMENTS_DIR)
     @mock.patch('amo.utils.LocalFileStorage.save')
@@ -2082,8 +2073,8 @@ def test_attachment_email(self, save_mock):
             'Review attachment not added to email')
         for attachment in mail.outbox[0].attachments:
             self.assertNotEqual(len(attachment), 0, '0-length attachment')
-        eq_(save_mock.call_args_list,
-            [mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.txt'), mock.ANY)])
+        eq_(save_mock.call_args_list[0],
+            mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.txt'), mock.ANY))
 
     @override_settings(REVIEWER_ATTACHMENTS_PATH=ATTACHMENTS_DIR)
     @mock.patch('amo.utils.LocalFileStorage.save')
@@ -2095,9 +2086,8 @@ def test_attachment_email_multiple(self, save_mock):
         self.post(self._attachment_form_data(num=2, action='reject'))
         eq_(len(mail.outbox[0].attachments), 2,
             'Review attachments not added to email')
-        eq_(save_mock.call_args_list,
-            [mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.txt'), mock.ANY),
-             mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.jpg'), mock.ANY)])
+        eq_(save_mock.call_args_list[0],
+            mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.txt'), mock.ANY))
 
     @override_settings(REVIEWER_ATTACHMENTS_PATH=ATTACHMENTS_DIR)
     @mock.patch('amo.utils.LocalFileStorage.save')
@@ -2110,8 +2100,8 @@ def test_attachment_email_escalate(self, save_mock):
         self.post(self._attachment_form_data(num=1, action='escalate'))
         eq_(len(mail.outbox[0].attachments), 1,
             'Review attachment not added to email')
-        eq_(save_mock.call_args_list,
-            [mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.txt'), mock.ANY)])
+        eq_(save_mock.call_args_list[0],
+            mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.txt'), mock.ANY))
 
     @override_settings(REVIEWER_ATTACHMENTS_PATH=ATTACHMENTS_DIR)
     @mock.patch('amo.utils.LocalFileStorage.save')
@@ -2123,8 +2113,8 @@ def test_attachment_email_requestinfo(self, save_mock):
         self.post(self._attachment_form_data(num=1, action='info'))
         eq_(len(mail.outbox[0].attachments), 1,
             'Review attachment not added to email')
-        eq_(save_mock.call_args_list,
-            [mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.txt'), mock.ANY)])
+        eq_(save_mock.call_args_list[0],
+            mock.call(os.path.join(ATTACHMENTS_DIR, 'bacon.txt'), mock.ANY))
 
     def test_idn_app_domain(self):
         response = self.client.get(self.url)
@@ -2583,6 +2573,16 @@ def test_disable(self):
         # Test emails.
         self._check_email_dev_and_contact()
 
+    def test_attachments(self):
+        data = {'action': 'comment', 'comments': 'huh'}
+        data.update(self._attachment_management_form(num=2))
+        data.update(self._attachments(num=2))
+        self._post(data)
+
+        # Test attachments.
+        note = self._get_note()
+        eq_(note.attachments.count(), 2)
+
 
 class TestAbuseReports(amo.tests.TestCase):
     fixtures = fixture('user_999', 'user_admin', 'group_admin',

mkt/reviewers/utils.py

@@ -61,18 +61,6 @@ def __init__(self, request, addon, version, review_type,
         self.in_escalate = EscalationQueue.objects.filter(
             addon=self.addon).exists()
 
-    def _create_comm_note(self, note_type, attachments=None):
-        # Permissions default to developers + reviewers + Mozilla contacts.
-        # For escalation/comment, exclude the developer from the conversation.
-        perm_overrides = {
-            comm.ESCALATION: {'developer': False},
-            comm.REVIEWER_COMMENT: {'developer': False},
-        }
-        self.comm_thread, self.comm_note = create_comm_note(
-            self.addon, self.version, self.request.amo_user,
-            self.data['comments'], note_type=note_type,
-            perms=perm_overrides.get(note_type), no_switch=False)
-
     def get_attachments(self):
         """
         Returns a list of triples suitable to be attached to an email.
@@ -112,19 +100,33 @@ def set_files(self, status, files, copy_to_mirror=False,
             if hide_disabled_file:
                 file.hide_disabled_file()
 
-    def log_action(self, action):
+    def create_note(self, action):
+        """
+        Permissions default to developers + reviewers + Mozilla contacts.
+        For escalation/comment, exclude the developer from the conversation.
+        """
         details = {'comments': self.data['comments'],
                    'reviewtype': self.review_type}
         if self.files:
             details['files'] = [f.id for f in self.files]
 
         # Commbadge (the future).
-        self._create_comm_note(comm.ACTION_MAP(action.id),
-                               attachments=self.attachment_formset)
+        perm_overrides = {
+            comm.ESCALATION: {'developer': False},
+            comm.REVIEWER_COMMENT: {'developer': False},
+        }
+        note_type = comm.ACTION_MAP(action.id)
+        self.comm_thread, self.comm_note = create_comm_note(
+            self.addon, self.version, self.request.amo_user,
+            self.data['comments'], note_type=note_type,
+            # Ignore switch so we don't have to re-migrate new notes.
+            perms=perm_overrides.get(note_type), no_switch=True,
+            attachments=self.attachment_formset)
+
         # ActivityLog (ye olde).
         amo.log(action, self.addon, self.version, user=self.user.get_profile(),
                 created=datetime.now(), details=details,
-                attachments=self.attachment_formset)  # ActivityLog.
+                attachments=self.attachment_formset)
 
     def notify_email(self, template, subject, fresh_thread=False):
         """Notify the authors that their app has been reviewed."""
@@ -177,7 +179,7 @@ def get_context_data(self):
     def request_information(self):
         """Send a request for information to the authors."""
         emails = list(self.addon.authors.values_list('email', flat=True))
-        self.log_action(amo.LOG.REQUEST_INFORMATION)
+        self.create_note(amo.LOG.REQUEST_INFORMATION)
         self.version.update(has_info_request=True)
         log.info(u'Sending request for information for %s to %s' %
                  (self.addon, emails))
@@ -229,7 +231,7 @@ def _process_public_waiting(self):
                            highest_status=amo.STATUS_PUBLIC_WAITING)
         self.set_reviewed()
 
-        self.log_action(amo.LOG.APPROVE_VERSION_WAITING)
+        self.create_note(amo.LOG.APPROVE_VERSION_WAITING)
         self.notify_email('pending_to_public_waiting',
                           u'App Approved but waiting: %s')
 
@@ -268,7 +270,7 @@ def _process_public_immediately(self):
         if waffle.switch_is_active('iarc'):
             self.addon.set_iarc_storefront_data()
 
-        self.log_action(amo.LOG.APPROVE_VERSION)
+        self.create_note(amo.LOG.APPROVE_VERSION)
         self.notify_email('pending_to_public', u'App Approved: %s')
 
         log.info(u'Making %s public' % self.addon)
@@ -297,7 +299,7 @@ def process_reject(self):
         if self.in_rereview:
             RereviewQueue.objects.filter(addon=self.addon).delete()
 
-        self.log_action(amo.LOG.REJECT_VERSION)
+        self.create_note(amo.LOG.REJECT_VERSION)
         self.notify_email('pending_to_sandbox', u'Submission Update: %s')
 
         log.info(u'Making %s disabled' % self.addon)
@@ -313,10 +315,9 @@ def process_escalate(self):
         Creates Escalation note/email.
         """
         EscalationQueue.objects.get_or_create(addon=self.addon)
+        self.create_note(amo.LOG.ESCALATE_MANUAL)
         log.info(u'Escalated review requested for %s' % self.addon)
         self.notify_email('author_super_review', u'Submission Update: %s')
-
-        self.log_action(amo.LOG.ESCALATE_MANUAL)
         log.info(u'Escalated review requested for %s' % self.addon)
 
         # Special senior reviewer email.
@@ -334,7 +335,7 @@ def process_comment(self):
         Creates Reviewer Comment note/email.
         """
         self.version.update(has_editor_comment=True)
-        self.log_action(amo.LOG.COMMENT_VERSION)
+        self.create_note(amo.LOG.COMMENT_VERSION)
 
     def process_clear_escalation(self):
         """
@@ -343,7 +344,7 @@ def process_clear_escalation(self):
         Doesn't create note/email.
         """
         EscalationQueue.objects.filter(addon=self.addon).delete()
-        self.log_action(amo.LOG.ESCALATION_CLEARED)
+        self.create_note(amo.LOG.ESCALATION_CLEARED)
         log.info(u'Escalation cleared for app: %s' % self.addon)
 
     def process_clear_rereview(self):
@@ -353,7 +354,7 @@ def process_clear_rereview(self):
         Doesn't create note/email.
         """
         RereviewQueue.objects.filter(addon=self.addon).delete()
-        self.log_action(amo.LOG.REREVIEW_CLEARED)
+        self.create_note(amo.LOG.REREVIEW_CLEARED)
         log.info(u'Re-review cleared for app: %s' % self.addon)
         # Assign reviewer incentive scores.
         return ReviewerScore.award_points(self.request.amo_user, self.addon,
@@ -381,7 +382,7 @@ def process_disable(self):
         self.addon.set_iarc_storefront_data(disable=True)
 
         self.notify_email('disabled', u'App disabled by reviewer: %s')
-        self.log_action(amo.LOG.APP_DISABLED)
+        self.create_note(amo.LOG.APP_DISABLED)
         log.info(u'App %s has been disabled by a reviewer.' % self.addon)