[damned-lies] [vertimus] Send mail to state participants if not to mailing list

[Claude Paroz <claudep src gnome org>]

commit 160b8405414cd0ec2be73cfb5e5fd463314785b1
Author: Claude Paroz <claude 2xlibre net>
Date:   Fri Sep 5 09:44:08 2014 +0200

    [vertimus] Send mail to state participants if not to mailing list
    
    When an action generates an email to the mailing list by default,
    and the user unchecked the matching box in the form, still send mail
    to the other state participants.

 vertimus/models.py      |   66 +++++++++++++++++++---------------------------
 vertimus/tests/tests.py |   19 +++++++++++++
 2 files changed, 46 insertions(+), 39 deletions(-)
---
diff --git a/vertimus/models.py b/vertimus/models.py
index 857a1a6..ae2884b 100644
--- a/vertimus/models.py
+++ b/vertimus/models.py
@@ -29,7 +29,7 @@ from django.db import models
 from django.db.models import Max
 from django.db.models.signals import post_save, pre_delete
 from django.dispatch import receiver
-from django.utils.translation import get_language, activate, ugettext, ugettext_noop, ugettext_lazy as _
+from django.utils.translation import override, ugettext, ugettext_noop, ugettext_lazy as _
 
 from stats.models import Branch, Domain, Statistics, PoFile
 from stats.signals import pot_has_changed
@@ -409,6 +409,9 @@ class ActionAbstract(models.Model):
 
 
 class Action(ActionAbstract):
+    default_message = ugettext_noop(
+        u"The new state of %(module)s - %(branch)s - %(domain)s (%(language)s) is now '%(new_state)s'.")
+
     class Meta:
         db_table = 'action'
         verbose_name = 'action'
@@ -451,6 +454,11 @@ class Action(ActionAbstract):
 
         if form_data.get('send_to_ml'):
             self.send_mail_new_state(state, (state.language.team.mailing_list,))
+        elif self.send_mail_to_ml or self.name == 'WC':
+            # Action normally send to ML, unchecked by user in the form, then
+            # still send messages to state participants
+            recipients = set(state.involved_persons().exclude(pk=self.person.pk).values_list('email', 
flat=True))
+            self.send_mail_new_state(state, recipients)
 
     def get_previous_action_with_po(self):
         """
@@ -489,9 +497,9 @@ class Action(ActionAbstract):
             os.remove(temp_path)
         self.merged_file.update_stats()
 
-    def send_mail_new_state(self, state, recipient_list, msg=None):
-        """ msg is an optional untranslated string, which will be translated after
-            state.language will be activated
+    def send_mail_new_state(self, state, recipient_list):
+        """
+        Prepare and send an email to recipient_list, informing about the action.
         """
         # Remove None and empty string items from the list
         recipient_list = filter(lambda x: x and x is not None, recipient_list)
@@ -499,12 +507,6 @@ class Action(ActionAbstract):
         if not recipient_list:
             return
 
-        current_lang = get_language()
-        activate(state.language.locale)
-        if not msg:
-            msg = ugettext(u"The new state of %(module)s - %(branch)s - %(domain)s (%(language)s) is now 
'%(new_state)s'.")
-        else:
-            msg = ugettext(msg)
         current_site = Site.objects.get_current()
         url = "https://%s%s"; % (current_site.domain, urlresolvers.reverse(
             'vertimus_by_names',
@@ -514,24 +516,23 @@ class Action(ActionAbstract):
                 state.domain.name,
                 state.language.locale)))
         subject = state.branch.module.name + u' - ' + state.branch.name
