From d2f148b5f9fbf33af1d70ce052bb169921be71e1 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 13 Feb 2015 10:22:55 -0500 Subject: [PATCH 1/6] Bump MongoDBProxy to handle AutoReconnect during find_one --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 292d26ccea..5e1efdc422 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -11,7 +11,7 @@ -e git+https://github.com/edx/django-pipeline.git@88ec8a011e481918fdc9d2682d4017c835acd8be#egg=django-pipeline -e git+https://github.com/edx/django-wiki.git@cd0b2b31997afccde519fe5b3365e61a9edb143f#egg=django-wiki -e git+https://github.com/edx/django-oauth2-provider.git@0.2.7-fork-edx-2#egg=django-oauth2-provider --e git+https://github.com/edx/MongoDBProxy.git@efe14679c9263ab491916ed960f5930127e05faf#egg=mongodb_proxy +-e git+https://github.com/edx/MongoDBProxy.git@25b99097615bda06bd7cdfe5669ed80dc2a7fed0#egg=mongodb_proxy -e git+https://github.com/dementrock/pystache_custom.git@776973740bdaad83a3b029f96e415a7d1e8bec2f#egg=pystache_custom-dev -e git+https://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk -e git+https://github.com/un33k/django-ipware.git@42cb1bb1dc680a60c6452e8bb2b843c2a0382c90#egg=django-ipware From 8b599ee424d70e0856c1ea20585ea0986759ba93 Mon Sep 17 00:00:00 2001 From: Syed Hassan Raza Date: Fri, 13 Feb 2015 16:21:13 +0500 Subject: [PATCH 2/6] python standard logging --- lms/djangoapps/bulk_email/tasks.py | 4 ++-- lms/djangoapps/instructor_task/subtasks.py | 4 ++-- lms/djangoapps/instructor_task/tasks_helper.py | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index 4cb5384137..d136f42a65 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/lms/djangoapps/bulk_email/tasks.py @@ -7,6 +7,7 @@ import random import json from time import sleep from collections import Counter +import logging import dogstats_wrapper as dog_stats_api from smtplib import SMTPServerDisconnected, SMTPDataError, SMTPConnectError, SMTPException @@ -24,7 +25,6 @@ from boto.ses.exceptions import ( from boto.exception import AWSConnectionError from celery import task, current_task -from celery.utils.log import get_task_logger from celery.states import SUCCESS, FAILURE, RETRY from celery.exceptions import RetryTaskError @@ -48,7 +48,7 @@ from instructor_task.subtasks import ( ) from util.query import use_read_replica_if_available -log = get_task_logger(__name__) +log = logging.getLogger('edx.celery.task') # Errors that an individual email is failing to be sent, and should just diff --git a/lms/djangoapps/instructor_task/subtasks.py b/lms/djangoapps/instructor_task/subtasks.py index 093eddce98..dae83f9d6b 100644 --- a/lms/djangoapps/instructor_task/subtasks.py +++ b/lms/djangoapps/instructor_task/subtasks.py @@ -7,8 +7,8 @@ from uuid import uuid4 import math import psutil from contextlib import contextmanager +import logging -from celery.utils.log import get_task_logger from celery.states import SUCCESS, READY_STATES, RETRY import dogstats_wrapper as dog_stats_api @@ -17,7 +17,7 @@ from django.core.cache import cache from instructor_task.models import InstructorTask, PROGRESS, QUEUING -TASK_LOG = get_task_logger(__name__) +TASK_LOG = logging.getLogger('edx.celery.task') # Lock expiration should be long enough to allow a subtask to complete. SUBTASK_LOCK_EXPIRE = 60 * 10 # Lock expires in 10 minutes diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index 7f48999633..e6b0b62c4d 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -7,9 +7,9 @@ import json from datetime import datetime from time import time import unicodecsv +import logging from celery import Task, current_task -from celery.utils.log import get_task_logger from celery.states import SUCCESS, FAILURE from django.contrib.auth.models import User from django.core.files.storage import DefaultStorage @@ -38,7 +38,7 @@ from student.models import CourseEnrollment # define different loggers for use within tasks and on client side -TASK_LOG = get_task_logger(__name__) +TASK_LOG = logging.getLogger('edx.celery.task') # define value to use when no task_id is provided: UNKNOWN_TASK_ID = 'unknown-task_id' From 4384e57f0f16da9e7eb9e5f5c173850750babb5e Mon Sep 17 00:00:00 2001 From: Will Daly Date: Wed, 18 Feb 2015 17:27:28 -0500 Subject: [PATCH 3/6] Force English for email opt in text appearing on the marketing site --- lms/djangoapps/courseware/tests/test_views.py | 8 ++++++-- lms/djangoapps/courseware/views.py | 18 +++++++++--------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 002b94bcff..166117c925 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -370,13 +370,17 @@ class ViewsTestCase(ModuleStoreTestCase): # in the label escaped as expected. self._email_opt_in_checkbox(response, cgi.escape(self.org_html)) - @patch.dict(settings.FEATURES, {'IS_EDX_DOMAIN': True}) + @patch.dict(settings.FEATURES, { + 'IS_EDX_DOMAIN': True, + 'ENABLE_MKTG_EMAIL_OPT_IN': True + }) def test_mktg_about_language_edx_domain(self): # Since we're in an edx-controlled domain, and our marketing site # supports only English, override the language setting # and use English. - response = self._load_mktg_about(language='eo') + response = self._load_mktg_about(language='eo', org=self.org_html) self.assertContains(response, "Enroll in") + self.assertContains(response, "and learn about its other programs") @patch.dict(settings.FEATURES, {'IS_EDX_DOMAIN': False}) def test_mktg_about_language_openedx(self): diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index b69c5c7fbd..04360d12a3 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -899,6 +899,15 @@ def mktg_course_about(request, course_id): 'course_modes': course_modes, } + # The edx.org marketing site currently displays only in English. + # To avoid displaying a different language in the register / access button, + # we force the language to English. + # However, OpenEdX installations with a different marketing front-end + # may want to respect the language specified by the user or the site settings. + force_english = settings.FEATURES.get('IS_EDX_DOMAIN', False) + if force_english: + translation.activate('en-us') + if settings.FEATURES.get('ENABLE_MKTG_EMAIL_OPT_IN'): # Drupal will pass organization names using a GET parameter, as follows: # ?org=Harvard @@ -932,15 +941,6 @@ def mktg_course_about(request, course_id): len(org_list) ).format(institution_series=org_name_string) - # The edx.org marketing site currently displays only in English. - # To avoid displaying a different language in the register / access button, - # we force the language to English. - # However, OpenEdX installations with a different marketing front-end - # may want to respect the language specified by the user or the site settings. - force_english = settings.FEATURES.get('IS_EDX_DOMAIN', False) - if force_english: - translation.activate('en-us') - try: return render_to_response('courseware/mktg_course_about.html', context) finally: From 5c00db9fc24d6086379c7d1c0b0b30901ba5aa41 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Fri, 20 Feb 2015 11:35:08 -0500 Subject: [PATCH 4/6] Make the linked in configuration migration backwards compatible --- ..._field.py => 0044_linkedin_add_company_identifier.py} | 9 +-------- common/djangoapps/student/models.py | 3 +++ 2 files changed, 4 insertions(+), 8 deletions(-) rename common/djangoapps/student/migrations/{0044_rename_linkedin_config_field.py => 0044_linkedin_add_company_identifier.py} (96%) diff --git a/common/djangoapps/student/migrations/0044_rename_linkedin_config_field.py b/common/djangoapps/student/migrations/0044_linkedin_add_company_identifier.py similarity index 96% rename from common/djangoapps/student/migrations/0044_rename_linkedin_config_field.py rename to common/djangoapps/student/migrations/0044_linkedin_add_company_identifier.py index d656c9c683..0f6b749591 100644 --- a/common/djangoapps/student/migrations/0044_rename_linkedin_config_field.py +++ b/common/djangoapps/student/migrations/0044_linkedin_add_company_identifier.py @@ -8,9 +8,6 @@ from django.db import models class Migration(SchemaMigration): def forwards(self, orm): - # Deleting field 'LinkedInAddToProfileConfiguration.dashboard_tracking_code' - db.delete_column('student_linkedinaddtoprofileconfiguration', 'dashboard_tracking_code') - # Adding field 'LinkedInAddToProfileConfiguration.company_identifier' db.add_column('student_linkedinaddtoprofileconfiguration', 'company_identifier', self.gf('django.db.models.fields.TextField')(default=''), @@ -18,11 +15,6 @@ class Migration(SchemaMigration): def backwards(self, orm): - # Adding field 'LinkedInAddToProfileConfiguration.dashboard_tracking_code' - db.add_column('student_linkedinaddtoprofileconfiguration', 'dashboard_tracking_code', - self.gf('django.db.models.fields.TextField')(default='', blank=True), - keep_default=False) - # Deleting field 'LinkedInAddToProfileConfiguration.company_identifier' db.delete_column('student_linkedinaddtoprofileconfiguration', 'company_identifier') @@ -109,6 +101,7 @@ class Migration(SchemaMigration): 'change_date': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), 'changed_by': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True', 'on_delete': 'models.PROTECT'}), 'company_identifier': ('django.db.models.fields.TextField', [], {}), + 'dashboard_tracking_code': ('django.db.models.fields.TextField', [], {'default': "''", 'blank': 'True'}), 'enabled': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}) }, diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index c92736608c..7af3830650 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -1464,6 +1464,9 @@ class LinkedInAddToProfileConfiguration(ConfigurationModel): ) ) + # Deprecated + dashboard_tracking_code = models.TextField(default="", blank=True) + def add_to_profile_url(self, course_name, enrollment_mode, cert_url, source="o"): """Construct the URL for the "add to profile" button. From c12b54213684a7c1f5119a0e63b34f99b8b61ea6 Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Wed, 18 Feb 2015 17:46:41 -0500 Subject: [PATCH 5/6] split up bulk email query for students and unenrolled course staff (TNL-1332) (TNL-1143) --- lms/djangoapps/bulk_email/tasks.py | 43 +++++++++++-------- lms/djangoapps/bulk_email/tests/test_email.py | 16 ++++++- lms/djangoapps/instructor_task/subtasks.py | 29 +++++++------ .../instructor_task/tests/test_subtasks.py | 4 +- 4 files changed, 56 insertions(+), 36 deletions(-) diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index d136f42a65..31c57c6e1f 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/lms/djangoapps/bulk_email/tasks.py @@ -36,6 +36,7 @@ from django.core.urlresolvers import reverse from bulk_email.models import ( CourseEmail, Optout, CourseEmailTemplate, SEND_TO_MYSELF, SEND_TO_ALL, TO_OPTIONS, + SEND_TO_STAFF, ) from courseware.courses import get_course, course_image_url from student.roles import CourseStaffRole, CourseInstructorRole @@ -92,25 +93,30 @@ BULK_EMAIL_FAILURE_ERRORS = ( ) -def _get_recipient_queryset(user_id, to_option, course_id, course_location): +def _get_recipient_querysets(user_id, to_option, course_id): """ - Returns a query set of email recipients corresponding to the requested to_option category. + Returns a list of query sets of email recipients corresponding to the + requested `to_option` category. `to_option` is either SEND_TO_MYSELF, SEND_TO_STAFF, or SEND_TO_ALL. - Recipients who are in more than one category (e.g. enrolled in the course and are staff or self) - will be properly deduped. + Recipients who are in more than one category (e.g. enrolled in the course + and are staff or self) will be properly deduped. """ if to_option not in TO_OPTIONS: log.error("Unexpected bulk email TO_OPTION found: %s", to_option) raise Exception("Unexpected bulk email TO_OPTION found: {0}".format(to_option)) if to_option == SEND_TO_MYSELF: - recipient_qset = User.objects.filter(id=user_id) + user = User.objects.filter(id=user_id) + return [use_read_replica_if_available(user)] else: staff_qset = CourseStaffRole(course_id).users_with_role() instructor_qset = CourseInstructorRole(course_id).users_with_role() - recipient_qset = (staff_qset | instructor_qset).distinct() + staff_instructor_qset = (staff_qset | instructor_qset).distinct() + if to_option == SEND_TO_STAFF: + return [use_read_replica_if_available(staff_instructor_qset)] + if to_option == SEND_TO_ALL: # We also require students to have activated their accounts to # provide verification that the provided email address is valid. @@ -119,20 +125,19 @@ def _get_recipient_queryset(user_id, to_option, course_id, course_location): courseenrollment__course_id=course_id, courseenrollment__is_active=True ) - # Now we do some queryset sidestepping to avoid doing a DISTINCT - # query across the course staff and the enrolled students, which - # forces the creation of a temporary table in the db. - unenrolled_staff_qset = recipient_qset.exclude( + + # to avoid duplicates, we only want to email unenrolled course staff + # members here + unenrolled_staff_qset = staff_instructor_qset.exclude( courseenrollment__course_id=course_id, courseenrollment__is_active=True ) - # use read_replica if available: - unenrolled_staff_qset = use_read_replica_if_available(unenrolled_staff_qset) - unenrolled_staff_ids = [user.id for user in unenrolled_staff_qset] - recipient_qset = enrollment_qset | User.objects.filter(id__in=unenrolled_staff_ids) - - # again, use read_replica if available to lighten the load for large queries - return use_read_replica_if_available(recipient_qset) + # use read_replica if available + recipient_qsets = [ + use_read_replica_if_available(unenrolled_staff_qset), + use_read_replica_if_available(enrollment_qset), + ] + return recipient_qsets def _get_course_email_context(course): @@ -230,7 +235,7 @@ def perform_delegate_email_batches(entry_id, course_id, task_input, action_name) ) return new_subtask - recipient_qset = _get_recipient_queryset(user_id, to_option, course_id, course.location) + recipient_qsets = _get_recipient_querysets(user_id, to_option, course_id) recipient_fields = ['profile__name', 'email'] log.info(u"Task %s: Preparing to queue subtasks for sending emails for course %s, email %s, to_option %s", @@ -240,7 +245,7 @@ def perform_delegate_email_batches(entry_id, course_id, task_input, action_name) entry, action_name, _create_send_email_subtask, - recipient_qset, + recipient_qsets, recipient_fields, settings.BULK_EMAIL_EMAILS_PER_TASK, ) diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py index 5dba89ff1e..d26597894c 100644 --- a/lms/djangoapps/bulk_email/tests/test_email.py +++ b/lms/djangoapps/bulk_email/tests/test_email.py @@ -179,6 +179,7 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) response = self.client.post(self.send_mail_url, test_email) self.assertEquals(json.loads(response.content), self.success_content) + # the 1 is for the instructor self.assertEquals(len(mail.outbox), 1 + len(self.staff) + len(self.students)) self.assertItemsEqual( [e.to[0] for e in mail.outbox], @@ -195,12 +196,25 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) def test_no_duplicate_emails_enrolled_staff(self): """ - Test that no duplicate emials are sent to a course instructor that is + Test that no duplicate emails are sent to a course instructor that is also enrolled in the course """ CourseEnrollment.enroll(self.instructor, self.course.id) self.test_send_to_all() + def test_no_duplicate_emails_unenrolled_staff(self): + """ + Test that no duplicate emails are sent to a course staff that is + not enrolled in the course, but is enrolled in other courses + """ + course_1 = CourseFactory.create() + course_2 = CourseFactory.create() + # make sure self.instructor isn't enrolled in the course + self.assertFalse(CourseEnrollment.is_enrolled(self.instructor, self.course.id)) + CourseEnrollment.enroll(self.instructor, course_1.id) + CourseEnrollment.enroll(self.instructor, course_2.id) + self.test_send_to_all() + def test_unicode_subject_send_to_all(self): """ Make sure email (with Unicode characters) send to all goes there. diff --git a/lms/djangoapps/instructor_task/subtasks.py b/lms/djangoapps/instructor_task/subtasks.py index dae83f9d6b..483e3591e0 100644 --- a/lms/djangoapps/instructor_task/subtasks.py +++ b/lms/djangoapps/instructor_task/subtasks.py @@ -71,7 +71,7 @@ def track_memory_usage(metric, course_id): def _generate_items_for_subtask( - item_queryset, + item_querysets, item_fields, total_num_items, items_per_task, @@ -82,10 +82,10 @@ def _generate_items_for_subtask( Generates a chunk of "items" that should be passed into a subtask. Arguments: - `item_queryset` : a query set that defines the "items" that should be passed to subtasks. + `item_querysets` : a list of query sets, each of which defines the "items" that should be passed to subtasks. `item_fields` : the fields that should be included in the dict that is returned. These are in addition to the 'pk' field. - `total_num_items` : the result of item_queryset.count(). + `total_num_items` : the result of summing the count of each queryset in `item_querysets`. `items_per_query` : size of chunks to break the query operation into. `items_per_task` : maximum size of chunks to break each query chunk into for use by a subtask. `course_id` : course_id of the course. Only needed for the track_memory_usage context manager. @@ -102,13 +102,14 @@ def _generate_items_for_subtask( items_for_task = [] with track_memory_usage('course_email.subtask_generation.memory', course_id): - for item in item_queryset.values(*all_item_fields).iterator(): - if len(items_for_task) == items_per_task and num_subtasks < total_num_subtasks - 1: - yield items_for_task - num_items_queued += items_per_task - items_for_task = [] - num_subtasks += 1 - items_for_task.append(item) + for queryset in item_querysets: + for item in queryset.values(*all_item_fields).iterator(): + if len(items_for_task) == items_per_task and num_subtasks < total_num_subtasks - 1: + yield items_for_task + num_items_queued += items_per_task + items_for_task = [] + num_subtasks += 1 + items_for_task.append(item) # yield remainder items for task, if any if items_for_task: @@ -275,7 +276,7 @@ def initialize_subtask_info(entry, action_name, total_num, subtask_id_list): return task_progress -def queue_subtasks_for_query(entry, action_name, create_subtask_fcn, item_queryset, item_fields, items_per_task): +def queue_subtasks_for_query(entry, action_name, create_subtask_fcn, item_querysets, item_fields, items_per_task): """ Generates and queues subtasks to each execute a chunk of "items" generated by a queryset. @@ -285,7 +286,7 @@ def queue_subtasks_for_query(entry, action_name, create_subtask_fcn, item_querys `create_subtask_fcn` : a function of two arguments that constructs the desired kind of subtask object. Arguments are the list of items to be processed by this subtask, and a SubtaskStatus object reflecting initial status (and containing the subtask's id). - `item_queryset` : a query set that defines the "items" that should be passed to subtasks. + `item_querysets` : a list of query sets that define the "items" that should be passed to subtasks. `item_fields` : the fields that should be included in the dict that is returned. These are in addition to the 'pk' field. `items_per_task` : maximum size of chunks to break each query chunk into for use by a subtask. @@ -294,7 +295,7 @@ def queue_subtasks_for_query(entry, action_name, create_subtask_fcn, item_querys """ task_id = entry.task_id - total_num_items = item_queryset.count() + total_num_items = sum([item_queryset.count() for item_queryset in item_querysets]) # Calculate the number of tasks that will be created, and create a list of ids for each task. total_num_subtasks = _get_number_of_subtasks(total_num_items, items_per_task) @@ -313,7 +314,7 @@ def queue_subtasks_for_query(entry, action_name, create_subtask_fcn, item_querys # Construct a generator that will return the recipients to use for each subtask. # Pass in the desired fields to fetch for each recipient. item_list_generator = _generate_items_for_subtask( - item_queryset, + item_querysets, item_fields, total_num_items, items_per_task, diff --git a/lms/djangoapps/instructor_task/tests/test_subtasks.py b/lms/djangoapps/instructor_task/tests/test_subtasks.py index 7edd06ab92..bd19caca2f 100644 --- a/lms/djangoapps/instructor_task/tests/test_subtasks.py +++ b/lms/djangoapps/instructor_task/tests/test_subtasks.py @@ -38,7 +38,7 @@ class TestSubtasks(InstructorTaskCourseTestCase): ) self._enroll_students_in_course(self.course.id, initial_count) - task_queryset = CourseEnrollment.objects.filter(course_id=self.course.id) + task_querysets = [CourseEnrollment.objects.filter(course_id=self.course.id)] def initialize_subtask_info(*args): # pylint: disable=unused-argument """Instead of initializing subtask info enroll some more students into course.""" @@ -51,7 +51,7 @@ class TestSubtasks(InstructorTaskCourseTestCase): entry=instructor_task, action_name='action_name', create_subtask_fcn=create_subtask_fcn, - item_queryset=task_queryset, + item_querysets=task_querysets, item_fields=[], items_per_task=items_per_task, ) From 338ab4a6a9380797ba8858cc59f1db10ab0064e1 Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Tue, 24 Feb 2015 08:20:04 -0500 Subject: [PATCH 6/6] fix quality violation --- lms/djangoapps/instructor_task/subtasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor_task/subtasks.py b/lms/djangoapps/instructor_task/subtasks.py index c0da0f4207..f863dfc598 100644 --- a/lms/djangoapps/instructor_task/subtasks.py +++ b/lms/djangoapps/instructor_task/subtasks.py @@ -71,7 +71,7 @@ def track_memory_usage(metric, course_id): def _generate_items_for_subtask( - item_querysets, + item_querysets, # pylint: disable=bad-continuation item_fields, total_num_items, items_per_task,