From ce39f526809dd32154779fec263e4cfed285caaa Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Fri, 17 Sep 2021 15:17:23 -0400 Subject: [PATCH] chore: add tests for new goal reminder email management command --- lms/djangoapps/course_goals/admin.py | 19 +- .../commands/goal_reminder_email.py | 103 ++++++----- .../management/commands/tests/__init__.py | 0 .../tests/test_goal_reminder_email.py | 166 ++++++++++++++++++ .../0008_coursegoalreminderstatus.py | 30 ++++ lms/djangoapps/course_goals/models.py | 19 +- .../course_goals/tests/factories.py | 36 ++++ 7 files changed, 328 insertions(+), 45 deletions(-) create mode 100644 lms/djangoapps/course_goals/management/commands/tests/__init__.py create mode 100644 lms/djangoapps/course_goals/management/commands/tests/test_goal_reminder_email.py create mode 100644 lms/djangoapps/course_goals/migrations/0008_coursegoalreminderstatus.py create mode 100644 lms/djangoapps/course_goals/tests/factories.py diff --git a/lms/djangoapps/course_goals/admin.py b/lms/djangoapps/course_goals/admin.py index b7c1384b1a..f60a319017 100644 --- a/lms/djangoapps/course_goals/admin.py +++ b/lms/djangoapps/course_goals/admin.py @@ -2,7 +2,7 @@ from django.contrib import admin -from lms.djangoapps.course_goals.models import CourseGoal, UserActivity +from lms.djangoapps.course_goals.models import CourseGoal, CourseGoalReminderStatus, UserActivity @admin.register(CourseGoal) @@ -17,6 +17,23 @@ class CourseGoalAdmin(admin.ModelAdmin): search_fields = ('user__username', 'course_key') +@admin.register(CourseGoalReminderStatus) +class CourseGoalReminderStatusAdmin(admin.ModelAdmin): + """Admin for CourseGoalReminderStatus""" + list_display = ('id', + 'goal_user', + 'goal_course_key', + 'email_reminder_sent') + raw_id_fields = ('goal',) + search_fields = ('goal__user__username', 'goal__course_key') + + def goal_user(self, obj): + return obj.goal.user.username + + def goal_course_key(self, obj): + return obj.goal.course_key + + @admin.register(UserActivity) class UserActivityAdmin(admin.ModelAdmin): """Admin for UserActivity""" diff --git a/lms/djangoapps/course_goals/management/commands/goal_reminder_email.py b/lms/djangoapps/course_goals/management/commands/goal_reminder_email.py index f44c506fb0..0acdf1b995 100644 --- a/lms/djangoapps/course_goals/management/commands/goal_reminder_email.py +++ b/lms/djangoapps/course_goals/management/commands/goal_reminder_email.py @@ -2,27 +2,36 @@ Command to trigger sending reminder emails for learners to achieve their Course Goals """ from datetime import date, timedelta +import logging +from django.contrib.sites.models import Site from django.core.management.base import BaseCommand from common.djangoapps.student.models import CourseEnrollment from lms.djangoapps.certificates.api import get_certificate_for_user_id from lms.djangoapps.certificates.data import CertificateStatuses -from lms.djangoapps.course_goals.models import CourseGoal, UserActivity +from lms.djangoapps.course_goals.models import CourseGoal, CourseGoalReminderStatus, UserActivity from lms.djangoapps.course_goals.toggles import COURSE_GOALS_NUMBER_OF_DAYS_GOALS from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.lib.celery.task_utils import emulate_http_request from openedx.features.course_duration_limits.access import get_user_course_expiration_date +log = logging.getLogger(__name__) + MONDAY_WEEKDAY = 0 SUNDAY_WEEKDAY = 6 +def ace_send(): + """Just here as a mock hook for tests - drop this once we fix AA-909""" + + class Command(BaseCommand): """ Example usage: $ ./manage.py lms goal_reminder_email """ - help = '' + help = 'Send emails to users that are in danger of missing their course goals for the week' def handle(self, *args, **options): """ @@ -38,55 +47,67 @@ class Command(BaseCommand): # Monday is the start of when we consider user's activity towards counting towards their weekly # goal. As such, we use Mondays to clear out the email reminders sent from the previous week. if today.weekday() == MONDAY_WEEKDAY: - CourseGoal.objects.filter(email_reminder_sent=True).update(email_reminder_sent=False) + CourseGoalReminderStatus.objects.filter(email_reminder_sent=True).update(email_reminder_sent=False) + log.info('Cleared all reminder statuses') return - course_goals = CourseGoal.objects.filter(days_per_week__gt=0, subscribed_to_reminders=True, email_reminder_sent=False) + course_goals = CourseGoal.objects.filter( + days_per_week__gt=0, subscribed_to_reminders=True, + ).exclude( + reminder_status__email_reminder_sent=True, + ) all_goal_course_keys = course_goals.values_list('course_key', flat=True).distinct() # Exclude all courses whose end dates are earlier than Sunday so we don't send an email about hitting # a course goal when it may not even be possible. - # NOTE TO MT: Should we maybe also use this list and set all of the course goals in this to be unsubscribed? courses_to_exclude = CourseOverview.objects.filter( - id__in=all_goal_course_keys, end__date__lt=sunday_date + id__in=all_goal_course_keys, end__date__lte=sunday_date ).values_list('id', flat=True) + count = 0 course_goals = course_goals.exclude(course_key__in=courses_to_exclude).select_related('user').order_by('user') - for goal in course_goals: - if not COURSE_GOALS_NUMBER_OF_DAYS_GOALS.is_enabled(goal.course_key): - continue + with emulate_http_request(site=Site.objects.get_current()): # emulate a request for waffle's benefit + for goal in course_goals: + if self.handle_goal(goal, today, sunday_date, monday_date): + count += 1 - enrollment = CourseEnrollment.objects.prefetch_related('course_overview').get(user=goal.user, course__id=goal.course_key) - # If you're not actively enrolled in the course or your enrollment was this week - if not enrollment.is_active or enrollment.created.date() >= monday_date: - continue + log.info(f'Sent {count} emails') - audit_access_expiration_date = get_user_course_expiration_date(user, enrollment.course_overview) - # If an audit user's access expires this week, exclude them from the email since they may not - # be able to hit their goal anyway - if audit_access_expiration_date and audit_access_expiration_date.date() < sunday_date: - # NOTE TO MT: Is this overkill? If they upgrade, I think they'd need to reselect it, but if they - # don't, then we at least stop doing this for them for the rest of time. - goal.update(subscribed_to_reminders=False) - continue + @staticmethod + def handle_goal(goal, today, sunday_date, monday_date): + """Sends an email reminder for a single CourseGoal, if it passes all our checks""" + if not COURSE_GOALS_NUMBER_OF_DAYS_GOALS.is_enabled(goal.course_key): + return False - # NOTE TO MT: I had a thread with Aperture Eng (Justin Hynes) about which function to use here. This was the recommended one - cert = get_certificate_for_user_id(goal.user, goal.course_key) - # If a user has a downloadable certificate, we will consider them as having completed - # the course and opt them out of receiving emails - if cert and cert.status == CertificateStatuses.downloadable: - # NOTE TO MT: Is this overkill? If their cert status changes, they would remain unsubscribed. - # Also just not sure if it's bad for us to unsubscribe them anyway. Could be a weird thing to - # come back to. I leave to your discretion. - goal.update(subscribed_to_reminders=False) - continue + enrollment = CourseEnrollment.get_enrollment(goal.user, goal.course_key, select_related=['course']) + # If you're not actively enrolled in the course or your enrollment was this week + if not enrollment or not enrollment.is_active or enrollment.created.date() >= monday_date: + return False - # Check the number of days left to successfully hit their goal - week_activity_count = UserActivity.objects.filter(user=goal.user, course_key=goal.course_key, date__gte=monday_date).count() - required_days_left = goal.days_per_week - week_activity_count - # The weekdays are 0 indexed, but we want this to be 1 to match required_days_left. - # Essentially, if today is Sunday, days_left_in_week should be 1 since they have Sunday to hit their goal. - days_left_in_week = SUNDAY_WEEKDAY - today.weekday() + 1 - if required_days_left == days_left_in_week: - # TODO: hook up email https://openedx.atlassian.net/browse/AA-909 - # ace.send(msg) - goal.update(email_reminder_sent=True) + audit_access_expiration_date = get_user_course_expiration_date(goal.user, enrollment.course_overview) + # If an audit user's access expires this week, exclude them from the email since they may not + # be able to hit their goal anyway + if audit_access_expiration_date and audit_access_expiration_date.date() <= sunday_date: + return False + + cert = get_certificate_for_user_id(goal.user, goal.course_key) + # If a user has a downloadable certificate, we will consider them as having completed + # the course and opt them out of receiving emails + if cert and cert.status == CertificateStatuses.downloadable: + return False + + # Check the number of days left to successfully hit their goal + week_activity_count = UserActivity.objects.filter( + user=goal.user, course_key=goal.course_key, date__gte=monday_date, + ).count() + required_days_left = goal.days_per_week - week_activity_count + # The weekdays are 0 indexed, but we want this to be 1 to match required_days_left. + # Essentially, if today is Sunday, days_left_in_week should be 1 since they have Sunday to hit their goal. + days_left_in_week = SUNDAY_WEEKDAY - today.weekday() + 1 + if required_days_left == days_left_in_week: + # TODO: hook up email AA-909 + # ace.send(msg) + ace_send() # temporary for tests, drop with AA-909 and just mock ace.send directly + CourseGoalReminderStatus.objects.update_or_create(goal=goal, defaults={'email_reminder_sent': True}) + return True + + return False diff --git a/lms/djangoapps/course_goals/management/commands/tests/__init__.py b/lms/djangoapps/course_goals/management/commands/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lms/djangoapps/course_goals/management/commands/tests/test_goal_reminder_email.py b/lms/djangoapps/course_goals/management/commands/tests/test_goal_reminder_email.py new file mode 100644 index 0000000000..1d7d9ee976 --- /dev/null +++ b/lms/djangoapps/course_goals/management/commands/tests/test_goal_reminder_email.py @@ -0,0 +1,166 @@ +"""Tests for the goal_reminder_email command""" + +from datetime import datetime +from pytz import UTC +from unittest import mock + +import ddt +from django.core.management import call_command +from django.test import TestCase +from edx_toggles.toggles.testutils import override_waffle_flag +from freezegun import freeze_time + +from common.djangoapps.student.models import CourseEnrollment +from common.djangoapps.student.tests.factories import CourseEnrollmentFactory +from lms.djangoapps.course_goals.models import CourseGoalReminderStatus +from lms.djangoapps.course_goals.tests.factories import ( + CourseGoalFactory, CourseGoalReminderStatusFactory, UserActivityFactory, +) +from lms.djangoapps.course_goals.toggles import COURSE_GOALS_NUMBER_OF_DAYS_GOALS +from lms.djangoapps.certificates.data import CertificateStatuses +from lms.djangoapps.certificates.tests.factories import GeneratedCertificateFactory +from openedx.core.djangolib.testing.utils import skip_unless_lms + +# Some constants just for clarity of tests (assuming week starts on a Monday, as March 2021 used below does) +MONDAY = 0 +TUESDAY = 1 +WEDNESDAY = 2 +THURSDAY = 3 +FRIDAY = 4 +SATURDAY = 5 +SUNDAY = 6 + + +@ddt.ddt +@skip_unless_lms +@override_waffle_flag(COURSE_GOALS_NUMBER_OF_DAYS_GOALS, active=True) +class TestGoalReminderEmailCommand(TestCase): + """ + Test goal_reminder_email management command. + + A lot of these methods will hardcode references to March 2021. This is just a convenient anchor point for us + because it started on a Monday. Calls to the management command will freeze time so it's during March. + """ + def make_valid_goal(self, **kwargs): + """Creates a goal that will cause an email to be sent as the goal is valid but has been missed""" + kwargs.setdefault('days_per_week', 6) + kwargs.setdefault('subscribed_to_reminders', True) + kwargs.setdefault('overview__start', datetime(2021, 1, 1, tzinfo=UTC)) + kwargs.setdefault('overview__end', datetime(2021, 4, 1, tzinfo=UTC)) # Have it end in the future + goal = CourseGoalFactory(**kwargs) + + with freeze_time('2021-02-01'): # Create enrollment before March + CourseEnrollmentFactory(user=goal.user, course_id=goal.course_key) + + return goal + + def call_command(self, day=TUESDAY, expect_sent=None, expect_send_count=None): + """Calls the management command with a frozen time and optionally checks whether we sent an email""" + with mock.patch('lms.djangoapps.course_goals.management.commands.goal_reminder_email.ace_send') as mock_send: + with freeze_time(f'2021-03-0{day + 1}'): # March 2021 starts on a Monday + call_command('goal_reminder_email') + + if expect_sent is not None: + assert CourseGoalReminderStatus.objects.filter(email_reminder_sent=True).exists() == expect_sent + + if expect_send_count is None: + expect_send_count = 1 if expect_sent else 0 + assert mock_send.call_count == expect_send_count + + def test_happy_path(self): + """Confirm that with default arguments, our test methods send an email""" + # A lot of our "negative" tests below assume that these methods called with these arguments will give you a + # working "email sent" state. And then tweak one thing to confirm it didn't send. + # So, if you change this method at all, go also change the "failure case" tests below to match. + self.make_valid_goal() + self.call_command(expect_sent=True) + + def test_clear_all_on_monday(self): + """Verify that we reset all email tracking on Monday""" + CourseGoalReminderStatusFactory(email_reminder_sent=True) + CourseGoalReminderStatusFactory(email_reminder_sent=False) + self.call_command(MONDAY) + assert CourseGoalReminderStatus.objects.filter(email_reminder_sent=False).count() == 2 + assert CourseGoalReminderStatus.objects.filter(email_reminder_sent=True).count() == 0 + + @ddt.data( + (4, 5, SATURDAY, False), # Already made target + (2, 0, FRIDAY, False), # Just before end of week cutoff + (2, 0, SATURDAY, True), # Just after end of week cutoff + (2, 0, SUNDAY, False), # Day after end of week cutoff + (2, 1, SATURDAY, False), # With some activity in the bag already, that cutoff moves up + (2, 1, SUNDAY, True), # ...to Sunday + (7, 3, WEDNESDAY, False), # Same as above, but with some more interesting numbers + (7, 3, THURSDAY, True), # ditto (after cutoff) + (7, 3, FRIDAY, False), # ditto (day after) + (7, 0, MONDAY, False), # We never send on Mondays, only clear + # Here are some unrealistic edge cases - just want to make sure we don't blow up with an exception + (9, 1, TUESDAY, False), + (9, 9, TUESDAY, False), + (1, 9, TUESDAY, False), + ) + @ddt.unpack + def test_will_send_on_right_day(self, days_per_week, days_of_activity, current_day, expect_sent): + """Verify that via the normal conditions, we either send or not based on the days of activity""" + goal = self.make_valid_goal(days_per_week=days_per_week) + for day in range(days_of_activity): + UserActivityFactory(user=goal.user, course_key=goal.course_key, date=datetime(2021, 3, day + 1, tzinfo=UTC)) + + self.call_command(day=current_day, expect_sent=expect_sent) + + def test_feature_disabled(self): + self.make_valid_goal() + with override_waffle_flag(COURSE_GOALS_NUMBER_OF_DAYS_GOALS, active=False): + self.call_command(expect_sent=False) + + def test_never_enrolled(self): + self.make_valid_goal() + CourseEnrollment.objects.all().delete() + self.call_command(expect_sent=False) + + def test_inactive_enrollment(self): + self.make_valid_goal() + CourseEnrollment.objects.update(is_active=False) + self.call_command(expect_sent=False) + + def test_recent_enrollment(self): + self.make_valid_goal() + CourseEnrollment.objects.update(created=datetime(2021, 3, 1, tzinfo=UTC)) + self.call_command(expect_sent=False) + + @ddt.data( + (datetime(2021, 3, 8, tzinfo=UTC), True), + (datetime(2021, 3, 7, tzinfo=UTC), False), + ) + @ddt.unpack + @mock.patch('lms.djangoapps.course_goals.management.commands.goal_reminder_email.get_user_course_expiration_date') + def test_access_expired(self, expiration_date, expect_sent, mock_get_expiration_date): + self.make_valid_goal() + mock_get_expiration_date.return_value = expiration_date # awkward to set up, so just mock it + self.call_command(expect_sent=expect_sent) + + def test_cert_already_generated(self): + goal = self.make_valid_goal() + GeneratedCertificateFactory(user=goal.user, course_id=goal.course_key, status=CertificateStatuses.downloadable) + self.call_command(expect_sent=False) + + def test_unsubscribed(self): + self.make_valid_goal(subscribed_to_reminders=False) + self.call_command(expect_sent=False) + + def test_no_double_sends(self): + self.make_valid_goal() + self.call_command(expect_sent=True, expect_send_count=1) + self.call_command(expect_sent=True, expect_send_count=0) + + def test_no_days_per_week(self): + self.make_valid_goal(days_per_week=0) + self.call_command(expect_sent=False) + + @ddt.data( + datetime(2021, 2, 1, tzinfo=UTC), # very over and done with + datetime(2021, 3, 7, tzinfo=UTC), # ending this Sunday + ) + def test_old_course(self, end): + self.make_valid_goal(overview__end=end) + self.call_command(expect_sent=False) diff --git a/lms/djangoapps/course_goals/migrations/0008_coursegoalreminderstatus.py b/lms/djangoapps/course_goals/migrations/0008_coursegoalreminderstatus.py new file mode 100644 index 0000000000..660a7852ef --- /dev/null +++ b/lms/djangoapps/course_goals/migrations/0008_coursegoalreminderstatus.py @@ -0,0 +1,30 @@ +# Generated by Django 2.2.24 on 2021-09-20 16:42 + +from django.db import migrations, models +import django.db.models.deletion +import django.utils.timezone +import model_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('course_goals', '0007_set_unsubscribe_token_default'), + ] + + operations = [ + migrations.CreateModel( + name='CourseGoalReminderStatus', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')), + ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')), + ('email_reminder_sent', models.BooleanField(default=False, help_text='Tracks if the email reminder to complete the Course Goal has been sent this week.')), + ('goal', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='reminder_status', to='course_goals.CourseGoal')), + ], + options={ + 'abstract': False, + 'verbose_name_plural': 'Course goal reminder statuses', + }, + ), + ] diff --git a/lms/djangoapps/course_goals/models.py b/lms/djangoapps/course_goals/models.py index da3f3c71fd..fc045a9eb7 100644 --- a/lms/djangoapps/course_goals/models.py +++ b/lms/djangoapps/course_goals/models.py @@ -8,6 +8,7 @@ from django.contrib.auth import get_user_model from django.db import models from django.utils.translation import ugettext_lazy as _ from model_utils import Choices +from model_utils.models import TimeStampedModel from opaque_keys.edx.django.models import CourseKeyField from simple_history.models import HistoricalRecords @@ -36,9 +37,6 @@ class CourseGoal(models.Model): course_key = CourseKeyField(max_length=255, db_index=True) # The goal a user has set for the number of days they want to learn per week days_per_week = models.PositiveIntegerField(default=0) - # email_reminder_sent = models.BooleanField( - # default=False, help_text='Tracks if the email reminder to complete the Course Goal has been sent this week.' - # ) # Controls whether a user will receive emails reminding them to stay on track with their learning goal subscribed_to_reminders = models.BooleanField(default=False) @@ -67,6 +65,21 @@ class CourseGoal(models.Model): super().save(**kwargs) +class CourseGoalReminderStatus(TimeStampedModel): + """ + Tracks whether we've sent a reminder about a particular goal this week. + + See the management command goal_reminder_email for more detail about how this is used. + """ + class Meta: + verbose_name_plural = "Course goal reminder statuses" + + goal = models.OneToOneField(CourseGoal, on_delete=models.CASCADE, related_name='reminder_status') + email_reminder_sent = models.BooleanField( + default=False, help_text='Tracks if the email reminder to complete the Course Goal has been sent this week.' + ) + + class UserActivity(models.Model): """ Tracks the date a user performs an activity in a course for goal purposes. diff --git a/lms/djangoapps/course_goals/tests/factories.py b/lms/djangoapps/course_goals/tests/factories.py new file mode 100644 index 0000000000..9be379e648 --- /dev/null +++ b/lms/djangoapps/course_goals/tests/factories.py @@ -0,0 +1,36 @@ +"""Provides factories for course goals.""" + +import factory +from factory.django import DjangoModelFactory + +from common.djangoapps.student.tests.factories import UserFactory +from lms.djangoapps.course_goals.models import CourseGoal, CourseGoalReminderStatus, UserActivity +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory + + +class CourseGoalFactory(DjangoModelFactory): + """Factory for CourseGoal, which will make user and course for you""" + class Meta: + model = CourseGoal + + class Params: + overview = factory.SubFactory(CourseOverviewFactory) + + user = factory.SubFactory(UserFactory) + course_key = factory.SelfAttribute('overview.id') + + +class CourseGoalReminderStatusFactory(DjangoModelFactory): + """Factory for CourseGoalReminderStatus""" + class Meta: + model = CourseGoalReminderStatus + + goal = factory.SubFactory(CourseGoalFactory) + + +class UserActivityFactory(DjangoModelFactory): + """Factory for UserActivity""" + class Meta: + model = UserActivity + + user = factory.SubFactory(UserFactory)