From d341d6d26da54d317f621199ca90b53a698bac39 Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Thu, 22 Aug 2013 17:04:38 -0700 Subject: [PATCH] Change optout to use user.id instead of email. Includes Data + Schema migrations for optout email -> user.id change. Note that migrations should be reversible. --- common/djangoapps/student/views.py | 11 ++- lms/djangoapps/bulk_email/admin.py | 2 +- .../bulk_email/migrations/0001_initial.py | 2 - .../migrations/0002_change_field_names.py | 6 -- .../migrations/0003_add_optout_user.py | 91 +++++++++++++++++++ .../migrations/0004_migrate_optout_user.py | 91 +++++++++++++++++++ .../migrations/0005_remove_optout_email.py | 78 ++++++++++++++++ lms/djangoapps/bulk_email/models.py | 10 +- lms/djangoapps/bulk_email/tasks.py | 64 ++++++++----- .../bulk_email/tests/test_course_optout.py | 3 + lms/djangoapps/bulk_email/tests/test_email.py | 58 +++++++++++- .../bulk_email/tests/test_err_handling.py | 3 +- lms/djangoapps/instructor/views/legacy.py | 41 ++++----- lms/envs/aws.py | 2 +- lms/envs/common.py | 3 +- 15 files changed, 397 insertions(+), 68 deletions(-) create mode 100644 lms/djangoapps/bulk_email/migrations/0003_add_optout_user.py create mode 100644 lms/djangoapps/bulk_email/migrations/0004_migrate_optout_user.py create mode 100644 lms/djangoapps/bulk_email/migrations/0005_remove_optout_email.py diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index ebd067942e..b1b8f6ff1e 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -1,3 +1,6 @@ +""" +Student Views +""" import datetime import feedparser import json @@ -271,7 +274,7 @@ def dashboard(request): log.error("User {0} enrolled in non-existent course {1}" .format(user.username, enrollment.course_id)) - course_optouts = Optout.objects.filter(email=user.email).values_list('course_id', flat=True) + course_optouts = Optout.objects.filter(user=user).values_list('course_id', flat=True) message = "" if not user.is_active: @@ -1289,13 +1292,13 @@ def change_email_settings(request): course_id = request.POST.get("course_id") receive_emails = request.POST.get("receive_emails") if receive_emails: - optout_object = Optout.objects.filter(email=user.email, course_id=course_id) + optout_object = Optout.objects.filter(user=user, course_id=course_id) if optout_object: optout_object.delete() - log.info(u"User {0} ({1}) opted to receive emails from course {2}".format(user.username, user.email, course_id)) + log.info(u"User {0} ({1}) opted in to receive emails from course {2}".format(user.username, user.email, course_id)) track.views.server_track(request, "change-email-settings", {"receive_emails": "yes", "course": course_id}, page='dashboard') else: - Optout.objects.get_or_create(email=request.user.email, course_id=course_id) + Optout.objects.get_or_create(user=user, course_id=course_id) log.info(u"User {0} ({1}) opted out of receiving emails from course {2}".format(user.username, user.email, course_id)) track.views.server_track(request, "change-email-settings", {"receive_emails": "no", "course": course_id}, page='dashboard') diff --git a/lms/djangoapps/bulk_email/admin.py b/lms/djangoapps/bulk_email/admin.py index 40da36629c..3b40290f5d 100644 --- a/lms/djangoapps/bulk_email/admin.py +++ b/lms/djangoapps/bulk_email/admin.py @@ -13,7 +13,7 @@ class CourseEmailAdmin(admin.ModelAdmin): class OptoutAdmin(admin.ModelAdmin): """Admin for optouts.""" - list_display = ('email', 'course_id') + list_display = ('user', 'course_id') admin.site.register(CourseEmail, CourseEmailAdmin) diff --git a/lms/djangoapps/bulk_email/migrations/0001_initial.py b/lms/djangoapps/bulk_email/migrations/0001_initial.py index 99c99d4efc..c3672a6de8 100644 --- a/lms/djangoapps/bulk_email/migrations/0001_initial.py +++ b/lms/djangoapps/bulk_email/migrations/0001_initial.py @@ -1,8 +1,6 @@ # -*- coding: utf-8 -*- -import datetime from south.db import db from south.v2 import SchemaMigration -from django.db import models class Migration(SchemaMigration): diff --git a/lms/djangoapps/bulk_email/migrations/0002_change_field_names.py b/lms/djangoapps/bulk_email/migrations/0002_change_field_names.py index 95c0db339f..93fa33a544 100644 --- a/lms/djangoapps/bulk_email/migrations/0002_change_field_names.py +++ b/lms/djangoapps/bulk_email/migrations/0002_change_field_names.py @@ -1,8 +1,6 @@ # -*- coding: utf-8 -*- -import datetime from south.db import db from south.v2 import SchemaMigration -from django.db import models class Migration(SchemaMigration): @@ -19,7 +17,6 @@ class Migration(SchemaMigration): self.gf('django.db.models.fields.TextField')(null=True, blank=True), keep_default=False) - def backwards(self, orm): # Renaming field 'CourseEmail.to_option' db.rename_column('bulk_email_courseemail', 'to_option', 'to') @@ -30,9 +27,6 @@ class Migration(SchemaMigration): # Deleting field 'CourseEmail.text_message' db.delete_column('bulk_email_courseemail', 'text_message') - - - models = { 'auth.group': { 'Meta': {'object_name': 'Group'}, diff --git a/lms/djangoapps/bulk_email/migrations/0003_add_optout_user.py b/lms/djangoapps/bulk_email/migrations/0003_add_optout_user.py new file mode 100644 index 0000000000..1bf344f6e9 --- /dev/null +++ b/lms/djangoapps/bulk_email/migrations/0003_add_optout_user.py @@ -0,0 +1,91 @@ +# -*- coding: utf-8 -*- +from south.db import db +from south.v2 import SchemaMigration + + +class Migration(SchemaMigration): + + def forwards(self, orm): + + # Adding field 'Optout.user' + db.add_column('bulk_email_optout', 'user', + self.gf('django.db.models.fields.related.ForeignKey')(to=orm['auth.User'], null=True), + keep_default=False) + + # Removing unique constraint on 'Optout', fields ['course_id', 'email'] + db.delete_unique('bulk_email_optout', ['course_id', 'email']) + + # Adding unique constraint on 'Optout', fields ['course_id', 'user'] + db.create_unique('bulk_email_optout', ['course_id', 'user_id']) + + def backwards(self, orm): + + # Removing unique constraint on 'Optout', fields ['course_id', 'user'] + db.delete_unique('bulk_email_optout', ['course_id', 'user_id']) + + # Deleting field 'Optout.email' + db.delete_column('bulk_email_optout', 'user_id') + + # Creating unique constraint on 'Optout', fields ['course_id', 'email'] + db.create_unique('bulk_email_optout', ['course_id', 'email']) + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'bulk_email.courseemail': { + 'Meta': {'object_name': 'CourseEmail'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'html_message': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'modified': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), + 'sender': ('django.db.models.fields.related.ForeignKey', [], {'default': '1', 'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}), + 'slug': ('django.db.models.fields.CharField', [], {'max_length': '128', 'db_index': 'True'}), + 'subject': ('django.db.models.fields.CharField', [], {'max_length': '128', 'blank': 'True'}), + 'text_message': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'to_option': ('django.db.models.fields.CharField', [], {'default': "'myself'", 'max_length': '64'}) + }, + 'bulk_email.optout': { + 'Meta': {'unique_together': "(('user', 'course_id'),)", 'object_name': 'Optout'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'email': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + } + } + + complete_apps = ['bulk_email'] diff --git a/lms/djangoapps/bulk_email/migrations/0004_migrate_optout_user.py b/lms/djangoapps/bulk_email/migrations/0004_migrate_optout_user.py new file mode 100644 index 0000000000..6dd2129466 --- /dev/null +++ b/lms/djangoapps/bulk_email/migrations/0004_migrate_optout_user.py @@ -0,0 +1,91 @@ +# -*- coding: utf-8 -*- +from south.db import db +from south.v2 import DataMigration +from django.core.exceptions import ObjectDoesNotExist + + +class Migration(DataMigration): + + def forwards(self, orm): + + # forwards data migration to copy over existing emails to associated ids + if not db.dry_run: + for optout in orm.Optout.objects.all(): + try: + user = orm['auth.User'].objects.get(email=optout.email) + optout.user = user + optout.save() + except ObjectDoesNotExist: + # if user is not found (because they have already changed their email) + # then delete the optout, as it's no longer useful. + optout.delete() + + def backwards(self, orm): + + # backwards data migration to copy over emails of students to old email slot + if not db.dry_run: + for optout in orm.Optout.objects.all(): + if optout.user is not None: + optout.email = optout.user.email + optout.save() + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'bulk_email.courseemail': { + 'Meta': {'object_name': 'CourseEmail'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'html_message': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'modified': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), + 'sender': ('django.db.models.fields.related.ForeignKey', [], {'default': '1', 'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}), + 'slug': ('django.db.models.fields.CharField', [], {'max_length': '128', 'db_index': 'True'}), + 'subject': ('django.db.models.fields.CharField', [], {'max_length': '128', 'blank': 'True'}), + 'text_message': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'to_option': ('django.db.models.fields.CharField', [], {'default': "'myself'", 'max_length': '64'}) + }, + 'bulk_email.optout': { + 'Meta': {'unique_together': "(('user', 'course_id'),)", 'object_name': 'Optout'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'email': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + } + } + + complete_apps = ['bulk_email'] diff --git a/lms/djangoapps/bulk_email/migrations/0005_remove_optout_email.py b/lms/djangoapps/bulk_email/migrations/0005_remove_optout_email.py new file mode 100644 index 0000000000..3639d1e473 --- /dev/null +++ b/lms/djangoapps/bulk_email/migrations/0005_remove_optout_email.py @@ -0,0 +1,78 @@ +# -*- coding: utf-8 -*- +from south.db import db +from south.v2 import SchemaMigration + + +class Migration(SchemaMigration): + + def forwards(self, orm): + + # Deleting field 'Optout.email' + db.delete_column('bulk_email_optout', 'email') + + def backwards(self, orm): + + # Adding field 'Optout.email' + db.add_column('bulk_email_optout', 'email', + self.gf('django.db.models.fields.CharField')(max_length=255, null=True, blank=True), + keep_default=False) + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'bulk_email.courseemail': { + 'Meta': {'object_name': 'CourseEmail'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'html_message': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'modified': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), + 'sender': ('django.db.models.fields.related.ForeignKey', [], {'default': '1', 'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}), + 'slug': ('django.db.models.fields.CharField', [], {'max_length': '128', 'db_index': 'True'}), + 'subject': ('django.db.models.fields.CharField', [], {'max_length': '128', 'blank': 'True'}), + 'text_message': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'to_option': ('django.db.models.fields.CharField', [], {'default': "'myself'", 'max_length': '64'}) + }, + 'bulk_email.optout': { + 'Meta': {'unique_together': "(('user', 'course_id'),)", 'object_name': 'Optout'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + } + } + + complete_apps = ['bulk_email'] diff --git a/lms/djangoapps/bulk_email/models.py b/lms/djangoapps/bulk_email/models.py index 8fa864b955..72c9569cc1 100644 --- a/lms/djangoapps/bulk_email/models.py +++ b/lms/djangoapps/bulk_email/models.py @@ -30,7 +30,7 @@ class Email(models.Model): created = models.DateTimeField(auto_now_add=True) modified = models.DateTimeField(auto_now=True) - class Meta: + class Meta: # pylint: disable=C0111 abstract = True @@ -61,10 +61,10 @@ class CourseEmail(Email, models.Model): class Optout(models.Model): """ - Stores emails that have opted out of receiving emails from a course. + Stores users that have opted out of receiving emails from a course. """ - email = models.CharField(max_length=255, db_index=True) + user = models.ForeignKey(User, db_index=True, null=True) course_id = models.CharField(max_length=255, db_index=True) - class Meta: - unique_together = ('email', 'course_id') + class Meta: # pylint: disable=C0111 + unique_together = ('user', 'course_id') diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index 35c57b9dad..7afd286570 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/lms/djangoapps/bulk_email/tasks.py @@ -2,10 +2,10 @@ This module contains celery task functions for handling the sending of bulk email to a course. """ -import logging import math import re import time +import gc from smtplib import SMTPServerDisconnected, SMTPDataError, SMTPConnectError @@ -14,13 +14,14 @@ from django.contrib.auth.models import User, Group from django.core.mail import EmailMultiAlternatives, get_connection from django.http import Http404 from celery import task, current_task +from celery.utils.log import get_task_logger from bulk_email.models import CourseEmail, Optout from courseware.access import _course_staff_group_name, _course_instructor_group_name from courseware.courses import get_course_by_id from mitxmako.shortcuts import render_to_string -log = logging.getLogger(__name__) +log = get_task_logger(__name__) @task(default_retry_delay=10, max_retries=5) # pylint: disable=E1102 @@ -47,37 +48,42 @@ def delegate_email_batches(email_id, to_option, course_id, course_url, user_id): raise delegate_email_batches.retry(arg=[email_id, to_option, course_id, course_url, user_id], exc=exc) if to_option == "myself": - recipient_qset = User.objects.filter(id=user_id).values('profile__name', 'email') - + recipient_qset = User.objects.filter(id=user_id) elif to_option == "all" or to_option == "staff": staff_grpname = _course_staff_group_name(course.location) staff_group, _ = Group.objects.get_or_create(name=staff_grpname) - staff_qset = staff_group.user_set.values('profile__name', 'email') + staff_qset = staff_group.user_set.all() instructor_grpname = _course_instructor_group_name(course.location) instructor_group, _ = Group.objects.get_or_create(name=instructor_grpname) - instructor_qset = instructor_group.user_set.values('profile__name', 'email') + instructor_qset = instructor_group.user_set.all() recipient_qset = staff_qset | instructor_qset if to_option == "all": - # Two queries are executed per performance considerations for MySQL. - # See https://docs.djangoproject.com/en/1.2/ref/models/querysets/#in. - course_optouts = Optout.objects.filter(course_id=course_id).values_list('email', flat=True) - enrollment_qset = User.objects.filter(courseenrollment__course_id=course_id).exclude(email__in=list(course_optouts)).values('profile__name', 'email') + enrollment_qset = User.objects.filter(courseenrollment__course_id=course_id, + courseenrollment__is_active=True) recipient_qset = recipient_qset | enrollment_qset recipient_qset = recipient_qset.distinct() - else: log.error("Unexpected bulk email TO_OPTION found: %s", to_option) raise Exception("Unexpected bulk email TO_OPTION found: {0}".format(to_option)) - recipient_list = list(recipient_qset) - total_num_emails = len(recipient_list) - num_workers = int(math.ceil(float(total_num_emails) / float(settings.EMAILS_PER_TASK))) - 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(email_id, to_list, course.display_name, course_url, False) + recipient_qset = recipient_qset.order_by('pk') + total_num_emails = recipient_qset.count() + num_queries = int(math.ceil(float(total_num_emails) / float(settings.EMAILS_PER_QUERY))) + last_pk = recipient_qset[0].pk - 1 + num_workers = 0 + for j in range(num_queries): + recipient_sublist = list(recipient_qset.order_by('pk').filter(pk__gt=last_pk) + .values('profile__name', 'email', 'pk')[:settings.EMAILS_PER_QUERY]) + last_pk = recipient_sublist[-1]['pk'] + num_emails_this_query = len(recipient_sublist) + num_tasks_this_query = int(math.ceil(float(num_emails_this_query) / float(settings.EMAILS_PER_TASK))) + chunk = int(math.ceil(float(num_emails_this_query) / float(num_tasks_this_query))) + for i in range(num_tasks_this_query): + to_list = recipient_sublist[i * chunk:i * chunk + chunk] + course_email.delay(email_id, to_list, course.display_name, course_url, False) + num_workers += num_tasks_this_query + gc.collect() return num_workers @@ -89,12 +95,22 @@ def course_email(email_id, to_list, course_title, course_url, throttle=False): being the only "to". Emails are sent multipart, in both plain text and html. """ + try: msg = CourseEmail.objects.get(id=email_id) except CourseEmail.DoesNotExist as exc: log.exception(exc.args[0]) raise exc + # exclude optouts + optouts = Optout.objects.filter(course_id=msg.course_id, + user__email__in=[i['email'] for i in to_list])\ + .values_list('user__email', flat=True) + + num_optout = len(optouts) + + to_list = filter(lambda x: x['email'] not in optouts, to_list) + subject = "[" + course_title + "] " + msg.subject course_title_no_quotes = re.sub(r'"', '', course_title) @@ -114,9 +130,9 @@ def course_email(email_id, to_list, course_title, course_url, throttle=False): } while to_list: - (name, email) = to_list[-1].values() - email_context['name'] = name + email = to_list[-1]['email'] email_context['email'] = email + email_context['name'] = to_list[-1]['profile__name'] html_footer = render_to_string( 'emails/email_footer.html', @@ -157,7 +173,7 @@ def course_email(email_id, to_list, course_title, course_url, throttle=False): to_list.pop() connection.close() - return course_email_result(num_sent, num_error) + return course_email_result(num_sent, num_error, num_optout) except (SMTPDataError, SMTPConnectError, SMTPServerDisconnected) as exc: # Error caught here cause the email to be retried. The entire task is actually retried without popping the list @@ -175,6 +191,6 @@ def course_email(email_id, to_list, course_title, course_url, throttle=False): # This string format code is wrapped in this function to allow mocking for a unit test -def course_email_result(num_sent, num_error): +def course_email_result(num_sent, num_error, num_optout): """Return the formatted result of course_email sending.""" - return "Sent {0}, Fail {1}".format(num_sent, num_error) + return "Sent {0}, Fail {1}, Optout {2}".format(num_sent, num_error, num_optout) diff --git a/lms/djangoapps/bulk_email/tests/test_course_optout.py b/lms/djangoapps/bulk_email/tests/test_course_optout.py index 499ccb0b95..18f04cebc1 100644 --- a/lms/djangoapps/bulk_email/tests/test_course_optout.py +++ b/lms/djangoapps/bulk_email/tests/test_course_optout.py @@ -10,6 +10,7 @@ from django.test.utils import override_settings from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE from student.tests.factories import UserFactory, AdminFactory, CourseEnrollmentFactory +from student.models import CourseEnrollment from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -94,6 +95,8 @@ class TestOptoutCourseEmails(ModuleStoreTestCase): self.client.logout() + self.assertTrue(CourseEnrollment.is_enrolled(self.student, self.course.id)) + self.client.login(username=self.instructor.username, password="test") self.navigate_to_email_view() diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py index 67f49d1f51..b0cf8dc06e 100644 --- a/lms/djangoapps/bulk_email/tests/test_email.py +++ b/lms/djangoapps/bulk_email/tests/test_email.py @@ -2,7 +2,6 @@ """ Unit tests for sending course email """ - from django.test.utils import override_settings from django.conf import settings from django.core import mail @@ -14,12 +13,29 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from bulk_email.tasks import delegate_email_batches, course_email -from bulk_email.models import CourseEmail +from bulk_email.models import CourseEmail, Optout from mock import patch STAFF_COUNT = 3 STUDENT_COUNT = 10 +LARGE_NUM_EMAILS = 137 + + +class MockCourseEmailResult(object): + """ + A small closure-like class to keep count of emails sent over all tasks, recorded + by mock object side effects + """ + emails_sent = 0 + + def get_mock_course_email_result(self): + """Wrapper for mock email function.""" + def mock_course_email_result(sent, failed, output, **kwargs): # pylint: disable=W0613 + """Increments count of number of emails sent.""" + self.emails_sent += sent + return True + return mock_course_email_result @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) @@ -110,6 +126,7 @@ class TestEmailSendFromDashboard(ModuleStoreTestCase): self.assertContains(response, "Your email was successfully queued for sending.") + # the 1 is for the instructor in this test and others self.assertEquals(len(mail.outbox), 1 + len(self.staff)) self.assertItemsEqual( [e.to[0] for e in mail.outbox], @@ -225,6 +242,43 @@ class TestEmailSendFromDashboard(ModuleStoreTestCase): [self.instructor.email] + [s.email for s in self.staff] + [s.email for s in self.students] ) + @override_settings(EMAILS_PER_TASK=3, EMAILS_PER_QUERY=7) + @patch('bulk_email.tasks.course_email_result') + def test_chunked_queries_send_numerous_emails(self, email_mock): + """ + Test sending a large number of emails, to test the chunked querying + """ + mock_factory = MockCourseEmailResult() + email_mock.side_effect = mock_factory.get_mock_course_email_result() + added_users = [] + for _ in xrange(LARGE_NUM_EMAILS): + user = UserFactory() + added_users.append(user) + CourseEnrollmentFactory.create(user=user, course_id=self.course.id) + + optouts = [] + for i in [1, 3, 9, 10, 18]: # 5 random optouts + user = added_users[i] + optouts.append(user) + optout = Optout(user=user, course_id=self.course.id) + optout.save() + + test_email = { + 'action': 'Send email', + 'to_option': 'all', + 'subject': 'test subject for all', + 'message': 'test message for all' + } + response = self.client.post(self.url, test_email) + self.assertContains(response, "Your email was successfully queued for sending.") + self.assertEquals(mock_factory.emails_sent, + 1 + len(self.staff) + len(self.students) + LARGE_NUM_EMAILS - len(optouts)) + self.assertItemsEqual( + [e.to[0] for e in mail.outbox], + [self.instructor.email] + [s.email for s in self.staff] + [s.email for s in self.students] + + [s.email for s in added_users if s not in optouts] + ) + @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) class TestEmailSendExceptions(ModuleStoreTestCase): diff --git a/lms/djangoapps/bulk_email/tests/test_err_handling.py b/lms/djangoapps/bulk_email/tests/test_err_handling.py index e7d2e62d4a..606a0bef88 100644 --- a/lms/djangoapps/bulk_email/tests/test_err_handling.py +++ b/lms/djangoapps/bulk_email/tests/test_err_handling.py @@ -94,7 +94,8 @@ class TestEmailErrors(ModuleStoreTestCase): # We shouldn't retry when hitting a 5xx error self.assertFalse(retry.called) # Test that after the rejected email, the rest still successfully send - ((sent, fail), _) = result.call_args + ((sent, fail, optouts), _) = result.call_args + self.assertEquals(optouts, 0) self.assertEquals(fail, 1) self.assertEquals(sent, settings.EMAILS_PER_TASK - 1) diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index a2ccf0a5bc..15b0df59c1 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/lms/djangoapps/instructor/views/legacy.py @@ -56,7 +56,6 @@ from mitxmako.shortcuts import render_to_string from bulk_email.models import CourseEmail from html_to_text import html_to_text -import datetime from bulk_email import tasks log = logging.getLogger(__name__) @@ -66,11 +65,11 @@ FORUM_ROLE_ADD = 'add' FORUM_ROLE_REMOVE = 'remove' -def split_by_comma_and_whitespace(s): +def split_by_comma_and_whitespace(a_str): """ - Return string s, split by , or whitespace + Return string a_str, split by , or whitespace """ - return re.split(r'[\s,]', s) + return re.split(r'[\s,]', a_str) @ensure_csrf_cookie @@ -124,13 +123,13 @@ def instructor_dashboard(request, course_id): datatable['data'] = data return datatable - def return_csv(fn, datatable, fp=None): + def return_csv(func, datatable, file_pointer=None): """Outputs a CSV file from the contents of a datatable.""" - if fp is None: + if file_pointer is None: response = HttpResponse(mimetype='text/csv') - response['Content-Disposition'] = 'attachment; filename={0}'.format(fn) + response['Content-Disposition'] = 'attachment; filename={0}'.format(func) else: - response = fp + response = file_pointer writer = csv.writer(response, dialect='excel', quotechar='"', quoting=csv.QUOTE_ALL) writer.writerow(datatable['header']) for datarow in datatable['data']: @@ -279,11 +278,11 @@ def instructor_dashboard(request, course_id): msg += 'Failed to create a background task for rescoring "{0}".'.format(problem_url) else: track.views.server_track(request, "rescore-all-submissions", {"problem": problem_url, "course": course_id}, page="idashboard") - except ItemNotFoundError as e: + except ItemNotFoundError as err: msg += 'Failed to create a background task for rescoring "{0}": problem not found.'.format(problem_url) - except Exception as e: - log.error("Encountered exception from rescore: {0}".format(e)) - msg += 'Failed to create a background task for rescoring "{0}": {1}.'.format(problem_url, e.message) + except Exception as err: + log.error("Encountered exception from rescore: {0}".format(err)) + msg += 'Failed to create a background task for rescoring "{0}": {1}.'.format(problem_url, err.message) elif "Reset ALL students' attempts" in action: problem_urlname = request.POST.get('problem_for_all_students', '') @@ -294,12 +293,12 @@ def instructor_dashboard(request, course_id): msg += 'Failed to create a background task for resetting "{0}".'.format(problem_url) else: track.views.server_track(request, "reset-all-attempts", {"problem": problem_url, "course": course_id}, page="idashboard") - except ItemNotFoundError as e: - log.error('Failure to reset: unknown problem "{0}"'.format(e)) + except ItemNotFoundError as err: + log.error('Failure to reset: unknown problem "{0}"'.format(err)) msg += 'Failed to create a background task for resetting "{0}": problem not found.'.format(problem_url) - except Exception as e: - log.error("Encountered exception from reset: {0}".format(e)) - msg += 'Failed to create a background task for resetting "{0}": {1}.'.format(problem_url, e.message) + except Exception as err: + log.error("Encountered exception from reset: {0}".format(err)) + msg += 'Failed to create a background task for resetting "{0}": {1}.'.format(problem_url, err.message) elif "Show Background Task History for Student" in action: # put this before the non-student case, since the use of "in" will cause this to be missed @@ -475,10 +474,10 @@ def instructor_dashboard(request, course_id): return return_csv('grades %s.csv' % aname, datatable) elif 'remote gradebook' in action: - fp = StringIO() - return_csv('', datatable, fp=fp) - fp.seek(0) - files = {'datafile': fp} + file_pointer = StringIO() + return_csv('', datatable, file_pointer=file_pointer) + file_pointer.seek(0) + files = {'datafile': file_pointer} msg2, _ = _do_remote_gradebook(request.user, course, 'post-grades', files=files) msg += msg2 diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 45738e6d31..a22fbc5bb6 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -106,7 +106,7 @@ EMAIL_HOST = ENV_TOKENS.get('EMAIL_HOST', 'localhost') # django default is loca EMAIL_PORT = ENV_TOKENS.get('EMAIL_PORT', 25) # django default is 25 EMAIL_USE_TLS = ENV_TOKENS.get('EMAIL_USE_TLS', False) # django default is False EMAILS_PER_TASK = ENV_TOKENS.get('EMAILS_PER_TASK', 100) - +EMAILS_PER_QUERY = ENV_TOKENS.get('EMAILS_PER_QUERY', 1000) SITE_NAME = ENV_TOKENS['SITE_NAME'] SESSION_ENGINE = ENV_TOKENS.get('SESSION_ENGINE', SESSION_ENGINE) SESSION_COOKIE_DOMAIN = ENV_TOKENS.get('SESSION_COOKIE_DOMAIN') diff --git a/lms/envs/common.py b/lms/envs/common.py index 51fba25e69..5ad81d5f03 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -364,7 +364,8 @@ IGNORABLE_404_ENDS = ('favicon.ico') EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend' DEFAULT_FROM_EMAIL = 'registration@edx.org' DEFAULT_BULK_FROM_EMAIL = 'course-updates@edx.org' -EMAILS_PER_TASK = 10 +EMAILS_PER_TASK = 100 +EMAILS_PER_QUERY = 1000 DEFAULT_FEEDBACK_EMAIL = 'feedback@edx.org' SERVER_EMAIL = 'devops@edx.org' TECH_SUPPORT_EMAIL = 'technical@edx.org'