From ee7159471494befd3f63c542e3c31e86ed24f863 Mon Sep 17 00:00:00 2001 From: "Albert (AJ) St. Aubin" Date: Tue, 4 May 2021 19:25:27 -0400 Subject: [PATCH] refactor!: Remove backpopulate_program_credentials in favor of using notify_credentials command In this PR we are removing a management command that has performance issues and duplicates logic not available in the notify_credentials management command. --- openedx/core/djangoapps/programs/admin.py | 3 +- .../backpopulate_program_credentials.py | 163 -------- .../0014_delete_customprogramsconfig.py | 16 + openedx/core/djangoapps/programs/models.py | 18 - .../test_backpopulate_program_credentials.py | 390 ------------------ 5 files changed, 17 insertions(+), 573 deletions(-) delete mode 100644 openedx/core/djangoapps/programs/management/commands/backpopulate_program_credentials.py create mode 100644 openedx/core/djangoapps/programs/migrations/0014_delete_customprogramsconfig.py delete mode 100644 openedx/core/djangoapps/programs/tests/test_backpopulate_program_credentials.py diff --git a/openedx/core/djangoapps/programs/admin.py b/openedx/core/djangoapps/programs/admin.py index 90b461bbc2..af9cb0597d 100644 --- a/openedx/core/djangoapps/programs/admin.py +++ b/openedx/core/djangoapps/programs/admin.py @@ -6,7 +6,7 @@ django admin pages for program support models from config_models.admin import ConfigurationModelAdmin from django.contrib import admin -from openedx.core.djangoapps.programs.models import ProgramsApiConfig, CustomProgramsConfig +from openedx.core.djangoapps.programs.models import ProgramsApiConfig class ProgramsApiConfigAdmin(ConfigurationModelAdmin): @@ -14,4 +14,3 @@ class ProgramsApiConfigAdmin(ConfigurationModelAdmin): admin.site.register(ProgramsApiConfig, ProgramsApiConfigAdmin) -admin.site.register(CustomProgramsConfig, ConfigurationModelAdmin) diff --git a/openedx/core/djangoapps/programs/management/commands/backpopulate_program_credentials.py b/openedx/core/djangoapps/programs/management/commands/backpopulate_program_credentials.py deleted file mode 100644 index d3ab2822ad..0000000000 --- a/openedx/core/djangoapps/programs/management/commands/backpopulate_program_credentials.py +++ /dev/null @@ -1,163 +0,0 @@ -"""Management command for backpopulating missing program credentials.""" - - -import logging -from collections import namedtuple -from functools import reduce # pylint: disable=redefined-builtin, useless-suppression -from django.contrib.sites.models import Site -from django.core.management.base import BaseCommand, CommandError -from django.db.models import Q -from opaque_keys.edx.keys import CourseKey - -from common.djangoapps.course_modes.models import CourseMode -from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCertificate -from openedx.core.djangoapps.catalog.utils import get_programs -from openedx.core.djangoapps.programs.tasks import award_program_certificates -from openedx.core.djangoapps.programs.models import CustomProgramsConfig - - -# TODO: Log to console, even with debug mode disabled? -logger = logging.getLogger(__name__) # pylint: disable=invalid-name -CourseRun = namedtuple('CourseRun', ['key', 'type']) - - -class Command(BaseCommand): - """Management command for backpopulating missing program credentials. - - The command's goal is to pass a narrow subset of usernames to an idempotent - Celery task for further (parallelized) processing. - """ - help = 'Backpopulate missing program credentials.' - course_runs = None - usernames = None - - def add_arguments(self, parser): - parser.add_argument( - '-c', '--commit', - action='store_true', - dest='commit', - default=False, - help='Submit tasks for processing.' - ) - parser.add_argument( - '--args-from-database', - action='store_true', - default=False, - help='Use arguments from the Config model instead of the command line.', - ) - parser.add_argument( - '--program-uuids', - nargs='+', - help='Award certificates only for specific programs.', - ) - parser.add_argument( - '--usernames', - nargs='+', - help='Award certificates only to specific users.', - ) - - def get_args_from_database(self): - """ Returns an options dictionary from the current NotifyCredentialsConfig model. """ - config = CustomProgramsConfig.current() - if not config.enabled: - raise CommandError('CustomProgramsConfig is disabled, but --args-from-database was requested.') - - # We don't need fancy shell-style whitespace/quote handling - none of our arguments are complicated - argv = config.arguments.split() - - parser = self.create_parser('manage.py', 'backpopulate_program_credentials') - return parser.parse_args(argv).__dict__ # we want a dictionary, not a non-iterable Namespace object - - def handle(self, *args, **options): - program_uuids, usernames = None, None - if options['args_from_database']: - logger.info('Loading arguments from the database for custom programs or learners.') - - arguments = self.get_args_from_database() - program_uuids = arguments.get('program-uuids', None) - usernames = arguments.get('usernames', None) - - logger.info('Loading programs from the catalog.') - self._load_course_runs(program_uuids=program_uuids) - - logger.info('Looking for users who may be eligible for a program certificate.') - self._load_usernames(users=usernames) - - if options.get('commit'): - logger.info('Enqueuing program certification tasks for %d candidates.', len(self.usernames)) - else: - logger.info( - 'Found %d candidates. To enqueue program certification tasks, pass the -c or --commit flags.', - len(self.usernames) - ) - return - - succeeded, failed = 0, 0 - for username in self.usernames: - try: - award_program_certificates.delay(username) - except: # pylint: disable=bare-except - failed += 1 - logger.exception('Failed to enqueue task for user [%s]', username) - else: - succeeded += 1 - logger.debug('Successfully enqueued task for user [%s]', username) - - logger.info( - 'Done. Successfully enqueued tasks for %d candidates. ' - 'Failed to enqueue tasks for %d candidates.', - succeeded, - failed - ) - - def _load_course_runs(self, program_uuids=None): - """Find all course runs which are part of a program.""" - programs = [] - if program_uuids: - programs.extend(get_programs(uuids=program_uuids)) - else: - for site in Site.objects.all(): - logger.info('Loading programs from the catalog for site %s.', site.domain) - programs.extend(get_programs(site)) - - self.course_runs = self._flatten(programs) - - def _flatten(self, programs): - """Flatten programs into a set of course runs.""" - course_runs = set() - for program in programs: - for course in program['courses']: - for course_run in course['course_runs']: - key = CourseKey.from_string(course_run['key']) - course_runs.add( - CourseRun(key, course_run['type']) - ) - - return course_runs - - def _load_usernames(self, users=None): - """Identify a subset of users who may be eligible for a program certificate. - - This is done by finding users who have earned a qualifying certificate in - at least one program course's course run. - """ - status_query = Q(status__in=CertificateStatuses.PASSED_STATUSES) - course_run_query = reduce( - lambda x, y: x | y, - [Q(course_id=course_run.key, mode=course_run.type) for course_run in self.course_runs] - ) - - # Account for the fact that no-id-professional and professional are equivalent - for course_run in self.course_runs: - if course_run.type == CourseMode.PROFESSIONAL: - course_run_query |= Q(course_id=course_run.key, mode=CourseMode.NO_ID_PROFESSIONAL_MODE) - - query = status_query & course_run_query - - username_dicts = GeneratedCertificate.eligible_available_certificates.filter( - query - ).values('user__username').distinct() - self.usernames = [d['user__username'] for d in username_dicts] - if users: - # keeping only those learners who are in the arguments - self.usernames = list(set(self.usernames) & set(users)) diff --git a/openedx/core/djangoapps/programs/migrations/0014_delete_customprogramsconfig.py b/openedx/core/djangoapps/programs/migrations/0014_delete_customprogramsconfig.py new file mode 100644 index 0000000000..374cbc35f3 --- /dev/null +++ b/openedx/core/djangoapps/programs/migrations/0014_delete_customprogramsconfig.py @@ -0,0 +1,16 @@ +# Generated by Django 2.2.20 on 2021-05-04 19:36 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('programs', '0013_customprogramsconfig'), + ] + + operations = [ + migrations.DeleteModel( + name='CustomProgramsConfig', + ), + ] diff --git a/openedx/core/djangoapps/programs/models.py b/openedx/core/djangoapps/programs/models.py index 4917364bef..b524d39b9c 100644 --- a/openedx/core/djangoapps/programs/models.py +++ b/openedx/core/djangoapps/programs/models.py @@ -22,21 +22,3 @@ class ProgramsApiConfig(ConfigurationModel): 'Path used to construct URLs to programs marketing pages (e.g., "/foo").' ) ) - - -class CustomProgramsConfig(ConfigurationModel): # pylint: disable=model-missing-unicode, useless-suppression - """ - Manages configuration for a run of the backpopulate_program_credentials management command. - """ - class Meta: - app_label = 'programs' - verbose_name = 'backpopulate_program_credentials argument' - - arguments = models.TextField( - blank=True, - help_text='Useful for manually running a Jenkins job. Specify like "--usernames A B --program-uuids X Y".', - default='', - ) - - def __str__(self): - return str(self.arguments) diff --git a/openedx/core/djangoapps/programs/tests/test_backpopulate_program_credentials.py b/openedx/core/djangoapps/programs/tests/test_backpopulate_program_credentials.py deleted file mode 100644 index 13fd4861f1..0000000000 --- a/openedx/core/djangoapps/programs/tests/test_backpopulate_program_credentials.py +++ /dev/null @@ -1,390 +0,0 @@ -"""Tests for the backpopulate_program_credentials management command.""" - -from unittest import mock - -import ddt -from django.core.management import call_command -from django.test import TestCase -from opaque_keys.edx.keys import CourseKey - -from common.djangoapps.course_modes.models import CourseMode -from lms.djangoapps.certificates.api import MODES -from lms.djangoapps.certificates.models import CertificateStatuses -from lms.djangoapps.certificates.tests.factories import GeneratedCertificateFactory -from openedx.core.djangoapps.catalog.tests.factories import ( - CourseFactory, - CourseRunFactory, - ProgramFactory, - generate_course_run_key -) -from openedx.core.djangoapps.catalog.tests.mixins import CatalogIntegrationMixin -from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory -from openedx.core.djangoapps.credentials.tests.mixins import CredentialsApiConfigMixin -from openedx.core.djangolib.testing.utils import skip_unless_lms -from common.djangoapps.student.tests.factories import UserFactory - -COMMAND_MODULE = 'openedx.core.djangoapps.programs.management.commands.backpopulate_program_credentials' - - -@ddt.ddt -@mock.patch(COMMAND_MODULE + '.get_programs') -@mock.patch(COMMAND_MODULE + '.award_program_certificates.delay') -@skip_unless_lms -class BackpopulateProgramCredentialsTests(CatalogIntegrationMixin, CredentialsApiConfigMixin, TestCase): - """Tests for the backpopulate_program_credentials management command.""" - course_run_key, alternate_course_run_key, course_run_key_no_course_overview = ( - generate_course_run_key() for __ in range(3) - ) - # Constants for the _get_programs_data hierarchy types used in test_flatten() - SEPARATE_PROGRAMS = 'separate_programs' - SEPARATE_COURSES = 'separate_courses' - SAME_COURSE = 'same_course' - - def setUp(self): - super().setUp() - - self.alice = UserFactory() - self.bob = UserFactory() - - # We need CourseOverview instances to exist for the test courses - CourseOverviewFactory(id=CourseKey.from_string(self.course_run_key)) - CourseOverviewFactory(id=CourseKey.from_string(self.alternate_course_run_key)) - - # Disable certification to prevent the task from being triggered when - # setting up test data (i.e., certificates with a passing status), thereby - # skewing mock call counts. - self.create_credentials_config(enable_learner_issuance=False) - - catalog_integration = self.create_catalog_integration() - UserFactory(username=catalog_integration.service_username) - - def _get_programs_data(self, hierarchy_type): - """ - Generate a mock response for get_programs() with the given type of - course hierarchy. Dramatically simplifies (and makes consistent - between test runs) the ddt-generated test_flatten methods. - """ - if hierarchy_type == self.SEPARATE_PROGRAMS: - return [ - ProgramFactory( - courses=[ - CourseFactory(course_runs=[ - CourseRunFactory(key=self.course_run_key), - ]), - ] - ), - ProgramFactory( - courses=[ - CourseFactory(course_runs=[ - CourseRunFactory(key=self.alternate_course_run_key), - ]), - ] - ), - ] - elif hierarchy_type == self.SEPARATE_COURSES: - return [ - ProgramFactory( - courses=[ - CourseFactory(course_runs=[ - CourseRunFactory(key=self.course_run_key), - ]), - CourseFactory(course_runs=[ - CourseRunFactory(key=self.alternate_course_run_key), - ]), - ] - ), - ] - else: # SAME_COURSE - return [ - ProgramFactory( - courses=[ - CourseFactory(course_runs=[ - CourseRunFactory(key=self.course_run_key), - CourseRunFactory(key=self.alternate_course_run_key), - ]), - ] - ), - ] - - @ddt.data(True, False) - def test_handle(self, commit, mock_task, mock_get_programs): - """ - Verify that relevant tasks are only enqueued when the commit option is passed. - """ - data = [ - ProgramFactory( - courses=[ - CourseFactory(course_runs=[ - CourseRunFactory(key=self.course_run_key), - ]), - ] - ), - ] - mock_get_programs.return_value = data - - GeneratedCertificateFactory( - user=self.alice, - course_id=self.course_run_key, - mode=MODES.verified, - status=CertificateStatuses.downloadable, - ) - - GeneratedCertificateFactory( - user=self.bob, - course_id=self.alternate_course_run_key, - mode=MODES.verified, - status=CertificateStatuses.downloadable, - ) - - call_command('backpopulate_program_credentials', commit=commit) - - if commit: - mock_task.assert_called_once_with(self.alice.username) - else: - mock_task.assert_not_called() - - def test_handle_professional(self, mock_task, mock_get_programs): - """ Verify the task can handle both professional and no-id-professional modes. """ - mock_get_programs.return_value = [ - ProgramFactory( - courses=[ - CourseFactory(course_runs=[ - CourseRunFactory(key=self.course_run_key, type='professional'), - ]), - ] - ), - ] - - GeneratedCertificateFactory( - user=self.alice, - course_id=self.course_run_key, - mode=CourseMode.PROFESSIONAL, - status=CertificateStatuses.downloadable, - ) - - GeneratedCertificateFactory( - user=self.bob, - course_id=self.course_run_key, - mode=CourseMode.NO_ID_PROFESSIONAL_MODE, - status=CertificateStatuses.downloadable, - ) - - call_command('backpopulate_program_credentials', commit=True) - - # The task should be called for both users since professional and no-id-professional are equivalent. - mock_task.assert_has_calls([mock.call(self.alice.username), mock.call(self.bob.username)], any_order=True) - - @ddt.data(SEPARATE_PROGRAMS, SEPARATE_COURSES, SAME_COURSE) - def test_handle_flatten(self, hierarchy_type, mock_task, mock_get_programs): - """Verify that program structures are flattened correctly.""" - mock_get_programs.return_value = self._get_programs_data(hierarchy_type) - - GeneratedCertificateFactory( - user=self.alice, - course_id=self.course_run_key, - mode=MODES.verified, - status=CertificateStatuses.downloadable, - ) - - GeneratedCertificateFactory( - user=self.bob, - course_id=self.alternate_course_run_key, - mode=MODES.verified, - status=CertificateStatuses.downloadable, - ) - - call_command('backpopulate_program_credentials', commit=True) - - calls = [ - mock.call(self.alice.username), - mock.call(self.bob.username) - ] - mock_task.assert_has_calls(calls, any_order=True) - - def test_handle_username_dedup(self, mock_task, mock_get_programs): - """ - Verify that only one task is enqueued for a user with multiple eligible - course run certificates. - """ - data = [ - ProgramFactory( - courses=[ - CourseFactory(course_runs=[ - CourseRunFactory(key=self.course_run_key), - CourseRunFactory(key=self.alternate_course_run_key), - ]), - ] - ), - ] - mock_get_programs.return_value = data - - GeneratedCertificateFactory( - user=self.alice, - course_id=self.course_run_key, - mode=MODES.verified, - status=CertificateStatuses.downloadable, - ) - - GeneratedCertificateFactory( - user=self.alice, - course_id=self.alternate_course_run_key, - mode=MODES.verified, - status=CertificateStatuses.downloadable, - ) - - call_command('backpopulate_program_credentials', commit=True) - - mock_task.assert_called_once_with(self.alice.username) - - def test_handle_mode_slugs(self, mock_task, mock_get_programs): - """ - Verify that course run types are taken into account when identifying - qualifying course run certificates. - """ - data = [ - ProgramFactory( - courses=[ - CourseFactory(course_runs=[ - CourseRunFactory(key=self.course_run_key, type='honor'), - ]), - ] - ), - ] - mock_get_programs.return_value = data - - GeneratedCertificateFactory( - user=self.alice, - course_id=self.course_run_key, - mode=MODES.honor, - status=CertificateStatuses.downloadable, - ) - - GeneratedCertificateFactory( - user=self.bob, - course_id=self.course_run_key, - mode=MODES.verified, - status=CertificateStatuses.downloadable, - ) - - call_command('backpopulate_program_credentials', commit=True) - - mock_task.assert_called_once_with(self.alice.username) - - def test_handle_passing_status(self, mock_task, mock_get_programs): - """ - Verify that only course run certificates with a passing status are selected. - """ - data = [ - ProgramFactory( - courses=[ - CourseFactory(course_runs=[ - CourseRunFactory(key=self.course_run_key), - ]), - ] - ), - ] - mock_get_programs.return_value = data - - passing_status = CertificateStatuses.downloadable - failing_status = CertificateStatuses.notpassing - - assert passing_status in CertificateStatuses.PASSED_STATUSES - assert failing_status not in CertificateStatuses.PASSED_STATUSES - - GeneratedCertificateFactory( - user=self.alice, - course_id=self.course_run_key, - mode=MODES.verified, - status=passing_status, - ) - - GeneratedCertificateFactory( - user=self.bob, - course_id=self.course_run_key, - mode=MODES.verified, - status=failing_status, - ) - - call_command('backpopulate_program_credentials', commit=True) - - mock_task.assert_called_once_with(self.alice.username) - - def test_handle_no_course_overview(self, mock_task, mock_get_programs): - """ - Verify that the task is not enqueued for a user whose only certificate - is for a course with no CourseOverview. - """ - data = [ - ProgramFactory( - courses=[ - CourseFactory(course_runs=[ - CourseRunFactory(key=self.course_run_key), - CourseRunFactory(key=self.course_run_key_no_course_overview), - ]), - ] - ), - ] - mock_get_programs.return_value = data - - GeneratedCertificateFactory( - user=self.alice, - course_id=self.course_run_key, - mode=MODES.verified, - status=CertificateStatuses.downloadable, - ) - GeneratedCertificateFactory( - user=self.bob, - course_id=self.course_run_key_no_course_overview, - mode=MODES.verified, - status=CertificateStatuses.downloadable, - ) - - call_command('backpopulate_program_credentials', commit=True) - - mock_task.assert_called_once_with(self.alice.username) - - @mock.patch(COMMAND_MODULE + '.logger.exception') - def test_handle_enqueue_failure(self, mock_log, mock_task, mock_get_programs): - """Verify that failure to enqueue a task doesn't halt execution.""" - - def side_effect(username): - """Simulate failure to enqueue a task.""" - if username == self.alice.username: - raise Exception - - mock_task.side_effect = side_effect - - data = [ - ProgramFactory( - courses=[ - CourseFactory(course_runs=[ - CourseRunFactory(key=self.course_run_key), - ]), - ] - ), - ] - mock_get_programs.return_value = data - - GeneratedCertificateFactory( - user=self.alice, - course_id=self.course_run_key, - mode=MODES.verified, - status=CertificateStatuses.downloadable, - ) - - GeneratedCertificateFactory( - user=self.bob, - course_id=self.course_run_key, - mode=MODES.verified, - status=CertificateStatuses.downloadable, - ) - - call_command('backpopulate_program_credentials', commit=True) - - assert mock_log.called - - calls = [ - mock.call(self.alice.username), - mock.call(self.bob.username) - ] - mock_task.assert_has_calls(calls, any_order=True)