-
Notifications
You must be signed in to change notification settings - Fork 4.3k
DO NOT MERGE bulk email work for discussion #30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
192dbed
ae6e1d4
368e211
63c63d4
7e007d7
5d7a39e
77fafd4
e3cb34b
32c755b
e41cfb9
469567e
ec046aa
509fd7a
4e0a32d
99f520d
ec64311
a1e17b1
3da0d33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| from django.db import models | ||
| from django.contrib.auth.models import User | ||
|
|
||
| class Email(models.Model): | ||
| sender = models.ForeignKey(User, default=1, blank=True, null=True) | ||
| hash = models.CharField(max_length=128, db_index=True) | ||
| subject = models.CharField(max_length=128, blank=True) | ||
| html_message = models.TextField(null=True, blank=True) | ||
| created = models.DateTimeField(auto_now_add=True) | ||
| modified = models.DateTimeField(auto_now=True) | ||
| class Meta: | ||
| abstract = True | ||
|
|
||
| class CourseEmail(Email, models.Model): | ||
| course_id = models.CharField(max_length=255, db_index=True) | ||
| to = models.CharField(max_length=64) | ||
|
|
||
| def __unicode__(self): | ||
| return self.subject |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| from celery import task | ||
| from django.conf import settings | ||
| from django.contrib.auth.models import User | ||
| from bulk_email.models import * | ||
|
|
||
| import math | ||
|
|
||
| EMAILS_PER_WORKER=getattr(settings, 'EMAILS_PER_WORKER', 10) | ||
|
|
||
| @task() | ||
| def delegate_emails(hash_for_msg, recipient, course): | ||
| ''' | ||
| Delegates emails by querying for the list of recipients who should | ||
| get the mail, chopping up into batches of EMAILS_PER_WORKER size, | ||
| and queueing up worker jobs. | ||
|
|
||
| Recipient is {'students', 'staff', or 'all'} | ||
|
|
||
| Returns the number of batches (workers) kicked off. | ||
| ''' | ||
|
|
||
| recipient_qset = User.objects.all() | ||
| if recipient == "students": | ||
| #get student list | ||
| pass | ||
| elif recipient == "staff": | ||
| #get staff list | ||
| pass | ||
| else: | ||
| #everyone | ||
| pass | ||
| recipient_list = list(recipient_qset) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will pull potentially all user objects into memory, which seems less than ideal. Why doesn't each worker do the query for their chunk of the data?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we originally coded this up we passed around a query set. This was more efficient, as you said, since you would only have to instantiate user objects that were needed per batch, instead of all at once here. But this is a good observation, @cpennington, that we should be careful that we can actually execute this query without hitting memory limits on Also: if we're going to the trouble to actually get all this data, then we should probably build a dict for the values that we will need for each user here and actually pass that along in the batch. I believe the four fields we will need are just name, email address, optout flag, and optout code. |
||
|
|
||
| total_num_emails = recipient_qset.count() | ||
| num_workers=int(math.ceil(float(total_num_emails)/float(EMAILS_PER_WORKER))) | ||
| chunk=int(math.ceil(float(total_num_emails)/float(num_workers))) | ||
|
|
||
| for i in range(num_workers): | ||
| to_list=recipient_list[i*chunk:i*chunk+chunk] | ||
| course_email.delay(hash_for_msg, to_list, False, course) | ||
| return num_workers | ||
|
|
||
|
|
||
| @task(default_retry_delay=15, max_retries=5) | ||
| def course_email(hash_for_msg, to_list, course, throttle=False): | ||
| """ | ||
| Takes a subject and an html formatted email and sends it from | ||
| sender to all addresses in the to_list, with each recipient | ||
| being the only "to". Emails are sent multipart, in both plain | ||
| text and html. | ||
| """ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doc string looks out of date, since this function can't be called with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep |
||
|
|
||
| msg = CourseEmail.objects.get(hash=hash_for_msg) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| """ | ||
| Unit tests for email feature in instructor dashboard | ||
|
|
||
| Based on (and depends on) unit tests for courseware. | ||
| """ | ||
|
|
||
| from django.test.utils import override_settings | ||
|
|
||
| # Need access to internal func to put users in the right group | ||
| from django.contrib.auth.models import Group | ||
|
|
||
| from django.conf import settings | ||
| from django.core.urlresolvers import reverse | ||
|
|
||
| from courseware.access import _course_staff_group_name | ||
| from courseware.tests.tests import LoginEnrollmentTestCase, TEST_DATA_XML_MODULESTORE, get_user | ||
| from xmodule.modulestore.django import modulestore | ||
| import xmodule.modulestore.django | ||
|
|
||
| @override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) | ||
| class TestInstructorDashboardEmailView(LoginEnrollmentTestCase): | ||
| ''' | ||
| Check for email view displayed with flag | ||
| ''' | ||
|
|
||
| def setUp(self): | ||
| xmodule.modulestore.django._MODULESTORES = {} | ||
|
|
||
| self.toy = modulestore().get_course("edX/toy/2012_Fall") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure we're moving away from using this way of mocking courses and using CourseFactory instead. There are a few examples of this floating around, and the definition for it is in common/lib/xmodule/xmodule/modulestore/tests/factories.py . @wedaly would have better information on how to use it and how to improve it if you need it to do more than it currently does.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe @kluo just followed examples he found elsewhere. If you need him to recode in that manner, then he could if need be. I can't tell however if the new method is just desire/preference or the must-do way for some stronger reason (safer? more robust?). I guess before asking someone to recode something I'd like to explain why. |
||
|
|
||
| # Create instructor account | ||
| self.instructor = 'view@test.com' | ||
| self.password = 'foo' | ||
| self.create_account('u1', self.instructor, self.password) | ||
| self.activate_user(self.instructor) | ||
|
|
||
| group_name = _course_staff_group_name(self.toy.location) | ||
| g = Group.objects.create(name=group_name) | ||
| g.user_set.add(get_user(self.instructor)) | ||
|
|
||
| self.logout() | ||
| self.login(self.instructor, self.password) | ||
| self.enroll(self.toy) | ||
|
|
||
| def test_email_flag_true(self): | ||
| oldEmailFlag = settings.MITX_FEATURES['ENABLE_INSTRUCTOR_EMAIL'] | ||
| settings.MITX_FEATURES['ENABLE_INSTRUCTOR_EMAIL'] = True | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of hacking around the settings this way, I would recommend using django's settings override feature: https://docs.djangoproject.com/en/1.4/topics/testing/#overriding-settings I've had some trouble dealing with it myself in the past, but it might work in this case.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't use override_settings here because I'm just changing one pair of the MITX_FEATURES dictionary, and it can't take in a dict key expression. I think it would only work if I overrode the entire MITX_FEATURES dictionary.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe @gwprice had had luck using patch.dict from the mock library to -Cale
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One nice pattern in that override settings page might be useful here. If you use the decorator it will ensure cleanup to settings when you make modifications. More robust, ie. when an exception happened somewhere before you had a chance to clean up. That seems pretty clean to me |
||
| response = self.client.get(reverse('instructor_dashboard', | ||
| kwargs={'course_id': self.toy.id})) | ||
| email_link = '<a href="#" onclick="goto(\'Email\')" class="None">Email</a>' | ||
| self.assertTrue(email_link in response.content) | ||
|
|
||
| session = self.client.session | ||
| session['idash_mode'] = 'Email' | ||
| session.save() | ||
| response = self.client.get(reverse('instructor_dashboard', | ||
| kwargs={'course_id': self.toy.id})) | ||
| selected_email_link = '<a href="#" onclick="goto(\'Email\')" class="selectedmode">Email</a>' | ||
| self.assertTrue(selected_email_link in response.content) | ||
| send_to_label = '<label for="id_to">Send to:</label>' | ||
| self.assertTrue(send_to_label in response.content) | ||
|
|
||
| del self.client.session['idash_mode'] | ||
| settings.MITX_FEATURES['ENABLE_INSTRUCTOR_EMAIL'] = oldEmailFlag | ||
|
|
||
| def test_email_flag_false(self): | ||
| oldEmailFlag = settings.MITX_FEATURES['ENABLE_INSTRUCTOR_EMAIL'] | ||
| settings.MITX_FEATURES['ENABLE_INSTRUCTOR_EMAIL'] = False | ||
| response = self.client.get(reverse('instructor_dashboard', | ||
| kwargs={'course_id': self.toy.id})) | ||
| email_link = '<a href="#" onclick="goto(\'Email\')" class="None">Email</a>' | ||
| self.assertFalse(email_link in response.content) | ||
|
|
||
| settings.MITX_FEATURES['ENABLE_INSTRUCTOR_EMAIL'] = oldEmailFlag | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,11 @@ | |
|
|
||
| from .offline_gradecalc import student_grades, offline_grades_available | ||
|
|
||
| from bulk_email.models import CourseEmail | ||
| import datetime | ||
| from hashlib import md5 | ||
| from bulk_email.tasks import delegate_emails | ||
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| # internal commands for managing forum roles: | ||
|
|
@@ -586,7 +591,24 @@ def getdat(u): | |
| ret = _do_enroll_students(course, course_id, students, overload=overload) | ||
| datatable = ret['datatable'] | ||
|
|
||
|
|
||
| #---------------------------------------- | ||
|
|
||
| elif action == 'Send email': | ||
| to = request.POST.get("to") | ||
| subject = request.POST.get("subject") | ||
| html_message = request.POST.get("message") | ||
|
|
||
| email = CourseEmail(course_id=course_id, | ||
| sender=request.user, | ||
| to=to, | ||
| subject=subject, | ||
| html_message=html_message, | ||
| hash=md5((html_message+subject+datetime.datetime.isoformat(datetime.datetime.now())).encode('utf-8')).hexdigest()) | ||
| email.save() | ||
|
|
||
| delegate_emails(email.hash, email.to, course) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I worry a little bit about transactions and race conditions. In particular, this transaction won't end until the request does, which means if the worker is especially speedy, it could try and look up the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be called with .delay?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To your concern, @cpennington, all the more reason why this should be queued for fanout later. That way the delegation job is queued, and then the request is done and the transaction commits. Writing the message should happen then. The message isn't read until worker tasks are starting to be spun up by the remote machine. So there is a race potential here but feels unlikely. Safeguard for this would be to test for the content (query on hash) before doing the delegation. If it's not there, requeue so it can be retried. Celery supports retries like this. |
||
|
|
||
| #---------------------------------------- | ||
| # psychometrics | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this an
@task? It looks to me like it's supposed to be run on the webapp side, and it triggers the remote tasks (which seems to me to say that it isn't a task itself).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This task that fans out all the other tasks itself can take a while to run, so we don't want it to be synchronous on the web app side. For a 50k member MOOC, say, it has to kick off 5k tasks, assuming 10 mails per batch, which is what we used on Class2Go.