-        message = ugettext(u"Hello,") + u"\n\n" + msg + u"\n%(url)s\n\n"
-        message = message % {
-            'module': state.branch.module.name,
-            'branch': state.branch.name,
-            'domain': state.domain.name,
-            'language': state.language.get_name(),
-            'new_state': state.description,
-            'url': url
-        }
-        message += self.comment or ugettext("Without comment")
-        message += "\n\n" + self.person.name
-        message += "\n--\n" + ugettext(u"This is an automated message sent from %s.") % current_site.domain
+        with override(state.language.locale):
+            message = ugettext(u"Hello,") + u"\n\n" + ugettext(self.default_message) + u"\n%(url)s\n\n"
+            message = message % {
+                'module': state.branch.module.name,
+                'branch': state.branch.name,
+                'domain': state.domain.name,
+                'language': state.language.get_name(),
+                'new_state': state.description,
+                'url': url
+            }
+            message += self.comment or ugettext("Without comment")
+            message += "\n\n" + self.person.name
+            message += "\n--\n" + ugettext(u"This is an automated message sent from %s.") % 
current_site.domain
         try:
             mail.send_mail(subject, message, settings.DEFAULT_FROM_EMAIL, recipient_list)
         except Exception as exc:
             raise SendMailFailed("Sending message failed: %r" % exc)
-        finally:
-            activate(current_lang)
 
 
 def generate_archive_filename(instance, original_filename):
@@ -560,25 +561,12 @@ class ActionArchived(ActionAbstract):
 class ActionWC(Action):
     name = 'WC'
     comment_is_required = True
+    default_message = ugettext_noop(
+        u"A new comment has been posted on %(module)s - %(branch)s - %(domain)s (%(language)s).")
 
     class Meta:
         proxy = True
 
-    def apply_on(self, state, form_data):
-        super(ActionWC, self).apply_on(state, {})
-
-        if form_data.get('send_to_ml'):
-            recipients = (state.language.team.mailing_list,)
-        else:
-            # Send an email to all translators of the page
-            translator_emails = set(state.involved_persons().values_list('email', flat=True))
-            # Remove None items from the list
-            recipients = filter(lambda x: x is not None, translator_emails)
-
-        self.send_mail_new_state(state, recipients,
-            msg=ugettext_noop(u"A new comment has been posted on %(module)s - %(branch)s - %(domain)s 
(%(language)s)."))
-
-
 class ActionRT(Action):
     name = 'RT'
     target_state = StateTranslating
diff --git a/vertimus/tests/tests.py b/vertimus/tests/tests.py
index 5c78d79..cf1edd0 100644
--- a/vertimus/tests/tests.py
+++ b/vertimus/tests/tests.py
@@ -219,6 +219,10 @@ class VertimusTest(TeamsAndRolesTests):
 
         action = Action.new_by_name('WC', person=self.pt, comment="Hi!")
         action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        self.assertEqual(len(mail.outbox), 0)
+        # Second comment by someone else, mail sent to the other person
+        action = Action.new_by_name('WC', person=self.pr, comment="Great!")
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
         self.assertEqual(len(mail.outbox), 1)
         self.assertEqual(mail.outbox[0].recipients(), [self.pt.email])
         # Test that submitting a comment without text generates a validation error
@@ -291,6 +295,21 @@ class VertimusTest(TeamsAndRolesTests):
         action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
         self.files_to_clean.append(action.file.path)
         self.assertTrue(isinstance(state, StateProofread))
+        # Mail sent to mailing list
+        self.assertEqual(len(mail.outbox), 1)
+        self.assertEqual(mail.outbox[0].recipients(), [self.l.team.mailing_list])
+
+        # Comment made by someone else, file reviewed again, checkbox "Send to mailing list" unckecked
+        # => mail sent to the comment author
+        action = Action.new_by_name('WC', person=self.pt, comment="Hi!")
+        action.apply_on(state, {'send_to_ml': False})
+        action = Action.new_by_name('RP', person=self.pr, comment="Reserved by a reviewer!")
+        action.apply_on(state, {'send_to_ml': False})
+        mail.outbox = []
+        action = Action.new_by_name('UP', person=self.pr, comment="Done second time.", file=test_file)
+        action.apply_on(state, {'send_to_ml': False})
+        self.assertEqual(len(mail.outbox), 1)
+        self.assertEqual(mail.outbox[0].recipients(), [self.pt.email])
 
     def test_action_tc(self):
         state = StateProofread(branch=self.b, domain=self.d, language=self.l)