From 26ae8b4bcd584abccc9955cb934670321bc7c857 Mon Sep 17 00:00:00 2001 From: Justin Hynes Date: Thu, 30 Jun 2022 08:43:30 -0400 Subject: [PATCH] feat: add management command to clean stale certificate availability dates [MICROBA-1806] We are aware of product issue where it is possible for a self-paced course-run to get have a `certificate_availability_date` created in the course settings. This can have an adverse effect on the Credentials IDA where a learner's Program Record does not correctly display the course certificates they have earned because of this data. This not only causes confusion for our learners, as it appears that a course certificate a learner can access and share in the LMS is displayed as unearned in the Credential's program record, but this can also cause issues when a learner attempts to share their program record through a credit pathway and the program record would not accurately reflect their program completion. Unfortunately, the settings that manage the certificate availability date are hidden for self-paced courses in Studio (as they should only be used in instructor-paced courses). For this reason, we are introducing a management command that will remove a certificate available date for a specified (self-paced) course-run. This will allow us to fix issues for individual learners while we work on a longer-term fix for the larger issue. * Add new `clean_stale_certificate_available_dates` management command * Add new `CleanStaleCertificateAvailabilityDates` Configuration Model * Add tests for the new management command * (Unrelated cleanup) Fix potential issue with private.py settings in the CMS being overwritten in devstack.py for developers using devstack. --- cms/djangoapps/contentstore/admin.py | 11 +- ...clean_stale_certificate_available_dates.py | 124 ++++++++++++++++ ...an_stale_certificate_availability_dates.py | 139 ++++++++++++++++++ ...stalecertificateavailabilitydatesconfig.py | 30 ++++ cms/djangoapps/contentstore/models.py | 20 +++ cms/envs/devstack.py | 11 +- 6 files changed, 329 insertions(+), 6 deletions(-) create mode 100644 cms/djangoapps/contentstore/management/commands/clean_stale_certificate_available_dates.py create mode 100644 cms/djangoapps/contentstore/management/commands/tests/test_clean_stale_certificate_availability_dates.py create mode 100644 cms/djangoapps/contentstore/migrations/0008_cleanstalecertificateavailabilitydatesconfig.py diff --git a/cms/djangoapps/contentstore/admin.py b/cms/djangoapps/contentstore/admin.py index 12f9dfcf6f..5b58c9495f 100644 --- a/cms/djangoapps/contentstore/admin.py +++ b/cms/djangoapps/contentstore/admin.py @@ -10,7 +10,11 @@ from django.contrib.admin.helpers import ACTION_CHECKBOX_NAME from django.utils.translation import gettext as _ from edx_django_utils.admin.mixins import ReadOnlyAdminMixin -from cms.djangoapps.contentstore.models import BackfillCourseTabsConfig, VideoUploadConfig +from cms.djangoapps.contentstore.models import ( + BackfillCourseTabsConfig, + CleanStaleCertificateAvailabilityDatesConfig, + VideoUploadConfig +) from cms.djangoapps.contentstore.outlines_regenerate import CourseOutlineRegenerate from openedx.core.djangoapps.content.learning_sequences.api import key_supports_outlines @@ -78,6 +82,11 @@ class CourseOutlineRegenerateAdmin(ReadOnlyAdminMixin, admin.ModelAdmin): return super().changelist_view(request, extra_context) +class CleanStaleCertificateAvailabilityDatesConfigAdmin(ConfigurationModelAdmin): + pass + + admin.site.register(BackfillCourseTabsConfig, ConfigurationModelAdmin) admin.site.register(VideoUploadConfig, ConfigurationModelAdmin) admin.site.register(CourseOutlineRegenerate, CourseOutlineRegenerateAdmin) +admin.site.register(CleanStaleCertificateAvailabilityDatesConfig, ConfigurationModelAdmin) diff --git a/cms/djangoapps/contentstore/management/commands/clean_stale_certificate_available_dates.py b/cms/djangoapps/contentstore/management/commands/clean_stale_certificate_available_dates.py new file mode 100644 index 0000000000..2069155b75 --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/clean_stale_certificate_available_dates.py @@ -0,0 +1,124 @@ +""" +A management command that can be used to remove a stale `certificate_available_date` from a course-run. +""" +import logging +import shlex + +from django.core.management.base import BaseCommand, CommandError +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey + +from cms.djangoapps.contentstore.models import CleanStaleCertificateAvailabilityDatesConfig +from openedx.core.lib.courses import get_course_by_id +from xmodule.data import CertificatesDisplayBehaviors +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore + +log = logging.getLogger(__name__) + + +class Command(BaseCommand): + """ + Example usage: + + To see info regarding the course-run's certificate display behavior and availability date data: + $ ./manage.py cms clean_stale_certificate_available_dates --course-runs + To remove a stale certificate availability date: + $ ./manage.py cms clean_stale_certificate_available_dates --course-runs --delete + To run this command using arguments stored in the database (as part of the + `CleanStaleCertificateAvailabilityDatesConfig` configuration model): + $ ./manage.py cms clean_stale_certificate_available_dates --args-from-database + """ + help = ( + "Removes the certificate_available_date data from the specified self-paced course-runs." + ) + + def add_arguments(self, parser): + parser.add_argument( + "--course-runs", + nargs="+", + help=( + "A space seperated list of course-runs where we want to remove the `certificate_available_date` from" + ) + ) + parser.add_argument( + "--delete", + action='store_true', + help=( + "Optional. If included, this flag tells the management command to remove the " + "`certificiate_available_date` data for the given courses" + ) + ) + parser.add_argument( + "--args-from-database", + action='store_true', + help=( + "Use arguments from the CleanStaleCertificateAvailabilityDatesConfig configuration model instead of " + "the command line", + ) + ) + + def get_args_from_database(self): + """ + Returns an options dictionary from the current CleanStaleCertificateAvailabilityDatesConfig model. + """ + config = CleanStaleCertificateAvailabilityDatesConfig.current() + if not config.enabled: + raise CommandError( + "CleanStaleCertificateAvailabilityDatesConfig is disabled, but --args-from-database was requested." + ) + + args = shlex.split(config.arguments) + parser = self.create_parser("manage.py", "clean_stale_certificate_available_dates") + return vars(parser.parse_args(args)) + + def log_certificate_info_for_course(self, course, course_run): + """ + A utility function that prints information regarding the current course's pacing, certificate display behavior, + and certificate availability date. + """ + logging.info(f"Is course '{course_run}' self-paced? {course.self_paced}") + logging.info( + f"The certificate display behavior for course '{course_run}' is {course.certificates_display_behavior}" + ) + logging.info( + f"The certificate_available_date for course '{course_run}' is '{course.certificate_available_date}'" + ) + + def update_course_settings(self, course): + """ + A utility function that updates-and-commits the certificate display behavior and certificate_available_date + for the course to fix the stale certificate availability date. + """ + del course.certificate_available_date + course.certificates_display_behavior = CertificatesDisplayBehaviors.EARLY_NO_INFO + # commit the changes + modulestore().update_item(course, ModuleStoreEnum.UserID.mgmt_command) + + def handle(self, *args, **options): + if options["args_from_database"]: + options = self.get_args_from_database() + + log.info("The `clean_stale_certificate_available_dates` command is starting.") + + course_runs = options.get("course_runs") + for run in course_runs: + # verify that the course-key specified is valid + logging.info(f"Checking that '{run}' is a parsable CourseKey") + try: + course_key = CourseKey.from_string(run) + except InvalidKeyError as key_error: + raise CommandError(f"{run} is not a parsable CourseKey") from key_error + + course = get_course_by_id(course_key) + self.log_certificate_info_for_course(course, run) + + if options['delete']: + if not course.self_paced: + raise CommandError( + f"The course '{run}' is instructor-paced and the certificate availability date can be adjusted " + "via Studio in the UI. Aborting operation." + ) + + logging.info(f"Attempting to remove the certificate available date in course '{run}'") + self.update_course_settings(course) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_clean_stale_certificate_availability_dates.py b/cms/djangoapps/contentstore/management/commands/tests/test_clean_stale_certificate_availability_dates.py new file mode 100644 index 0000000000..42b3ca408a --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/tests/test_clean_stale_certificate_availability_dates.py @@ -0,0 +1,139 @@ +""" +Tests for the `clean_stale_certificate_available_dates` management command. +""" +from datetime import datetime, timedelta + +from django.core.management import CommandError, call_command +import pytz + +from cms.djangoapps.contentstore.models import CleanStaleCertificateAvailabilityDatesConfig +from openedx.core.lib.courses import get_course_by_id +from xmodule.data import CertificatesDisplayBehaviors +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory + + +class CleanStaleCertAvailableDateTests(ModuleStoreTestCase): + """ + Tests for the `clean_stale_certificate_available_dates` management command. + """ + def setUp(self): + super().setUp() + + self.self_paced_course1 = CourseFactory() + self.self_paced_course2 = CourseFactory() + self.instructor_paced_course1 = CourseFactory() + + # set the self-paced courses to self-paced, create and insert an invalid certificate available date for them + self.certificate_available_date = datetime.now(pytz.UTC) + timedelta(days=30) + self.self_paced_course1.self_paced = True + self.self_paced_course1.certificate_available_date = self.certificate_available_date + self.self_paced_course2.self_paced = True + self.self_paced_course2.certificate_available_date = self.certificate_available_date + self.instructor_paced_course1.self_paced = False + self.instructor_paced_course1.certificate_available_date = self.certificate_available_date + + self.update_course(self.self_paced_course1, ModuleStoreEnum.UserID.test) + self.update_course(self.self_paced_course2, ModuleStoreEnum.UserID.test) + self.update_course(self.instructor_paced_course1, ModuleStoreEnum.UserID.test) + + def test_remove_certificate_available_date_from_self_paced_courses(self): + """ + Happy path test that attempts to remove a certificate available date from a course that shouldn't have one. + """ + call_command( + "clean_stale_certificate_available_dates", + "--course-runs", + self.self_paced_course1.id, + self.self_paced_course2.id, + "--delete" + ) + + course = get_course_by_id(self.self_paced_course1.id) + assert course.certificate_available_date is None + assert course.certificates_display_behavior == CertificatesDisplayBehaviors.EARLY_NO_INFO + course = get_course_by_id(self.self_paced_course2.id) + assert course.certificate_available_date is None + assert course.certificates_display_behavior == CertificatesDisplayBehaviors.EARLY_NO_INFO + + def test_remove_certificate_available_date_no_delete_flag(self): + """ + Verifies the behavior of the management command when the delete flag is _not_ passed in. When the management + command is run without this flag, we should just log information about the course and no action should be taken + to remove the data from the course. + """ + call_command( + "clean_stale_certificate_available_dates", + "--course-runs", + self.self_paced_course1.id + ) + + course = get_course_by_id(self.self_paced_course1.id) + assert course.certificate_available_date is not None + # I can't compare the two datetime's directly as the datetime coming from the course/modulestore is slightly + # different (the TZ info is saved as a FixedOffset instead of the specific time zone, so I compare the two + # datetimes as an ISO8601 string) + assert ( + course.certificate_available_date.isoformat(timespec='minutes') == + self.certificate_available_date.isoformat(timespec='minutes') + ) + assert course.certificates_display_behavior == CertificatesDisplayBehaviors.END + + def test_remove_certificate_available_date_from_instructor_paced_course_expect_error(self): + """ + Verifies behavior of the management command when it is run with an instructor-paced course. + """ + expected_exception_message = ( + f"The course '{self.instructor_paced_course1.id}' is instructor-paced and the certificate availability " + "date can be adjusted via Studio in the UI. Aborting operation." + ) + + with self.assertRaises(CommandError) as error: + call_command( + "clean_stale_certificate_available_dates", + "--course-runs", + self.instructor_paced_course1.id, + "--delete" + ) + + assert str(error.exception) == expected_exception_message + + def test_remove_certificate_available_date_with_args_from_database(self): + """ + Verifies the behavior of the management command when it is run via using arguments stored in our database as + part of the `CleanStaleCertificateAvailabilityDatesConfig` configuration model. + """ + # try running the command but expect it to fail since there are no options in the database and the configuration + # model is disabled + expected_error_message = ( + "CleanStaleCertificateAvailabilityDatesConfig is disabled, but --args-from-database was requested." + ) + with self.assertRaises(CommandError) as error: + call_command("clean_stale_certificate_available_dates", "--args-from-database") + + assert str(error.exception) == expected_error_message + + # add a configuration and enable it + config = CleanStaleCertificateAvailabilityDatesConfig.current() + config.arguments = f"--course-runs {self.self_paced_course1.id} {self.self_paced_course2.id} --delete" + config.enabled = True + config.save() + + call_command("clean_stale_certificate_available_dates", "--args-from-database") + + course = get_course_by_id(self.self_paced_course1.id) + assert course.certificate_available_date is None + assert course.certificates_display_behavior == CertificatesDisplayBehaviors.EARLY_NO_INFO + course = get_course_by_id(self.self_paced_course2.id) + assert course.certificate_available_date is None + assert course.certificates_display_behavior == CertificatesDisplayBehaviors.EARLY_NO_INFO + + # explicitly disable the configuration and try running one more time + config.enabled = False + config.save() + + with self.assertRaises(CommandError) as disabled_error: + call_command("clean_stale_certificate_available_dates", "--args-from-database") + + assert str(error.exception) == expected_error_message diff --git a/cms/djangoapps/contentstore/migrations/0008_cleanstalecertificateavailabilitydatesconfig.py b/cms/djangoapps/contentstore/migrations/0008_cleanstalecertificateavailabilitydatesconfig.py new file mode 100644 index 0000000000..50187a8cac --- /dev/null +++ b/cms/djangoapps/contentstore/migrations/0008_cleanstalecertificateavailabilitydatesconfig.py @@ -0,0 +1,30 @@ +# Generated by Django 3.2.13 on 2022-07-11 17:08 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('contentstore', '0007_backfillcoursetabsconfig'), + ] + + operations = [ + migrations.CreateModel( + name='CleanStaleCertificateAvailabilityDatesConfig', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('change_date', models.DateTimeField(auto_now_add=True, verbose_name='Change date')), + ('enabled', models.BooleanField(default=False, verbose_name='Enabled')), + ('arguments', models.TextField(blank=True, help_text="A space seperated collection of arguments to be used when running the `clean_stale_certificate_available_dates` management command.' See the management command for options.")), + ('changed_by', models.ForeignKey(editable=False, null=True, on_delete=django.db.models.deletion.PROTECT, to=settings.AUTH_USER_MODEL, verbose_name='Changed by')), + ], + options={ + 'verbose_name': "Arguments for 'clean_stale_certificate_availability_dates'", + 'verbose_name_plural': "Arguments for 'clean_stale_certificate_availability_dates'", + }, + ), + ] diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index 66d884d8dd..f3b39397cf 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -43,3 +43,23 @@ class BackfillCourseTabsConfig(ConfigurationModel): help_text='How many courses to backfill in this run (or zero for all courses)', default=0, ) + + +class CleanStaleCertificateAvailabilityDatesConfig(ConfigurationModel): + """ + Manages configuration for a run of the `clean_stale_certificate_availability_dates` management command. + + .. no_pii: + """ + class Meta: + app_label = "contentstore" + verbose_name = "Arguments for 'clean_stale_certificate_availability_dates'" + verbose_name_plural = "Arguments for 'clean_stale_certificate_availability_dates'" + + arguments = TextField( + blank=True, + help_text=( + "A space seperated collection of arguments to be used when running the " + "`clean_stale_certificate_available_dates` management command.' See the management command for options." + ) + ) diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index 52e1acbfad..2b1a429ca2 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -248,11 +248,6 @@ MODULESTORE = convert_module_store_setting_if_needed(MODULESTORE) # Dummy secret key for dev SECRET_KEY = '85920908f28904ed733fe576320db18cabd7b6cd' -############################################################################### -# See if the developer has any local overrides. -if os.path.isfile(join(dirname(abspath(__file__)), 'private.py')): - from .private import * # pylint: disable=import-error,wildcard-import - ############# CORS headers for cross-domain requests ################# FEATURES['ENABLE_CORS_HEADERS'] = True CORS_ALLOW_CREDENTIALS = True @@ -295,3 +290,9 @@ SOCIAL_AUTH_REDIRECT_IS_HTTPS = False #################### Network configuration #################### # Devstack is directly exposed to the caller CLOSEST_CLIENT_IP_FROM_HEADERS = [] + +################# New settings must go ABOVE this line ################# +######################################################################## +# See if the developer has any local overrides. +if os.path.isfile(join(dirname(abspath(__file__)), 'private.py')): + from .private import * # pylint: disable=import-error,wildcard-import