add reply tokens in reviewer emails (Bug 879535)

dash1291 · dash1291 · commit 600e471f4490 · 2013-08-09T01:14:25.000-07:00
diff --git a/apps/comm/utils.py b/apps/comm/utils.py
@@ -5,9 +5,11 @@
 from email_reply_parser import EmailReplyParser
 
 from access import acl
+from access.models import Group
 from comm.models import (CommunicationNote, CommunicationNoteRead,
                          CommunicationThreadCC, CommunicationThreadToken)
 from mkt.constants import comm
+from users.models import UserProfile
 
 
 log = commonware.log.getLogger('comm')
@@ -140,3 +142,75 @@ def filter_notes_by_read_status(queryset, profile, read_status=True):
     else:
         # Exclude read notes if they exist.
         return queryset.exclude(pk__in=notes) if notes else queryset.all()
+
+
+def get_reply_token(thread, user_id):
+    tok, created = CommunicationThreadToken.objects.get_or_create(
+        thread=thread, user_id=user_id)
+
+    # We expire a token after it has been used for a maximum number of times.
+    # This is usually to prevent overusing a single token to spam to threads.
+    # Since we're re-using tokens, we need to make sure they are valid for
+    # replying to new notes so we reset their `use_count`.
+    if not created:
+        tok.update(use_count=0)
+    else:
+        log.info('Created token with UUID %s for user_id: %s.' %
+                 (tok.uuid, user_id))
+    return tok
+
+
+def get_recipients(note, fresh_thread=False):
+    """
+    Create/refresh tokens for users based on the thread permissions.
+    """
+    thread = note.thread
+
+    # TODO: Possible optimization.
+    # Fetch tokens from the database if `fresh_thread=False` and use them to
+    # derive the list of recipients instead of doing a couple of multi-table
+    # DB queries.
+    recipients = set(thread.thread_cc.values_list('user__id', 'user__email'))
+
+    # Include devs.
+    if note.read_permission_developer:
+        recipients.update(thread.addon.authors.values_list('id', 'email'))
+
+    groups_list = []
+    # Include app reviewers.
+    if note.read_permission_reviewer:
+        groups_list.append('App Reviewers')
+
+    # Include senior app reviewers.
+    if note.read_permission_senior_reviewer:
+        groups_list.append('Senior App Reviewers')
+
+    # Include admins.
+    if note.read_permission_staff:
+        groups_list.append('Admins')
+
+    if len(groups_list) > 0:
+        groups = Group.objects.filter(name__in=groups_list)
+        for group in groups:
+            recipients.update(group.users.values_list('id', 'email'))
+
+    # Include Mozilla contact.
+    if (note.read_permission_mozilla_contact and
+        thread.addon.mozilla_contact):
+        for moz_contact in thread.addon.get_mozilla_contacts():
+            try:
+                user = UserProfile.objects.get(email=moz_contact)
+            except UserProfile.DoesNotExist:
+                pass
+            else:
+                recipients.add((user.id, moz_contact))
+
+    if (note.author.id, note.author.email) in recipients:
+        recipients.remove((note.author.id, note.author.email))
+
+    new_recipients_list = []
+    for user_id, user_email in recipients:
+        tok = get_reply_token(note.thread, user_id)
+        new_recipients_list.append((user_email, tok.uuid))
+
+    return new_recipients_list
diff --git a/mkt/comm/api.py b/mkt/comm/api.py
@@ -12,6 +12,7 @@
                                    ListModelMixin, RetrieveModelMixin)
 from rest_framework.permissions import BasePermission
 from rest_framework.relations import PrimaryKeyRelatedField, RelatedField
+from rest_framework.response import Response
 from rest_framework.serializers import ModelSerializer, SerializerMethodField
 from rest_framework.viewsets import GenericViewSet
 
@@ -24,8 +25,8 @@
 from mkt.api.authentication import (RestOAuthAuthentication,
                                     RestSharedSecretAuthentication)
 from mkt.api.base import CORSMixin
+from mkt.reviewers.utils import send_note_emails
 from mkt.webpay.forms import PrepareForm
-from rest_framework.response import Response
 
 
 class AuthorSerializer(ModelSerializer):
@@ -262,8 +263,13 @@ def inherit_permissions(self, obj, parent):
         for key in ('developer', 'reviewer', 'senior_reviewer',
                     'mozilla_contact', 'staff'):
             perm = 'read_permission_%s' % key
+
             setattr(obj, perm, getattr(parent, perm))
 
+    def post_save(self, obj, created=False):
+        if created:
+            send_note_emails(obj)
+
     def pre_save(self, obj):
         """Inherit permissions from the thread."""
         self.inherit_permissions(obj, self.comm_thread)
diff --git a/mkt/comm/tests/test_api.py b/mkt/comm/tests/test_api.py
@@ -1,6 +1,7 @@
 import json
 
 from django.conf import settings
+from django.core import mail
 from django.core.urlresolvers import reverse
 
 import mock
