diff --git a/openedx/core/djangoapps/content/course_overviews/signals.py b/openedx/core/djangoapps/content/course_overviews/signals.py index e4076c82ad..4ae2f4bf0f 100644 --- a/openedx/core/djangoapps/content/course_overviews/signals.py +++ b/openedx/core/djangoapps/content/course_overviews/signals.py @@ -11,7 +11,8 @@ from django.dispatch import Signal from django.dispatch.dispatcher import receiver from openedx.core.djangoapps.signals.signals import COURSE_CERT_DATE_CHANGE -from xmodule.modulestore.django import SignalHandler # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.data import CertificatesDisplayBehaviors +from xmodule.modulestore.django import SignalHandler from .models import CourseOverview @@ -30,8 +31,8 @@ DELETE_COURSE_DETAILS = Signal() @receiver(SignalHandler.course_published) def _listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable=unused-argument """ - Catches the signal that a course has been published in Studio and - updates the corresponding CourseOverview cache entry. + Catches the signal that a course has been published in Studio and updates the corresponding CourseOverview cache + entry. """ try: previous_course_overview = CourseOverview.objects.get(id=course_key) @@ -72,13 +73,37 @@ def trigger_import_course_details_signal(sender, instance, created, **kwargs): def _check_for_course_changes(previous_course_overview, updated_course_overview): + """ + Utility function responsible for calling other utility functions that check for specific changes in a course + overview after a course run has been updated and published. + + Args: + previous_course_overview (CourseOverview): the current course overview instance for a particular course run + updated_course_overview (CourseOverview): an updated course overview instance, reflecting the current state of + data from the modulestore/Mongo + + Returns: + None + """ if previous_course_overview: - _check_for_course_date_changes(previous_course_overview, updated_course_overview) + _check_for_course_start_date_changes(previous_course_overview, updated_course_overview) _check_for_pacing_changes(previous_course_overview, updated_course_overview) - _check_for_cert_availability_date_changes(previous_course_overview, updated_course_overview) + _check_for_cert_date_changes(previous_course_overview, updated_course_overview) -def _check_for_course_date_changes(previous_course_overview, updated_course_overview): +def _check_for_course_start_date_changes(previous_course_overview, updated_course_overview): + """ + Checks if a course run's start date has been updated. If so, we emit the `COURSE_START_DATE_CHANGED` signal to + ensure other parts of the system are aware of the change. + + Args: + previous_course_overview (CourseOverview): the current course overview instance for a particular course run + updated_course_overview (CourseOverview): an updated course overview instance, reflecting the current state of + data from the modulestore/Mongo + + Returns: + None + """ if previous_course_overview.start != updated_course_overview.start: _log_start_date_change(previous_course_overview, updated_course_overview) COURSE_START_DATE_CHANGED.send( @@ -88,21 +113,46 @@ def _check_for_course_date_changes(previous_course_overview, updated_course_over ) -def _log_start_date_change(previous_course_overview, updated_course_overview): # lint-amnesty, pylint: disable=missing-function-docstring +def _log_start_date_change(previous_course_overview, updated_course_overview): + """ + Utility function to log a course run's start date when updating a course overview. This log only appears when the + start date has been changed (see the `_check_for_course_date_changes` function above). + + Args: + previous_course_overview (CourseOverview): the current course overview instance for a particular course run + updated_course_overview (CourseOverview): an updated course overview instance, reflecting the current state of + data from the modulestore/Mongo + + Returns: + None + """ previous_start_str = 'None' if previous_course_overview.start is not None: previous_start_str = previous_course_overview.start.isoformat() new_start_str = 'None' if updated_course_overview.start is not None: new_start_str = updated_course_overview.start.isoformat() - LOG.info('Course start date changed: course={} previous={} new={}'.format( - updated_course_overview.id, - previous_start_str, - new_start_str, - )) + LOG.info( + f"Course start date changed: course={updated_course_overview.id} previous={previous_start_str} " + f"new={new_start_str}" + ) def _check_for_pacing_changes(previous_course_overview, updated_course_overview): + """ + Checks if a course run's pacing has been updated. If so, we emit the `COURSE_PACING_CHANGED` signal to ensure other + parts of the system are aware of the change. The `programs` and `certificates` apps listen for this signal in + order to manage certificate generation features in the LMS and certificate visibility settings in the Credentials + IDA. + + Args: + previous_course_overview (CourseOverview): the current course overview instance for a particular course run + updated_course_overview (CourseOverview): an updated course overview instance, reflecting the current state of + data from the modulestore/Mongo + + Returns: + None + """ if previous_course_overview.self_paced != updated_course_overview.self_paced: COURSE_PACING_CHANGED.send( sender=None, @@ -111,19 +161,58 @@ def _check_for_pacing_changes(previous_course_overview, updated_course_overview) ) -def _check_for_cert_availability_date_changes(previous_course_overview, updated_course_overview): - """ Checks if the cert available date has changed and if so, sends a COURSE_CERT_DATE_CHANGE signal""" - if previous_course_overview.certificate_available_date != updated_course_overview.certificate_available_date: +def _check_for_cert_date_changes(previous_course_overview, updated_course_overview): + """ + Checks if the certificate available date (CAD) or the certificates display behavior (CDB) of a course run has + changed during a course overview update. If so, we emit the COURSE_CERT_DATE_CHANGE signal to ensure other parts of + the system are aware of the change. The `credentials` app listens for this signal in order to keep our certificate + visibility settings in the Credentials IDA up to date. + + Args: + previous_course_overview (CourseOverview): the current course overview instance for a particular course run + updated_course_overview (CourseOverview): an updated course overview instance, reflecting the current state of + data from the modulestore/Mongo + + Returns: + None + """ + def _send_course_cert_date_change_signal(): + """ + A callback used to fire the COURSE_CERT_DATE_CHANGE Django signal *after* the ORM has successfully commited the + update. + """ + COURSE_CERT_DATE_CHANGE.send_robust(sender=None, course_key=str(updated_course_overview.id)) + + course_run_id = str(updated_course_overview.id) + prev_available_date = previous_course_overview.certificate_available_date + prev_display_behavior = previous_course_overview.certificates_display_behavior + prev_end_date = previous_course_overview.end # `end_date` is a deprecated field, use `end` instead + updated_available_date = updated_course_overview.certificate_available_date + updated_display_behavior = updated_course_overview.certificates_display_behavior + updated_end_date = updated_course_overview.end # `end_date` is a deprecated field, use `end` instead + send_signal = False + + if prev_available_date != updated_available_date: LOG.info( - f"Certificate availability date for {str(updated_course_overview.id)} has changed from " + - f"{previous_course_overview.certificate_available_date} to " + - f"{updated_course_overview.certificate_available_date}. Sending COURSE_CERT_DATE_CHANGE signal." + f"The certificate available date for {course_run_id} has changed from {prev_available_date} to " + f"{updated_available_date}" ) + send_signal = True - def _send_course_cert_date_change_signal(): - COURSE_CERT_DATE_CHANGE.send_robust( - sender=None, - course_key=updated_course_overview.id, - ) + if prev_display_behavior != updated_display_behavior: + LOG.info( + f"The certificates display behavior for {course_run_id} has changed from {prev_display_behavior} to " + f"{updated_display_behavior}" + ) + send_signal = True + # edge case -- if a course run with a cert display behavior of "End date of course" has changed its end date, we + # should fire our signal to ensure visibility of certificates managed by the Credentials IDA are corrected too + if (updated_display_behavior == CertificatesDisplayBehaviors.END and prev_end_date != updated_end_date): + LOG.info( + f"The end date for {course_run_id} has changed from {prev_end_date} to {updated_end_date}." + ) + send_signal = True + + if send_signal: transaction.on_commit(_send_course_cert_date_change_signal) diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py b/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py index 8c4fb25c8a..49adf55400 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py @@ -1,4 +1,7 @@ -# lint-amnesty, pylint: disable=missing-module-docstring +""" +Tests for the course_overviews app's signal functionality. +""" + import datetime from unittest.mock import patch @@ -29,13 +32,32 @@ class CourseOverviewSignalsTestCase(ModuleStoreTestCase): TODAY = datetime.datetime.utcnow().replace(tzinfo=UTC) NEXT_WEEK = TODAY + datetime.timedelta(days=7) + def assert_changed_signal_sent(self, changes, mock_signal): + """ + Utility function used to verify that an emulated change to a course overview results in the expected signals + being fired by the system. + """ + course = CourseFactory.create( + emit_signals=True, + **{change.field_name: change.initial_value for change in changes} + ) + + # changing display name doesn't fire the signal + with self.captureOnCommitCallbacks(execute=True) as callbacks: + course.display_name = course.display_name + 'changed' + course = self.store.update_item(course, ModuleStoreEnum.UserID.test) + assert not mock_signal.called + + # changing the given field fires the signal + with self.captureOnCommitCallbacks(execute=True) as callbacks: + for change in changes: + setattr(course, change.field_name, change.changed_value) + self.store.update_item(course, ModuleStoreEnum.UserID.test) + assert mock_signal.called + def test_caching(self): """ Tests that CourseOverview structures are actually getting cached. - - Arguments: - modulestore_type (ModuleStoreEnum.Type): type of store to create the - course in. """ # Creating a new course will trigger a publish event and the course will be cached course = CourseFactory.create(emit_signals=True) @@ -46,12 +68,7 @@ class CourseOverviewSignalsTestCase(ModuleStoreTestCase): def test_cache_invalidation(self): """ - Tests that when a course is published or deleted, the corresponding - course_overview is removed from the cache. - - Arguments: - modulestore_type (ModuleStoreEnum.Type): type of store to create the - course in. + Tests that when a course is published or deleted, the corresponding course_overview is removed from the cache. """ # Create a course where mobile_available is True. course = CourseFactory.create(mobile_available=True) @@ -74,31 +91,18 @@ class CourseOverviewSignalsTestCase(ModuleStoreTestCase): self.store.delete_course(course.id, ModuleStoreEnum.UserID.test) CourseOverview.get_from_id(course.id) - def assert_changed_signal_sent(self, changes, mock_signal): # lint-amnesty, pylint: disable=missing-function-docstring - course = CourseFactory.create( - emit_signals=True, - **{change.field_name: change.initial_value for change in changes} - ) - - # changing display name doesn't fire the signal - with self.captureOnCommitCallbacks(execute=True) as callbacks: - course.display_name = course.display_name + 'changed' - course = self.store.update_item(course, ModuleStoreEnum.UserID.test) - assert not mock_signal.called - - # changing the given field fires the signal - with self.captureOnCommitCallbacks(execute=True) as callbacks: - for change in changes: - setattr(course, change.field_name, change.changed_value) - self.store.update_item(course, ModuleStoreEnum.UserID.test) - assert mock_signal.called - @patch('openedx.core.djangoapps.content.course_overviews.signals.COURSE_START_DATE_CHANGED.send') def test_start_changed(self, mock_signal): + """ + A test that ensures the `COURSE_STATE_DATE_CHANGED` signal is emit when the start date of course run is updated. + """ self.assert_changed_signal_sent([Change('start', self.TODAY, self.NEXT_WEEK)], mock_signal) @patch('openedx.core.djangoapps.content.course_overviews.signals.COURSE_PACING_CHANGED.send') def test_pacing_changed(self, mock_signal): + """ + A test that ensures the `COURSE_PACING_CHANGED` signal is emit when the pacing type of a course run is updated. + """ self.assert_changed_signal_sent([Change('self_paced', True, False)], mock_signal) @patch('openedx.core.djangoapps.content.course_overviews.signals.COURSE_CERT_DATE_CHANGE.send_robust') @@ -112,3 +116,21 @@ class CourseOverviewSignalsTestCase(ModuleStoreTestCase): ) ] self.assert_changed_signal_sent(changes, mock_signal) + + @patch('openedx.core.djangoapps.content.course_overviews.signals.COURSE_CERT_DATE_CHANGE.send_robust') + def test_cert_end_date_changed(self, mock_signal): + """ + This test ensures when an instructor-paced course with a certificates display behavior of "END" updates its end + date that we emit the `COURSE_CERT_DATE_CHANGE` signal. + """ + course = CourseFactory.create( + emit_signals=True, + end=self.TODAY, + certificates_display_behavior=CertificatesDisplayBehaviors.END + ) + + with self.captureOnCommitCallbacks(execute=True): + course.end = self.NEXT_WEEK + self.store.update_item(course, ModuleStoreEnum.UserID.test) + + assert mock_signal.called diff --git a/openedx/core/djangoapps/credentials/api.py b/openedx/core/djangoapps/credentials/api.py new file mode 100644 index 0000000000..eb386ab133 --- /dev/null +++ b/openedx/core/djangoapps/credentials/api.py @@ -0,0 +1,15 @@ +""" +Python APIs exposed by the credentials app to other in-process apps. +""" + + +from openedx.core.djangoapps.credentials.models import CredentialsApiConfig + + +def is_credentials_enabled(): + """ + A utility function wrapping the `is_learner_issurance_enabled` utility function of the CredentialsApiConfig model. + Intended to be an easier to read/grok utility function that informs the caller if use of the Credentials IDA is + enabled for this Open edX instance. + """ + return CredentialsApiConfig.current().is_learner_issuance_enabled diff --git a/openedx/core/djangoapps/credentials/tests/test_api.py b/openedx/core/djangoapps/credentials/tests/test_api.py new file mode 100644 index 0000000000..30e640ff29 --- /dev/null +++ b/openedx/core/djangoapps/credentials/tests/test_api.py @@ -0,0 +1,46 @@ +""" +Tests for the utility functions defined as part of the credentials app's public Python API. +""" + + +from django.test import TestCase + +from openedx.core.djangoapps.credentials.api import is_credentials_enabled +from openedx.core.djangoapps.credentials.models import CredentialsApiConfig +from openedx.core.djangoapps.credentials.tests.mixins import CredentialsApiConfigMixin +from openedx.core.djangolib.testing.utils import skip_unless_lms + + +class CredentialsApiTests(CredentialsApiConfigMixin, TestCase): + """ + Tests for the Public Pyton API exposed by the credentials Django app. + """ + def setUp(self): + super().setUp() + CredentialsApiConfig.objects.all().delete() + + @skip_unless_lms + def test_is_credentials_enabled_config_enabled(self): + """ + A test that verifies the output of the `is_credentials_enabled` utility function when a CredentialsApiConfig + exists and is enabled. + """ + self.create_credentials_config(enabled=True) + assert is_credentials_enabled() + + @skip_unless_lms + def test_is_credentials_enabled_config_disabled(self): + """ + A test that verifies the output of the `is_credentials_enabled` utility function when a CredentialsApiConfig + exists and is disabled. + """ + self.create_credentials_config(enabled=False) + assert not is_credentials_enabled() + + @skip_unless_lms + def test_is_credentials_enabled_config_absent(self): + """ + A test that verifies the output of the `is_credentials_enabled` utility function when a CredentialsApiConfig + does not exist. + """ + assert not is_credentials_enabled() diff --git a/openedx/core/djangoapps/programs/signals.py b/openedx/core/djangoapps/programs/signals.py index 60d12764fd..89292f565b 100644 --- a/openedx/core/djangoapps/programs/signals.py +++ b/openedx/core/djangoapps/programs/signals.py @@ -7,6 +7,8 @@ import logging from django.dispatch import receiver +from openedx.core.djangoapps.content.course_overviews.signals import COURSE_PACING_CHANGED +from openedx.core.djangoapps.credentials.api import is_credentials_enabled from openedx.core.djangoapps.credentials.helpers import is_learner_records_enabled_for_org from openedx.core.djangoapps.signals.signals import ( COURSE_CERT_AWARDED, @@ -21,41 +23,24 @@ LOGGER = logging.getLogger(__name__) @receiver(COURSE_CERT_AWARDED) def handle_course_cert_awarded(sender, user, course_key, mode, status, **kwargs): # pylint: disable=unused-argument """ - If programs is enabled and a learner is awarded a course certificate, - schedule a celery task to process any programs certificates for which - the learner may now be eligible. + If use of the Credentials IDA is enabled and a learner is awarded a course certificate, schedule a celery task to + determine if the learner is also eligible to be awarded any program certificates. Args: - sender: - class of the object instance that sent this signal - user: - django.contrib.auth.User - the user to whom a cert was awarded - course_key: - refers to the course run for which the cert was awarded - mode: - mode / certificate type, e.g. "verified" - status: - either "downloadable" or "generating" + sender: class of the object instance that sent this signal + user(User): The user to whom a course certificate was awarded + course_key(CourseLocator): The course run key for which the course certificate was awarded + mode(str): The "mode" of the course (e.g. Audit, Honor, Verified, etc.) + status(str): The status of the course certificate that was awarded (e.g. "downloadable") Returns: None - """ - # Import here instead of top of file since this module gets imported before - # the credentials app is loaded, resulting in a Django deprecation warning. - from openedx.core.djangoapps.credentials.models import CredentialsApiConfig - - # Avoid scheduling new tasks if certification is disabled. - if not CredentialsApiConfig.current().is_learner_issuance_enabled: + if not is_credentials_enabled(): return - # schedule background task to process LOGGER.debug( - 'handling COURSE_CERT_AWARDED: username=%s, course_key=%s, mode=%s, status=%s', - user, - course_key, - mode, - status, + f"Handling COURSE_CERT_AWARDED: user={user}, course_key={course_key}, mode={mode}, status={status}" ) # import here, because signal is registered at startup, but items in tasks are not yet able to be loaded from openedx.core.djangoapps.programs.tasks import award_program_certificates @@ -63,75 +48,43 @@ def handle_course_cert_awarded(sender, user, course_key, mode, status, **kwargs) @receiver(COURSE_CERT_CHANGED) -def handle_course_cert_changed(sender, user, course_key, mode, status, **kwargs): +def handle_course_cert_changed(sender, user, course_key, mode, status, **kwargs): # pylint: disable=unused-argument """ - If a learner is awarded a course certificate, - schedule a celery task to process that course certificate + When the system updates a course certificate, enqueue a celery task responsible for syncing this change in the + Credentials IDA - Args: - sender: - class of the object instance that sent this signal - user: - django.contrib.auth.User - the user to whom a cert was awarded - course_key: - refers to the course run for which the cert was awarded - mode: - mode / certificate type, e.g. "verified" - status: - "downloadable" + ***** Important ***** + While the current name of the enqueue'd task is `award_course_certificate` it is *actually* responsible for both + awarding and revocation of course certificates in Credentials. + ********************* - Returns: - None + Args: + sender: class of the object instance that sent this signal + user(User): The user to whom a course certificate was awarded + course_key(CourseLocator): The course run key for which the course certificate was awarded + mode(str): The "mode" of the course (e.g. Audit, Honor, Verified, etc.) + status(str): The status of the course certificate that was awarded (e.g. "downloadable") + + Returns: + None """ - # Import here instead of top of file since this module gets imported before - # the credentials app is loaded, resulting in a Django deprecation warning. - from openedx.core.djangoapps.credentials.models import CredentialsApiConfig - verbose = kwargs.get('verbose', False) if verbose: - msg = "Starting handle_course_cert_changed with params: "\ - "sender [{sender}], "\ - "user [{username}], "\ - "course_key [{course_key}], "\ - "mode [{mode}], "\ - "status [{status}], "\ - "kwargs [{kw}]"\ - .format( - sender=sender, - username=getattr(user, 'username', None), - course_key=str(course_key), - mode=mode, - status=status, - kw=kwargs - ) + LOGGER.info( + f"Starting handle_course_cert_changed with params: sender [{sender}], user [{user}], course_key " + f"[{course_key}], mode [{mode}], status [{status}], kwargs [{kwargs}]" + ) - LOGGER.info(msg) - - # Avoid scheduling new tasks if certification is disabled. - if not CredentialsApiConfig.current().is_learner_issuance_enabled: - if verbose: - LOGGER.info("Skipping send cert: is_learner_issuance_enabled False") + if not is_credentials_enabled(): return # Avoid scheduling new tasks if learner records are disabled for this site (right now, course certs are only # used for learner records -- when that changes, we can remove this bit and always send course certs). if not is_learner_records_enabled_for_org(course_key.org): - if verbose: - LOGGER.info( - "Skipping send cert: ENABLE_LEARNER_RECORDS False for org [{org}]".format( - org=course_key.org - ) - ) + LOGGER.warning(f"Skipping send cert: the Learner Record feature is disabled for org [{course_key.org}]") return - # schedule background task to process - LOGGER.debug( - 'handling COURSE_CERT_CHANGED: username=%s, course_key=%s, mode=%s, status=%s', - user, - course_key, - mode, - status, - ) + LOGGER.debug(f"Handling COURSE_CERT_CHANGED: user={user}, course_key={course_key}, mode={mode}, status={status}") # import here, because signal is registered at startup, but items in tasks are not yet able to be loaded from openedx.core.djangoapps.programs.tasks import award_course_certificate award_course_certificate.delay(user.username, str(course_key)) @@ -140,68 +93,76 @@ def handle_course_cert_changed(sender, user, course_key, mode, status, **kwargs) @receiver(COURSE_CERT_REVOKED) def handle_course_cert_revoked(sender, user, course_key, mode, status, **kwargs): # pylint: disable=unused-argument """ - If programs is enabled and a learner's course certificate is revoked, - schedule a celery task to revoke any related program certificates. + If use of the Credentials IDA is enabled and a learner has a course certificate revoked, schedule a celery task + to determine if there are any program certificates that must be revoked too. Args: - sender: - class of the object instance that sent this signal - user: - django.contrib.auth.User - the user for which a cert was revoked - course_key: - refers to the course run for which the cert was revoked - mode: - mode / certificate type, e.g. "verified" - status: - revoked + sender: class of the object instance that sent this signal + user(User): The user to whom a course certificate was revoked + course_key(CourseLocator): The course run key for which the course certificate was revoked + mode(str): The "mode" of the course (e.g. "audit", "honor", "verified", etc.) + status(str): The status of the course certificate that was revoked (e.g. "revoked") Returns: None - """ - # Import here instead of top of file since this module gets imported before - # the credentials app is loaded, resulting in a Django deprecation warning. - from openedx.core.djangoapps.credentials.models import CredentialsApiConfig - - # Avoid scheduling new tasks if certification is disabled. - if not CredentialsApiConfig.current().is_learner_issuance_enabled: + if not is_credentials_enabled(): return - # schedule background task to process - LOGGER.info( - f"handling COURSE_CERT_REVOKED: user={user.id}, course_key={course_key}, mode={mode}, status={status}" - ) + LOGGER.info(f"Handling COURSE_CERT_REVOKED: user={user}, course_key={course_key}, mode={mode}, status={status}") # import here, because signal is registered at startup, but items in tasks are not yet able to be loaded from openedx.core.djangoapps.programs.tasks import revoke_program_certificates revoke_program_certificates.delay(user.username, str(course_key)) @receiver(COURSE_CERT_DATE_CHANGE, dispatch_uid='course_certificate_date_change_handler') -def handle_course_cert_date_change(sender, course_key, **kwargs): # lint-amnesty, pylint: disable=unused-argument +def handle_course_cert_date_change(sender, course_key, **kwargs): # pylint: disable=unused-argument """ - If a course-run's `certificate_available_date` is updated, schedule a celery task to update the `visible_date` - attribute of all (course) credentials awarded in the Credentials service. + When a course run's configuration has been updated, and the system has detected an update related to the display + behavior or availability date of the certificates issued in that course, we should enqueue celery tasks responsible + for: + - updating the `visible_date` attribute of any previously awarded certificates the Credentials IDA manages + - updating the certificate available date of the course run's course certificate configuration in Credentials Args: - course_key(CourseLocator): refers to the course whose certificate_available_date was updated. - """ - # Import here instead of top of file since this module gets imported before the credentials app is loaded, resulting - # in a Django deprecation warning. - from openedx.core.djangoapps.credentials.models import CredentialsApiConfig + sender: class of the object instance that sent this signal + course_key(CourseLocator): The course run key of the course run which was updated - # Avoid scheduling new tasks if we're not using the Credentials IDA - if not CredentialsApiConfig.current().is_learner_issuance_enabled: - LOGGER.warning( - f"Skipping handling of COURSE_CERT_DATE_CHANGE for course {course_key}. Use of the Credentials service is " - "disabled." - ) + Returns: + None + """ + if not is_credentials_enabled(): return LOGGER.info(f"Handling COURSE_CERT_DATE_CHANGE for course {course_key}") # import here, because signal is registered at startup, but items in tasks are not yet loaded - from openedx.core.djangoapps.programs.tasks import update_certificate_visible_date_on_course_update from openedx.core.djangoapps.programs.tasks import update_certificate_available_date_on_course_update - # update the awarded credentials `visible_date` attribute in the Credentials service after a date update + from openedx.core.djangoapps.programs.tasks import update_certificate_visible_date_on_course_update update_certificate_visible_date_on_course_update.delay(str(course_key)) - # update the (course) certificate configuration in the Credentials service after a date update update_certificate_available_date_on_course_update.delay(str(course_key)) + + +@receiver(COURSE_PACING_CHANGED, dispatch_uid="update_credentials_on_pacing_change") +def handle_course_pacing_change(sender, updated_course_overview, **kwargs): # pylint: disable=unused-argument + """ + If the pacing of a course run has been updated, we should enqueue the tasks responsible for updating the certificate + available date (CAD) stored in the Credentials IDA's internal records. This ensures that we are correctly managing + the visibiltiy of certificates on learners' program records. + + Args: + sender: class of the object instance that sent this signal + updated_course_overview(CourseOverview): The course overview of the course run which was just updated + + Returns: + None + """ + if not is_credentials_enabled(): + return + + course_id = str(updated_course_overview.id) + LOGGER.info(f"Handling COURSE_PACING_CHANGED for course {course_id}") + # import here, because signal is registered at startup, but items in tasks are not yet loaded + from openedx.core.djangoapps.programs.tasks import update_certificate_available_date_on_course_update + from openedx.core.djangoapps.programs.tasks import update_certificate_visible_date_on_course_update + update_certificate_available_date_on_course_update.delay(course_id) + update_certificate_visible_date_on_course_update.delay(course_id) diff --git a/openedx/core/djangoapps/programs/tasks.py b/openedx/core/djangoapps/programs/tasks.py index c8855ba4f4..96237ff26e 100644 --- a/openedx/core/djangoapps/programs/tasks.py +++ b/openedx/core/djangoapps/programs/tasks.py @@ -18,7 +18,9 @@ from requests.exceptions import HTTPError from common.djangoapps.course_modes.models import CourseMode from lms.djangoapps.certificates.api import available_date_for_certificate from lms.djangoapps.certificates.models import GeneratedCertificate +from openedx.core.djangoapps.content.course_overviews.api import get_course_overview_or_none from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.credentials.api import is_credentials_enabled from openedx.core.djangoapps.credentials.models import CredentialsApiConfig from openedx.core.djangoapps.credentials.utils import ( get_credentials, @@ -755,22 +757,21 @@ def update_certificate_visible_date_on_course_update(self, course_key): @set_code_owner_attribute def update_certificate_available_date_on_course_update(self, course_key): """ - This task is designed to be called whenever a course-run's `certificate_available_date` is updated. + This task is designed to be enqueued whenever a course run's Certificate Display Behavior (CDB) or Certificate + Available Date (CAD) has been updated in the CMS. - When executed, this task will determine if we need to enqueue an - `update_credentials_course_certificate_configuration_available_date` task associated with the specified course-run - key from this task. If so, this subtask is responsible for making a REST API call to the Credentials IDA to update - the specified course-run's Course Certificate configuration with the new `certificate_available_date` value. + When executed, this task is responsible for enqueuing an additional subtask responsible for syncing the updated CAD + value in the Credentials IDA's internal records. Args: - course_key(str): The course identifier + course_key(str): The course run's identifier """ countdown = 2**self.request.retries # If the CredentialsApiConfig configuration model is disabled for this feature, it may indicate a condition where # processing of such tasks has been temporarily disabled. Since this is a recoverable situation, mark this task for # retry instead of failing it. - if not CredentialsApiConfig.current().is_learner_issuance_enabled: + if not is_credentials_enabled(): error_msg = ( "Cannot execute the `update_certificate_visible_date_on_course_update` task. Issuing user credentials " "through the Credentials IDA is disabled." @@ -782,39 +783,43 @@ def update_certificate_available_date_on_course_update(self, course_key): ) raise self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) - course_overview = CourseOverview.get_from_id(course_key) - # Update the Credentials service's CourseCertificate configuration with the new `certificate_available_date` if: - # - The course-run is instructor-paced, AND - # - The `certificates_display_behavior` is set to "end_with_date", - if ( - course_overview - and course_overview.self_paced is False - and course_overview.certificates_display_behavior == CertificatesDisplayBehaviors.END_WITH_DATE - ): - LOGGER.info( - f"Queueing task to update the `certificate_available_date` of course-run {course_key} to " - f"[{course_overview.certificate_available_date}] in the Credentials service" - ) - update_credentials_course_certificate_configuration_available_date.delay( - str(course_key), str(course_overview.certificate_available_date) - ) - # OR, - # - The course-run is self-paced, AND - # - The `certificate_available_date` is (now) None. (This task will be executed after an update to the course - # overview) - # There are times when the CourseCertificate configuration of a self-paced course-run in Credentials can become - # associated with a `certificate_available_date`. This ends up causing learners' certificate to be incorrectly - # hidden. This is due to the Credentials IDA not understanding the concept of course pacing. Thus, we need a way - # to remove this value from self-paced courses in Credentials. - elif course_overview and course_overview.self_paced is True and course_overview.certificate_available_date is None: - LOGGER.info( - "Queueing task to remove the `certificate_available_date` in the Credentials service for course-run " - f"{course_key}" - ) - update_credentials_course_certificate_configuration_available_date.delay(str(course_key), None) - # ELSE, we don't meet the criteria to update the course cert config in the Credentials IDA - else: + course_overview = get_course_overview_or_none(course_key) + if not course_overview: LOGGER.warning( - f"Skipping update of the `certificate_available_date` for course {course_key} in the Credentials service. " - "This course-run does not meet the required criteria for an update." + f"Unable to send the updated certificate available date of course run [{course_key}] to Credentials. A " + "course overview for this course run could not be found" ) + return + + # When updating the certificate available date of instructor-paced course runs, + # - If the display behavior is set to "A date after the course end date" (END_WITH_DATE), we should send the + # certificate available date set by the course team in Studio (and stored as part of the course runs Course + # Overview) + # - If the display behavior is set to "End date of course" (END), we should send the end date of the course run + # as the certificate available date. We send the end date because the Credentials IDA doesn't understand the + # concept of course pacing and needs an explicit date in order to correctly gate the visibility of course and + # program certificates. + # - If the display behavior is set to "Immediately upon passing" (EARLY_NO_INFO), we should always send None for + # the course runs certificate available date. A course run configured with this display behavior must not have a + # certificate available date associated with or the Credentials system will incorrectly hide certificates from + # learners. + if course_overview.self_paced is False: + if course_overview.certificates_display_behavior == CertificatesDisplayBehaviors.END_WITH_DATE: + new_certificate_available_date = str(course_overview.certificate_available_date) + elif course_overview.certificates_display_behavior == CertificatesDisplayBehaviors.END: + new_certificate_available_date = str(course_overview.end) # `end_date` is deprecated, use `end` instead + elif course_overview.certificates_display_behavior == CertificatesDisplayBehaviors.EARLY_NO_INFO: + new_certificate_available_date = None + # Else, this course run is self-paced, and having a certificate available date associated with a self-paced course + # run is not allowed. Course runs with this type of pacing should always award a certificate to learners immediately + # upon passing. If the system detects that an update must be sent to Credentials, we *always* send a certificate + # available date of `None`. We are aware of a defect that sometimes allows a certificate available date to be saved + # for a self-paced course run. This is an attempt to prevent bad data from being synced to the Credentials service + # too. + else: + new_certificate_available_date = None + + update_credentials_course_certificate_configuration_available_date.delay( + str(course_key), + new_certificate_available_date + ) diff --git a/openedx/core/djangoapps/programs/tests/test_signals.py b/openedx/core/djangoapps/programs/tests/test_signals.py index 2b6e3a1451..afb128a506 100644 --- a/openedx/core/djangoapps/programs/tests/test_signals.py +++ b/openedx/core/djangoapps/programs/tests/test_signals.py @@ -9,17 +9,21 @@ from django.test import TestCase from opaque_keys.edx.keys import CourseKey from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content.course_overviews.tests.factories import ( + CourseOverviewFactory, +) from openedx.core.djangoapps.programs.signals import ( handle_course_cert_awarded, handle_course_cert_changed, handle_course_cert_date_change, - handle_course_cert_revoked + handle_course_cert_revoked, + handle_course_pacing_change, ) from openedx.core.djangoapps.signals.signals import ( COURSE_CERT_AWARDED, COURSE_CERT_CHANGED, COURSE_CERT_DATE_CHANGE, - COURSE_CERT_REVOKED + COURSE_CERT_REVOKED, ) from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory from openedx.core.djangolib.testing.utils import skip_unless_lms @@ -262,19 +266,19 @@ class CourseCertAvailableDateChangedReceiverTest(TestCase): mock_is_learner_issuance_enabled, mock_visible_date_task, mock_cad_task - ): # pylint: disable=unused-argument + ): """ Ensures the receiver function is invoked when COURSE_CERT_DATE_CHANGE is sent. - - Suboptimal: because we cannot mock the receiver function itself (due to the way django signals work), we mock a - configuration call that is known to take place inside the function. """ COURSE_CERT_DATE_CHANGE.send(**self.signal_kwargs) assert mock_is_learner_issuance_enabled.call_count == 1 + assert mock_visible_date_task.call_count == 0 + assert mock_cad_task.call_count == 0 def test_programs_disabled(self, mock_is_learner_issuance_enabled, mock_visible_date_task, mock_cad_task): """ - Ensures that the receiver function does nothing when the credentials API configuration is not enabled. + Ensures that the receiver function does not queue any celery tasks if the system is not configured to use the + Credentials service. """ handle_course_cert_date_change(**self.signal_kwargs) assert mock_is_learner_issuance_enabled.call_count == 1 @@ -283,13 +287,73 @@ class CourseCertAvailableDateChangedReceiverTest(TestCase): def test_programs_enabled(self, mock_is_learner_issuance_enabled, mock_visible_date_task, mock_cad_task): """ - Ensures that the receiver function invokes the expected celery task when the credentials API configuration is - enabled. + Ensures that the receiver function enqueues the expected celery tasks when the system is configured to use the + Credentials IDA. """ mock_is_learner_issuance_enabled.return_value = True handle_course_cert_date_change(**self.signal_kwargs) - assert mock_is_learner_issuance_enabled.call_count == 1 assert mock_visible_date_task.call_count == 1 assert mock_cad_task.call_count == 1 + + +@skip_unless_lms +@mock.patch('openedx.core.djangoapps.programs.tasks.update_certificate_visible_date_on_course_update.delay') +@mock.patch('openedx.core.djangoapps.programs.tasks.update_certificate_available_date_on_course_update.delay') +@mock.patch('openedx.core.djangoapps.programs.signals.is_credentials_enabled') +class CoursePacingChangedReceiverTest(TestCase): + """ + Tests for the `handle_course_pacing_change` signal handler function. + """ + + def setUp(self): + super().setUp() + self.course_overview = CourseOverviewFactory.create( + self_paced=False, + ) + + @property + def signal_kwargs(self): + """ + DRY helper. + """ + return { + 'sender': self.__class__, + 'updated_course_overview': self.course_overview, + } + + def test_handle_course_pacing_change_credentials_disabled( + self, + mock_is_creds_enabled, + mock_cad_task, + mock_visible_date_task, + ): + """ + Test the verifies the behavior of the `handle_course_pacing_change` signal receiver when use of the Credentials + IDA is disabled by configuration. + """ + mock_is_creds_enabled.return_value = False + + handle_course_pacing_change(**self.signal_kwargs) + assert mock_is_creds_enabled.call_count == 1 + assert mock_visible_date_task.call_count == 0 + assert mock_cad_task.call_count == 0 + + def test_handle_course_pacing_change_credentials_enabled( + self, + mock_is_creds_enabled, + mock_cad_task, + mock_visible_date_task + ): + """ + Test that verifies the behavior of the `handle_course_pacing_change` signal receiver when use of the Credentials + IDA is enabled by configuration. + """ + mock_is_creds_enabled.return_value = True + self.course_overview.self_paced = True + + handle_course_pacing_change(**self.signal_kwargs) + assert mock_is_creds_enabled.call_count == 1 + assert mock_visible_date_task.call_count == 1 + assert mock_cad_task.call_count == 1 diff --git a/openedx/core/djangoapps/programs/tests/test_tasks.py b/openedx/core/djangoapps/programs/tests/test_tasks.py index ca97580132..83fa66a256 100644 --- a/openedx/core/djangoapps/programs/tests/test_tasks.py +++ b/openedx/core/djangoapps/programs/tests/test_tasks.py @@ -1083,94 +1083,176 @@ class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigM def setUp(self): super().setUp() + self.end_date = datetime.now(pytz.UTC) + timedelta(days=90) self.credentials_api_config = self.create_credentials_config(enabled=False) def tearDown(self): super().tearDown() self.credentials_api_config = self.create_credentials_config(enabled=False) - def test_update_certificate_available_date_but_credentials_config_disabled(self): + def _create_course_overview(self, self_paced, display_behavior, available_date, end): """ - This test verifies the behavior of the `UpdateCertificateAvailableDateOnCourseUpdateTestCase` task when the - CredentialsApiConfig is disabled. + Utility function to generate a CourseOverview with required settings for functions under test. + """ + return CourseOverviewFactory.create( + self_paced=self_paced, + end=end, + certificate_available_date=available_date, + certificates_display_behavior=display_behavior, + ) - If the system is configured to _not_ use the Credentials IDA, we should expect this task to eventually throw an - exception when the max number of retries has reached. + def _update_credentials_api_config(self, is_enabled): """ - course = CourseOverviewFactory.create() + Utility function to enable or disable use of the Credentials IDA in our test environment for the functions + under test. + """ + self.credentials_api_config.enabled = True + self.credentials_api_config.enable_learner_issuance = True + + def test_update_certificate_available_date_credentials_config_disabled(self): + """ + A test that verifies we do not queue any subtasks to update a certificate available date if use of the + Credentials is disabled in config. + """ + course_overview = self._create_course_overview( + False, + CertificatesDisplayBehaviors.EARLY_NO_INFO, + None, + self.end_date, + ) with pytest.raises(MaxRetriesExceededError): - tasks.update_certificate_available_date_on_course_update(course.id) # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter - def test_update_certificate_available_date_with_self_paced_course(self): + @mock.patch(f"{TASKS_MODULE}.update_credentials_course_certificate_configuration_available_date.delay") + def test_update_certificate_available_date_instructor_paced_cdb_early_no_info(self, mock_update): """ - Happy path test. + This test checks that we enqueue an `update_credentials_course_certificate_configuration_available_date` celery + task with values we would expect. - This test verifies that we are queueing a `update_credentials_course_certificate_configuration_available_date` - task with the expected arguments when removing a "certificate available date" from a course cert config in - Credentials. + In this scenario, we have a course overview that... + - is instructor-paced + - has a certificates display behavior of "EARLY NO INFO" (certificates are visible immediately after + generation) + - has no certificate available date + + We expect that the task enqueued has a certificate available date of `None`, as the certificates should have no + visibility restrictions. """ - self.credentials_api_config.enabled = True - self.credentials_api_config.enable_learner_issuance = True + self._update_credentials_api_config(True) - course = CourseOverviewFactory.create(self_paced=True, certificate_available_date=None) - - with mock.patch( - f"{TASKS_MODULE}.update_credentials_course_certificate_configuration_available_date.delay" - ) as update_credentials_course_cert_config: - tasks.update_certificate_available_date_on_course_update(course.id) # pylint: disable=no-value-for-parameter - - update_credentials_course_cert_config.assert_called_once_with(str(course.id), None) - - def test_update_certificate_available_date_with_instructor_paced_course(self): - """ - Happy path test. - - This test verifies that we are queueing a `update_credentials_course_certificate_configuration_available_date` - task with the expected arguments when updating a "certificate available date" from a course cert config in - Credentials. - """ - self.credentials_api_config.enabled = True - self.credentials_api_config.enable_learner_issuance = True - - available_date = datetime.now(pytz.UTC) + timedelta(days=1) - - course = CourseOverviewFactory.create( - self_paced=False, - certificate_available_date=available_date, - certificates_display_behavior=CertificatesDisplayBehaviors.END_WITH_DATE, + course_overview = self._create_course_overview( + False, + CertificatesDisplayBehaviors.EARLY_NO_INFO, + None, + self.end_date, ) - with mock.patch( - f"{TASKS_MODULE}.update_credentials_course_certificate_configuration_available_date.delay" - ) as update_credentials_course_cert_config: - tasks.update_certificate_available_date_on_course_update(course.id) # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + mock_update.assert_called_once_with(str(course_overview.id), None) - update_credentials_course_cert_config.assert_called_once_with(str(course.id), str(available_date)) - - def test_update_certificate_available_date_with_expect_no_update(self): + @mock.patch(f"{TASKS_MODULE}.update_credentials_course_certificate_configuration_available_date.delay") + def test_update_certificate_available_date_instructor_paced_cdb_end(self, mock_update): """ - This test verifies that we do _not_ queue a task to update the course certificate configuration in Credentials - if the course-run does not meet the required criteria. + This test checks that we enqueue an `update_credentials_course_certificate_configuration_available_date` celery + task with values we would expect. + + In this scenario, we have a course overview that... + - is instructor-paced + - has a certificates display behavior of "END" ("End of the course") + - has no certificate available date + + We expect that the task enqueued has a certificate available date that matches the end date of the course. """ - self.credentials_api_config.enabled = True - self.credentials_api_config.enable_learner_issuance = True + self._update_credentials_api_config(True) - available_date = datetime.now(pytz.UTC) + timedelta(days=1) - - course = CourseOverviewFactory.create( - self_paced=False, - certificate_available_date=available_date, - certificates_display_behavior=CertificatesDisplayBehaviors.EARLY_NO_INFO, + course_overview = self._create_course_overview( + False, + CertificatesDisplayBehaviors.END, + None, + self.end_date, ) + tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + mock_update.assert_called_once_with(str(course_overview.id), str(self.end_date)) + + @mock.patch(f"{TASKS_MODULE}.update_credentials_course_certificate_configuration_available_date.delay") + def test_update_certificate_available_date_instructor_paced_cdb_end_with_date(self, mock_update): + """ + This test checks that we enqueue an `update_credentials_course_certificate_configuration_available_date` celery + task with values we would expect. + + In this scenario, we have a course overview that... + - is instructor-paced + - has a certificates display behavior of "END WITH DATE" ("A date after the course ends") + - has an end date set to 90 days from today + - has a certificate available date set to 120 days from today + + We expect that the task enqueued has a certificate available date that matches the certificate available date + explicitly set as part of the course overview. + """ + self._update_credentials_api_config(True) + certificate_available_date = datetime.now(pytz.UTC) + timedelta(days=120) + + course_overview = self._create_course_overview( + False, + CertificatesDisplayBehaviors.END_WITH_DATE, + certificate_available_date, + self.end_date, + ) + + tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + mock_update.assert_called_once_with(str(course_overview.id), str(certificate_available_date)) + + @mock.patch(f"{TASKS_MODULE}.update_credentials_course_certificate_configuration_available_date.delay") + def test_update_certificate_available_date_self_paced(self, mock_update): + """ + This test checks that we enqueue an `update_credentials_course_certificate_configuration_available_date` celery + task with values we would expect. + + In this scenario, we have a course overview that... + - is self-paced + - has a certificates display behavior of "END WITH DATE" ("A date after the course ends") + - has an end date set to 90 days from today + - has a cerificate available date set 120 days from today + + We expect that the task enqueued has a certificate available date that matches the certificate available date + explicitly set as part of the course overview. + + This test case also verifies a change in recent behavior. There is a product defect that allows a self-paced + course to sometimes pass a certificate available date to Credentials. This test case also verifies that, if + invalid data is set in a course overview, we don't pass it to Credentials. + """ + self._update_credentials_api_config(True) + certificate_available_date = datetime.now(pytz.UTC) + timedelta(days=120) + + course_overview = self._create_course_overview( + True, + None, + certificate_available_date, + self.end_date, + ) + + tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + mock_update.assert_called_once_with(str(course_overview.id), None) + + def test_update_certificate_available_date_no_course_overview(self): + """ + A test case that verifies some logging if the + `update_credentials_course_certificate_configuration_available_date` task is queued with an invalid course run + id. + """ + bad_course_run_key = "course-v1:OpenEdx+MtG101x+1T2024" expected_message = ( - f"Skipping update of the `certificate_available_date` for course {course.id} in the Credentials service. " - "This course-run does not meet the required criteria for an update." + f"Unable to send the updated certificate available date of course run [{bad_course_run_key}] to " + "Credentials. A course overview for this course run could not be found" ) + self._update_credentials_api_config(True) + with LogCapture(level=logging.WARNING) as log_capture: - tasks.update_certificate_available_date_on_course_update(course.id) # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(bad_course_run_key) # pylint: disable=no-value-for-parameter - assert len(log_capture.records) == 1 - assert log_capture.records[0].getMessage() == expected_message + log_capture.check_present( + ('openedx.core.djangoapps.programs.tasks', 'WARNING', expected_message), + )