[damned-lies] Send email to state participants if not sent to ML

From: Claude Paroz <claudep src gnome org>
To: commits-list gnome org
Cc:
Subject: [damned-lies] Send email to state participants if not sent to ML
Date: Sat, 18 Apr 2015 10:27:59 +0000 (UTC)
commit 6fa3de681ebf1357c482ea686ad2b9074302efe3
Author: Claude Paroz <claude 2xlibre net>
Date:   Sat Apr 18 11:57:47 2015 +0200

    Send email to state participants if not sent to ML
    
    This was previously the case for some actions, it's now the
    behavior for all actions. Thanks Federico Bruni for spotting the
    issue.

 vertimus/models.py      |    8 ++-
 vertimus/tests/tests.py |  179 ++++++++++++++++++++++++++---------------------
 vertimus/views.py       |    3 +-
 3 files changed, 106 insertions(+), 84 deletions(-)
---
diff --git a/vertimus/models.py b/vertimus/models.py
index 68702f0..afe2e52 100644
--- a/vertimus/models.py
+++ b/vertimus/models.py
@@ -443,6 +443,8 @@ class Action(ActionAbstract):
         self.state_db = state
         if self.file:
             self.file.save(self.file.name, self.file, save=False)
+        if form_data.get('comment'):
+            self.comment = form_data['comment']
         self.save()
         if self.target_state is not None:
             # All actions change state except Writing a comment
@@ -458,9 +460,9 @@ 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
+        elif self.send_mail_to_ml or self.comment:
+            # If the action is normally sent to ML (but unchecked) or if the form
+            # contains a comment, send message 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)
 
diff --git a/vertimus/tests/tests.py b/vertimus/tests/tests.py
index 78a6418..7abd4a8 100644
--- a/vertimus/tests/tests.py
+++ b/vertimus/tests/tests.py
@@ -70,8 +70,8 @@ class VertimusTest(TeamsAndRolesTests):
         """ Test utility to add an uploaded file to a state """
         test_file = ContentFile('test content')
         test_file.name = 'mytestfile.po'
