From 8c40057c1078806e43c8278750ede664309c958c Mon Sep 17 00:00:00 2001 From: Deborah Kaplan Date: Wed, 26 Jun 2024 11:45:28 -0400 Subject: [PATCH] feat: switching to celery native backoff for cert awarding (#35009) * feat: switching to celery native backoff for cert awarding Our homegrown backoff/retry was good enough for a while, but we ran into a huge disabling event when too many changes were made simultaneously. Since this code was first written, celery has built in good back off/retry functionality, including jitter, to make sure that all the retries don't happen simultaneously. * Switches to using celery native backoff * Refactors a huge try/catch block so the exception handling is on smaller subsets of code FIXES: APER-3510 * feat: switching to celery native backoff for cert awarding * Fixed the grammar in a couple of comments * fixed a couple of lending errors FIXES: APER-3510 * feat: limiting PII in logs per code review per code review FIXES: APER-3510 * feat: code review feedback * removing some more PII from logs, even where it was not requested * circuit breaker returned from grading attempt if course key is bad * add missing tests for that functionality FIXES: APER-3510 * feat: improved logging per code review, adding a log message explaining certificate mode issues FIXES: APER-3510 * feat: fixing a bad string lint error, and left off the format string modifier FIXES: APER-3510 * feat: style linter error FIXES: APER-3510 --- openedx/core/djangoapps/programs/tasks.py | 326 ++++++++++-------- .../djangoapps/programs/tests/test_tasks.py | 77 +++-- 2 files changed, 222 insertions(+), 181 deletions(-) diff --git a/openedx/core/djangoapps/programs/tasks.py b/openedx/core/djangoapps/programs/tasks.py index 72ccc79bddfc..3ea563857897 100644 --- a/openedx/core/djangoapps/programs/tasks.py +++ b/openedx/core/djangoapps/programs/tasks.py @@ -2,6 +2,7 @@ This file contains celery tasks and utility functions responsible for syncing course and program certificate metadata between the monolith and the Credentials IDA. """ + from typing import Dict, List from urllib.parse import urljoin @@ -13,6 +14,7 @@ from django.contrib.sites.models import Site from django.core.exceptions import ObjectDoesNotExist from edx_django_utils.monitoring import set_code_owner_attribute +from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from requests.exceptions import HTTPError @@ -250,7 +252,16 @@ def post_course_certificate_configuration(client, cert_config, certificate_avail response.raise_for_status() -@shared_task(bind=True, ignore_result=True) +# pylint: disable=unused-argument +@shared_task( + bind=True, + ignore_result=True, + autoretry_for=(Exception,), + max_retries=10, + retry_backoff=30, + retry_backoff_max=600, + retry_jitter=True, +) @set_code_owner_attribute def award_program_certificates(self, username): # lint-amnesty, pylint: disable=too-many-statements """ @@ -272,23 +283,14 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable Returns: None """ - def _retry_with_custom_exception(username, reason, countdown): - exception = MaxRetriesExceededError( - f"Failed to award a program certificate to user {username}. Reason: {reason}" - ) - return self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) - - countdown = 2**self.request.retries - # If the credentials config 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 altogether. + # tasks has been temporarily disabled. This is a recoverable situation, so let celery retry. if not is_credentials_enabled(): error_msg = ( "Task award_program_certificates cannot be executed, use of the Credentials service is disabled by config" ) LOGGER.warning(error_msg) - raise _retry_with_custom_exception(username=username, reason=error_msg, countdown=countdown) + raise MaxRetriesExceededError(f"Failed to award a program certificate. Reason: {error_msg}") try: student = User.objects.get(username=username) @@ -323,7 +325,9 @@ def _retry_with_custom_exception(username, reason, countdown): except Exception as exc: error_msg = f"Failed to determine program certificates to be awarded for user {student}: {exc}" LOGGER.exception(error_msg) - raise _retry_with_custom_exception(username=username, reason=error_msg, countdown=countdown) from exc + raise MaxRetriesExceededError( + f"Failed to award a program certificate to user {student.id}. Reason: {error_msg}" + ) from exc # For each completed program for which the student doesn't already have a certificate, award one now. # @@ -339,8 +343,10 @@ def _retry_with_custom_exception(username, reason, countdown): except Exception as exc: error_msg = "Failed to create a credentials API client to award program certificates" LOGGER.exception(error_msg) - # Retry because a misconfiguration could be fixed - raise _retry_with_custom_exception(username=username, reason=error_msg, countdown=countdown) from exc + # A misconfiguration could be fixed; let celery retry. + raise MaxRetriesExceededError( + f"Failed to award a program certificate to user {student.id}. Reason: {error_msg}" + ) from exc failed_program_certificate_award_attempts = [] for program_uuid in new_program_uuids: @@ -360,17 +366,13 @@ def _retry_with_custom_exception(username, reason, countdown): "not be configured correctly in Credentials" ) elif exc.response.status_code == 429: - rate_limit_countdown = 60 + # Let celery handle retry attempts and backoff error_msg = ( - f"Rate limited. Retrying task to award certificate to user {student} in program " - f"{program_uuid} in {rate_limit_countdown} seconds" + f"Rate limited. Attempting to award certificate to user {student} in program {program_uuid}." ) LOGGER.warning(error_msg) - # Retry after 60 seconds, when we should be in a new throttling window - raise _retry_with_custom_exception( - username=username, - reason=error_msg, - countdown=rate_limit_countdown, + raise MaxRetriesExceededError( + f"Failed to award a program certificate to user {student.id}. Reason: {error_msg}" ) from exc else: LOGGER.warning( @@ -378,7 +380,7 @@ def _retry_with_custom_exception(username, reason, countdown): "might not be configured correctly in Credentials" ) except Exception as exc: # pylint: disable=broad-except - # keep trying to award other certs, but retry the whole task to fix any missing entries + # keep trying to award other certs, but let celery retry the whole task to fix any missing entries LOGGER.exception( f"Failed to award program certificate to user {student} in program {program_uuid}: {exc}" ) @@ -394,7 +396,9 @@ def _retry_with_custom_exception(username, reason, countdown): f"Failed to award program certificate(s) for user {student} in programs " f"{failed_program_certificate_award_attempts}" ) - raise _retry_with_custom_exception(username=username, reason=error_msg, countdown=countdown) + raise MaxRetriesExceededError( + f"Failed to award a program certificate to user {student.id}. Reason: {error_msg}" + ) else: LOGGER.warning(f"User {student} is not eligible for any new program certificates") @@ -402,7 +406,15 @@ def _retry_with_custom_exception(username, reason, countdown): # pylint: disable=W0613 -@shared_task(bind=True, ignore_result=True) +@shared_task( + bind=True, + ignore_result=True, + autoretry_for=(Exception,), + max_retries=10, + retry_backoff=30, + retry_backoff_max=600, + retry_jitter=True, +) @set_code_owner_attribute def update_credentials_course_certificate_configuration_available_date( self, course_key, certificate_available_date=None @@ -427,7 +439,8 @@ def update_credentials_course_certificate_configuration_available_date( 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 + 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: @@ -448,7 +461,15 @@ def update_credentials_course_certificate_configuration_available_date( ) -@shared_task(bind=True, ignore_result=True) +@shared_task( + bind=True, + ignore_result=True, + autoretry_for=(Exception,), + max_retries=10, + retry_backoff=30, + retry_backoff_max=600, + retry_jitter=True, +) @set_code_owner_attribute def award_course_certificate(self, username, course_run_key): """ @@ -457,35 +478,21 @@ def award_course_certificate(self, username, course_run_key): It can be called independently for a username and a course_run, but is invoked on each GeneratedCertificate.save. - If this function is moved, make sure to update it's entry in EXPLICIT_QUEUES in the settings files so it runs in the + If this function is moved, make sure to update its entry in EXPLICIT_QUEUES in the settings files so it runs in the correct queue. Arguments: username (str): The user to award the Credentials course cert to course_run_key (str): The course run key to award the certificate for """ - def _retry_with_custom_exception(username, course_run_key, reason, countdown): - exception = MaxRetriesExceededError( - f"Failed to award course certificate for user {username} for course {course_run_key}. Reason: {reason}" - ) - return self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) - - countdown = 2**self.request.retries - # If the credentials config 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 altogether. + # tasks has been temporarily disabled. This is a recoverable situation, let celery retry. if not is_credentials_enabled(): error_msg = ( "Task award_course_certificate cannot be executed when credentials issuance is disabled in API config" ) LOGGER.warning(error_msg) - raise _retry_with_custom_exception( - username=username, - course_run_key=course_run_key, - reason=error_msg, - countdown=countdown, - ) + raise MaxRetriesExceededError(f"Failed to award course certificate. Reason: {error_msg}") try: user = User.objects.get(username=username) @@ -499,71 +506,98 @@ def _retry_with_custom_exception(username, course_run_key, reason, countdown): LOGGER.info(f"Running task award_course_certificate for user {user}") try: course_key = CourseKey.from_string(course_run_key) - # Get the cert for the course key and username if it's both passing and available in professional/verified - try: - certificate = GeneratedCertificate.eligible_certificates.get( - user=user.id, - course_id=course_key, - ) - except GeneratedCertificate.DoesNotExist: + except InvalidKeyError as exc: + error_msg = "Failed to determine course key" + LOGGER.warning( + f"Failed to award course certificate for user {user.id} for course {course_run_key}. Reason: {error_msg}" + ) + return + + # Get the cert for the course key and username if it's both passing and available in professional/verified + try: + certificate = GeneratedCertificate.eligible_certificates.get( + user=user.id, + course_id=course_key, + ) + except GeneratedCertificate.DoesNotExist: + LOGGER.warning( + f"Task award_course_certificate was called for user {user.id} in course run {course_key} but this learner " + "has not earned a course certificate in this course run" + ) + return + + try: + if ( + certificate.mode not in CourseMode.CERTIFICATE_RELEVANT_MODES + and not CourseMode.is_eligible_for_certificate(certificate.mode) + ): LOGGER.warning( - f"Task award_course_certificate was called for user {user} in course run {course_key} but this learner " - "has not earned a course certificate in this course run" + f"Task award_course_certificate was called for user {user.id} in course run {course_key} but " + f"this course has an ineligible mode of {certificate.mode} for a certificate on this instance." ) return + except Exception as exc: + error_msg = f"Failed to determine course mode certificate eligibility for {certificate}." + LOGGER.error(error_msg) + raise MaxRetriesExceededError( + f"Failed to award course certificate for user {user.id} for course {course_run_key}. Reason: {error_msg}" + ) from exc - if ( - certificate.mode in CourseMode.CERTIFICATE_RELEVANT_MODES - or CourseMode.is_eligible_for_certificate(certificate.mode) - ): - course_overview = get_course_overview_or_none(course_key) - if not course_overview: - LOGGER.warning( - f"Task award_course_certificate was called for user {user} in course {course_key} but no course " - "overview could be retrieved for the course run" - ) - return + course_overview = get_course_overview_or_none(course_key) + if not course_overview: + LOGGER.warning( + f"Task award_course_certificate was called for user {user.id} in course {course_key} but no course " + "overview could be retrieved for the course run" + ) + return - visible_date = available_date_for_certificate(course_overview, certificate) - LOGGER.info( - f"Task award_course_certificate will award a course certificate to user {user} in course run " - f"{course_key} with a visible date of {visible_date}" - ) + visible_date = available_date_for_certificate(course_overview, certificate) + LOGGER.info( + f"Task award_course_certificate will award a course certificate to user {user.id} in course run " + f"{course_key} with a visible date of {visible_date}" + ) - # If the certificate has an associated CertificateDateOverride, send it along - try: - date_override = certificate.date_override.date - LOGGER.info( - f"Task award_course_certificate will award a course certificate to user {user} in course run " - f"{course_key} with an override date of {date_override}" - ) - except ObjectDoesNotExist: - date_override = None + # If the certificate has an associated CertificateDateOverride, send it along + try: + date_override = certificate.date_override.date + LOGGER.info( + f"Task award_course_certificate will award a course certificate to user {user.id} in course run " + f"{course_key} with an override date of {date_override}" + ) + except ObjectDoesNotExist: + date_override = None - credentials_client = get_credentials_api_client( - User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME), - ) - post_course_certificate( - credentials_client, - username, - certificate, - visible_date, - date_override, - org=course_key.org, - ) - LOGGER.info(f"Awarded a course certificate to user {user} in course run {course_key}") + try: + credentials_client = get_credentials_api_client( + User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME), + ) + post_course_certificate( + credentials_client, + username, + certificate, + visible_date, + date_override, + org=course_key.org, + ) except Exception as exc: - error_msg = f"Failed to determine course certificates to be awarded for user {user}." - LOGGER.exception(error_msg) - raise _retry_with_custom_exception( - username=username, - course_run_key=course_run_key, - reason=error_msg, - countdown=countdown, + error_msg = f"Failed to post course certificate to be awarded for user {user}." + raise MaxRetriesExceededError( + f"Failed to award course certificate for user {user.id} for course {course_run_key}. Reason: {error_msg}" ) from exc + # Successfully posted the cert to credentials + LOGGER.info(f"Awarded a course certificate to user {user.id} in course run {course_key}") + -@shared_task(bind=True, ignore_result=True) +@shared_task( + bind=True, + ignore_result=True, + autoretry_for=(Exception,), + max_retries=10, + retry_backoff=30, + retry_backoff_max=600, + retry_jitter=True, +) @set_code_owner_attribute def revoke_program_certificates(self, username, course_key): # lint-amnesty, pylint: disable=too-many-statements """ @@ -572,7 +606,7 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py It will consult with a variety of APIs to determine whether or not the specified user's certificate should be revoked in one or more programs, and use the credentials service to revoke the said certificates if so. - If this function is moved, make sure to update it's entry in EXPLICIT_QUEUES in the settings files so it runs in the + If this function is moved, make sure to update its entry in EXPLICIT_QUEUES in the settings files so it runs in the correct queue. Args: @@ -582,28 +616,14 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py Returns: None """ - def _retry_with_custom_exception(username, course_key, reason, countdown): - exception = MaxRetriesExceededError( - f"Failed to revoke program certificate for user {username} for course {course_key}. Reason: {reason}" - ) - return self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) - - countdown = 2**self.request.retries - # If the credentials config 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 altogether. + # tasks has been temporarily disabled. Since this is a recoverable situation, let celery retry. if not is_credentials_enabled(): error_msg = ( "Task revoke_program_certificates cannot be executed, use of the Credentials service is disabled by config" ) LOGGER.warning(error_msg) - raise _retry_with_custom_exception( - username=username, - course_key=course_key, - reason=error_msg, - countdown=countdown, - ) + raise MaxRetriesExceededError(f"Failed to revoke program certificate. Reason: {error_msg}") try: student = User.objects.get(username=username) @@ -633,11 +653,8 @@ def _retry_with_custom_exception(username, course_key, reason, countdown): f"revoked from user {student}" ) LOGGER.exception(error_msg) - raise _retry_with_custom_exception( - username=username, - course_key=course_key, - reason=error_msg, - countdown=countdown, + raise MaxRetriesExceededError( + f"Failed to revoke program certificate for user {student.id} for course {course_key}. Reason: {error_msg}" ) from exc if program_uuids_to_revoke: @@ -648,8 +665,10 @@ def _retry_with_custom_exception(username, course_key, reason, countdown): except Exception as exc: error_msg = "Failed to create a credentials API client to revoke program certificates" LOGGER.exception(error_msg) - # Retry because a misconfiguration could be fixed - raise _retry_with_custom_exception(username, course_key, reason=exc, countdown=countdown) from exc + # Stil retryable because a misconfiguration could be fixed + raise MaxRetriesExceededError( + f"Failed to revoke program certificate for user {student.id} for course {course_key}. Reason: {exc}" + ) from exc failed_program_certificate_revoke_attempts = [] for program_uuid in program_uuids_to_revoke: @@ -663,25 +682,21 @@ def _retry_with_custom_exception(username, course_key, reason, countdown): "program certificate could not be found" ) elif exc.response.status_code == 429: - rate_limit_countdown = 60 + # Let celery handle retry attempts and backoff error_msg = ( - f"Rate limited. Retrying task to revoke a program certificate from user {student} in program " - f"{program_uuid} in {rate_limit_countdown} seconds" + f"Rate limited. Attempting to revoke a program certificate from user {student} in program " + f"{program_uuid}." ) LOGGER.warning(error_msg) - # Retry after 60 seconds, when we should be in a new throttling window - raise _retry_with_custom_exception( - username, - course_key, - reason=error_msg, - countdown=rate_limit_countdown, + raise MaxRetriesExceededError( + f"Failed to revoke program certificate for user {student.id} Reason: {error_msg}" ) from exc else: LOGGER.warning( f"Unable to revoke program certificate from user {student} in program {program_uuid}" ) except Exception as exc: # pylint: disable=broad-except - # keep trying to revoke other certs, but retry the whole task to fix any missing entries + # keep trying to revoke other certs, but let celery retry the whole task to fix any missing entries LOGGER.exception( f"Failed to revoke program certificate from user {student} in program {program_uuid}: {exc}" ) @@ -689,7 +704,7 @@ def _retry_with_custom_exception(username, course_key, reason, countdown): if failed_program_certificate_revoke_attempts: # N.B. This logic assumes that this task is idempotent - LOGGER.info(f"Retrying failed task to revoke program certificate(s) from user {student}") + LOGGER.info(f"Failed task to revoke program certificate(s) from user {student}") # The error message may change on each reattempt but will never be raised until the max number of retries # have been exceeded. It is unlikely that this list will change by the time it reaches its maximimum number # of attempts. @@ -697,14 +712,25 @@ def _retry_with_custom_exception(username, course_key, reason, countdown): f"Failed to revoke program certificate(s) from user {student} for programs " f"{failed_program_certificate_revoke_attempts}" ) - raise _retry_with_custom_exception(username, course_key, reason=error_msg, countdown=countdown) + raise MaxRetriesExceededError( + f"Failed to revoke program certificate for user {student.id} for course {course_key}. " + f"Reason: {error_msg}" + ) else: LOGGER.info(f"No program certificates to revoke from user {student}") LOGGER.info(f"Successfully completed the task revoke_program_certificates for user {student}") -@shared_task(bind=True, ignore_result=True) +@shared_task( + bind=True, + ignore_result=True, + autoretry_for=(Exception,), + max_retries=10, + retry_backoff=30, + retry_backoff_max=600, + retry_jitter=True, +) @set_code_owner_attribute def update_certificate_visible_date_on_course_update(self, course_key): """ @@ -714,28 +740,24 @@ def update_certificate_visible_date_on_course_update(self, course_key): Next, we will enqueue an additional `award_course_certificate` task for each learner in this list. These subtasks will be responsible for updating the `visible_date` attribute on each certificate the Credentials IDA knows about. - If this function is moved, make sure to update it's entry in EXPLICIT_QUEUES in the settings files so it runs in the + If this function is moved, make sure to update its entry in EXPLICIT_QUEUES in the settings files so it runs in the correct queue. Arguments: course_key(str): The course 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. + # processing of such tasks has been temporarily disabled. This is a recoverable situation, so let celery retry. 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." ) LOGGER.warning(error_msg) - exception = MaxRetriesExceededError( + raise MaxRetriesExceededError( f"Failed to update the `visible_date` attribute for certificates in course {course_key}. Reason: " f"{error_msg}" ) - raise self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) # Retrieve a list of all usernames of learners who have a certificate record in this course-run. The # Credentials IDA REST API still requires a username as the main identifier for the learner. @@ -751,7 +773,15 @@ def update_certificate_visible_date_on_course_update(self, course_key): award_course_certificate.delay(user, str(course_key)) -@shared_task(bind=True, ignore_result=True) +@shared_task( + bind=True, + ignore_result=True, + autoretry_for=(Exception,), + max_retries=10, + retry_backoff=30, + retry_backoff_max=600, + retry_jitter=True, +) @set_code_owner_attribute def update_certificate_available_date_on_course_update(self, course_key): """ @@ -764,8 +794,6 @@ def update_certificate_available_date_on_course_update(self, course_key): Args: 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. @@ -775,11 +803,10 @@ def update_certificate_available_date_on_course_update(self, course_key): "through the Credentials IDA is disabled." ) LOGGER.warning(error_msg) - exception = MaxRetriesExceededError( + raise MaxRetriesExceededError( "Failed to update the `certificate_available_date` in the Credentials service for course-run " f"{course_key}. Reason: {error_msg}" ) - raise self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) course_overview = get_course_overview_or_none(course_key) if not course_overview: @@ -818,6 +845,5 @@ def update_certificate_available_date_on_course_update(self, course_key): new_certificate_available_date = None update_credentials_course_certificate_configuration_available_date.delay( - str(course_key), - new_certificate_available_date + str(course_key), new_certificate_available_date ) diff --git a/openedx/core/djangoapps/programs/tests/test_tasks.py b/openedx/core/djangoapps/programs/tests/test_tasks.py index 4b3a143fc944..0c3732349942 100644 --- a/openedx/core/djangoapps/programs/tests/test_tasks.py +++ b/openedx/core/djangoapps/programs/tests/test_tasks.py @@ -2,7 +2,6 @@ Tests for programs celery tasks. """ - import json import logging from datetime import datetime, timedelta @@ -22,21 +21,13 @@ from common.djangoapps.course_modes.tests.factories import CourseModeFactory from common.djangoapps.student.tests.factories import UserFactory -from lms.djangoapps.certificates.tests.factories import ( - CertificateDateOverrideFactory, - GeneratedCertificateFactory, -) +from lms.djangoapps.certificates.tests.factories import CertificateDateOverrideFactory, GeneratedCertificateFactory from openedx.core.djangoapps.catalog.tests.mixins import CatalogIntegrationMixin -from openedx.core.djangoapps.content.course_overviews.tests.factories import ( - CourseOverviewFactory, -) +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.credentials.tests.mixins import CredentialsApiConfigMixin from openedx.core.djangoapps.oauth_dispatch.tests.factories import ApplicationFactory from openedx.core.djangoapps.programs import tasks -from openedx.core.djangoapps.site_configuration.tests.factories import ( - SiteConfigurationFactory, - SiteFactory, -) +from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory from openedx.core.djangolib.testing.utils import skip_unless_lms from xmodule.data import CertificatesDisplayBehaviors @@ -311,7 +302,7 @@ def test_failure_to_create_api_client_retries( tasks.award_program_certificates.delay(self.student.username).get() assert mock_exception.called - assert mock_get_api_client.call_count == (tasks.MAX_RETRIES + 1) + assert mock_get_api_client.call_count == (tasks.MAX_RETRIES) assert not mock_award_program_certificate.called def _make_side_effect(self, side_effects): @@ -611,6 +602,17 @@ def test_award_course_cert_not_called_if_user_not_found(self, mock_post_course_c assert mock_exception.called assert not mock_post_course_certificate.called + def test_award_course_cert_not_called_if_course_overview_not_found(self, mock_post_course_certificate): + """ + Test that the post method is never called if the CourseOverview isn't found + """ + self.course.delete() + with mock.patch(TASKS_MODULE + ".LOGGER.warning") as mock_exception: + # Use the certificate course id here since the course will be deleted + tasks.award_course_certificate.delay(self.student.username, str(self.certificate.course_id)).get() + assert mock_exception.called + assert not mock_post_course_certificate.called + def test_award_course_cert_not_called_if_certificate_not_found(self, mock_post_course_certificate): """ Test that the post method is never called if the certificate doesn't exist for the user and course @@ -621,16 +623,21 @@ def test_award_course_cert_not_called_if_certificate_not_found(self, mock_post_c assert mock_exception.called assert not mock_post_course_certificate.called - def test_award_course_cert_not_called_if_course_overview_not_found(self, mock_post_course_certificate): + def test_award_course_cert_not_called_if_course_run_key_is_bad(self, mock_post_course_certificate): """ - Test that the post method is never called if the CourseOverview isn't found + Test that the post method is never called if the course run key is invalid """ - self.course.delete() - with mock.patch(TASKS_MODULE + ".LOGGER.warning") as mock_exception: - # Use the certificate course id here since the course will be deleted - tasks.award_course_certificate.delay(self.student.username, str(self.certificate.course_id)).get() - assert mock_exception.called - assert not mock_post_course_certificate.called + bad_course_run_key = "I/Am/The/Keymaster" + expected_message = ( + f"Failed to award course certificate for user {self.student.id} for course " + f"{bad_course_run_key}. Reason: Failed to determine course key" + ) + with LogCapture(level=logging.WARNING) as log_capture: + tasks.award_course_certificate.delay(self.student.username, bad_course_run_key).get() + assert not mock_post_course_certificate.called + log_capture.check_present( + ("openedx.core.djangoapps.programs.tasks", "WARNING", expected_message), + ) def test_award_course_cert_not_called_if_certificated_not_verified_mode(self, mock_post_course_certificate): """ @@ -922,7 +929,7 @@ def test_get_api_client_failure_retries( with pytest.raises(MaxRetriesExceededError): tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() assert mock_exception.called - assert mock_get_api_client.call_count == (tasks.MAX_RETRIES + 1) + assert mock_get_api_client.call_count == (tasks.MAX_RETRIES) assert not mock_revoke_program_certificate.called @@ -1050,7 +1057,8 @@ def test_update_visible_dates_but_credentials_config_disabled(self): exception when the max number of retries has reached. """ with pytest.raises(MaxRetriesExceededError): - tasks.update_certificate_visible_date_on_course_update(self.course.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_visible_date_on_course_update(self.course.id) def test_update_visible_dates(self): """ @@ -1065,7 +1073,8 @@ def test_update_visible_dates(self): self.credentials_api_config.enable_learner_issuance = True with mock.patch(f"{TASKS_MODULE}.award_course_certificate.delay") as award_course_cert: - tasks.update_certificate_visible_date_on_course_update(self.course.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_visible_date_on_course_update(self.course.id) assert award_course_cert.call_count == 3 @@ -1117,7 +1126,8 @@ def test_update_certificate_available_date_credentials_config_disabled(self): ) with pytest.raises(MaxRetriesExceededError): - tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(course_overview.id) @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): @@ -1143,7 +1153,8 @@ def test_update_certificate_available_date_instructor_paced_cdb_early_no_info(se self.end_date, ) - tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(course_overview.id) mock_update.assert_called_once_with(str(course_overview.id), None) @mock.patch(f"{TASKS_MODULE}.update_credentials_course_certificate_configuration_available_date.delay") @@ -1168,7 +1179,8 @@ def test_update_certificate_available_date_instructor_paced_cdb_end(self, mock_u self.end_date, ) - tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(course_overview.id) 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") @@ -1196,7 +1208,8 @@ def test_update_certificate_available_date_instructor_paced_cdb_end_with_date(se self.end_date, ) - tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(course_overview.id) 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") @@ -1228,7 +1241,8 @@ def test_update_certificate_available_date_self_paced(self, mock_update): self.end_date, ) - tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(course_overview.id) mock_update.assert_called_once_with(str(course_overview.id), None) def test_update_certificate_available_date_no_course_overview(self): @@ -1246,8 +1260,9 @@ def test_update_certificate_available_date_no_course_overview(self): self._update_credentials_api_config(True) with LogCapture(level=logging.WARNING) as log_capture: - tasks.update_certificate_available_date_on_course_update(bad_course_run_key) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(bad_course_run_key) log_capture.check_present( - ('openedx.core.djangoapps.programs.tasks', 'WARNING', expected_message), + ("openedx.core.djangoapps.programs.tasks", "WARNING", expected_message), )