@@ -248,6 +249,22 @@ def test_reply_create(self):
         eq_(res.status_code, 201)
         eq_(note.replies.count(), 1)
 
+    def test_note_emails(self):
+        self.create_switch(name='comm-dashboard')
+        note = CommunicationNote.objects.create(author=self.profile,
+            thread=self.thread, note_type=0, body='something',
+            read_permission_developer=True)
+        res = self.client.post(reverse('comm-note-replies-list',
+                                       kwargs={'thread_id': self.thread.id,
+                                               'note_id': note.id}),
+                               data=json.dumps({'note_type': '0',
+                                                'body': 'something'}))
+        eq_(res.status_code, 201)
+
+        # Decrement authors.count() by 1 because the author of the note is
+        # one of the authors of the addon.
+        eq_(len(mail.outbox), self.thread.addon.authors.count() - 1)
+
 
     def test_mark_read(self):
         note = CommunicationNote.objects.create(author=self.profile,
diff --git a/mkt/reviewers/templates/reviewers/emails/decisions/includes/questions.txt b/mkt/reviewers/templates/reviewers/emails/decisions/includes/questions.txt
@@ -1 +1,5 @@
 If you have any questions about this review, please reply to this email or join #app-reviewers on irc.mozilla.org. For general developer support, please contact {{ MKT_SUPPORT_EMAIL }}.
+
+{% if waffle.switch('comm-dashboard') %}
+You can also post to {{ ('/comm/thread/' + thread_id)|absolutify }}
+{% endif %}
diff --git a/mkt/reviewers/templates/reviewers/emails/decisions/post.txt b/mkt/reviewers/templates/reviewers/emails/decisions/post.txt
@@ -0,0 +1,8 @@
+{% extends 'reviewers/emails/base.txt' -%}
+{% block content %}
+A user posted on a discussion of {{ name }}.
+
+{{ sender }} wrote:
+{{ comments }}
+{% include 'reviewers/emails/decisions/includes/questions.txt' %}
+{% endblock %}
diff --git a/mkt/reviewers/tests/test_views.py b/mkt/reviewers/tests/test_views.py
@@ -22,7 +22,7 @@
 import amo.tests
 import reviews
 from abuse.models import AbuseReport
-from access.models import GroupUser
+from access.models import Group, GroupUser
 from addons.models import AddonDeviceType
 from amo.helpers import absolutify
 from amo.tests import (app_factory, check_links, days_ago,
@@ -982,10 +982,6 @@ def get_app(self):
         return Webapp.objects.get(id=337141)
 
     def post(self, data, queue='pending'):
-        # Set some action visibility form values.
-        data['action_visibility'] = ('senior_reviewer', 'reviewer', 'staff',
-                                     'mozilla_contact')
-        self.create_switch(name='comm-dashboard')
         res = self.client.post(self.url, data)
         self.assert3xx(res, reverse('reviewers.apps.queue_%s' % queue))
 
@@ -1063,7 +1059,7 @@ def _check_thread(self):
         eq_(thread.count(), 1)
 
         thread = thread.get()
-        perms = ('senior_reviewer', 'reviewer', 'staff', 'mozilla_contact')
+        perms = ('developer', 'reviewer', 'staff')
 
         for key in perms:
             assert getattr(thread, 'read_permission_%s' % key)
@@ -1090,6 +1086,26 @@ def _check_score(self, reviewed_type):
         eq_(scores[0].score, amo.REVIEWED_SCORES[reviewed_type])
         eq_(scores[0].note_key, reviewed_type)
 
+    def test_comm_emails(self):
+        data = {'action': 'reject', 'comments': 'suxor',
+                'action_visibility': ('developer', 'reviewer', 'staff')}
+        data.update(self._attachment_management_form(num=0))
+        self.create_switch(name='comm-dashboard')
+        self.post(data)
+        self._check_thread()
+
+        recipients = set(self.app.authors.values_list('email', flat=True))
+        recipients.update(Group.objects.get(
+            name='App Reviewers').users.values_list('email', flat=True))
+        recipients.update(Group.objects.get(
+            name='Admins').users.values_list('email', flat=True))
+
+        recipients.remove('editor@mozilla.com')
+
+        eq_(len(mail.outbox), len(recipients))
+        eq_(mail.outbox[0].subject, '%s has been reviewed.' %
+            self.get_app().name)
+
     def test_xss(self):
         data = {'action': 'comment',
                 'comments': '<script>alert("xss")</script>'}
@@ -1120,7 +1136,6 @@ def test_pending_to_public_w_device_overrides(self):
         msg = mail.outbox[0]
         self._check_email(msg, 'App Approved but waiting')
         self._check_email_body(msg)
-        self._check_thread()
 
     def test_pending_to_reject_w_device_overrides(self):
         # This shouldn't be possible unless there's form hacking.
@@ -1144,7 +1159,6 @@ def test_pending_to_reject_w_device_overrides(self):
         msg = mail.outbox[0]
         self._check_email(msg, 'Submission Update')
         self._check_email_body(msg)
-        self._check_thread()
 
     def test_pending_to_public_w_requirements_overrides(self):
         self.create_switch(name='buchets')
@@ -1209,7 +1223,6 @@ def test_pending_to_public(self, update_name, update_locales,
         msg = mail.outbox[0]
         self._check_email(msg, 'App Approved')
         self._check_email_body(msg)
-        self._check_thread()
         self._check_score(amo.REVIEWED_WEBAPP_HOSTED)
 
         assert update_name.called
@@ -1281,7 +1294,6 @@ def test_pending_to_public_no_mozilla_contact(self):
         msg = mail.outbox[0]
         self._check_email(msg, 'App Approved', with_mozilla_contact=False)
         self._check_email_body(msg)
-        self._check_thread()
         self._check_score(amo.REVIEWED_WEBAPP_HOSTED)
 
     @mock.patch('addons.tasks.index_addons')
@@ -1306,7 +1318,6 @@ def test_pending_to_public_waiting(self, update_name, update_locales,
         msg = mail.outbox[0]
         self._check_email(msg, 'App Approved but waiting')
         self._check_email_body(msg)
-        self._check_thread()
         self._check_score(amo.REVIEWED_WEBAPP_HOSTED)
 
         assert not update_name.called
@@ -1340,7 +1351,6 @@ def test_pending_to_reject(self):
         msg = mail.outbox[0]
         self._check_email(msg, 'Submission Update')
         self._check_email_body(msg)
-        self._check_thread()
         self._check_score(amo.REVIEWED_WEBAPP_HOSTED)
 
     def test_multiple_versions_reject_hosted(self):
@@ -1394,7 +1404,6 @@ def test_pending_to_escalation(self):
         self._check_email(dev_msg, 'Submission Update')
         adm_msg = mail.outbox[1]
         self._check_admin_email(adm_msg, 'Escalated Review Requested')
-        self._check_thread()
 
     def test_pending_to_disable_senior_reviewer(self):
         self.login_as_senior_reviewer()
@@ -1409,7 +1418,6 @@ def test_pending_to_disable_senior_reviewer(self):
         self._check_log(amo.LOG.APP_DISABLED)
         eq_(len(mail.outbox), 1)
         self._check_email(mail.outbox[0], 'App disabled by reviewer')
-        self._check_thread()
 
     def test_pending_to_disable(self):
         self.app.update(status=amo.STATUS_PUBLIC)
@@ -1438,7 +1446,6 @@ def test_escalation_to_public(self):
         msg = mail.outbox[0]
         self._check_email(msg, 'App Approved')
         self._check_email_body(msg)
-        self._check_thread()
 
     def test_escalation_to_reject(self):
         EscalationQueue.objects.create(addon=self.app)
@@ -1457,7 +1464,6 @@ def test_escalation_to_reject(self):
         eq_(len(mail.outbox), 1)
         msg = mail.outbox[0]
         self._check_email(msg, 'Submission Update')
-        self._check_thread()
         self._check_email_body(msg)
         self._check_score(amo.REVIEWED_WEBAPP_HOSTED)
 
@@ -1476,7 +1482,6 @@ def test_escalation_to_disable_senior_reviewer(self):
         eq_(EscalationQueue.objects.count(), 0)
         eq_(len(mail.outbox), 1)
         self._check_email(mail.outbox[0], 'App disabled by reviewer')
-        self._check_thread()
 
     def test_escalation_to_disable(self):
         EscalationQueue.objects.create(addon=self.app)
@@ -1517,7 +1522,6 @@ def test_rereview_to_reject(self):
         self._check_email(msg, 'Submission Update')
         self._check_email_body(msg)
         self._check_score(amo.REVIEWED_WEBAPP_REREVIEW)
-        self._check_thread()
 
     def test_rereview_to_disable_senior_reviewer(self):
         self.login_as_senior_reviewer()
@@ -1533,7 +1537,6 @@ def test_rereview_to_disable_senior_reviewer(self):
         eq_(RereviewQueue.objects.filter(addon=self.app).count(), 0)
         eq_(len(mail.outbox), 1)
         self._check_email(mail.outbox[0], 'App disabled by reviewer')
-        self._check_thread()
 
     def test_rereview_to_disable(self):
         RereviewQueue.objects.create(addon=self.app)
@@ -1572,7 +1575,6 @@ def test_rereview_to_escalation(self):
         self._check_email(dev_msg, 'Submission Update')
         adm_msg = mail.outbox[1]
         self._check_admin_email(adm_msg, 'Escalated Review Requested')
-        self._check_thread()
 
     def test_more_information(self):
         # Test the same for all queues.
@@ -1607,7 +1609,6 @@ def test_comment(self):
         self.post(data)
         eq_(len(mail.outbox), 0)
         self._check_log(amo.LOG.COMMENT_VERSION)
-        self._check_thread()
 
     def test_receipt_no_node(self):
         res = self.client.get(self.url)
diff --git a/mkt/reviewers/utils.py b/mkt/reviewers/utils.py