-        action = Action.new_by_name(action_code, person=pers or self.pr, comment="Done.", file=test_file)
-        action.apply_on(to_state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name(action_code, person=pers or self.pr, file=test_file)
+        action.apply_on(to_state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Done."})
         self.files_to_clean.append(action.file.path)
         return action.file
 
@@ -182,8 +182,8 @@ class VertimusTest(TeamsAndRolesTests):
         state = StateProofreading(branch=self.b, domain=self.d, language=self.l, person=self.pr)
         state.save()
         self.upload_file(state, 'UP')
-        action = Action.new_by_name('RC', person=self.pc, comment="Reserve the commit")
-        action.apply_on(state, {})
+        action = Action.new_by_name('RC', person=self.pc)
+        action.apply_on(state, {'comment': "Reserve the commit"})
         self.assertEqual(state.name, 'Committing')
 
         for p in (self.pn, self.pt, self.pr):
@@ -217,12 +217,12 @@ class VertimusTest(TeamsAndRolesTests):
         state.save()
         prev_updated = state.updated
 
-        action = Action.new_by_name('WC', person=self.pt, comment="Hi!")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('WC', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Hi!"})
         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})
+        action = Action.new_by_name('WC', person=self.pr)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Great!"})
         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
@@ -232,8 +232,8 @@ class VertimusTest(TeamsAndRolesTests):
 
         # Test send comment to mailing list
         mail.outbox = []
-        action = Action.new_by_name('WC', person=self.pt, comment="Hi again!")
-        action.apply_on(state, {'send_to_ml': True})
+        action = Action.new_by_name('WC', person=self.pt)
+        action.apply_on(state, {'send_to_ml': True, 'comment': "Hi again!"})
         self.assertEqual(len(mail.outbox), 1)
         self.assertEqual(mail.outbox[0].recipients(), [self.l.team.mailing_list])
         self.assertIn(u"Hi again!", mail.outbox[0].body)
@@ -242,8 +242,8 @@ class VertimusTest(TeamsAndRolesTests):
         state = StateNone(branch=self.b, domain=self.d, language=self.l)
         state.save()
 
-        action = Action.new_by_name('RT', person=self.pt, comment="Reserved!")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('RT', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Reserved!"})
         self.assertTrue(isinstance(state, StateTranslating))
 
     @test_scratchdir
@@ -258,8 +258,8 @@ class VertimusTest(TeamsAndRolesTests):
 
         test_file = open(os.path.join(os.path.dirname(os.path.abspath(__file__)), "valid_po.po"), 'r')
 
-        action = Action.new_by_name('UT', person=self.pt, comment="Done by translator.", 
file=File(test_file))
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('UT', person=self.pt, file=File(test_file))
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Done by translator."})
 
         self.assertEqual(action.file.url, '/media/upload/gedit-gnome-2-24-po-fr-%d.po' % state.id)
         self.assertEqual(action.merged_file.url, '/media/upload/gedit-gnome-2-24-po-fr-%d.merged.po' % 
state.id)
@@ -280,9 +280,21 @@ class VertimusTest(TeamsAndRolesTests):
         state = StateTranslated(branch=self.b, domain=self.d, language=self.l)
         state.save()
 
-        action = Action.new_by_name('RP', person=self.pr, comment="Reserved by a reviewer!")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('RP', person=self.pr)
+        action.apply_on(state, {
+            'send_to_ml': action.send_mail_to_ml, 'comment': "Reserved by a reviewer!",
+        })
         self.assertTrue(isinstance(state, StateProofreading))
+        self.assertEqual(len(mail.outbox), 0)
+
+    def test_action_rp_with_comment(self):
+        state = StateTranslated.objects.create(branch=self.b, domain=self.d, language=self.l)
+        action = Action.new_by_name('WC', person=self.pt)
+        action.apply_on(state, {'send_to_ml': False, 'comment': "Hi!"})
+        action = Action.new_by_name('RP', person=self.pr)
+        action.apply_on(state, {'send_to_ml': False, 'comment': "I'm reviewing this!"})
+        # If a comment is set, a message should be sent
+        self.assertEqual(len(mail.outbox), 1)
 
     def test_action_up(self):
         state = StateProofreading(branch=self.b, domain=self.d, language=self.l, person=self.pr)
@@ -291,8 +303,8 @@ class VertimusTest(TeamsAndRolesTests):
         test_file = ContentFile('test content')
         test_file.name = 'mytestfile.po'
 
-        action = Action.new_by_name('UP', person=self.pr, comment="Done.", file=test_file)
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('UP', person=self.pr, file=test_file)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Done."})
         self.files_to_clean.append(action.file.path)
         self.assertTrue(isinstance(state, StateProofread))
         # Mail sent to mailing list
@@ -301,13 +313,13 @@ class VertimusTest(TeamsAndRolesTests):
 
         # 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})
+        action = Action.new_by_name('WC', person=self.pt)
+        action.apply_on(state, {'send_to_ml': False, 'comment': "Hi!"})
+        action = Action.new_by_name('RP', person=self.pr)
+        action.apply_on(state, {'send_to_ml': False, 'comment': "Reserved by a reviewer!"})
         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})
+        action = Action.new_by_name('UP', person=self.pr, file=test_file)
+        action.apply_on(state, {'send_to_ml': False, 'comment': "Done second time."})
         self.assertEqual(len(mail.outbox), 1)
         self.assertEqual(mail.outbox[0].recipients(), [self.pt.email])
 
@@ -315,16 +327,16 @@ class VertimusTest(TeamsAndRolesTests):
         state = StateProofread(branch=self.b, domain=self.d, language=self.l)
         state.save()
 
-        action = Action.new_by_name('TC', person=self.pr, comment="Ready!")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('TC', person=self.pr)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Ready!"})
         self.assertTrue(isinstance(state, StateToCommit))
 
     def test_action_rc(self):
         state = StateToCommit(branch=self.b, domain=self.d, language=self.l)
         state.save()
 
-        action = Action.new_by_name('RC', person=self.pc, comment="This work is mine!")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('RC', person=self.pc)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "This work is mine!"})
         self.assertTrue(isinstance(state, StateCommitting))
 
     def test_action_ci(self):
