From f8d41b976478ce12f2f911afb32ed7f4650037bf Mon Sep 17 00:00:00 2001 From: castellanese Date: Mon, 25 Sep 2017 14:41:00 -0400 Subject: [PATCH 1/7] Differentiated emails - unverified users receive email with upsell button. --- .../tests/test_send_recurring_nudge.py | 2 +- openedx/core/djangoapps/schedules/tasks.py | 84 ++++++++++++++----- .../recurringnudge_day3/email/body.html | 46 ++++++++-- .../recurringnudge_day3/email/body.txt | 7 ++ 4 files changed, 108 insertions(+), 31 deletions(-) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index 1eacc49303..87725f6d6b 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py @@ -233,7 +233,7 @@ class TestSendRecurringNudge(CacheIsolationTestCase): patch_channels(self, [mock_channel]) sent_messages = [] - + templates_override = deepcopy(settings.TEMPLATES) templates_override[0]['OPTIONS']['string_if_invalid'] = "TEMPLATE WARNING - MISSING VARIABLE [%s]" with self.settings(TEMPLATES=templates_override): diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index c9c7a7968b..0434a5ad00 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -18,6 +18,7 @@ from edx_ace.message import Message from edx_ace.recipient import Recipient from edx_ace.utils.date import deserialize from opaque_keys.edx.keys import CourseKey +from lms.djangoapps.experiments.utils import check_and_get_upgrade_link from edxmako.shortcuts import marketing_link from openedx.core.djangoapps.schedules.message_type import ScheduleMessageType @@ -105,6 +106,40 @@ def _recurring_nudge_schedule_send(site_id, msg_str): # TODO: delete once _recurring_nudge_schedules_for_bin is fully rolled out def _recurring_nudge_schedules_for_hour(site, target_hour, org_list, exclude_orgs=False): + users, schedules = _gather_users_and_schedules_for_target_hour(target_hour, org_list, exclude_orgs) + + dashboard_relative_url = reverse('dashboard') + for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): + user_schedules = list(user_schedules) + course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] + + first_schedule = user_schedules[0] + + template_context = { + 'student_name': user.profile.name, + + 'course_name': first_schedule.enrollment.course.display_name, + 'course_url': absolute_url(site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)])), + + # This is used by the bulk email optout policy + 'course_ids': course_id_strs, + # Platform information + 'homepage_url': encode_url(marketing_link('ROOT')), + 'dashboard_url': absolute_url(site, dashboard_relative_url), + 'template_revision': settings.EDX_PLATFORM_REVISION, + 'platform_name': settings.PLATFORM_NAME, + 'contact_mailing_address': settings.CONTACT_MAILING_ADDRESS, + 'social_media_urls': encode_urls_in_dict(getattr(settings, 'SOCIAL_MEDIA_FOOTER_URLS', {})), + 'mobile_store_urls': encode_urls_in_dict(getattr(settings, 'MOBILE_STORE_URLS', {})), + } + + # Information for including upsell messaging in template. + _add_upsell_button_to_email_template(user, first_schedule, template_context) + + yield (user, first_schedule.enrollment.course.language, template_context) + + +def _gather_users_and_schedules_for_target_hour(target_hour, org_list, exclude_orgs): beginning_of_day = target_hour.replace(hour=0, minute=0, second=0) users = User.objects.filter( courseenrollment__schedule__start__gte=beginning_of_day, @@ -138,32 +173,39 @@ def _recurring_nudge_schedules_for_hour(site, target_hour, org_list, exclude_org LOG.debug('Scheduled Nudge: Query = %r', schedules.query.sql_with_params()) - dashboard_relative_url = reverse('dashboard') + return users, schedules - for (user, user_schedules) in groupby(schedules, lambda s: s.enrollment.user): - user_schedules = list(user_schedules) - course_id_strs = [str(schedule.enrollment.course_id) for schedule in user_schedules] - first_schedule = user_schedules[0] - template_context = { - 'student_name': user.profile.name, +def _should_user_be_upsold(enrollment): + enrollment_mode = None + is_active = None - 'course_name': first_schedule.enrollment.course.display_name, - 'course_url': absolute_url(site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)])), + if enrollment: + enrollment_mode = enrollment.mode + is_active = enrollment.is_active - # This is used by the bulk email optout policy - 'course_ids': course_id_strs, + # Return `true` if user is not enrolled in course + if enrollment_mode is None and is_active is None: + return True - # Platform information - 'homepage_url': encode_url(marketing_link('ROOT')), - 'dashboard_url': absolute_url(site, dashboard_relative_url), - 'template_revision': settings.EDX_PLATFORM_REVISION, - 'platform_name': settings.PLATFORM_NAME, - 'contact_mailing_address': settings.CONTACT_MAILING_ADDRESS, - 'social_media_urls': encode_urls_in_dict(getattr(settings, 'SOCIAL_MEDIA_FOOTER_URLS', {})), - 'mobile_store_urls': encode_urls_in_dict(getattr(settings, 'MOBILE_STORE_URLS', {})), - } - yield (user, first_schedule.enrollment.course.language, template_context) + # Show the summary if user enrollment is in which allow user to upsell + return is_active and enrollment_mode in ["Verified", "Professional"] + + +def _add_upsell_button_to_email_template(a_user, a_schedule, template_context): + show_upsell = _should_user_be_upsold(a_schedule.enrollment) + # Check and upgrade link performs a query on CourseMode, which is triggering failures in + # test_send_recurring_nudge.py + upgrade_data = check_and_get_upgrade_link(a_user, a_schedule.enrollment.course_id) + + upsell_link = '' + + if upgrade_data: + upsell_link = upgrade_data.link + + template_context['show_upsell'] = show_upsell + template_context['upsell_link'] = upsell_link + template_context['user_schedule_upgrade_deadline_time'] = upgrade_data.date @task(ignore_result=True, routing_key=ROUTING_KEY) diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.html b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.html index 0d7dacd7da..e60fa8378a 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.html +++ b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.html @@ -36,7 +36,7 @@

