From 1e653d74e75d1fb391fda93a7df1a896b35dc2a7 Mon Sep 17 00:00:00 2001 From: Justin Hynes Date: Wed, 12 Jun 2024 09:44:11 -0400 Subject: [PATCH] fix: remove broken management command (and celery task) logic (#34972) [APER-3385] This PR fixes an existing management command that now has incorrect logic. We have recently done a lot of work to improve certificate-related date business logic to fix data inconsistencies between systems. Instead of maintaining separate and duplicated logic for sending date data to Credentials, instead we can use an existing (and tested) Celery task that will determine and send the correct date to the Credentials IDA. Additionally, the original version of this management command skipped self-paced courses completely. This is no longer the case as we _know_ that there are self-paced courses that have been associated with a certificate available date because of bugs in the product. This management command will serve as a means to do a mass data fixup for data stored by the Credentials IDA. --- .../update_credentials_available_date.py | 15 ++- .../djangoapps/credentials/tasks/v1/tasks.py | 54 ++-------- .../credentials/tests/test_tasks.py | 101 +++++++++++++++++- 3 files changed, 116 insertions(+), 54 deletions(-) diff --git a/openedx/core/djangoapps/credentials/management/commands/update_credentials_available_date.py b/openedx/core/djangoapps/credentials/management/commands/update_credentials_available_date.py index d39bb33cb02b..81d94ec4a37b 100644 --- a/openedx/core/djangoapps/credentials/management/commands/update_credentials_available_date.py +++ b/openedx/core/djangoapps/credentials/management/commands/update_credentials_available_date.py @@ -1,10 +1,6 @@ """ -A manangement command to populate the `certificate_available_date` field of the CourseCertificateConfiguration model in -the Credentials IDA. - -This command is designed to be ran once to initially backpopulate data. Otherwise, anytime an existing course run -adjusts its certificate available date or certificates display behavior settings, updates will automatically be queued -and transmit to the Credentials IDA. +A manangement command to populate or correct the `certificate_available_date` data of the +CourseCertificateConfiguration model instances stored by the Credentials IDA. """ from django.core.management.base import BaseCommand @@ -13,8 +9,11 @@ class Command(BaseCommand): """ - A management command reponsible for populating the `certificate_available_date` field of - CourseCertificateConfiguration instances in the Credentials IDA. + Enqueue the `backfill_date_for_all_course_runs` Celery task, which will enqueue additional subtasks responsible for + sending certificate availability updates to the Credentials IDA. + + Example usage: + $ ./manage.py lms update_credentials_available_date """ def handle(self, *args, **options): backfill_date_for_all_course_runs.delay() diff --git a/openedx/core/djangoapps/credentials/tasks/v1/tasks.py b/openedx/core/djangoapps/credentials/tasks/v1/tasks.py index 547c3e752b9b..8c010f1c5b26 100644 --- a/openedx/core/djangoapps/credentials/tasks/v1/tasks.py +++ b/openedx/core/djangoapps/credentials/tasks/v1/tasks.py @@ -30,6 +30,7 @@ handle_course_cert_changed, handle_course_cert_revoked, ) +from openedx.core.djangoapps.programs.tasks import update_certificate_available_date_on_course_update from openedx.core.djangoapps.site_configuration.models import SiteConfiguration User = get_user_model() @@ -429,51 +430,18 @@ def is_course_run_in_a_program(course_run_key): @set_code_owner_attribute def backfill_date_for_all_course_runs(): """ - This task will update the course certificate configuration's certificate_available_date in credentials for all - course runs. This is different from the "visable_date" attribute. This date will either be the available date that - is set in studio for a given course, or it will be None. This will exclude any course runs that do not have a - certificate_available_date or are self paced. + This task enqueues an `update_certificate_available_date_on_course_update` subtask for each course overview in the + system in order to determine and update the certificate date stored by the Credentials IDA. """ - course_run_list = CourseOverview.objects.exclude(self_paced=True).exclude(certificate_available_date=None) - for index, course_run in enumerate(course_run_list): + course_overviews = CourseOverview.objects.all() + for index, course_overview in enumerate(course_overviews): logger.info( - f"updating certificate_available_date for course {course_run.id} " - f"with date {course_run.certificate_available_date}" + "Enqueueing an `update_certificate_available_date_on_course_update` task for course run " + f"`{course_overview.id}`, self_paced={course_overview.self_paced}, end={course_overview.end}, " + f"available_date={course_overview.certificate_available_date}, and " + f"display_behavior={course_overview.certificates_display_behavior}" ) - course_key = str(course_run.id) - course_modes = CourseMode.objects.filter(course_id=course_key) - # There should only ever be one certificate relevant mode per course run - modes = [ - mode.slug for mode in course_modes - if mode.slug in CourseMode.CERTIFICATE_RELEVANT_MODES or CourseMode.is_eligible_for_certificate(mode.slug) - ] - if len(modes) != 1: - logger.exception( - f'Either course {course_key} has no certificate mode or multiple modes. Task failed.' - ) - # if there is only one relevant mode, post to credentials - else: - try: - credentials_client = get_credentials_api_client( - User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME), - ) - api_url = urljoin(f"{get_credentials_api_base_url()}/", "course_certificates/") - response = credentials_client.post( - api_url, - json={ - "course_id": course_key, - "certificate_type": modes[0], - "certificate_available_date": course_run.certificate_available_date.strftime( - '%Y-%m-%dT%H:%M:%SZ' - ), - "is_active": True, - } - ) - response.raise_for_status() - - logger.info(f"certificate_available_date updated for course {course_key}") - except Exception: # lint-amnesty, pylint: disable=W0703 - error_msg = f"Failed to send certificate_available_date for course {course_key}." - logger.exception(error_msg) + update_certificate_available_date_on_course_update.delay(str(course_overview.id)) + if index % 10 == 0: time.sleep(3) diff --git a/openedx/core/djangoapps/credentials/tests/test_tasks.py b/openedx/core/djangoapps/credentials/tests/test_tasks.py index 1ad372e4f8ad..1b09e38ce148 100644 --- a/openedx/core/djangoapps/credentials/tests/test_tasks.py +++ b/openedx/core/djangoapps/credentials/tests/test_tasks.py @@ -2,8 +2,9 @@ Test credentials tasks """ +import logging from unittest import mock -from datetime import datetime, timezone +from datetime import datetime, timezone, timedelta import ddt import pytest @@ -13,6 +14,7 @@ from django.test import TestCase, override_settings from freezegun import freeze_time from opaque_keys.edx.keys import CourseKey +from testfixtures import LogCapture from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.certificates.api import get_recently_modified_certificates @@ -23,14 +25,16 @@ from lms.djangoapps.grades.tests.utils import mock_passing_grade from lms.djangoapps.certificates.tests.factories import CertificateDateOverrideFactory, GeneratedCertificateFactory from openedx.core.djangoapps.catalog.tests.factories import CourseFactory, CourseRunFactory, ProgramFactory +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.credentials.helpers import is_learner_records_enabled +from openedx.core.djangoapps.credentials.tasks.v1 import tasks from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory from openedx.core.djangolib.testing.utils import skip_unless_lms - -from openedx.core.djangoapps.credentials.tasks.v1 import tasks +from xmodule.data import CertificatesDisplayBehaviors User = get_user_model() +LOGGER_NAME = "openedx.core.djangoapps.credentials.tasks.v1.tasks" TASKS_MODULE = 'openedx.core.djangoapps.credentials.tasks.v1.tasks' @@ -757,3 +761,94 @@ def test_send_notifications_revoke_programs( else: mock_revoked.assert_not_called() mock_awarded.assert_called_once_with(**expected_signal_args) + + +@skip_unless_lms +class TestBackfillDateForAllCourseRuns(TestCase): + """ + Unit Tests for the `backfill_date_for_all_course_runs` Celery task. + """ + def setUp(self): + super().setUp() + self.co_instructor_paced_cdb_early_no_info_key = "course-v1:OpenEdX+InstructorPacedEarly+Run1" + self.co_instructor_paced_cbd_end_key = "course-v1:OpenEdX+InstructorPacedEnd+Run1" + self.co_instructor_paced_cdb_end_with_date_key = "course-v1:OpenEdX+InstructorPacedCAD+Run1" + self.co_self_paced_key = "course-v1:OpenEdX+SelfPaced+Run1" + self.course_run_keys = [ + self.co_instructor_paced_cdb_early_no_info_key, + self.co_instructor_paced_cbd_end_key, + self.co_instructor_paced_cdb_end_with_date_key, + self.co_self_paced_key, + ] + self.co_instructor_paced_cdb_early_no_info = CourseOverviewFactory( + certificate_available_date=None, + certificates_display_behavior=CertificatesDisplayBehaviors.EARLY_NO_INFO, + id=CourseKey.from_string(self.co_instructor_paced_cdb_early_no_info_key), + self_paced=False, + ) + self.co_instructor_paced_cbd_end = CourseOverviewFactory( + certificate_available_date=None, + certificates_display_behavior=CertificatesDisplayBehaviors.END, + id=CourseKey.from_string(self.co_instructor_paced_cbd_end_key), + self_paced=False, + ) + self.co_instructor_paced_cdb_end_with_date = CourseOverviewFactory( + certificate_available_date=(datetime.now(timezone.utc) + timedelta(days=30)), + certificates_display_behavior=CertificatesDisplayBehaviors.END_WITH_DATE, + id=CourseKey.from_string(self.co_instructor_paced_cdb_end_with_date_key), + self_paced=False, + ) + self.co_self_paced = CourseOverviewFactory( + id=CourseKey.from_string(self.co_self_paced_key), + self_paced=True, + ) + + @mock.patch( + 'openedx.core.djangoapps.credentials.tasks.v1.tasks.update_certificate_available_date_on_course_update.delay' + ) + def test_backfill_dates(self, mock_update): + """ + Tests that the `backfill_date_for_all_course_runs` task enqueues the expected number of + `update_certificate_available_date_on_course_update` subtasks. We also capture and verify the contents of the + logs to ensure debugging capabilities by humans. + """ + expected_messages = [ + # instructor-paced, cdb=early_no_info msg + "Enqueueing an `update_certificate_available_date_on_course_update` task for course run " + f"`{self.co_instructor_paced_cdb_early_no_info.id}`, " + f"self_paced={self.co_instructor_paced_cdb_early_no_info.self_paced}, " + f"end={self.co_instructor_paced_cdb_early_no_info.end}, " + f"available_date={self.co_instructor_paced_cdb_early_no_info.certificate_available_date}, and " + f"display_behavior={self.co_instructor_paced_cdb_early_no_info.certificates_display_behavior.value}", + # instructor-paced, cdb=end msg + "Enqueueing an `update_certificate_available_date_on_course_update` task for course run " + f"`{self.co_instructor_paced_cbd_end.id}`, " + f"self_paced={self.co_instructor_paced_cbd_end.self_paced}, " + f"end={self.co_instructor_paced_cbd_end.end}, " + f"available_date={self.co_instructor_paced_cbd_end.certificate_available_date}, and " + f"display_behavior={self.co_instructor_paced_cbd_end.certificates_display_behavior.value}", + # instructor-paced, cdb=end_with_date msg + "Enqueueing an `update_certificate_available_date_on_course_update` task for course run " + f"`{self.co_instructor_paced_cdb_end_with_date.id}`, " + f"self_paced={self.co_instructor_paced_cdb_end_with_date.self_paced}, " + f"end={self.co_instructor_paced_cdb_end_with_date.end}, " + f"available_date={self.co_instructor_paced_cdb_end_with_date.certificate_available_date}, and " + f"display_behavior={self.co_instructor_paced_cdb_end_with_date.certificates_display_behavior.value}", + # self-paced course run msg + "Enqueueing an `update_certificate_available_date_on_course_update` task for course run " + f"`{self.co_self_paced.id}`, self_paced={self.co_self_paced.self_paced}, end={self.co_self_paced.end}, " + f"available_date={self.co_self_paced.certificate_available_date}, and " + f"display_behavior={self.co_self_paced.certificates_display_behavior}", + ] + + with LogCapture(LOGGER_NAME, level=logging.INFO) as log: + tasks.backfill_date_for_all_course_runs.delay() + + # verify the content of captured log messages + for message in expected_messages: + log.check_present((LOGGER_NAME, 'INFO', message)) + + # verify that our mocked function has the expected calls + assert mock_update.call_count == len(expected_messages) + for mock_call in mock_update.call_args_list: + assert mock_call.args[0] in self.course_run_keys