@@ -333,11 +345,11 @@ class VertimusTest(TeamsAndRolesTests):
         state = StateProofreading(branch=self.b, domain=self.d, language=self.l, person=self.pr)
         state.save()
 
-        action = Action.new_by_name('WC', person=self.pt, comment="Hi!")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('WC', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Hi!"})
         p3 = Person.objects.create(username=u'ûsername')
-        action = Action.new_by_name('WC', person=p3, comment="Hello!")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('WC', person=p3)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Hello!"})
 
         self.upload_file(state, 'UP')
 
@@ -367,10 +379,11 @@ class VertimusTest(TeamsAndRolesTests):
         state = StateProofreading(branch=self.b, domain=self.d, language=self.l, person=pers)
         state.save()
         self.upload_file(state, 'UP', pers=pers)
-        form = ActionForm(state, state.get_available_actions(self.pcoo), True, {'action': 'CI', 'author': 
''})
+        form = ActionForm(state, state.get_available_actions(self.pcoo), True, {
+            'action': 'CI', 'author': '', 'comment': ''})
         self.assertTrue(form.is_valid())
         with patch_shell_command() as cmds:
-            action = Action.new_by_name('CI', person=self.pcoo, comment='')
+            action = Action.new_by_name('CI', person=self.pcoo)
             action.apply_on(state, form.cleaned_data)
             self.assertIn("git commit", cmds[-2])
             self.assertNotIn("--author", cmds[-2])
@@ -388,16 +401,16 @@ class VertimusTest(TeamsAndRolesTests):
         file_path = os.path.join(settings.MEDIA_ROOT, action_file.name)
         self.assertTrue(os.access(file_path, os.W_OK))
 
-        action = Action.new_by_name('TC', person=self.pc, comment="To commit.")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('TC', person=self.pc)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': ''})
         self.assertEqual(len(mail.outbox), 1) # Mail sent to committers
         mail.outbox = []
 
-        action = Action.new_by_name('RC', person=self.pc, comment="Reserved commit.")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('RC', person=self.pc)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': ''})
 
-        action = Action.new_by_name('IC', person=self.pc, comment="Committed.")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('IC', person=self.pc)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Committed."})
         # Mail sent to mailing list
         self.assertEqual(len(mail.outbox), 1)
         self.assertEqual(mail.outbox[0].recipients(), [self.l.team.mailing_list])
@@ -417,16 +430,18 @@ class VertimusTest(TeamsAndRolesTests):
         state = StateTranslated(branch=self.b, domain=self.d, language=self.l)
         state.save()
 
-        action = Action.new_by_name('TR', person=self.pc, comment="Bad work :-/")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('TR', person=self.pc)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Bad work :-/"})
         self.assertTrue(isinstance(state, StateToReview))
 
     def test_action_aa(self):
         state = StateCommitted(branch=self.b, domain=self.d, language=self.l, person=self.pr)
         state.save()
 
-        action = Action.new_by_name('AA', person=self.pc, comment="I don't want to disappear :)")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('AA', person=self.pc)
+        action.apply_on(state, {
+            'send_to_ml': action.send_mail_to_ml, 'comment': "I don't want to disappear :)",
+        })
 
         state = State.objects.get(branch=self.b, domain=self.d, language=self.l)
         self.assertTrue(isinstance(state, StateNone))
@@ -436,39 +451,41 @@ class VertimusTest(TeamsAndRolesTests):
         state = StateNone(branch=self.b, domain=self.d, language=self.l)
         state.save()
 
-        action = Action.new_by_name('RT', person=self.pt, comment="Reserved!")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('RT', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Reserved!"})
 
-        action = Action.new_by_name('UNDO', person=self.pt, comment="Ooops! I don't want to do that. Sorry.")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('UNDO', person=self.pt)
+        action.apply_on(state, {
+            'send_to_ml': action.send_mail_to_ml, 'comment': "Ooops! I don't want to do that. Sorry.",
+        })
 
         self.assertEqual(state.name, 'None')
 
-        action = Action.new_by_name('RT', person=self.pt, comment="Translating")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('RT', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Translating"})
 
-        action = Action.new_by_name('UT', person=self.pt, comment="Translated")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('UT', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Translated"})
 
-        action = Action.new_by_name('RT', person=self.pt, comment="Reserved!")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('RT', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Reserved!"})
 
-        action = Action.new_by_name('UNDO', person=self.pt, comment="Ooops! I don't want to do that. Sorry.")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('UNDO', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Ooops! I don't want to do 
that. Sorry."})
 
         self.assertEqual(state.name, 'Translated')
 
-        action = Action.new_by_name('RT', person=self.pt, comment="Translating 1")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('RT', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Translating 1"})
 
-        action = Action.new_by_name('UNDO', person=self.pt, comment="Undo 1")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('UNDO', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Undo 1"})
 
-        action = Action.new_by_name('RT', person=self.pt, comment="Translating 2")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('RT', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Translating 2"})
 
-        action = Action.new_by_name('UNDO', person=self.pt, comment="Undo 2")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('UNDO', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Undo 2"})
 
         self.assertEqual(state.name, 'Translated')
 
@@ -477,8 +494,8 @@ class VertimusTest(TeamsAndRolesTests):
         state = StateNone(branch=self.b, domain=self.d, language=self.l)
         state.save()
 
-        action = Action.new_by_name('WC', person=self.pt, comment="Hi!")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('WC', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Hi!"})
 
         self.m.delete()
         self.assertEqual(Action.objects.all().count(), 0)
@@ -528,8 +545,8 @@ class VertimusTest(TeamsAndRolesTests):
         state = StateNone(branch=self.b, domain=self.d, language=self.l)
         state.save()
 
-        action = Action.new_by_name('RT', person=self.pt, comment="Translating")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('RT', person=self.pt)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Translating"})
 
         response = self.client.get(reverse('lang_feed', args=[self.l.locale]))
         self.assertContains(response,
@@ -558,22 +575,26 @@ class VertimusTest(TeamsAndRolesTests):
         state.save()
 
         action = Action.new_by_name('RT', person=self.pr, comment="Reserved!")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Reserved!"})
 
-        action = Action.new_by_name('UNDO', person=self.pr, comment="Ooops! I don't want to do that. Sorry.")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('UNDO', person=self.pr)
+        action.apply_on(state, {
+            'send_to_ml': action.send_mail_to_ml, 'comment': "Ooops! I don't want to do that. Sorry.",
+        })
 
-        action = Action.new_by_name('RT', person=self.pr, comment="Translating")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('RT', person=self.pr)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Translating"})
 
-        action = Action.new_by_name('UT', person=self.pr, comment="Translated")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('UT', person=self.pr)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Translated"})
 
-        action = Action.new_by_name('RP', person=self.pr, comment="Proofreading")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('RP', person=self.pr)
+        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml, 'comment': "Proofreading"})
 
-        action = Action.new_by_name('UNDO', person=self.pr, comment="Ooops! I don't want to do that. Sorry.")
-        action.apply_on(state, {'send_to_ml': action.send_mail_to_ml})
+        action = Action.new_by_name('UNDO', person=self.pr)
+        action.apply_on(state, {
+            'send_to_ml': action.send_mail_to_ml, 'comment': "Ooops! I don't want to do that. Sorry.",
+        })
 
         actions_db = Action.objects.filter(state_db__id=state.id).exclude(name='WC').order_by('-id')
 
diff --git a/vertimus/views.py b/vertimus/views.py
index 44f2bad..edf905d 100644
--- a/vertimus/views.py
+++ b/vertimus/views.py
@@ -103,9 +103,8 @@ def vertimus(request, branch, domain, language, stats=None, level="0"):
             if action_form.is_valid():
                 # Process the data in form.cleaned_data
                 action = action_form.cleaned_data['action']
-                comment = action_form.cleaned_data['comment']
 
-                action = Action.new_by_name(action, person=person, comment=comment,
+                action = Action.new_by_name(action, person=person,
                     file=request.FILES.get('file', None))
                 try:
                     action.apply_on(state, action_form.cleaned_data)
[Date Prev][Date Next] [Thread Prev][Thread Next] [Thread Index] [Date Index] [Author Index]