- + {# email client support for style sheets is pretty spotty, so we have to inline all of these styles #} 1 %} href="{{ dashboard_url }}" @@ -46,20 +46,48 @@ style=" color: #ffffff; text-decoration: none; - border-radius: 4px; - -webkit-border-radius: 4px; - -moz-border-radius: 4px; + border-radius: .3rem; + -webkit-border-radius: .3rem; + -moz-border-radius: .3rem; background-color: #005686; - border-top: 10px solid #005686; - border-bottom: 10px solid #005686; - border-right: 16px solid #005686; - border-left: 16px solid #005686; + border-top: .15rem solid #005686; + border-bottom: .15rem solid #005686; + border-right: .15rem solid #005686; + border-left: .15rem solid #005686; display: inline-block; + padding: 1rem 5rem; "> - + {# old email clients require the use of the font tag :( #} {% trans "Start learning now" %}

+ {% if show_upsell %} +

+ {% blocktrans trimmed %} + Don't miss the opportunity to highlight your new knowledge and skills by earning a verified + certificate. Upgrade by {{ user_schedule_upgrade_deadline_time|date:"l, F dS, Y" }}. + {% endblocktrans %} +

+

+ + {% trans "Upgrade Now" %} + +

+ {% endif %} diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.txt b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.txt index a3abd340a1..f04db8ce7e 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.txt +++ b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.txt @@ -15,3 +15,10 @@ {% trans "Start learning now" %} <{{ course_url }}> {% endif %} + +{% blocktrans trimmed %} + Don't miss the opportunity to highlight your new knowledge and skills by earning a verified + certificate. Upgrade by {{ user_schedule_upgrade_deadline_time|date:"l, F dS, Y" }}. + + Upgrade Now! <{{ upsell_link }}> +{% endblocktrans %} From dd53edc6e035eb9656b1b276a40d51efcf1480c1 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 29 Sep 2017 14:22:58 -0400 Subject: [PATCH 2/7] Cache upgrade_deadline on CourseEnrollment objects --- common/djangoapps/student/models.py | 2 +- common/djangoapps/student/tests/test_models.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 459342a493..934c250f2b 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -1696,7 +1696,7 @@ class CourseEnrollment(models.Model): def verified_mode(self): return CourseMode.verified_mode_for_course(self.course_id) - @property + @cached_property def upgrade_deadline(self): """ Returns the upgrade deadline for this enrollment, if it is upgradeable. diff --git a/common/djangoapps/student/tests/test_models.py b/common/djangoapps/student/tests/test_models.py index e8c73458d1..cbee80d12e 100644 --- a/common/djangoapps/student/tests/test_models.py +++ b/common/djangoapps/student/tests/test_models.py @@ -132,6 +132,21 @@ class CourseEnrollmentTests(SharedModuleStoreTestCase): self.assertEqual(Schedule.objects.all().count(), 0) self.assertEqual(enrollment.upgrade_deadline, course_mode.expiration_datetime) + + @skip_unless_lms + # NOTE: We mute the post_save signal to prevent Schedules from being created for new enrollments + @factory.django.mute_signals(signals.post_save) + def test_upgrade_deadline_with_schedule(self): + """ The property should use either the CourseMode or related Schedule to determine the deadline. """ + course = CourseFactory(self_paced=True) + CourseModeFactory( + course_id=course.id, + mode_slug=CourseMode.VERIFIED, + # This must be in the future to ensure it is returned by downstream code. + expiration_datetime=datetime.datetime.now(pytz.UTC) + datetime.timedelta(days=1), + ) + enrollment = CourseEnrollmentFactory(course_id=course.id, mode=CourseMode.AUDIT) + # The schedule's upgrade deadline should be used if a schedule exists DynamicUpgradeDeadlineConfiguration.objects.create(enabled=True) schedule = ScheduleFactory(enrollment=enrollment) From 8468357ac40b5257e125395caeafe665025d3703 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 29 Sep 2017 14:25:10 -0400 Subject: [PATCH 3/7] Separate the date and link logic out of VerifiedUpgradeDeadlineBlock, so that it can be called directly with prefetched data for check_and_get_upgrade_link_and_date --- .../djangoapps/student/tests/test_models.py | 1 - .../tests/test_field_override_performance.py | 54 ++++++------- lms/djangoapps/ccx/tests/test_views.py | 3 + lms/djangoapps/courseware/date_summary.py | 78 +++++++++++++++---- lms/djangoapps/courseware/tests/test_views.py | 16 ++-- lms/djangoapps/courseware/testutils.py | 2 + lms/djangoapps/experiments/utils.py | 45 ++++++++--- .../tests/test_send_recurring_nudge.py | 23 +++++- openedx/core/djangoapps/schedules/tasks.py | 42 ++++------ .../tests/views/test_course_updates.py | 2 +- 10 files changed, 178 insertions(+), 88 deletions(-) diff --git a/common/djangoapps/student/tests/test_models.py b/common/djangoapps/student/tests/test_models.py index cbee80d12e..9b08398237 100644 --- a/common/djangoapps/student/tests/test_models.py +++ b/common/djangoapps/student/tests/test_models.py @@ -132,7 +132,6 @@ class CourseEnrollmentTests(SharedModuleStoreTestCase): self.assertEqual(Schedule.objects.all().count(), 0) self.assertEqual(enrollment.upgrade_deadline, course_mode.expiration_datetime) - @skip_unless_lms # NOTE: We mute the post_save signal to prevent Schedules from being created for new enrollments @factory.django.mute_signals(signals.post_save) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index aa400bc7da..7ed0b0566f 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -237,18 +237,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): # # of sql queries to default, # # of mongo queries, # ) - ('no_overrides', 1, True, False): (19, 1), - ('no_overrides', 2, True, False): (19, 1), - ('no_overrides', 3, True, False): (19, 1), - ('ccx', 1, True, False): (19, 1), - ('ccx', 2, True, False): (19, 1), - ('ccx', 3, True, False): (19, 1), - ('no_overrides', 1, False, False): (19, 1), - ('no_overrides', 2, False, False): (19, 1), - ('no_overrides', 3, False, False): (19, 1), - ('ccx', 1, False, False): (19, 1), - ('ccx', 2, False, False): (19, 1), - ('ccx', 3, False, False): (19, 1), + ('no_overrides', 1, True, False): (18, 1), + ('no_overrides', 2, True, False): (18, 1), + ('no_overrides', 3, True, False): (18, 1), + ('ccx', 1, True, False): (18, 1), + ('ccx', 2, True, False): (18, 1), + ('ccx', 3, True, False): (18, 1), + ('no_overrides', 1, False, False): (18, 1), + ('no_overrides', 2, False, False): (18, 1), + ('no_overrides', 3, False, False): (18, 1), + ('ccx', 1, False, False): (18, 1), + ('ccx', 2, False, False): (18, 1), + ('ccx', 3, False, False): (18, 1), } @@ -260,19 +260,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - ('no_overrides', 1, True, False): (19, 3), - ('no_overrides', 2, True, False): (19, 3), - ('no_overrides', 3, True, False): (19, 3), - ('ccx', 1, True, False): (19, 3), - ('ccx', 2, True, False): (19, 3), - ('ccx', 3, True, False): (19, 3), - ('ccx', 1, True, True): (20, 3), - ('ccx', 2, True, True): (20, 3), - ('ccx', 3, True, True): (20, 3), - ('no_overrides', 1, False, False): (19, 3), - ('no_overrides', 2, False, False): (19, 3), - ('no_overrides', 3, False, False): (19, 3), - ('ccx', 1, False, False): (19, 3), - ('ccx', 2, False, False): (19, 3), - ('ccx', 3, False, False): (19, 3), + ('no_overrides', 1, True, False): (18, 3), + ('no_overrides', 2, True, False): (18, 3), + ('no_overrides', 3, True, False): (18, 3), + ('ccx', 1, True, False): (18, 3), + ('ccx', 2, True, False): (18, 3), + ('ccx', 3, True, False): (18, 3), + ('ccx', 1, True, True): (19, 3), + ('ccx', 2, True, True): (19, 3), + ('ccx', 3, True, True): (19, 3), + ('no_overrides', 1, False, False): (18, 3), + ('no_overrides', 2, False, False): (18, 3), + ('no_overrides', 3, False, False): (18, 3), + ('ccx', 1, False, False): (18, 3), + ('ccx', 2, False, False): (18, 3), + ('ccx', 3, False, False): (18, 3), } diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 7d271e7668..21c00d1152 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -36,6 +36,7 @@ from lms.djangoapps.ccx.utils import ccx_course, is_email from lms.djangoapps.ccx.views import get_date from lms.djangoapps.grades.tasks import compute_all_grades_for_course from lms.djangoapps.instructor.access import allow_access, list_with_level +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from request_cache.middleware import RequestCache from student.models import CourseEnrollment, CourseEnrollmentAllowed from student.roles import CourseCcxCoachRole, CourseInstructorRole, CourseStaffRole @@ -1061,6 +1062,7 @@ class TestCCXGrades(FieldOverrideTestMixin, SharedModuleStoreTestCase, LoginEnro def setUpClass(cls): super(TestCCXGrades, cls).setUpClass() cls._course = course = CourseFactory.create(enable_ccx=True) + CourseOverview.load_from_module_store(course.id) # Create a course outline cls.mooc_start = start = datetime.datetime( @@ -1122,6 +1124,7 @@ class TestCCXGrades(FieldOverrideTestMixin, SharedModuleStoreTestCase, LoginEnro # which emulates how a student would get access. self.ccx_key = CCXLocator.from_course_locator(self._course.id, unicode(ccx.id)) self.course = get_course_by_id(self.ccx_key, depth=None) + CourseOverview.load_from_module_store(self.course.id) setup_students_and_grades(self) self.client.login(username=coach.username, password="test") self.addCleanup(RequestCache.clear_request_cache) diff --git a/lms/djangoapps/courseware/date_summary.py b/lms/djangoapps/courseware/date_summary.py index a68ed55801..4cb2af0ab1 100644 --- a/lms/djangoapps/courseware/date_summary.py +++ b/lms/djangoapps/courseware/date_summary.py @@ -20,6 +20,7 @@ from course_modes.models import CourseMode, get_cosmetic_verified_display_price from lms.djangoapps.commerce.utils import EcommerceService from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification, VerificationDeadline from openedx.core.djangoapps.certificates.api import can_show_certificate_available_date_field +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangolib.markup import HTML, Text from openedx.features.course_experience import CourseHomeMessages, UPGRADE_DEADLINE_MESSAGE from student.models import CourseEnrollment @@ -380,6 +381,67 @@ class CertificateAvailableDate(DateSummary): ) +def verified_upgrade_deadline_link(user, course=None, course_id=None): + """ + Format the correct verified upgrade link for the specified ``user`` + in a course. + + One of ``course`` or ``course_id`` must be supplied. If both are specified, + ``course`` will take priority. + + Arguments: + user (:class:`~django.contrib.auth.models.User`): The user to display + the link for. + course (:class:`.CourseOverview`): The course to render a link for. + course_id (:class:`.CourseKey`): The course_id of the course to render for. + + Returns: + The formatted link to that will allow the user to upgrade to verified + in this course. + """ + if course is not None: + course_id = course.id + + ecommerce_service = EcommerceService() + if ecommerce_service.is_enabled(user): + if course is not None and isinstance(course, CourseOverview): + course_mode = [ + mode + for mode in course.modes + if mode.slug == CourseMode.VERIFIED + ] + else: + course_mode = CourseMode.objects.get( + course_id=course_id, mode_slug=CourseMode.VERIFIED + ) + return ecommerce_service.get_checkout_page_url(course_mode.sku) + return reverse('verify_student_upgrade_and_verify', args=(course_id,)) + + +def verified_upgrade_link_is_valid(enrollment=None): + """ + Return whether this enrollment can be upgraded. + + Arguments: + enrollment (:class:`.CourseEnrollment`): The enrollment under consideration. + If None, then the enrollment is considered to be upgradeable. + """ + # Return `true` if user is not enrolled in course + if enrollment is None: + return False + + upgrade_deadline = enrollment.upgrade_deadline + + if upgrade_deadline is None: + return False + + if datetime.datetime.now(utc).date() > upgrade_deadline.date(): + return False + + # Show the summary if user enrollment is in which allow user to upsell + return enrollment.is_active and enrollment.mode in CourseMode.UPSELL_TO_VERIFIED_MODES + + class VerifiedUpgradeDeadlineDate(DateSummary): """ Displays the date before which learners must upgrade to the @@ -395,7 +457,7 @@ class VerifiedUpgradeDeadlineDate(DateSummary): @property def link(self): - return EcommerceService().upgrade_url(self.user, self.course_id) + return verified_upgrade_deadline_link(self.user, self.course, self.course_id) @cached_property def enrollment(self): @@ -413,19 +475,7 @@ class VerifiedUpgradeDeadlineDate(DateSummary): if not is_enabled: return False - enrollment_mode = None - is_active = None - - if self.enrollment: - enrollment_mode = self.enrollment.mode - is_active = self.enrollment.is_active - - # Return `true` if user is not enrolled in course - if enrollment_mode is None and is_active is None: - return True - - # Show the summary if user enrollment is in which allow user to upsell - return is_active and enrollment_mode in CourseMode.UPSELL_TO_VERIFIED_MODES + return verified_upgrade_link_is_valid(self.enrollment) @lazy def date(self): diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index a200dfc22d..50dda88ea4 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -213,8 +213,8 @@ class IndexQueryTestCase(ModuleStoreTestCase): NUM_PROBLEMS = 20 @ddt.data( - (ModuleStoreEnum.Type.mongo, 10, 147), - (ModuleStoreEnum.Type.split, 4, 147), + (ModuleStoreEnum.Type.mongo, 10, 146), + (ModuleStoreEnum.Type.split, 4, 146), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): @@ -1047,6 +1047,7 @@ class BaseDueDateTests(ModuleStoreTestCase): course = modulestore().get_course(course.id) self.assertIsNotNone(course.get_children()[0].get_children()[0].due) CourseEnrollmentFactory(user=self.user, course_id=course.id) + CourseOverview.load_from_module_store(course.id) return course def setUp(self): @@ -1456,13 +1457,13 @@ class ProgressPageTests(ProgressPageBaseTests): """Test that query counts remain the same for self-paced and instructor-paced courses.""" SelfPacedConfiguration(enabled=self_paced_enabled).save() self.setup_course(self_paced=self_paced) - with self.assertNumQueries(36, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST), check_mongo_calls(1): + with self.assertNumQueries(35, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST), check_mongo_calls(1): self._get_progress_page() @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) @ddt.data( - (False, 43, 27), - (True, 36, 23) + (False, 42, 26), + (True, 35, 22) ) @ddt.unpack def test_progress_queries(self, enable_waffle, initial, subsequent): @@ -2213,6 +2214,7 @@ class TestIndexView(ModuleStoreTestCase): state=json.dumps({'state': unicode(item.scope_ids.usage_id)}) ) + CourseOverview.load_from_module_store(course.id) CourseEnrollmentFactory(user=user, course_id=course.id) self.assertTrue(self.client.login(username=user.username, password='test')) @@ -2241,6 +2243,7 @@ class TestIndexView(ModuleStoreTestCase): vertical = ItemFactory.create(parent=section, category='vertical', display_name="Vertical") ItemFactory.create(parent=vertical, category='id_checker', display_name="ID Checker") + CourseOverview.load_from_module_store(course.id) CourseEnrollmentFactory(user=user, course_id=course.id) self.assertTrue(self.client.login(username=user.username, password='test')) @@ -2281,6 +2284,8 @@ class TestIndexViewWithVerticalPositions(ModuleStoreTestCase): ItemFactory.create(parent=self.section, category='vertical', display_name="Vertical2") ItemFactory.create(parent=self.section, category='vertical', display_name="Vertical3") + CourseOverview.load_from_module_store(self.course.id) + self.client.login(username=self.user, password='test') CourseEnrollmentFactory(user=self.user, course_id=self.course.id) @@ -2505,6 +2510,7 @@ class EnterpriseConsentTestCase(EnterpriseTestConsentRequired, ModuleStoreTestCa self.user = UserFactory.create() self.assertTrue(self.client.login(username=self.user.username, password='test')) self.course = CourseFactory.create() + CourseOverview.load_from_module_store(self.course.id) CourseEnrollmentFactory(user=self.user, course_id=self.course.id) def test_consent_required(self): diff --git a/lms/djangoapps/courseware/testutils.py b/lms/djangoapps/courseware/testutils.py index 91ba8d813a..f90188c3b3 100644 --- a/lms/djangoapps/courseware/testutils.py +++ b/lms/djangoapps/courseware/testutils.py @@ -12,6 +12,7 @@ from mock import patch from lms.djangoapps.courseware.field_overrides import OverrideModulestoreFieldData from lms.djangoapps.courseware.url_helpers import get_redirect_url +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from student.tests.factories import AdminFactory, CourseEnrollmentFactory, UserFactory from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore @@ -104,6 +105,7 @@ class RenderXBlockTestMixin(object): category='html', data="

Test HTML Content

" ) + CourseOverview.load_from_module_store(self.course.id) # block_name_to_be_tested can be `html_block` or `vertical_block`. # These attributes help ensure the positive and negative tests are in sync. diff --git a/lms/djangoapps/experiments/utils.py b/lms/djangoapps/experiments/utils.py index a8e28f68ef..346a8eb991 100644 --- a/lms/djangoapps/experiments/utils.py +++ b/lms/djangoapps/experiments/utils.py @@ -3,21 +3,41 @@ from course_modes.models import ( get_cosmetic_verified_display_price ) from courseware.date_summary import ( - VerifiedUpgradeDeadlineDate + verified_upgrade_deadline_link, verified_upgrade_link_is_valid ) -def check_and_get_upgrade_link(user, course_id): +def check_and_get_upgrade_link_and_date(user, enrollment=None, course=None): """ For an authenticated user, return a link to allow them to upgrade in the specified course. """ - if user.is_authenticated(): - upgrade_data = VerifiedUpgradeDeadlineDate(None, user, course_id=course_id) - if upgrade_data.is_enabled: - return upgrade_data + if enrollment is None and course is None: + raise ValueError("Must specify either an enrollment or a course") - return None + if enrollment: + if course is None: + course = enrollment.course + elif enrollment.course_id != course.id: + raise ValueError("{} refers to a different course than {} which was supplied".format( + enrollment, course + )) + + if enrollment.user_id != user.id: + raise ValueError("{} refers to a different user than {} which was supplied".format( + enrollment, user + )) + + if enrollment is None: + enrollment = CourseEnrollment.get_enrollment(user, course.id) + + if user.is_authenticated() and verified_upgrade_link_is_valid(enrollment): + return ( + verified_upgrade_deadline_link(user, course), + enrollment.upgrade_deadline + ) + + return (None, None) def get_experiment_user_metadata_context(course, user): @@ -26,23 +46,26 @@ def get_experiment_user_metadata_context(course, user): """ enrollment_mode = None enrollment_time = None + enrollment = None try: - enrollment = CourseEnrollment.objects.get(user_id=user.id, course_id=course.id) + enrollment = CourseEnrollment.objects.select_related( + 'course' + ).get(user_id=user.id, course_id=course.id) if enrollment.is_active: enrollment_mode = enrollment.mode enrollment_time = enrollment.created except CourseEnrollment.DoesNotExist: pass # Not enrolled, used the default None values - upgrade_data = check_and_get_upgrade_link(user, course.id) + upgrade_link, upgrade_date = check_and_get_upgrade_link_and_date(user, enrollment, course) return { - 'upgrade_link': upgrade_data and upgrade_data.link, + 'upgrade_link': upgrade_link, 'upgrade_price': unicode(get_cosmetic_verified_display_price(course)), 'enrollment_mode': enrollment_mode, 'enrollment_time': enrollment_time, 'pacing_type': 'self_paced' if course.self_paced else 'instructor_paced', - 'upgrade_deadline': upgrade_data and upgrade_data.date, + 'upgrade_deadline': upgrade_date, 'course_key': course.id, 'course_start': course.start, 'course_end': course.end, diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index 87725f6d6b..c7760eda2e 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py @@ -22,6 +22,25 @@ from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_un from student.tests.factories import UserFactory +# Populating recurring nudge emails requires three queries +# 1a) Find all users whose first enrollment during a day was in the specified hour window +# 1b) All schedules for all enrollments for that day for users in that window (with 1a as a subquery) +# 2) Check whether debugging is enabled +CONST_QUERIES = 2 + +# 1) Prefetch all course modes for those schedules +# 2) (When not cached) load the DynamicUpgradeDeadlineConfiguration +SCHEDULE_QUERIES = 2 + +# 1) Load the current django site +# 2) Load the ScheduleConfig +SEND_QUERIES = 2 + +# 1) (When not cached) load the CourseDynamicUpgradeDeadlineConfiguration +# 2) Load the VERIFIED course mode for the course +PER_COURSE_QUERIES = 2 + + @ddt.ddt @skip_unless_lms @skipUnless('openedx.core.djangoapps.schedules.apps.SchedulesConfig' in settings.INSTALLED_APPS, @@ -29,6 +48,8 @@ from student.tests.factories import UserFactory class TestSendRecurringNudge(CacheIsolationTestCase): # pylint: disable=protected-access + ENABLED_CACHES = ['default'] + def setUp(self): super(TestSendRecurringNudge, self).setUp() @@ -233,7 +254,7 @@ class TestSendRecurringNudge(CacheIsolationTestCase): patch_channels(self, [mock_channel]) sent_messages = [] - + templates_override = deepcopy(settings.TEMPLATES) templates_override[0]['OPTIONS']['string_if_invalid'] = "TEMPLATE WARNING - MISSING VARIABLE [%s]" with self.settings(TEMPLATES=templates_override): diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 0434a5ad00..5f2998be8c 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -18,7 +18,7 @@ from edx_ace.message import Message from edx_ace.recipient import Recipient from edx_ace.utils.date import deserialize from opaque_keys.edx.keys import CourseKey -from lms.djangoapps.experiments.utils import check_and_get_upgrade_link +from lms.djangoapps.experiments.utils import check_and_get_upgrade_link_and_date from edxmako.shortcuts import marketing_link from openedx.core.djangoapps.schedules.message_type import ScheduleMessageType @@ -155,6 +155,8 @@ def _gather_users_and_schedules_for_target_hour(target_hour, org_list, exclude_o schedules = Schedule.objects.select_related( 'enrollment__user__profile', 'enrollment__course', + ).prefetch_related( + 'enrollment__course__modes' ).filter( enrollment__user__in=users, start__gte=beginning_of_day, @@ -176,37 +178,15 @@ def _gather_users_and_schedules_for_target_hour(target_hour, org_list, exclude_o return users, schedules -def _should_user_be_upsold(enrollment): - enrollment_mode = None - is_active = None - - if enrollment: - enrollment_mode = enrollment.mode - is_active = enrollment.is_active - - # Return `true` if user is not enrolled in course - if enrollment_mode is None and is_active is None: - return True - - # Show the summary if user enrollment is in which allow user to upsell - return is_active and enrollment_mode in ["Verified", "Professional"] - - def _add_upsell_button_to_email_template(a_user, a_schedule, template_context): - show_upsell = _should_user_be_upsold(a_schedule.enrollment) # Check and upgrade link performs a query on CourseMode, which is triggering failures in # test_send_recurring_nudge.py - upgrade_data = check_and_get_upgrade_link(a_user, a_schedule.enrollment.course_id) - - upsell_link = '' - - if upgrade_data: - upsell_link = upgrade_data.link - - template_context['show_upsell'] = show_upsell - template_context['upsell_link'] = upsell_link - template_context['user_schedule_upgrade_deadline_time'] = upgrade_data.date + upgrade_link, upgrade_date = check_and_get_upgrade_link_and_date(a_user, a_schedule.enrollment) + template_context['show_upsell'] = upgrade_link is not None + template_context['upsell_link'] = upgrade_link + template_context['user_schedule_upgrade_deadline_time'] = upgrade_date + @task(ignore_result=True, routing_key=ROUTING_KEY) def recurring_nudge_schedule_bin( @@ -261,6 +241,10 @@ def _recurring_nudge_schedules_for_bin(site, target_day, bin_num, org_list, excl # This is used by the bulk email optout policy 'course_ids': course_id_strs, }) + + # Information for including upsell messaging in template. + _add_upsell_button_to_email_template(user, first_schedule, template_context) + yield (user, first_schedule.enrollment.course.language, template_context) @@ -386,6 +370,8 @@ def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, target_d schedules = Schedule.objects.select_related( 'enrollment__user__profile', 'enrollment__course', + ).prefetch_related( + 'enrollment__course__modes' ).filter( enrollment__user__in=users, enrollment__is_active=True, diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py index a2f8eaa93b..4bcae0c965 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/openedx/features/course_experience/tests/views/test_course_updates.py @@ -126,7 +126,7 @@ class TestCourseUpdatesPage(SharedModuleStoreTestCase): course_updates_url(self.course) # Fetch the view and verify that the query counts haven't changed - with self.assertNumQueries(33, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(32, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): with check_mongo_calls(4): url = course_updates_url(self.course) self.client.get(url) From 956cb919554c8103149fa6442254bdfed0ce32d1 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 4 Oct 2017 12:27:14 -0400 Subject: [PATCH 4/7] Add an import of a submodule to make pytest less complainy --- lms/djangoapps/experiments/factories.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/djangoapps/experiments/factories.py b/lms/djangoapps/experiments/factories.py index d0aa1d89e1..fe7ea054dc 100644 --- a/lms/djangoapps/experiments/factories.py +++ b/lms/djangoapps/experiments/factories.py @@ -1,4 +1,5 @@ import factory +import factory.fuzzy from experiments.models import ExperimentData, ExperimentKeyValue from student.tests.factories import UserFactory From fc7c1652f85f9f37c44cfbb46ee68861cfffd342 Mon Sep 17 00:00:00 2001 From: sandroroux Date: Thu, 5 Oct 2017 09:24:04 -0400 Subject: [PATCH 5/7] Modified criteria for upselling. User must have a dynamic deadline to receive an email with an upsell button --- lms/djangoapps/courseware/date_summary.py | 6 +----- .../tests/test_send_recurring_nudge.py | 5 +++-- openedx/core/djangoapps/schedules/tasks.py | 19 +++++++++++++++---- .../recurringnudge_day3/email/body.html | 2 +- .../recurringnudge_day3/email/body.txt | 4 +++- 5 files changed, 23 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/courseware/date_summary.py b/lms/djangoapps/courseware/date_summary.py index 4cb2af0ab1..98478899fb 100644 --- a/lms/djangoapps/courseware/date_summary.py +++ b/lms/djangoapps/courseware/date_summary.py @@ -405,11 +405,7 @@ def verified_upgrade_deadline_link(user, course=None, course_id=None): ecommerce_service = EcommerceService() if ecommerce_service.is_enabled(user): if course is not None and isinstance(course, CourseOverview): - course_mode = [ - mode - for mode in course.modes - if mode.slug == CourseMode.VERIFIED - ] + course_mode = course.modes.get(mode_slug=CourseMode.VERIFIED) else: course_mode = CourseMode.objects.get( course_id=course_id, mode_slug=CourseMode.VERIFIED diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index c7760eda2e..bc86491251 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py @@ -269,8 +269,9 @@ class TestSendRecurringNudge(CacheIsolationTestCase): self.assertEqual(len(sent_messages), 1) - for args in sent_messages: - tasks._recurring_nudge_schedule_send(*args) + with self.assertNumQueries(SEND_QUERIES): + for args in sent_messages: + tasks._recurring_nudge_schedule_send(*args) self.assertEqual(mock_channel.deliver.call_count, 1) for (_name, (_msg, email), _kwargs) in mock_channel.deliver.mock_calls: diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 5f2998be8c..3c843c4546 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -182,11 +182,22 @@ def _add_upsell_button_to_email_template(a_user, a_schedule, template_context): # Check and upgrade link performs a query on CourseMode, which is triggering failures in # test_send_recurring_nudge.py upgrade_link, upgrade_date = check_and_get_upgrade_link_and_date(a_user, a_schedule.enrollment) + has_dynamic_deadline = a_schedule.upgrade_deadline is not None + has_upgrade_link = upgrade_link is not None + show_upsell = has_dynamic_deadline and has_upgrade_link + + template_context['show_upsell'] = show_upsell + if show_upsell: + template_context['upsell_link'] = upgrade_link + template_context['user_schedule_upgrade_deadline_time'] = dateformat.format( + upgrade_date, + get_format( + 'DATE_FORMAT', + lang=a_schedule.enrollment.course.language, + use_l10n=True + ) + ) - template_context['show_upsell'] = upgrade_link is not None - template_context['upsell_link'] = upgrade_link - template_context['user_schedule_upgrade_deadline_time'] = upgrade_date - @task(ignore_result=True, routing_key=ROUTING_KEY) def recurring_nudge_schedule_bin( diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.html b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.html index e60fa8378a..bee7b4e05a 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.html +++ b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.html @@ -65,7 +65,7 @@

{% blocktrans trimmed %} Don't miss the opportunity to highlight your new knowledge and skills by earning a verified - certificate. Upgrade by {{ user_schedule_upgrade_deadline_time|date:"l, F dS, Y" }}. + certificate. Upgrade by {{ user_schedule_upgrade_deadline_time }}. {% endblocktrans %}

diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.txt b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.txt index f04db8ce7e..a6704ac851 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.txt +++ b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/recurringnudge_day3/email/body.txt @@ -16,9 +16,11 @@ {% trans "Start learning now" %} <{{ course_url }}> {% endif %} +{% if show_upsell %} {% blocktrans trimmed %} Don't miss the opportunity to highlight your new knowledge and skills by earning a verified - certificate. Upgrade by {{ user_schedule_upgrade_deadline_time|date:"l, F dS, Y" }}. + certificate. Upgrade by {{ user_schedule_upgrade_deadline_time }}. Upgrade Now! <{{ upsell_link }}> {% endblocktrans %} +{% endif %} From d571adfb996505eda334540d15faf0f6913d5ad1 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Fri, 6 Oct 2017 16:09:24 -0400 Subject: [PATCH 6/7] Use upsell_link in upgrade reminder template --- openedx/core/djangoapps/schedules/tasks.py | 10 ++-------- .../schedules/edx_ace/upgradereminder/email/body.html | 4 ++-- .../schedules/edx_ace/upgradereminder/email/body.txt | 2 +- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 3c843c4546..67cd0af11f 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -326,14 +326,6 @@ def _upgrade_reminder_schedules_for_bin(site, target_day, bin_num, org_list, exc template_context.update({ 'student_name': user.profile.name, 'user_personal_address': user.profile.name if user.profile.name else user.username, - 'user_schedule_upgrade_deadline_time': dateformat.format( - schedule.upgrade_deadline, - get_format( - 'DATE_FORMAT', - lang=first_schedule.enrollment.course.language, - use_l10n=True - ) - ), 'course_name': first_schedule.enrollment.course.display_name, 'course_url': absolute_url(site, reverse('course_root', args=[str(first_schedule.enrollment.course_id)])), @@ -343,6 +335,8 @@ def _upgrade_reminder_schedules_for_bin(site, target_day, bin_num, org_list, exc 'cert_image': absolute_url(site, static('course_experience/images/verified-cert.png')), }) + _add_upsell_button_to_email_template(user, first_schedule, template_context) + yield (user, first_schedule.enrollment.course.language, template_context) diff --git a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.html b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.html index d33c12d25f..68a1bd2fe9 100644 --- a/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.html +++ b/openedx/core/djangoapps/schedules/templates/schedules/edx_ace/upgradereminder/email/body.html @@ -30,7 +30,7 @@ {% endblocktrans %}

- + {% blocktrans %}Example print-out of a verified certificate{% endblocktrans %} +{% trans "Upgrade now at" %} <{{ upsell_link }}> From 40d3f4f2fc91164316eac44349752148fef3ba48 Mon Sep 17 00:00:00 2001 From: sandroroux Date: Tue, 10 Oct 2017 16:43:18 -0400 Subject: [PATCH 7/7] Unit tests for "_add_upsell_button_to_email_template". --- common/djangoapps/student/models.py | 62 ++++-- .../djangoapps/student/tests/test_models.py | 10 +- .../tests/test_field_override_performance.py | 54 ++--- lms/djangoapps/courseware/date_summary.py | 2 +- .../migrations/0004_auto_20171010_1639.py | 19 ++ lms/djangoapps/courseware/models.py | 2 +- .../courseware/tests/test_date_summary.py | 12 - lms/djangoapps/courseware/tests/test_views.py | 10 +- .../tests/test_send_recurring_nudge.py | 205 +++++++++++++++--- .../tests/test_send_upgrade_reminder.py | 67 ++++-- openedx/core/djangoapps/schedules/signals.py | 2 +- openedx/core/djangoapps/schedules/tasks.py | 60 ++--- .../tests/views/test_course_updates.py | 2 +- 13 files changed, 365 insertions(+), 142 deletions(-) create mode 100644 lms/djangoapps/courseware/migrations/0004_auto_20171010_1639.py diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 934c250f2b..c313955a5d 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -1700,12 +1700,9 @@ class CourseEnrollment(models.Model): def upgrade_deadline(self): """ Returns the upgrade deadline for this enrollment, if it is upgradeable. - If the seat cannot be upgraded, None is returned. - Note: When loading this model, use `select_related` to retrieve the associated schedule object. - Returns: datetime|None """ @@ -1717,40 +1714,61 @@ class CourseEnrollment(models.Model): ) return None + if self.dynamic_upgrade_deadline is not None: + return self.dynamic_upgrade_deadline + + return self.course_upgrade_deadline + + @cached_property + def dynamic_upgrade_deadline(self): + try: - schedule_driven_deadlines_enabled = ( - DynamicUpgradeDeadlineConfiguration.is_enabled() - or CourseDynamicUpgradeDeadlineConfiguration.is_enabled(self.course_id) + course_overview = self.course + except CourseOverview.DoesNotExist: + course_overview = self.course_overview + + if not course_overview.self_paced: + return None + + if not DynamicUpgradeDeadlineConfiguration.is_enabled(): + return None + + course_config = CourseDynamicUpgradeDeadlineConfiguration.current(self.course_id) + if course_config.enabled and course_config.opt_out: + return None + + try: + if not self.schedule: + return None + + log.debug( + 'Schedules: Pulling upgrade deadline for CourseEnrollment %d from Schedule %d.', + self.id, self.schedule.id ) - if ( - schedule_driven_deadlines_enabled - and self.course_overview.self_paced - and self.schedule - and self.schedule.upgrade_deadline is not None - ): - log.debug( - 'Schedules: Pulling upgrade deadline for CourseEnrollment %d from Schedule %d.', - self.id, self.schedule.id - ) - return self.schedule.upgrade_deadline + upgrade_deadline = self.schedule.upgrade_deadline except ObjectDoesNotExist: # NOTE: Schedule has a one-to-one mapping with CourseEnrollment. If no schedule is associated # with this enrollment, Django will raise an exception rather than return None. log.debug('Schedules: No schedule exists for CourseEnrollment %d.', self.id) - pass + return None + if upgrade_deadline is None or datetime.now(UTC) >= upgrade_deadline: + return None + + return upgrade_deadline + + @cached_property + def course_upgrade_deadline(self): try: if self.verified_mode: log.debug('Schedules: Defaulting to verified mode expiration date-time for %s.', self.course_id) return self.verified_mode.expiration_datetime else: log.debug('Schedules: No verified mode located for %s.', self.course_id) + return None except CourseMode.DoesNotExist: log.debug('Schedules: %s has no verified mode.', self.course_id) - pass - - log.debug('Schedules: Returning default of `None`') - return None + return None def is_verified_enrollment(self): """ diff --git a/common/djangoapps/student/tests/test_models.py b/common/djangoapps/student/tests/test_models.py index 9b08398237..d8422e0960 100644 --- a/common/djangoapps/student/tests/test_models.py +++ b/common/djangoapps/student/tests/test_models.py @@ -14,6 +14,7 @@ from django.db.models.functions import Lower from course_modes.models import CourseMode from course_modes.tests.factories import CourseModeFactory from courseware.models import DynamicUpgradeDeadlineConfiguration +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.schedules.models import Schedule from openedx.core.djangoapps.schedules.tests.factories import ScheduleFactory from openedx.core.djangolib.testing.utils import skip_unless_lms @@ -142,9 +143,14 @@ class CourseEnrollmentTests(SharedModuleStoreTestCase): course_id=course.id, mode_slug=CourseMode.VERIFIED, # This must be in the future to ensure it is returned by downstream code. - expiration_datetime=datetime.datetime.now(pytz.UTC) + datetime.timedelta(days=1), + expiration_datetime=datetime.datetime.now(pytz.UTC) + datetime.timedelta(days=30), + ) + course_overview = CourseOverview.load_from_module_store(course.id) + enrollment = CourseEnrollmentFactory( + course_id=course.id, + mode=CourseMode.AUDIT, + course=course_overview, ) - enrollment = CourseEnrollmentFactory(course_id=course.id, mode=CourseMode.AUDIT) # The schedule's upgrade deadline should be used if a schedule exists DynamicUpgradeDeadlineConfiguration.objects.create(enabled=True) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 7ed0b0566f..eb5dbde16a 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -237,18 +237,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): # # of sql queries to default, # # of mongo queries, # ) - ('no_overrides', 1, True, False): (18, 1), - ('no_overrides', 2, True, False): (18, 1), - ('no_overrides', 3, True, False): (18, 1), - ('ccx', 1, True, False): (18, 1), - ('ccx', 2, True, False): (18, 1), - ('ccx', 3, True, False): (18, 1), - ('no_overrides', 1, False, False): (18, 1), - ('no_overrides', 2, False, False): (18, 1), - ('no_overrides', 3, False, False): (18, 1), - ('ccx', 1, False, False): (18, 1), - ('ccx', 2, False, False): (18, 1), - ('ccx', 3, False, False): (18, 1), + ('no_overrides', 1, True, False): (16, 1), + ('no_overrides', 2, True, False): (16, 1), + ('no_overrides', 3, True, False): (16, 1), + ('ccx', 1, True, False): (16, 1), + ('ccx', 2, True, False): (16, 1), + ('ccx', 3, True, False): (16, 1), + ('no_overrides', 1, False, False): (16, 1), + ('no_overrides', 2, False, False): (16, 1), + ('no_overrides', 3, False, False): (16, 1), + ('ccx', 1, False, False): (16, 1), + ('ccx', 2, False, False): (16, 1), + ('ccx', 3, False, False): (16, 1), } @@ -260,19 +260,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - ('no_overrides', 1, True, False): (18, 3), - ('no_overrides', 2, True, False): (18, 3), - ('no_overrides', 3, True, False): (18, 3), - ('ccx', 1, True, False): (18, 3), - ('ccx', 2, True, False): (18, 3), - ('ccx', 3, True, False): (18, 3), - ('ccx', 1, True, True): (19, 3), - ('ccx', 2, True, True): (19, 3), - ('ccx', 3, True, True): (19, 3), - ('no_overrides', 1, False, False): (18, 3), - ('no_overrides', 2, False, False): (18, 3), - ('no_overrides', 3, False, False): (18, 3), - ('ccx', 1, False, False): (18, 3), - ('ccx', 2, False, False): (18, 3), - ('ccx', 3, False, False): (18, 3), + ('no_overrides', 1, True, False): (16, 3), + ('no_overrides', 2, True, False): (16, 3), + ('no_overrides', 3, True, False): (16, 3), + ('ccx', 1, True, False): (16, 3), + ('ccx', 2, True, False): (16, 3), + ('ccx', 3, True, False): (16, 3), + ('ccx', 1, True, True): (17, 3), + ('ccx', 2, True, True): (17, 3), + ('ccx', 3, True, True): (17, 3), + ('no_overrides', 1, False, False): (16, 3), + ('no_overrides', 2, False, False): (16, 3), + ('no_overrides', 3, False, False): (16, 3), + ('ccx', 1, False, False): (16, 3), + ('ccx', 2, False, False): (16, 3), + ('ccx', 3, False, False): (16, 3), } diff --git a/lms/djangoapps/courseware/date_summary.py b/lms/djangoapps/courseware/date_summary.py index 98478899fb..b885beaf7c 100644 --- a/lms/djangoapps/courseware/date_summary.py +++ b/lms/djangoapps/courseware/date_summary.py @@ -396,7 +396,7 @@ def verified_upgrade_deadline_link(user, course=None, course_id=None): course_id (:class:`.CourseKey`): The course_id of the course to render for. Returns: - The formatted link to that will allow the user to upgrade to verified + The formatted link that will allow the user to upgrade to verified in this course. """ if course is not None: diff --git a/lms/djangoapps/courseware/migrations/0004_auto_20171010_1639.py b/lms/djangoapps/courseware/migrations/0004_auto_20171010_1639.py new file mode 100644 index 0000000000..9fa9ec6b4c --- /dev/null +++ b/lms/djangoapps/courseware/migrations/0004_auto_20171010_1639.py @@ -0,0 +1,19 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('courseware', '0003_auto_20170825_0935'), + ] + + operations = [ + migrations.AlterField( + model_name='coursedynamicupgradedeadlineconfiguration', + name='opt_out', + field=models.BooleanField(default=False, help_text='Disable the dynamic upgrade deadline for this course run.'), + ), + ] diff --git a/lms/djangoapps/courseware/models.py b/lms/djangoapps/courseware/models.py index c69e22e3eb..00b58c82d8 100644 --- a/lms/djangoapps/courseware/models.py +++ b/lms/djangoapps/courseware/models.py @@ -398,5 +398,5 @@ class CourseDynamicUpgradeDeadlineConfiguration(ConfigurationModel): ) opt_out = models.BooleanField( default=False, - help_text=_('This does not do anything and is no longer used. Setting enabled=False has the same effect.') + help_text=_('Disable the dynamic upgrade deadline for this course run.') ) diff --git a/lms/djangoapps/courseware/tests/test_date_summary.py b/lms/djangoapps/courseware/tests/test_date_summary.py index bee9bd9599..340e7883a6 100644 --- a/lms/djangoapps/courseware/tests/test_date_summary.py +++ b/lms/djangoapps/courseware/tests/test_date_summary.py @@ -617,18 +617,6 @@ class TestScheduleOverrides(SharedModuleStoreTestCase): block = VerifiedUpgradeDeadlineDate(course, enrollment.user) self.assertEqual(block.date, expected) - @override_waffle_flag(CREATE_SCHEDULE_WAFFLE_FLAG, True) - def test_date_with_self_paced_with_single_course(self): - """ If the global switch is off, a single course can still be enabled. """ - course = create_self_paced_course_run(days_till_start=-1) - DynamicUpgradeDeadlineConfiguration.objects.create(enabled=False) - course_config = CourseDynamicUpgradeDeadlineConfiguration.objects.create(enabled=True, course_id=course.id) - enrollment = CourseEnrollmentFactory(course_id=course.id, mode=CourseMode.AUDIT) - - block = VerifiedUpgradeDeadlineDate(course, enrollment.user) - expected = enrollment.created + timedelta(days=course_config.deadline_days) - self.assertEqual(block.date, expected) - @override_waffle_flag(CREATE_SCHEDULE_WAFFLE_FLAG, True) def test_date_with_existing_schedule(self): """ If a schedule is created while deadlines are disabled, they shouldn't magically appear once the feature is diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 50dda88ea4..b65169d5e5 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -213,8 +213,8 @@ class IndexQueryTestCase(ModuleStoreTestCase): NUM_PROBLEMS = 20 @ddt.data( - (ModuleStoreEnum.Type.mongo, 10, 146), - (ModuleStoreEnum.Type.split, 4, 146), + (ModuleStoreEnum.Type.mongo, 10, 145), + (ModuleStoreEnum.Type.split, 4, 145), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): @@ -1457,13 +1457,13 @@ class ProgressPageTests(ProgressPageBaseTests): """Test that query counts remain the same for self-paced and instructor-paced courses.""" SelfPacedConfiguration(enabled=self_paced_enabled).save() self.setup_course(self_paced=self_paced) - with self.assertNumQueries(35, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST), check_mongo_calls(1): + with self.assertNumQueries(34 if self_paced else 33, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST), check_mongo_calls(1): self._get_progress_page() @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) @ddt.data( - (False, 42, 26), - (True, 35, 22) + (False, 40, 26), + (True, 33, 22) ) @ddt.unpack def test_progress_queries(self, enable_waffle, initial, subsequent): diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py index bc86491251..5674a9e3ea 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_recurring_nudge.py @@ -10,42 +10,38 @@ from django.conf import settings from edx_ace.channel import ChannelType from edx_ace.test_utils import StubPolicy, patch_channels, patch_policies from edx_ace.utils.date import serialize +from edx_ace.message import Message from mock import Mock, patch from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import CourseLocator +from course_modes.models import CourseMode +from course_modes.tests.factories import CourseModeFactory +from courseware.models import DynamicUpgradeDeadlineConfiguration from openedx.core.djangoapps.schedules import resolvers, tasks from openedx.core.djangoapps.schedules.management.commands import send_recurring_nudge as nudge from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory, ScheduleFactory from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory -from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms +from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES +from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms, FilteredQueryCountMixin from student.tests.factories import UserFactory -# Populating recurring nudge emails requires three queries -# 1a) Find all users whose first enrollment during a day was in the specified hour window -# 1b) All schedules for all enrollments for that day for users in that window (with 1a as a subquery) -# 2) Check whether debugging is enabled -CONST_QUERIES = 2 - -# 1) Prefetch all course modes for those schedules -# 2) (When not cached) load the DynamicUpgradeDeadlineConfiguration -SCHEDULE_QUERIES = 2 - # 1) Load the current django site -# 2) Load the ScheduleConfig -SEND_QUERIES = 2 +# 2) Query the schedules to find all of the template context information +NUM_QUERIES_NO_MATCHING_SCHEDULES = 2 -# 1) (When not cached) load the CourseDynamicUpgradeDeadlineConfiguration -# 2) Load the VERIFIED course mode for the course -PER_COURSE_QUERIES = 2 +# 3) Query all course modes for all courses in returned schedules +NUM_QUERIES_WITH_MATCHES = NUM_QUERIES_NO_MATCHING_SCHEDULES + 1 + +NUM_COURSE_MODES_QUERIES = 1 @ddt.ddt @skip_unless_lms @skipUnless('openedx.core.djangoapps.schedules.apps.SchedulesConfig' in settings.INSTALLED_APPS, "Can't test schedules if the app isn't installed") -class TestSendRecurringNudge(CacheIsolationTestCase): +class TestSendRecurringNudge(FilteredQueryCountMixin, CacheIsolationTestCase): # pylint: disable=protected-access ENABLED_CACHES = ['default'] @@ -61,6 +57,8 @@ class TestSendRecurringNudge(CacheIsolationTestCase): self.site_config = SiteConfigurationFactory.create(site=site) ScheduleConfigFactory.create(site=self.site_config.site) + DynamicUpgradeDeadlineConfiguration.objects.create(enabled=True) + @patch.object(nudge.Command, 'resolver_class') def test_handle(self, mock_resolver): test_time = datetime.datetime(2017, 8, 1, tzinfo=pytz.UTC) @@ -94,16 +92,21 @@ class TestSendRecurringNudge(CacheIsolationTestCase): schedules = [ ScheduleFactory.create( start=datetime.datetime(2017, 8, 3, 18, 44, 30, tzinfo=pytz.UTC), - enrollment__user=UserFactory.create(), enrollment__course__id=CourseLocator('edX', 'toy', 'Bin') - ) for _ in range(schedule_count) + ) for i in range(schedule_count) ] + bins_in_use = frozenset((s.enrollment.user.id % tasks.RECURRING_NUDGE_NUM_BINS) for s in schedules) + test_time = datetime.datetime(2017, 8, 3, 18, tzinfo=pytz.UTC) test_time_str = serialize(test_time) for b in range(tasks.RECURRING_NUDGE_NUM_BINS): - # waffle flag takes an extra query before it is cached - with self.assertNumQueries(3 if b == 0 else 2): + expected_queries = NUM_QUERIES_NO_MATCHING_SCHEDULES + if b in bins_in_use: + # to fetch course modes for valid schedules + expected_queries += NUM_COURSE_MODES_QUERIES + + with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES): tasks.recurring_nudge_schedule_bin( self.site_config.site.id, target_day_str=test_time_str, day_offset=-3, bin_num=b, org_list=[schedules[0].enrollment.course.org], @@ -113,9 +116,9 @@ class TestSendRecurringNudge(CacheIsolationTestCase): @patch.object(tasks, '_recurring_nudge_schedule_send') def test_no_course_overview(self, mock_schedule_send): - schedule = ScheduleFactory.create( start=datetime.datetime(2017, 8, 3, 20, 34, 30, tzinfo=pytz.UTC), + enrollment__user=UserFactory.create(), ) schedule.enrollment.course_id = CourseKey.from_string('edX/toy/Not_2012_Fall') schedule.enrollment.save() @@ -123,8 +126,7 @@ class TestSendRecurringNudge(CacheIsolationTestCase): test_time = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) test_time_str = serialize(test_time) for b in range(tasks.RECURRING_NUDGE_NUM_BINS): - # waffle flag takes an extra query before it is cached - with self.assertNumQueries(3 if b == 0 else 2): + with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES, table_blacklist=WAFFLE_TABLES): tasks.recurring_nudge_schedule_bin( self.site_config.site.id, target_day_str=test_time_str, day_offset=-3, bin_num=b, org_list=[schedule.enrollment.course.org], @@ -196,7 +198,7 @@ class TestSendRecurringNudge(CacheIsolationTestCase): test_time = datetime.datetime(2017, 8, 3, 17, tzinfo=pytz.UTC) test_time_str = serialize(test_time) - with self.assertNumQueries(3): + with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): tasks.recurring_nudge_schedule_bin( limited_config.site.id, target_day_str=test_time_str, day_offset=-3, bin_num=0, org_list=org_list, exclude_orgs=exclude_orgs, @@ -220,7 +222,7 @@ class TestSendRecurringNudge(CacheIsolationTestCase): test_time = datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC) test_time_str = serialize(test_time) - with self.assertNumQueries(3): + with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): tasks.recurring_nudge_schedule_bin( self.site_config.site.id, target_day_str=test_time_str, day_offset=-3, bin_num=user.id % tasks.RECURRING_NUDGE_NUM_BINS, @@ -255,21 +257,21 @@ class TestSendRecurringNudge(CacheIsolationTestCase): sent_messages = [] - templates_override = deepcopy(settings.TEMPLATES) - templates_override[0]['OPTIONS']['string_if_invalid'] = "TEMPLATE WARNING - MISSING VARIABLE [%s]" - with self.settings(TEMPLATES=templates_override): + with self.settings(TEMPLATES=self._get_template_overrides()): with patch.object(tasks, '_recurring_nudge_schedule_send') as mock_schedule_send: mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args) - with self.assertNumQueries(3): + with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): tasks.recurring_nudge_schedule_bin( self.site_config.site.id, target_day_str=test_time_str, day_offset=day, - bin_num=user.id % tasks.RECURRING_NUDGE_NUM_BINS, org_list=[schedules[0].enrollment.course.org], + bin_num=self._calculate_bin_for_user(user), org_list=[schedules[0].enrollment.course.org], ) self.assertEqual(len(sent_messages), 1) - with self.assertNumQueries(SEND_QUERIES): + # Load the site + # Check the schedule config + with self.assertNumQueries(2): for args in sent_messages: tasks._recurring_nudge_schedule_send(*args) @@ -277,3 +279,142 @@ class TestSendRecurringNudge(CacheIsolationTestCase): for (_name, (_msg, email), _kwargs) in mock_channel.deliver.mock_calls: for template in attr.astuple(email): self.assertNotIn("TEMPLATE WARNING", template) + + def test_user_in_course_with_verified_coursemode_receives_upsell(self): + user = UserFactory.create() + course_id = CourseLocator('edX', 'toy', 'Course1') + + first_day_of_schedule = datetime.datetime.now(pytz.UTC) + verification_deadline = first_day_of_schedule + datetime.timedelta(days=21) + target_day = first_day_of_schedule + target_hour_as_string = serialize(target_day) + nudge_day = 3 + + schedule = ScheduleFactory.create(start=first_day_of_schedule, + enrollment__user=user, + enrollment__course__id=course_id) + schedule.enrollment.course.self_paced = True + schedule.enrollment.course.save() + + CourseModeFactory( + course_id=course_id, + mode_slug=CourseMode.VERIFIED, + expiration_datetime=verification_deadline + ) + schedule.upgrade_deadline = verification_deadline + + bin_task_parameters = [ + target_hour_as_string, + nudge_day, + user, + schedule.enrollment.course.org + ] + sent_messages = self._stub_sender_and_collect_sent_messages(bin_task=tasks.recurring_nudge_schedule_bin, + stubbed_send_task=patch.object(tasks, '_recurring_nudge_schedule_send'), + bin_task_params=bin_task_parameters) + + self.assertEqual(len(sent_messages), 1) + + message_attributes = sent_messages[0][1] + self.assertTrue(self._contains_upsell_attribute(message_attributes)) + + def test_no_upsell_button_when_DUDConfiguration_is_off(self): + DynamicUpgradeDeadlineConfiguration.objects.create(enabled=False) + + user = UserFactory.create() + course_id = CourseLocator('edX', 'toy', 'Course1') + + first_day_of_schedule = datetime.datetime.now(pytz.UTC) + target_day = first_day_of_schedule + target_hour_as_string = serialize(target_day) + nudge_day = 3 + + schedule = ScheduleFactory.create(start=first_day_of_schedule, + enrollment__user=user, + enrollment__course__id=course_id) + schedule.enrollment.course.self_paced = True + schedule.enrollment.course.save() + + bin_task_parameters = [ + target_hour_as_string, + nudge_day, + user, + schedule.enrollment.course.org + ] + sent_messages = self._stub_sender_and_collect_sent_messages(bin_task=tasks.recurring_nudge_schedule_bin, + stubbed_send_task=patch.object(tasks, '_recurring_nudge_schedule_send'), + bin_task_params=bin_task_parameters) + + self.assertEqual(len(sent_messages), 1) + + message_attributes = sent_messages[0][1] + self.assertFalse(self._contains_upsell_attribute(message_attributes)) + + def test_user_with_no_upgrade_deadline_is_not_upsold(self): + user = UserFactory.create() + course_id = CourseLocator('edX', 'toy', 'Course1') + + first_day_of_schedule = datetime.datetime.now(pytz.UTC) + target_day = first_day_of_schedule + target_hour_as_string = serialize(target_day) + nudge_day = 3 + + schedule = ScheduleFactory.create(start=first_day_of_schedule, + upgrade_deadline=None, + enrollment__user=user, + enrollment__course__id=course_id) + schedule.enrollment.course.self_paced = True + schedule.enrollment.course.save() + + verification_deadline = first_day_of_schedule + datetime.timedelta(days=21) + CourseModeFactory( + course_id=course_id, + mode_slug=CourseMode.VERIFIED, + expiration_datetime=verification_deadline + ) + schedule.upgrade_deadline = verification_deadline + + bin_task_parameters = [ + target_hour_as_string, + nudge_day, + user, + schedule.enrollment.course.org + ] + sent_messages = self._stub_sender_and_collect_sent_messages(bin_task=tasks.recurring_nudge_schedule_bin, + stubbed_send_task=patch.object(tasks, '_recurring_nudge_schedule_send'), + bin_task_params=bin_task_parameters) + + self.assertEqual(len(sent_messages), 1) + + message_attributes = sent_messages[0][1] + self.assertFalse(self._contains_upsell_attribute(message_attributes)) + + def _stub_sender_and_collect_sent_messages(self, bin_task, stubbed_send_task, bin_task_params): + sent_messages = [] + + with self.settings(TEMPLATES=self._get_template_overrides()), stubbed_send_task as mock_schedule_send: + + mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args) + + bin_task( + self.site_config.site.id, + target_day_str=bin_task_params[0], + day_offset=bin_task_params[1], + bin_num=self._calculate_bin_for_user(bin_task_params[2]), + org_list=[bin_task_params[3]] + ) + + return sent_messages + + def _get_template_overrides(self): + templates_override = deepcopy(settings.TEMPLATES) + templates_override[0]['OPTIONS']['string_if_invalid'] = "TEMPLATE WARNING - MISSING VARIABLE [%s]" + return templates_override + + def _calculate_bin_for_user(self, user): + return user.id % tasks.RECURRING_NUDGE_NUM_BINS + + def _contains_upsell_attribute(self, msg_attr): + msg = Message.from_string(msg_attr) + tmp = msg.context["show_upsell"] + return msg.context["show_upsell"] diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py index 8a6ed80d0f..9003aa45fd 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/test_send_upgrade_reminder.py @@ -14,21 +14,41 @@ from mock import Mock, patch from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import CourseLocator +from course_modes.models import CourseMode +from course_modes.tests.factories import CourseModeFactory +from courseware.models import DynamicUpgradeDeadlineConfiguration from openedx.core.djangoapps.schedules import resolvers, tasks from openedx.core.djangoapps.schedules.management.commands import send_upgrade_reminder as reminder from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory, ScheduleFactory from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory -from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms +from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES +from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms, FilteredQueryCountMixin from student.tests.factories import UserFactory +# 1) Load the current django site +# 2) Query the schedules to find all of the template context information +NUM_QUERIES_NO_MATCHING_SCHEDULES = 2 + +# 3) Query all course modes for all courses in returned schedules +NUM_QUERIES_WITH_MATCHES = NUM_QUERIES_NO_MATCHING_SCHEDULES + 1 + +# 1) Global dynamic deadline switch +# 2) E-commerce configuration +NUM_QUERIES_WITH_DEADLINE = 2 + +NUM_COURSE_MODES_QUERIES = 1 + + @ddt.ddt @skip_unless_lms @skipUnless('openedx.core.djangoapps.schedules.apps.SchedulesConfig' in settings.INSTALLED_APPS, "Can't test schedules if the app isn't installed") -class TestUpgradeReminder(CacheIsolationTestCase): +class TestUpgradeReminder(FilteredQueryCountMixin, CacheIsolationTestCase): # pylint: disable=protected-access + ENABLED_CACHES = ['default'] + def setUp(self): super(TestUpgradeReminder, self).setUp() @@ -74,20 +94,26 @@ class TestUpgradeReminder(CacheIsolationTestCase): schedules = [ ScheduleFactory.create( upgrade_deadline=datetime.datetime(2017, 8, 3, 18, 44, 30, tzinfo=pytz.UTC), - enrollment__user=UserFactory.create(), enrollment__course__id=CourseLocator('edX', 'toy', 'Bin') - ) for _ in range(schedule_count) + ) for i in range(schedule_count) ] + bins_in_use = frozenset((s.enrollment.user.id % tasks.UPGRADE_REMINDER_NUM_BINS) for s in schedules) + test_time = datetime.datetime(2017, 8, 3, 18, tzinfo=pytz.UTC) test_time_str = serialize(test_time) for b in range(tasks.UPGRADE_REMINDER_NUM_BINS): - # waffle flag takes an extra query before it is cached - with self.assertNumQueries(3 if b == 0 else 2): + expected_queries = NUM_QUERIES_NO_MATCHING_SCHEDULES + if b in bins_in_use: + # to fetch course modes for valid schedules + expected_queries += NUM_COURSE_MODES_QUERIES + + with self.assertNumQueries(expected_queries, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( self.site_config.site.id, target_day_str=test_time_str, day_offset=2, bin_num=b, org_list=[schedules[0].enrollment.course.org], ) + self.assertEqual(mock_schedule_send.apply_async.call_count, schedule_count) self.assertFalse(mock_ace.send.called) @@ -103,8 +129,7 @@ class TestUpgradeReminder(CacheIsolationTestCase): test_time = datetime.datetime(2017, 8, 3, 20, tzinfo=pytz.UTC) test_time_str = serialize(test_time) for b in range(tasks.UPGRADE_REMINDER_NUM_BINS): - # waffle flag takes an extra query before it is cached - with self.assertNumQueries(3 if b == 0 else 2): + with self.assertNumQueries(NUM_QUERIES_NO_MATCHING_SCHEDULES, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( self.site_config.site.id, target_day_str=test_time_str, day_offset=2, bin_num=b, org_list=[schedule.enrollment.course.org], @@ -176,7 +201,7 @@ class TestUpgradeReminder(CacheIsolationTestCase): test_time = datetime.datetime(2017, 8, 3, 17, tzinfo=pytz.UTC) test_time_str = serialize(test_time) - with self.assertNumQueries(3): + with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( limited_config.site.id, target_day_str=test_time_str, day_offset=2, bin_num=0, org_list=org_list, exclude_orgs=exclude_orgs, @@ -200,7 +225,7 @@ class TestUpgradeReminder(CacheIsolationTestCase): test_time = datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC) test_time_str = serialize(test_time) - with self.assertNumQueries(3): + with self.assertNumQueries(NUM_QUERIES_WITH_MATCHES, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( self.site_config.site.id, target_day_str=test_time_str, day_offset=2, bin_num=user.id % tasks.UPGRADE_REMINDER_NUM_BINS, @@ -212,18 +237,31 @@ class TestUpgradeReminder(CacheIsolationTestCase): @ddt.data(*itertools.product((1, 10, 100), (2, 10))) @ddt.unpack def test_templates(self, message_count, day): + DynamicUpgradeDeadlineConfiguration.objects.create(enabled=True) + now = datetime.datetime.now(pytz.UTC) + future_date = now + datetime.timedelta(days=21) user = UserFactory.create() schedules = [ ScheduleFactory.create( - upgrade_deadline=datetime.datetime(2017, 8, 3, 19, 44, 30, tzinfo=pytz.UTC), + upgrade_deadline=future_date, enrollment__user=user, enrollment__course__id=CourseLocator('edX', 'toy', 'Course{}'.format(course_num)) ) for course_num in range(message_count) ] - test_time = datetime.datetime(2017, 8, 3, 19, tzinfo=pytz.UTC) + for schedule in schedules: + schedule.enrollment.course.self_paced = True + schedule.enrollment.course.save() + + CourseModeFactory( + course_id=schedule.enrollment.course.id, + mode_slug=CourseMode.VERIFIED, + expiration_datetime=future_date + ) + + test_time = future_date test_time_str = serialize(test_time) patch_policies(self, [StubPolicy([ChannelType.PUSH])]) @@ -241,7 +279,10 @@ class TestUpgradeReminder(CacheIsolationTestCase): with patch.object(tasks, '_upgrade_reminder_schedule_send') as mock_schedule_send: mock_schedule_send.apply_async = lambda args, *_a, **_kw: sent_messages.append(args) - with self.assertNumQueries(3): + # we execute one query per course to see if it's opted out of dynamic upgrade deadlines, however, + # since we create a new course for each schedule in this test, we expect there to be one per message + num_expected_queries = NUM_QUERIES_WITH_MATCHES + NUM_QUERIES_WITH_DEADLINE + message_count + with self.assertNumQueries(num_expected_queries, table_blacklist=WAFFLE_TABLES): tasks.upgrade_reminder_schedule_bin( self.site_config.site.id, target_day_str=test_time_str, day_offset=day, bin_num=user.id % tasks.UPGRADE_REMINDER_NUM_BINS, diff --git a/openedx/core/djangoapps/schedules/signals.py b/openedx/core/djangoapps/schedules/signals.py index cbf5201fd4..685118aa64 100644 --- a/openedx/core/djangoapps/schedules/signals.py +++ b/openedx/core/djangoapps/schedules/signals.py @@ -112,7 +112,7 @@ def _get_upgrade_deadline_delta_setting(course_id): # Check if the course has a deadline course_config = CourseDynamicUpgradeDeadlineConfiguration.current(course_id) - if course_config.enabled: + if course_config.enabled and not course_config.opt_out: delta = course_config.deadline_days return delta diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 67cd0af11f..821c7997c5 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -12,13 +12,15 @@ from django.core.urlresolvers import reverse from django.db.models import F, Min from django.db.utils import DatabaseError from django.utils.formats import dateformat, get_format +import pytz from edx_ace import ace from edx_ace.message import Message from edx_ace.recipient import Recipient from edx_ace.utils.date import deserialize from opaque_keys.edx.keys import CourseKey -from lms.djangoapps.experiments.utils import check_and_get_upgrade_link_and_date + +from courseware.date_summary import verified_upgrade_deadline_link, verified_upgrade_link_is_valid from edxmako.shortcuts import marketing_link from openedx.core.djangoapps.schedules.message_type import ScheduleMessageType @@ -134,7 +136,7 @@ def _recurring_nudge_schedules_for_hour(site, target_hour, org_list, exclude_org } # Information for including upsell messaging in template. - _add_upsell_button_to_email_template(user, first_schedule, template_context) + _add_upsell_button_information_to_template_context(user, first_schedule, template_context) yield (user, first_schedule.enrollment.course.language, template_context) @@ -178,27 +180,6 @@ def _gather_users_and_schedules_for_target_hour(target_hour, org_list, exclude_o return users, schedules -def _add_upsell_button_to_email_template(a_user, a_schedule, template_context): - # Check and upgrade link performs a query on CourseMode, which is triggering failures in - # test_send_recurring_nudge.py - upgrade_link, upgrade_date = check_and_get_upgrade_link_and_date(a_user, a_schedule.enrollment) - has_dynamic_deadline = a_schedule.upgrade_deadline is not None - has_upgrade_link = upgrade_link is not None - show_upsell = has_dynamic_deadline and has_upgrade_link - - template_context['show_upsell'] = show_upsell - if show_upsell: - template_context['upsell_link'] = upgrade_link - template_context['user_schedule_upgrade_deadline_time'] = dateformat.format( - upgrade_date, - get_format( - 'DATE_FORMAT', - lang=a_schedule.enrollment.course.language, - use_l10n=True - ) - ) - - @task(ignore_result=True, routing_key=ROUTING_KEY) def recurring_nudge_schedule_bin( site_id, target_day_str, day_offset, bin_num, org_list, exclude_orgs=False, override_recipient_email=None, @@ -254,7 +235,7 @@ def _recurring_nudge_schedules_for_bin(site, target_day, bin_num, org_list, excl }) # Information for including upsell messaging in template. - _add_upsell_button_to_email_template(user, first_schedule, template_context) + _add_upsell_button_information_to_template_context(user, first_schedule, template_context) yield (user, first_schedule.enrollment.course.language, template_context) @@ -335,7 +316,7 @@ def _upgrade_reminder_schedules_for_bin(site, target_day, bin_num, org_list, exc 'cert_image': absolute_url(site, static('course_experience/images/verified-cert.png')), }) - _add_upsell_button_to_email_template(user, first_schedule, template_context) + _add_upsell_button_information_to_template_context(user, first_schedule, template_context) yield (user, first_schedule.enrollment.course.language, template_context) @@ -393,3 +374,32 @@ def get_schedules_with_target_date_by_bin_and_orgs(schedule_date_field, target_d schedules = schedules.using("read_replica") return schedules + + +def _add_upsell_button_information_to_template_context(user, schedule, template_context): + enrollment = schedule.enrollment + course = enrollment.course + + verified_upgrade_link = _get_link_to_purchase_verified_certificate(user, schedule) + has_verified_upgrade_link = verified_upgrade_link is not None + + if has_verified_upgrade_link: + template_context['upsell_link'] = verified_upgrade_link + template_context['user_schedule_upgrade_deadline_time'] = dateformat.format( + enrollment.dynamic_upgrade_deadline, + get_format( + 'DATE_FORMAT', + lang=course.language, + use_l10n=True + ) + ) + + template_context['show_upsell'] = has_verified_upgrade_link + + +def _get_link_to_purchase_verified_certificate(a_user, a_schedule): + enrollment = a_schedule.enrollment + if enrollment.dynamic_upgrade_deadline is None or not verified_upgrade_link_is_valid(enrollment): + return None + + return verified_upgrade_deadline_link(a_user, enrollment.course) diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py index 4bcae0c965..ab0bb2806c 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/openedx/features/course_experience/tests/views/test_course_updates.py @@ -126,7 +126,7 @@ class TestCourseUpdatesPage(SharedModuleStoreTestCase): course_updates_url(self.course) # Fetch the view and verify that the query counts haven't changed - with self.assertNumQueries(32, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(30, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): with check_mongo_calls(4): url = course_updates_url(self.course) self.client.get(url)