Skip to content

Commit

Permalink
fix: remove broken management command (and celery task) logic (#34972)
Browse files Browse the repository at this point in the history
[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.
  • Loading branch information
justinhynes committed Jun 12, 2024
1 parent 66fa388 commit 1e653d7
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 54 deletions.
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -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()
54 changes: 11 additions & 43 deletions openedx/core/djangoapps/credentials/tasks/v1/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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)
101 changes: 98 additions & 3 deletions openedx/core/djangoapps/credentials/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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'


Expand Down Expand Up @@ -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

0 comments on commit 1e653d7

Please sign in to comment.