From cafa50649c2e6ed7f889e5877ede90bf800840e8 Mon Sep 17 00:00:00 2001 From: Tasos Katsoulas Date: Tue, 1 Apr 2014 13:18:33 +0300 Subject: [PATCH] [fix bug 989110] Send email notification to Council for a budget request. * Remove send_mail_task. * Refactor send_remo_mail. --- remo/base/forms.py | 30 +++++------- remo/base/tasks.py | 47 +++++++++++++++++-- remo/base/tests/test_forms.py | 12 +++-- remo/base/tests/test_views.py | 14 ++---- remo/events/models.py | 5 +- remo/events/tests/test_views.py | 12 ++--- remo/profiles/models.py | 6 +-- remo/reports/models.py | 6 +-- remo/reports/tasks.py | 22 --------- remo/reports/tests/test_models.py | 12 +++-- remo/voting/cron.py | 7 +-- remo/voting/models.py | 14 ++++++ .../emails/review_budget_notify_council.txt | 16 +++++++ remo/voting/tests/test_commands.py | 5 +- remo/voting/tests/test_models.py | 16 ++++--- 15 files changed, 135 insertions(+), 89 deletions(-) create mode 100644 remo/voting/templates/emails/review_budget_notify_council.txt diff --git a/remo/base/forms.py b/remo/base/forms.py index db9fc3687..35d9e6396 100644 --- a/remo/base/forms.py +++ b/remo/base/forms.py @@ -3,7 +3,7 @@ from django import forms from django.contrib import messages -from remo.base.tasks import send_mail_task +from remo.base.tasks import send_remo_mail from remo.profiles.models import FunctionalArea, UserProfile @@ -28,16 +28,13 @@ def __init__(self, users, *args, **kwargs): Dynamically set fields for the recipients of the mail. """ super(EmailUsersForm, self).__init__(*args, **kwargs) - recipients = users.values_list('first_name', 'last_name', 'email') - - for first_name, last_name, email in recipients: - field_name = '%s %s <%s>' % (first_name, last_name, email) + for user in users: # Insert method is used to override the order of form fields form_widget = forms.CheckboxInput( attrs={'class': 'input-text-big'}) - self.fields.insert(0, field_name, + self.fields.insert(0, str(user.id), forms.BooleanField( - label=field_name, + label=user, initial=True, required=False, widget=form_widget)) @@ -48,12 +45,13 @@ def send_mail(self, request): for field in self.fields: if (isinstance(self.fields[field], forms.BooleanField) and self.cleaned_data[field]): - recipients_list.append(field) + recipients_list.append(long(field)) + if recipients_list: from_email = '%s <%s>' % (request.user.get_full_name(), request.user.email) - send_mail_task.delay(sender=from_email, - recipients=recipients_list, + send_remo_mail.delay(sender=from_email, + recipients_list=recipients_list, subject=self.cleaned_data['subject'], message=self.cleaned_data['body']) messages.success(request, 'Email sent successfully.') @@ -71,17 +69,13 @@ class EmailRepsForm(BaseEmailUsersFrom): def send_email(self, request, users): """Send mail to recipients list.""" - recipients = users.values_list('first_name', 'last_name', 'email') - recipients_list = [] - for first_name, last_name, email in recipients: - recipient = '%s %s <%s>' % (first_name, last_name, email) - recipients_list.append(recipient) + recipients = users.values_list('id', flat=True) - if recipients_list: + if recipients: from_email = '%s <%s>' % (request.user.get_full_name(), request.user.email) - send_mail_task.delay(sender=from_email, - recipients=recipients_list, + send_remo_mail.delay(sender=from_email, + recipients_list=recipients, subject=self.cleaned_data['subject'], message=self.cleaned_data['body']) messages.success(request, 'Email sent successfully.') diff --git a/remo/base/tasks.py b/remo/base/tasks.py index 9757dc413..67536cc20 100644 --- a/remo/base/tasks.py +++ b/remo/base/tasks.py @@ -1,18 +1,55 @@ +from django.conf import settings +from django.contrib.auth.models import User from django.core.mail import EmailMessage +from django.core.validators import email_re +from django.template.loader import render_to_string from celery.task import task @task -def send_mail_task(sender, recipients, subject, message): - """Send email from /sender/ to /recipients/ with /subject/ and +def send_remo_mail(subject, recipients_list, sender=None, + message=None, email_template=None, data=None, + headers=None): + """Send email from /sender/ to /recipients_list/ with /subject/ and /message/ as body. """ + # Make sure that there is either a message or a template + if not message and not email_template: + return + # Make sure that there is a recipient + if not recipients_list: + return + if not headers: + headers = {} + if not sender: + sender = settings.FROM_EMAIL + if not data: + data = {} + data.update({'SITE_URL': settings.SITE_URL, + 'FROM_EMAIL': settings.FROM_EMAIL}) # Make sure subject is one line. subject = subject.replace('\n', ' ') - email = EmailMessage(subject=subject, body=message, from_email=sender, - to=recipients, cc=[sender]) - email.send() + for recipient in recipients_list: + to = '' + if (isinstance(recipient, long) and + User.objects.filter(pk=recipient).exists()): + user = User.objects.get(pk=recipient) + to = '%s <%s>' % (user.get_full_name(), user.email) + ctx_data = {'user': user, + 'userprofile': user.userprofile} + data.update(ctx_data) + elif email_re.match(recipient): + to = recipient + else: + return + + if email_template: + message = render_to_string(email_template, data) + + email = EmailMessage(subject=subject, body=message, from_email=sender, + to=[to], cc=[sender], headers=headers) + email.send() diff --git a/remo/base/tests/test_forms.py b/remo/base/tests/test_forms.py index c66b052a0..08bed0b95 100644 --- a/remo/base/tests/test_forms.py +++ b/remo/base/tests/test_forms.py @@ -46,12 +46,16 @@ def test_send_mail(self, fake_messages): form.send_email(request, reps) - eq_(len(mail.outbox), 1) + eq_(len(mail.outbox), 20) address = lambda u: '%s %s <%s>' % (u.first_name, u.last_name, u.email) recipients = map(address, reps) - eq_(set(mail.outbox[0].to), set(recipients)) - eq_(mail.outbox[0].subject, data['subject']) - eq_(mail.outbox[0].body, data['body']) + receivers = [] + for i in range(0, len(mail.outbox)): + eq_(mail.outbox[i].subject, data['subject']) + eq_(mail.outbox[i].body, data['body']) + receivers.append(mail.outbox[i].to[0]) + + eq_(set(receivers), set(recipients)) fake_messages.assert_called_with(ANY, 'Email sent successfully.') diff --git a/remo/base/tests/test_views.py b/remo/base/tests/test_views.py index 2678fc3ba..fa0c33ea0 100644 --- a/remo/base/tests/test_views.py +++ b/remo/base/tests/test_views.py @@ -257,14 +257,11 @@ def test_view_dashboard_page(self): self.assertTemplateUsed(response, 'dashboard_reps.html') def test_email_my_mentees_mentor_with_send_True(self): - """Email mentees when mentor and checkbox - is checked.""" + """Email mentees when mentor and checkbox is checked.""" c = Client() c.login(username='mentor', password='passwd') rep1 = User.objects.get(first_name='Nick') - data = {'%s %s <%s>' % (rep1.first_name, - rep1.last_name, - rep1.email): 'True', + data = {'%s' % (rep1.id): 'True', 'subject': 'This is subject', 'body': ('This is my body\n', 'Multiline of course')} @@ -278,14 +275,11 @@ def test_email_my_mentees_mentor_with_send_True(self): eq_(len(mail.outbox[0].cc), 1) def test_email_my_mentees_mentor_with_send_False(self): - """Email mentees when mentor and checkbox is - not checked.""" + """Email mentees when mentor and checkbox is not checked.""" c = Client() c.login(username='mentor', password='passwd') rep1 = User.objects.get(first_name='Nick') - data = {'%s %s <%s>' % (rep1.first_name, - rep1.last_name, - rep1.email): 'False', + data = {'%s' % (rep1.id): 'False', 'subject': 'This is subject', 'body': ('This is my body\n', 'Multiline of course')} diff --git a/remo/events/models.py b/remo/events/models.py index d0513fc73..b89d7b1fe 100644 --- a/remo/events/models.py +++ b/remo/events/models.py @@ -16,10 +16,10 @@ from uuslug import uuslug as slugify from remo.base.models import GenericActiveManager +from remo.base.tasks import send_remo_mail from remo.base.utils import add_permissions_to_groups from remo.profiles.models import FunctionalArea from remo.remozilla.models import Bug -from remo.reports.tasks import send_remo_mail SIMILAR_EVENTS = 3 @@ -227,4 +227,5 @@ def email_event_owner_on_add_comment(sender, instance, **kwargs): if owner.userprofile.receive_email_on_add_event_comment: subject = subject % (instance.user.get_full_name(), instance.event.name) - send_remo_mail.delay([owner.id], subject, email_template, ctx_data) + send_remo_mail.delay(subject=subject, recipients_list=[owner.id], + email_template=email_template, data=ctx_data) diff --git a/remo/events/tests/test_views.py b/remo/events/tests/test_views.py index 3c3ec960c..5743c26a9 100644 --- a/remo/events/tests/test_views.py +++ b/remo/events/tests/test_views.py @@ -630,9 +630,7 @@ def test_email_event_attendees(self, mock_success): reps = event.attendees.all() valid_data = dict() for rep in reps: - field_name = '%s %s <%s>' % (rep.first_name, - rep.last_name, - rep.email) + field_name = '%s' % rep.id valid_data[field_name] = 'True' valid_data['subject'] = 'This is the mail subject' @@ -644,11 +642,11 @@ def test_email_event_attendees(self, mock_success): self.assertTemplateUsed(response, 'view_event.html') mock_success.assert_called_with(ANY, 'Email sent successfully.') - eq_(len(mail.outbox), 1) + eq_(len(mail.outbox), 4) - email = mail.outbox[0] - eq_(len(email.to), 4) - eq_(len(email.cc), 1) + for i in range(0, len(mail.outbox)): + eq_(len(mail.outbox[i].cc), 1) + eq_(len(mail.outbox[i].to), 1) @mock.patch('remo.events.views.iri_to_uri', wraps=iri_to_uri) def test_view_redirect_list_events(self, mocked_uri): diff --git a/remo/profiles/models.py b/remo/profiles/models.py index 89c683d19..b2aa8fd90 100644 --- a/remo/profiles/models.py +++ b/remo/profiles/models.py @@ -16,8 +16,8 @@ from uuslug import uuslug as slugify from remo.base.models import GenericActiveManager +from remo.base.tasks import send_remo_mail from remo.base.utils import add_permissions_to_groups -from remo.reports.tasks import send_remo_mail DISPLAY_NAME_MAX_LENGTH = 50 @@ -269,8 +269,8 @@ def email_mentor_notification(sender, instance, raw, **kwargs): instance.mentor.id] ctx_data = {'rep_user': instance.user, 'new_mentor': instance.mentor} - send_remo_mail.delay(recipients, subject, email_template, - ctx_data) + send_remo_mail.delay(recipients_list=recipients, subject=subject, + email_template=email_template, data=ctx_data) statsd.incr('profiles.change_mentor') diff --git a/remo/reports/models.py b/remo/reports/models.py index f666d779a..6d06dfc32 100644 --- a/remo/reports/models.py +++ b/remo/reports/models.py @@ -16,6 +16,7 @@ from remo.base.utils import (add_permissions_to_groups, get_object_or_none) from remo.base.models import GenericActiveManager +from remo.base.tasks import send_remo_mail from remo.base.utils import daterange, get_date from remo.events.helpers import get_event_link from remo.events.models import Attendance as EventAttendance, Event @@ -23,7 +24,6 @@ from remo.reports import (ACTIVITY_CAMPAIGN, ACTIVITY_EVENT_ATTEND, ACTIVITY_EVENT_CREATE, READONLY_ACTIVITIES, VERIFIABLE_ACTIVITIES) -from remo.reports.tasks import send_remo_mail @receiver(post_migrate, dispatch_uid='report_set_groups_signal') @@ -405,8 +405,8 @@ def email_commenters_on_add_ng_report_comment(sender, instance, **kwargs): 'comment': instance.comment, 'created_on': instance.created_on} subject = subject.format(instance.user.get_full_name(), report) - send_remo_mail.delay([user_id], subject, - email_template, ctx_data) + send_remo_mail.delay(subject=subject, recipients_list=[user_id], + email_template=email_template, data=ctx_data) @receiver(pre_delete, sender=NGReport, diff --git a/remo/reports/tasks.py b/remo/reports/tasks.py index 9b1e7fa20..777492fe7 100644 --- a/remo/reports/tasks.py +++ b/remo/reports/tasks.py @@ -16,28 +16,6 @@ DIGEST_SUBJECT = 'Your mentee activity for {date}' -@task() -def send_remo_mail(user_ids_list, subject, email_template, data=None): - """Send to user_list emails based rendered using email_template - and populated with data. - - """ - if not data: - data = {} - - data.update({'SITE_URL': settings.SITE_URL, - 'FROM_EMAIL': settings.FROM_EMAIL}) - - for user_id in user_ids_list: - if User.objects.filter(pk=user_id).exists(): - user = User.objects.get(pk=user_id) - ctx_data = {'user': user, - 'userprofile': user.userprofile} - ctx_data.update(data) - message = render_to_string(email_template, ctx_data) - send_mail(subject, message, settings.FROM_EMAIL, [user.email]) - - @task() def send_report_digest(): from remo.reports.models import NGReport diff --git a/remo/reports/tests/test_models.py b/remo/reports/tests/test_models.py index 5e287d173..64b3c680e 100644 --- a/remo/reports/tests/test_models.py +++ b/remo/reports/tests/test_models.py @@ -57,7 +57,7 @@ def test_different_current_longest_streak(self): today = now().date() past_day = now().date() - datetime.timedelta(days=30) user = UserFactory.create() - #longest streak + # longest streak for i in range(0, 3): NGReportFactory.create( user=user, report_date=past_day - datetime.timedelta(days=i)) @@ -246,7 +246,8 @@ def test_comment_one_user(self): comment='This is a comment') eq_(len(mail.outbox), 1) - eq_(reporter.email, mail.outbox[0].to[0]) + eq_('%s <%s>' % (reporter.get_full_name(), reporter.email), + mail.outbox[0].to[0]) msg = ('[Report] User {0} commented on {1}' .format(commenter.get_full_name(), report)) eq_(mail.outbox[0].subject, msg) @@ -282,8 +283,11 @@ def test_comment_multiple_users(self): comment='This is a comment') eq_(len(mail.outbox), 3) - recipients = [reporter.email, users_with_comments[0].email, - users_with_comments[1].email] + recipients = ['%s <%s>' % (reporter.get_full_name(), reporter.email), + '%s <%s>' % (users_with_comments[0].get_full_name(), + users_with_comments[0].email), + '%s <%s>' % (users_with_comments[1].get_full_name(), + users_with_comments[1].email)] receivers = [mail.outbox[0].to[0], mail.outbox[1].to[0], mail.outbox[2].to[0]] eq_(set(recipients), set(receivers)) diff --git a/remo/voting/cron.py b/remo/voting/cron.py index d4e9ca146..eb77b4e25 100644 --- a/remo/voting/cron.py +++ b/remo/voting/cron.py @@ -6,7 +6,7 @@ from cronjobs import register -from remo.reports.tasks import send_remo_mail +from remo.base.tasks import send_remo_mail from remo.voting.models import Poll # Intervals in seconds @@ -36,8 +36,9 @@ def poll_vote_reminder(): 'for "%s" now!' % poll.name) template_reminder = 'emails/voting_vote_reminder.txt' ctx_data = {'poll': poll} - send_remo_mail.delay(recipients, subject, - template_reminder, ctx_data) + send_remo_mail.delay(subject=subject, recipients_list=recipients, + email_template=template_reminder, + data=ctx_data) Poll.objects.filter(pk=poll.pk).update(last_notification=now()) diff --git a/remo/voting/models.py b/remo/voting/models.py index a20e1e816..baaf1b185 100644 --- a/remo/voting/models.py +++ b/remo/voting/models.py @@ -13,6 +13,7 @@ from south.signals import post_migrate from uuslug import uuslug +from remo.base.tasks import send_remo_mail from remo.base.utils import add_permissions_to_groups from remo.remozilla.models import Bug from remo.voting.tasks import send_voting_mail @@ -21,6 +22,7 @@ # Voting period in days BUDGET_REQUEST_PERIOD_START = 3 BUDGET_REQUEST_PERIOD_END = 6 +BUGZILLA_URL = 'https://bugzilla.mozilla.org/show_bug.cgi?id=' class Poll(models.Model): @@ -147,6 +149,18 @@ def poll_email_reminder(sender, instance, raw, **kwargs): end_template = 'emails/voting_results_reminder.txt' if not instance.task_start_id or instance.is_future_voting: + if instance.automated_poll: + template = 'emails/review_budget_notify_council.txt' + subject = ('[Bug %s] Budget request discussion' + % str(instance.bug_id)) + data = {'bug': instance.bug, + 'BUGZILLA_URL': BUGZILLA_URL} + send_remo_mail.delay( + subject=subject, email_template=template, + recipients_list=[settings.REPS_COUNCIL_ALIAS], + data=data, + headers={'Reply-To': settings.REPS_COUNCIL_ALIAS}) + start_reminder = send_voting_mail.apply_async( eta=instance.start, kwargs={'voting_id': instance.id, 'subject': subject_start, diff --git a/remo/voting/templates/emails/review_budget_notify_council.txt b/remo/voting/templates/emails/review_budget_notify_council.txt new file mode 100644 index 000000000..bc39e15df --- /dev/null +++ b/remo/voting/templates/emails/review_budget_notify_council.txt @@ -0,0 +1,16 @@ +Hello Council, + +A budget request is ready for your review [0]. +Reply all to this email to discuss the request with the Council, +and comment in the bug if you need more information from the Rep. +You will receive a notification in 72 hours so that you can vote on this request. + +[Bug {{ bug.bug_id }}] + +{{ bug.summary }} + +[0] {{ BUGZILLA_URL }}{{ bug.bug_id }} + +Cheers! + +Your lovely ReMo bot. diff --git a/remo/voting/tests/test_commands.py b/remo/voting/tests/test_commands.py index 87cd2cff5..a55e86821 100644 --- a/remo/voting/tests/test_commands.py +++ b/remo/voting/tests/test_commands.py @@ -38,8 +38,9 @@ def test_email_users_without_a_vote(self, fake_now): args = ['poll_vote_reminder'] management.call_command('cron', *args) eq_(len(mail.outbox), 3) - for email in mail.outbox: - eq_(email.to, ['counselor@example.com']) + eq_(mail.outbox[0].to, [u'counselor@example.com']) + eq_(mail.outbox[1].to, [u'counselor@example.com']) + eq_(mail.outbox[2].to, [u'Babis Sougias ']) @patch('remo.voting.cron.now') def test_extend_voting_period_by_24hours(self, fake_now): diff --git a/remo/voting/tests/test_models.py b/remo/voting/tests/test_models.py index 2348068cd..145f8174f 100644 --- a/remo/voting/tests/test_models.py +++ b/remo/voting/tests/test_models.py @@ -54,16 +54,20 @@ def test_send_email_to_council_members(self): """Test send emails to Council Members if an automated poll is created. """ + bug = BugFactory.create() automated_poll = Poll(name='automated_poll', start=self.start, end=self.end, valid_groups=self.group, - created_by=self.user, automated_poll=True) + created_by=self.user, automated_poll=True, + bug=bug) automated_poll.save() - eq_(len(mail.outbox), 4) - for email in mail.outbox: - if settings.REPS_COUNCIL_ALIAS in email.to: - break - else: + eq_(len(mail.outbox), 5) + if [settings.REPS_COUNCIL_ALIAS] not in [email.to + for email in mail.outbox]: raise Exception('No email sent to REPS_COUNCIL_ALIAS') + subject = '[Bug %s] Budget request discussion' % bug.id + error_message = 'No email sent to council for a new budget request.' + if subject not in [email.subject for email in mail.outbox]: + raise Exception(error_message) class AutomatedRadioPollTest(TestCase):