Permalink
Browse files

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

  • Loading branch information...
1 parent 2c4bbec commit e14f88e9b19a51d215251f459c9766b265dfab58 @ngokevin ngokevin committed Feb 7, 2014
Showing with 91 additions and 51 deletions.
  1. +8 −0 mkt/comm/tests/test_api.py
  2. +23 −1 mkt/comm/tests/test_utils_.py
  3. +12 −3 mkt/comm/utils.py
  4. +21 −21 mkt/reviewers/tests/test_views.py
  5. +27 −26 mkt/reviewers/utils.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 = {}
@@ -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)
View
@@ -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,
@@ -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',
View
@@ -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)

0 comments on commit e14f88e

Please sign in to comment.