diff --git a/openedx/core/djangoapps/credentials/utils.py b/openedx/core/djangoapps/credentials/utils.py index b17b7ffffa..437f7c8c95 100644 --- a/openedx/core/djangoapps/credentials/utils.py +++ b/openedx/core/djangoapps/credentials/utils.py @@ -1,5 +1,6 @@ """Helper functions for working with Credentials.""" import logging +from typing import Dict, List from urllib.parse import urljoin import requests @@ -42,7 +43,7 @@ def get_credentials_api_client(user): Arguments: user (User): The user to authenticate as when requesting credentials. """ - scopes = ['email', 'profile', 'user_id'] + scopes = ["email", "profile", "user_id"] jwt = create_jwt_for_user(user, scopes=scopes) client = requests.Session() @@ -65,7 +66,12 @@ def get_credentials_api_base_url(org=None): return url -def get_credentials(user, program_uuid=None, credential_type=None): +def get_credentials( + user: User, + program_uuid: str = None, + credential_type: str = None, + raise_on_error: bool = False, +) -> List[Dict]: """ Given a user, get credentials earned from the credentials service. @@ -75,6 +81,7 @@ def get_credentials(user, program_uuid=None, credential_type=None): Keyword Arguments: program_uuid (str): UUID of the program whose credential to retrieve. credential_type (str): Which type of credentials to return (course-run or program) + raise_on_error (bool): Reraise errors back to the caller, instead if returning empty results. Returns: list of dict, representing credentials returned by the Credentials @@ -82,31 +89,38 @@ def get_credentials(user, program_uuid=None, credential_type=None): """ credential_configuration = CredentialsApiConfig.current() - querystring = {'username': user.username, 'status': 'awarded', 'only_visible': 'True'} + querystring = { + "username": user.username, + "status": "awarded", + "only_visible": "True", + } if program_uuid: - querystring['program_uuid'] = program_uuid + querystring["program_uuid"] = program_uuid if credential_type: - querystring['type'] = credential_type + querystring["type"] = credential_type # Bypass caching for staff users, who may be generating credentials and # want to see them displayed immediately. use_cache = credential_configuration.is_cache_enabled and not user.is_staff - cache_key = f'{credential_configuration.CACHE_KEY}.{user.username}' if use_cache else None + cache_key = ( + f"{credential_configuration.CACHE_KEY}.{user.username}" if use_cache else None + ) if cache_key and program_uuid: - cache_key = f'{cache_key}.{program_uuid}' + cache_key = f"{cache_key}.{program_uuid}" api_client = get_credentials_api_client(user) base_api_url = get_credentials_api_base_url() return get_api_data( credential_configuration, - 'credentials', + "credentials", api_client=api_client, base_api_url=base_api_url, querystring=querystring, - cache_key=cache_key + cache_key=cache_key, + raise_on_error=raise_on_error, ) @@ -122,11 +136,13 @@ def get_courses_completion_status(username, course_run_ids): """ credential_configuration = CredentialsApiConfig.current() if not credential_configuration.enabled: - log.warning('%s configuration is disabled.', credential_configuration.API_NAME) + log.warning("%s configuration is disabled.", credential_configuration.API_NAME) return [], False - completion_status_url = (f'{settings.CREDENTIALS_INTERNAL_SERVICE_URL}/api' - '/credentials/v1/learner_cert_status/') + completion_status_url = ( + f"{settings.CREDENTIALS_INTERNAL_SERVICE_URL}/api" + "/credentials/v1/learner_cert_status/" + ) try: api_client = get_credentials_api_client( User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME) @@ -134,30 +150,36 @@ def get_courses_completion_status(username, course_run_ids): api_response = api_client.post( completion_status_url, json={ - 'username': username, - 'course_runs': course_run_ids, - } + "username": username, + "course_runs": course_run_ids, + }, ) api_response.raise_for_status() course_completion_response = api_response.json() except Exception as exc: # pylint: disable=broad-except - log.exception("An unexpected error occurred while reqeusting course completion statuses " - "for user [%s] for course_run_ids [%s] with exc [%s]:", - username, - course_run_ids, - exc - ) + log.exception( + "An unexpected error occurred while reqeusting course completion statuses " + "for user [%s] for course_run_ids [%s] with exc [%s]:", + username, + course_run_ids, + exc, + ) return [], True - log.info("Course completion status response for user [%s] for course_run_ids [%s] is [%s]", - username, - course_run_ids, - course_completion_response) + log.info( + "Course completion status response for user [%s] for course_run_ids [%s] is [%s]", + username, + course_run_ids, + course_completion_response, + ) # Yes, This is course_credentials_data. The key is named status but # it contains all the courses data from credentials. - course_credentials_data = course_completion_response.get('status', []) + course_credentials_data = course_completion_response.get("status", []) if course_credentials_data is not None: - filtered_records = [course_data['course_run']['key'] for course_data in course_credentials_data if - course_data['course_run']['key'] in course_run_ids and - course_data['status'] == settings.CREDENTIALS_COURSE_COMPLETION_STATE] + filtered_records = [ + course_data["course_run"]["key"] + for course_data in course_credentials_data + if course_data["course_run"]["key"] in course_run_ids + and course_data["status"] == settings.CREDENTIALS_COURSE_COMPLETION_STATE + ] return filtered_records, False return [], False diff --git a/openedx/core/djangoapps/programs/tasks.py b/openedx/core/djangoapps/programs/tasks.py index a22bff876d..2431f529d3 100644 --- a/openedx/core/djangoapps/programs/tasks.py +++ b/openedx/core/djangoapps/programs/tasks.py @@ -1,6 +1,7 @@ """ This file contains celery tasks for programs-related functionality. """ +from typing import Dict, List from urllib.parse import urljoin from celery import shared_task @@ -13,7 +14,6 @@ from django.core.exceptions import ObjectDoesNotExist from edx_django_utils.monitoring import set_code_owner_attribute from opaque_keys.edx.keys import CourseKey from requests.exceptions import HTTPError -from xmodule.data import CertificatesDisplayBehaviors from common.djangoapps.course_modes.models import CourseMode from lms.djangoapps.certificates.api import available_date_for_certificate @@ -27,6 +27,7 @@ from openedx.core.djangoapps.credentials.utils import ( ) from openedx.core.djangoapps.programs.utils import ProgramProgressMeter from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from xmodule.data import CertificatesDisplayBehaviors User = get_user_model() @@ -37,9 +38,9 @@ LOGGER = get_task_logger(__name__) # unwanted behavior: infinite retries. MAX_RETRIES = 11 -PROGRAM_CERTIFICATE = 'program' -COURSE_CERTIFICATE = 'course-run' -DATE_FORMAT = '%Y-%m-%dT%H:%M:%SZ' +PROGRAM_CERTIFICATE = "program" +COURSE_CERTIFICATE = "course-run" +DATE_FORMAT = "%Y-%m-%dT%H:%M:%SZ" def get_completed_programs(site, student): @@ -77,22 +78,28 @@ def get_inverted_programs(student): return inverted_programs -def get_certified_programs(student): +def get_certified_programs(student: User, raise_on_error: bool = False) -> List[str]: """ Find the UUIDs of all the programs for which the student has already been awarded a certificate. Args: - student: - User object representing the student + student: User object representing the student + + Keyword Arguments: + raise_on_error (bool): Reraise errors back to the caller, instead if returning empty results. Returns: str[]: UUIDs of the programs for which the student has been awarded a certificate """ certified_programs = [] - for credential in get_credentials(student, credential_type='program'): - certified_programs.append(credential['credential']['program_uuid']) + for credential in get_credentials( + student, + credential_type="program", + raise_on_error=raise_on_error, + ): + certified_programs.append(credential["credential"]["program_uuid"]) return certified_programs @@ -118,26 +125,22 @@ def award_program_certificate(client, user, program_uuid, visible_date): response = client.post( api_url, json={ - 'username': user.username, - 'lms_user_id': user.id, - 'credential': { - 'type': PROGRAM_CERTIFICATE, - 'program_uuid': program_uuid - }, - 'attributes': [ - { - 'name': 'visible_date', - 'value': visible_date.strftime(DATE_FORMAT) - } - ] - } + "username": user.username, + "lms_user_id": user.id, + "credential": {"type": PROGRAM_CERTIFICATE, "program_uuid": program_uuid}, + "attributes": [ + {"name": "visible_date", "value": visible_date.strftime(DATE_FORMAT)} + ], + }, ) response.raise_for_status() @shared_task(bind=True, ignore_result=True) @set_code_owner_attribute -def award_program_certificates(self, username): # lint-amnesty, pylint: disable=too-many-statements +def award_program_certificates( + self, username +): # lint-amnesty, pylint: disable=too-many-statements """ This task is designed to be called whenever a student's completion status changes with respect to one or more courses (primarily, when a course @@ -161,41 +164,42 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable None """ + def _retry_with_custom_exception(username, reason, countdown): exception = MaxRetriesExceededError( f"Failed to award program certificate for user {username}. Reason: {reason}" ) - return self.retry( - exc=exception, - countdown=countdown, - max_retries=MAX_RETRIES - ) + return self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) LOGGER.info(f"Running task award_program_certificates for username {username}") - programs_without_certificates = configuration_helpers.get_value('programs_without_certificates', []) + programs_without_certificates = configuration_helpers.get_value( + "programs_without_certificates", [] + ) if programs_without_certificates: if str(programs_without_certificates[0]).lower() == "all": # this check will prevent unnecessary logging for partners without program certificates return - countdown = 2 ** self.request.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. if not CredentialsApiConfig.current().is_learner_issuance_enabled: - error_msg = ( - "Task award_program_certificates cannot be executed when credentials issuance is disabled in API config" - ) + error_msg = "Task award_program_certificates cannot be executed when credentials issuance is disabled in API config" LOGGER.warning(error_msg) - raise _retry_with_custom_exception(username=username, reason=error_msg, countdown=countdown) + raise _retry_with_custom_exception( + username=username, reason=error_msg, countdown=countdown + ) try: try: student = User.objects.get(username=username) except User.DoesNotExist: - LOGGER.exception(f"Task award_program_certificates was called with invalid username {username}") + LOGGER.exception( + f"Task award_program_certificates was called with invalid username {username}" + ) # Don't retry for this case - just conclude the task. return completed_programs = {} @@ -204,7 +208,9 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable if not completed_programs: # No reason to continue beyond this point unless/until this # task gets updated to support revocation of program certs. - LOGGER.info(f"Task award_program_certificates was called for user {username} with no completed programs") + LOGGER.info( + f"Task award_program_certificates was called for user {username} with no completed programs" + ) return # Determine which program certificates the user has already been awarded, if any. @@ -212,12 +218,16 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable # we will skip all the programs which have already been awarded and we want to skip the programs # which are exit in site configuration in 'programs_without_certificates' list. - awarded_and_skipped_program_uuids = list(set(existing_program_uuids + list(programs_without_certificates))) + awarded_and_skipped_program_uuids = list( + set(existing_program_uuids + list(programs_without_certificates)) + ) except Exception as exc: error_msg = f"Failed to determine program certificates to be awarded for user {username}. {exc}" LOGGER.exception(error_msg) - raise _retry_with_custom_exception(username=username, reason=error_msg, countdown=countdown) from exc + raise _retry_with_custom_exception( + username=username, reason=error_msg, countdown=countdown + ) from exc # For each completed program for which the student doesn't already have a # certificate, award one now. @@ -225,7 +235,9 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable # This logic is important, because we will retry the whole task if awarding any particular program cert fails. # # N.B. the list is sorted to facilitate deterministic ordering, e.g. for tests. - new_program_uuids = sorted(list(set(completed_programs.keys()) - set(awarded_and_skipped_program_uuids))) + new_program_uuids = sorted( + list(set(completed_programs.keys()) - set(awarded_and_skipped_program_uuids)) + ) if new_program_uuids: try: credentials_client = get_credentials_api_client( @@ -235,20 +247,28 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable 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 + raise _retry_with_custom_exception( + username=username, reason=error_msg, countdown=countdown + ) from exc failed_program_certificate_award_attempts = [] for program_uuid in new_program_uuids: visible_date = completed_programs[program_uuid] try: - LOGGER.info(f"Visible date for user {username} : program {program_uuid} is {visible_date}") - award_program_certificate(credentials_client, student, program_uuid, visible_date) - LOGGER.info(f"Awarded certificate for program {program_uuid} to user {username}") + LOGGER.info( + f"Visible date for user {username} : program {program_uuid} is {visible_date}" + ) + award_program_certificate( + credentials_client, student, program_uuid, visible_date + ) + LOGGER.info( + f"Awarded certificate for program {program_uuid} to user {username}" + ) except HTTPError as exc: if exc.response.status_code == 404: LOGGER.exception( - f"Certificate for program {program_uuid} could not be found. " + - f"Unable to award certificate to user {username}. The program might not be configured." + f"Certificate for program {program_uuid} could not be found. " + + f"Unable to award certificate to user {username}. The program might not be configured." ) elif exc.response.status_code == 429: rate_limit_countdown = 60 @@ -261,7 +281,7 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable raise _retry_with_custom_exception( username=username, reason=error_msg, - countdown=rate_limit_countdown + countdown=rate_limit_countdown, ) from exc else: LOGGER.exception( @@ -270,12 +290,16 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable ) except Exception: # pylint: disable=broad-except # keep trying to award other certs, but retry the whole task to fix any missing entries - LOGGER.exception(f"Failed to award certificate for program {program_uuid} to user {username}.") + LOGGER.exception( + f"Failed to award certificate for program {program_uuid} to user {username}." + ) failed_program_certificate_award_attempts.append(program_uuid) if failed_program_certificate_award_attempts: # N.B. This logic assumes that this task is idempotent - LOGGER.info(f"Retrying task to award failed certificates to user {username}") + LOGGER.info( + f"Retrying task to award failed certificates to user {username}" + ) # 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. @@ -283,14 +307,20 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable f"Failed to award certificate for user {username} " f"for programs {failed_program_certificate_award_attempts}" ) - raise _retry_with_custom_exception(username=username, reason=error_msg, countdown=countdown) + raise _retry_with_custom_exception( + username=username, reason=error_msg, countdown=countdown + ) else: LOGGER.info(f"User {username} is not eligible for any new program certificates") - LOGGER.info(f"Successfully completed the task award_program_certificates for username {username}") + LOGGER.info( + f"Successfully completed the task award_program_certificates for username {username}" + ) -def post_course_certificate_configuration(client, cert_config, certificate_available_date=None): +def post_course_certificate_configuration( + client, cert_config, certificate_available_date=None +): """ Make a POST request to the Credentials IDA's `course_certificates` endpoint (/api/v2/course_certificates/). This endpoint manages the course certificate configurations within the Credentials IDA. @@ -303,16 +333,18 @@ def post_course_certificate_configuration(client, cert_config, certificate_avail in the form of an ISO 8601 DateTime String. """ credentials_api_base_url = get_credentials_api_base_url() - credentials_api_url = urljoin(f"{credentials_api_base_url}/", "course_certificates/") + credentials_api_url = urljoin( + f"{credentials_api_base_url}/", "course_certificates/" + ) response = client.post( credentials_api_url, json={ - "course_id": cert_config['course_id'], - "certificate_type": cert_config['mode'], + "course_id": cert_config["course_id"], + "certificate_type": cert_config["mode"], "certificate_available_date": certificate_available_date, - "is_active": True - } + "is_active": True, + }, ) # Sometimes helpful error context is swallowed when calling `raise_for_status()`. We try to print out any additional @@ -328,7 +360,9 @@ def post_course_certificate_configuration(client, cert_config, certificate_avail response.raise_for_status() -def post_course_certificate(client, username, certificate, visible_date, date_override=None, org=None): +def post_course_certificate( + client, username, certificate, visible_date, date_override=None, org=None +): """ POST a certificate that has been updated to Credentials """ @@ -338,21 +372,22 @@ def post_course_certificate(client, username, certificate, visible_date, date_ov response = client.post( api_url, json={ - 'username': username, - 'status': 'awarded' if certificate.is_valid() else 'revoked', # Only need the two options at this time - 'credential': { - 'course_run_key': str(certificate.course_id), - 'mode': certificate.mode, - 'type': COURSE_CERTIFICATE, + "username": username, + "status": "awarded" + if certificate.is_valid() + else "revoked", # Only need the two options at this time + "credential": { + "course_run_key": str(certificate.course_id), + "mode": certificate.mode, + "type": COURSE_CERTIFICATE, }, - 'date_override': {'date': date_override.strftime(DATE_FORMAT)} if date_override else None, - 'attributes': [ - { - 'name': 'visible_date', - 'value': visible_date.strftime(DATE_FORMAT) - } - ] - } + "date_override": {"date": date_override.strftime(DATE_FORMAT)} + if date_override + else None, + "attributes": [ + {"name": "visible_date", "value": visible_date.strftime(DATE_FORMAT)} + ], + }, ) response.raise_for_status() @@ -361,9 +396,7 @@ def post_course_certificate(client, username, certificate, visible_date, date_ov @shared_task(bind=True, ignore_result=True) @set_code_owner_attribute def update_credentials_course_certificate_configuration_available_date( - self, - course_key, - certificate_available_date=None + self, course_key, certificate_available_date=None ): """ This task will update the CourseCertificate configuration's available date @@ -384,10 +417,14 @@ def update_credentials_course_certificate_configuration_available_date( course_key = str(course_key) 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] + modes = [ + mode.slug + for mode in course_modes + if mode.slug in CourseMode.CERTIFICATE_RELEVANT_MODES + ] if len(modes) != 1: LOGGER.exception( - f'Either course {course_key} has no certificate mode or multiple modes. Task failed.' + f"Either course {course_key} has no certificate mode or multiple modes. Task failed." ) return @@ -395,13 +432,13 @@ def update_credentials_course_certificate_configuration_available_date( User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME), ) cert_config = { - 'course_id': course_key, - 'mode': modes[0], + "course_id": course_key, + "mode": modes[0], } post_course_certificate_configuration( client=credentials_client, cert_config=cert_config, - certificate_available_date=certificate_available_date + certificate_available_date=certificate_available_date, ) @@ -421,34 +458,29 @@ def award_course_certificate(self, username, course_run_key): 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 - ) + return self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) LOGGER.info(f"Running task award_course_certificate for username {username}") - countdown = 2 ** self.request.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. if not CredentialsApiConfig.current().is_learner_issuance_enabled: - error_msg = ( - "Task award_course_certificate cannot be executed when credentials issuance is disabled in API config" - ) + 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 + countdown=countdown, ) try: @@ -456,14 +488,15 @@ def award_course_certificate(self, username, course_run_key): try: user = User.objects.get(username=username) except User.DoesNotExist: - LOGGER.exception(f"Task award_course_certificate was called with invalid username {username}") + LOGGER.exception( + f"Task award_course_certificate was called with invalid username {username}" + ) # Don't retry for this case - just conclude the task. 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 + user=user.id, course_id=course_key ) except GeneratedCertificate.DoesNotExist: LOGGER.exception( @@ -505,10 +538,17 @@ def award_course_certificate(self, username, course_run_key): date_override = None post_course_certificate( - credentials_client, username, certificate, visible_date, date_override, org=course_key.org + credentials_client, + username, + certificate, + visible_date, + date_override, + org=course_key.org, ) - LOGGER.info(f"Awarded certificate for course {course_key} to user {username}") + LOGGER.info( + f"Awarded certificate for course {course_key} to user {username}" + ) except Exception as exc: error_msg = f"Failed to determine course certificates to be awarded for user {username}." LOGGER.exception(error_msg) @@ -516,11 +556,13 @@ def award_course_certificate(self, username, course_run_key): username=username, course_run_key=course_run_key, reason=error_msg, - countdown=countdown + countdown=countdown, ) from exc -def get_revokable_program_uuids(course_specific_programs, student): +def get_revokable_program_uuids( + course_specific_programs: List[Dict], student: User +) -> List[str]: """ Get program uuids for which certificate to be revoked. @@ -532,14 +574,19 @@ def get_revokable_program_uuids(course_specific_programs, student): student (User): Representing the student whose programs to check for. Returns: - list if program UUIDs for which certificates to be revoked + list of program UUIDs for which certificates to be revoked + Raises: + HttpError, if the API call generated by get_certified_programs fails """ program_uuids_to_revoke = [] - existing_program_uuids = get_certified_programs(student) + # Get any programs where the user has already been rewarded a certificate + # Failed API calls with get_certified_programs should raise exceptions, + # because an empty response would dangerously imply a false negative. + existing_program_uuids = get_certified_programs(student, raise_on_error=True) for program in course_specific_programs: - if program['uuid'] in existing_program_uuids: - program_uuids_to_revoke.append(program['uuid']) + if program["uuid"] in existing_program_uuids: + program_uuids_to_revoke.append(program["uuid"]) return program_uuids_to_revoke @@ -561,20 +608,19 @@ def revoke_program_certificate(client, username, program_uuid): response = client.post( api_url, json={ - 'username': username, - 'status': 'revoked', - 'credential': { - 'type': PROGRAM_CERTIFICATE, - 'program_uuid': program_uuid - } - } + "username": username, + "status": "revoked", + "credential": {"type": PROGRAM_CERTIFICATE, "program_uuid": program_uuid}, + }, ) response.raise_for_status() @shared_task(bind=True, ignore_result=True) @set_code_owner_attribute -def revoke_program_certificates(self, username, course_key): # lint-amnesty, pylint: disable=too-many-statements +def revoke_program_certificates( + self, username, course_key +): # lint-amnesty, pylint: disable=too-many-statements """ This task is designed to be called whenever a student's course certificate is revoked. @@ -594,38 +640,36 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py 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 - ) + return self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) - countdown = 2 ** self.request.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. if not CredentialsApiConfig.current().is_learner_issuance_enabled: - error_msg = ( - "Task revoke_program_certificates cannot be executed when credentials issuance is disabled in API config" - ) + error_msg = "Task revoke_program_certificates cannot be executed when credentials issuance is disabled in API config" LOGGER.warning(error_msg) raise _retry_with_custom_exception( username=username, course_key=course_key, reason=error_msg, - countdown=countdown + countdown=countdown, ) try: student = User.objects.get(username=username) except User.DoesNotExist: - LOGGER.exception(f"Task revoke_program_certificates was called with invalid username {username}", username) + LOGGER.exception( + f"Task revoke_program_certificates was called with invalid username {username}", + username, + ) # Don't retry for this case - just conclude the task. return @@ -641,7 +685,9 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py return # Determine which program certificates the user has already been awarded, if any. - program_uuids_to_revoke = get_revokable_program_uuids(course_specific_programs, student) + program_uuids_to_revoke = get_revokable_program_uuids( + course_specific_programs, student + ) except Exception as exc: error_msg = ( f"Failed to determine program certificates to be revoked for user {username} " @@ -652,7 +698,7 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py username=username, course_key=course_key, reason=error_msg, - countdown=countdown + countdown=countdown, ) from exc if program_uuids_to_revoke: @@ -664,13 +710,17 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py 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 + raise _retry_with_custom_exception( + username, course_key, reason=exc, countdown=countdown + ) from exc failed_program_certificate_revoke_attempts = [] for program_uuid in program_uuids_to_revoke: try: revoke_program_certificate(credentials_client, username, program_uuid) - LOGGER.info(f"Revoked certificate for program {program_uuid} for user {username}") + LOGGER.info( + f"Revoked certificate for program {program_uuid} for user {username}" + ) except HTTPError as exc: if exc.response.status_code == 404: LOGGER.exception( @@ -689,7 +739,7 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py username, course_key, reason=error_msg, - countdown=rate_limit_countdown + countdown=rate_limit_countdown, ) from exc else: LOGGER.exception( @@ -697,12 +747,16 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py ) except Exception: # pylint: disable=broad-except # keep trying to revoke other certs, but retry the whole task to fix any missing entries - LOGGER.warning(f"Failed to revoke certificate for program {program_uuid} of user {username}.") + LOGGER.warning( + f"Failed to revoke certificate for program {program_uuid} of user {username}." + ) failed_program_certificate_revoke_attempts.append(program_uuid) if failed_program_certificate_revoke_attempts: # N.B. This logic assumes that this task is idempotent - LOGGER.info(f"Retrying task to revoke failed certificates to user {username}") + LOGGER.info( + f"Retrying task to revoke failed certificates to user {username}" + ) # 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. @@ -711,15 +765,14 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py f"for programs {failed_program_certificate_revoke_attempts}" ) raise _retry_with_custom_exception( - username, - course_key, - reason=error_msg, - countdown=countdown + username, course_key, reason=error_msg, countdown=countdown ) else: LOGGER.info(f"There is no program certificates for user {username} to revoke") - LOGGER.info(f"Successfully completed the task revoke_program_certificates for username {username}") + LOGGER.info( + f"Successfully completed the task revoke_program_certificates for username {username}" + ) @shared_task(bind=True, ignore_result=True) @@ -738,7 +791,7 @@ def update_certificate_visible_date_on_course_update(self, course_key): Arguments: course_key(str): The course identifier """ - countdown = 2 ** self.request.retries + 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 @@ -758,10 +811,9 @@ def update_certificate_visible_date_on_course_update(self, course_key): # 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. users_with_certificates_in_course = ( - GeneratedCertificate - .eligible_available_certificates - .filter(course_id=course_key) - .values_list('user__username', flat=True) + GeneratedCertificate.eligible_available_certificates.filter( + course_id=course_key + ).values_list("user__username", flat=True) ) LOGGER.info( @@ -786,7 +838,7 @@ def update_certificate_available_date_on_course_update(self, course_key): Args: course_key(str): The course identifier """ - countdown = 2 ** self.request.retries + 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 @@ -808,17 +860,17 @@ def update_certificate_available_date_on_course_update(self, course_key): # - 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 + 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) + str(course_key), str(course_overview.certificate_available_date) ) # OR, # - The course-run is self-paced, AND @@ -829,15 +881,17 @@ def update_certificate_available_date_on_course_update(self, course_key): # 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 + 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) + 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: LOGGER.warning( diff --git a/openedx/core/djangoapps/programs/tests/test_tasks.py b/openedx/core/djangoapps/programs/tests/test_tasks.py index 7e0db4fc6a..ba9c915aa1 100644 --- a/openedx/core/djangoapps/programs/tests/test_tasks.py +++ b/openedx/core/djangoapps/programs/tests/test_tasks.py @@ -19,23 +19,31 @@ from django.test import TestCase, override_settings from edx_rest_api_client.auth import SuppliedJwtAuth from requests.exceptions import HTTPError from testfixtures import LogCapture -from xmodule.data import CertificatesDisplayBehaviors 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 log = logging.getLogger(__name__) -CREDENTIALS_INTERNAL_SERVICE_URL = 'https://credentials.example.com' -TASKS_MODULE = 'openedx.core.djangoapps.programs.tasks' +CREDENTIALS_INTERNAL_SERVICE_URL = "https://credentials.example.com" +TASKS_MODULE = "openedx.core.djangoapps.programs.tasks" @skip_unless_lms @@ -49,84 +57,101 @@ class GetAwardedCertificateProgramsTestCase(TestCase): Helper to make dummy results from the credentials API """ result = { - 'id': 1, - 'username': 'dummy-username', - 'credential': { - 'credential_id': None, - 'program_uuid': None, + "id": 1, + "username": "dummy-username", + "credential": { + "credential_id": None, + "program_uuid": None, }, - 'status': 'dummy-status', - 'uuid': 'dummy-uuid', - 'certificate_url': 'http://credentials.edx.org/credentials/dummy-uuid/' + "status": "dummy-status", + "uuid": "dummy-uuid", + "certificate_url": "http://credentials.edx.org/credentials/dummy-uuid/", } result.update(**kwargs) return result - @mock.patch(TASKS_MODULE + '.get_credentials') + @mock.patch(TASKS_MODULE + ".get_credentials") def test_get_certified_programs(self, mock_get_credentials): """ Ensure the API is called and results handled correctly. """ - student = UserFactory(username='test-username') + student = UserFactory(username="test-username") mock_get_credentials.return_value = [ - self.make_credential_result(status='awarded', credential={'program_uuid': 1}), + self.make_credential_result( + status="awarded", credential={"program_uuid": 1} + ), ] result = tasks.get_certified_programs(student) assert mock_get_credentials.call_args[0] == (student,) - assert mock_get_credentials.call_args[1] == {'credential_type': 'program'} + assert mock_get_credentials.call_args[1] == {"credential_type": "program"} assert result == [1] + def test_data_retrieval_raise_on_error(self, mock_raise): + """ + Verify that when raise_on_error is set and the API call gives an error response, + get_certified_programs reraises HttpError. + """ + mock_raise.return_value = HTTPError("An Error occured") + student = UserFactory(username="test-username") + with self.assertRaises(HTTPError): + tasks.get_certified_programs(student, raise_on_error=True) + @skip_unless_lms class AwardProgramCertificateTestCase(TestCase): """ Test the award_program_certificate function """ + @httpretty.activate - @mock.patch('openedx.core.djangoapps.programs.tasks.get_credentials_api_base_url') + @mock.patch("openedx.core.djangoapps.programs.tasks.get_credentials_api_base_url") def test_award_program_certificate(self, mock_get_api_base_url): """ Ensure the correct API call gets made """ - mock_get_api_base_url.return_value = 'http://test-server/' - student = UserFactory(username='test-username', email='test-email@email.com') + mock_get_api_base_url.return_value = "http://test-server/" + student = UserFactory(username="test-username", email="test-email@email.com") test_client = requests.Session() - test_client.auth = SuppliedJwtAuth('test-token') + test_client.auth = SuppliedJwtAuth("test-token") httpretty.register_uri( httpretty.POST, - 'http://test-server/credentials/', + "http://test-server/credentials/", ) - tasks.award_program_certificate(test_client, student, 123, datetime(2010, 5, 30)) + tasks.award_program_certificate( + test_client, student, 123, datetime(2010, 5, 30) + ) expected_body = { - 'username': student.username, - 'lms_user_id': student.id, - 'credential': { - 'program_uuid': 123, - 'type': tasks.PROGRAM_CERTIFICATE, + "username": student.username, + "lms_user_id": student.id, + "credential": { + "program_uuid": 123, + "type": tasks.PROGRAM_CERTIFICATE, }, - 'attributes': [ + "attributes": [ { - 'name': 'visible_date', - 'value': '2010-05-30T00:00:00Z', + "name": "visible_date", + "value": "2010-05-30T00:00:00Z", } - ] + ], } - last_request_body = httpretty.last_request().body.decode('utf-8') + last_request_body = httpretty.last_request().body.decode("utf-8") assert json.loads(last_request_body) == expected_body @skip_unless_lms @ddt.ddt -@mock.patch(TASKS_MODULE + '.award_program_certificate') -@mock.patch(TASKS_MODULE + '.get_certified_programs') -@mock.patch(TASKS_MODULE + '.get_completed_programs') -@override_settings(CREDENTIALS_SERVICE_USERNAME='test-service-username') -class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiConfigMixin, TestCase): +@mock.patch(TASKS_MODULE + ".award_program_certificate") +@mock.patch(TASKS_MODULE + ".get_certified_programs") +@mock.patch(TASKS_MODULE + ".get_completed_programs") +@override_settings(CREDENTIALS_SERVICE_USERNAME="test-service-username") +class AwardProgramCertificatesTestCase( + CatalogIntegrationMixin, CredentialsApiConfigMixin, TestCase +): """ Tests for the 'award_program_certificates' celery task. """ @@ -134,11 +159,11 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo def setUp(self): super().setUp() self.create_credentials_config() - self.student = UserFactory.create(username='test-student') + self.student = UserFactory.create(username="test-student") self.site = SiteFactory() self.site_configuration = SiteConfigurationFactory(site=self.site) self.catalog_integration = self.create_catalog_integration() - ApplicationFactory.create(name='credentials') + ApplicationFactory.create(name="credentials") UserFactory.create(username=settings.CREDENTIALS_SERVICE_USERNAME) def test_completion_check( @@ -177,20 +202,26 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo tasks.award_program_certificates.delay(self.student.username).get() - actual_program_uuids = [call[0][2] for call in mock_award_program_certificate.call_args_list] + actual_program_uuids = [ + call[0][2] for call in mock_award_program_certificate.call_args_list + ] assert actual_program_uuids == expected_awarded_program_uuids - actual_visible_dates = [call[0][3] for call in mock_award_program_certificate.call_args_list] + actual_visible_dates = [ + call[0][3] for call in mock_award_program_certificate.call_args_list + ] assert actual_visible_dates == expected_awarded_program_uuids # program uuids are same as mock dates - @mock.patch('openedx.core.djangoapps.site_configuration.helpers.get_current_site_configuration') + @mock.patch( + "openedx.core.djangoapps.site_configuration.helpers.get_current_site_configuration" + ) def test_awarding_certs_with_skip_program_certificate( - self, - mocked_get_current_site_configuration, - mock_get_completed_programs, - mock_get_certified_programs, - mock_award_program_certificate, + self, + mocked_get_current_site_configuration, + mock_get_completed_programs, + mock_get_certified_programs, + mock_award_program_certificate, ): """ Checks that the Credentials API is used to award certificates for @@ -204,9 +235,7 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo mock_get_certified_programs.return_value = [1] # programs to be skipped - self.site_configuration.site_values = { - "programs_without_certificates": [2] - } + self.site_configuration.site_values = {"programs_without_certificates": [2]} self.site_configuration.save() mocked_get_current_site_configuration.return_value = self.site_configuration @@ -215,28 +244,31 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo expected_awarded_program_uuids = [3, 4] tasks.award_program_certificates.delay(self.student.username).get() - actual_program_uuids = [call[0][2] for call in mock_award_program_certificate.call_args_list] + actual_program_uuids = [ + call[0][2] for call in mock_award_program_certificate.call_args_list + ] assert actual_program_uuids == expected_awarded_program_uuids - actual_visible_dates = [call[0][3] for call in mock_award_program_certificate.call_args_list] + actual_visible_dates = [ + call[0][3] for call in mock_award_program_certificate.call_args_list + ] assert actual_visible_dates == expected_awarded_program_uuids # program uuids are same as mock dates @ddt.data( - ('credentials', 'enable_learner_issuance'), + ("credentials", "enable_learner_issuance"), ) @ddt.unpack def test_retry_if_config_disabled( - self, - disabled_config_type, - disabled_config_attribute, - *mock_helpers + self, disabled_config_type, disabled_config_attribute, *mock_helpers ): """ Checks that the task is aborted if any relevant api configs are disabled. """ - getattr(self, f'create_{disabled_config_type}_config')(**{disabled_config_attribute: False}) - with mock.patch(TASKS_MODULE + '.LOGGER.warning') as mock_warning: + getattr(self, f"create_{disabled_config_type}_config")( + **{disabled_config_attribute: False} + ) + with mock.patch(TASKS_MODULE + ".LOGGER.warning") as mock_warning: with pytest.raises(MaxRetriesExceededError): tasks.award_program_certificates.delay(self.student.username).get() assert mock_warning.called @@ -248,8 +280,8 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo Checks that the task will be aborted and not retried if the username passed was not found, and that an exception is logged. """ - with mock.patch(TASKS_MODULE + '.LOGGER.exception') as mock_exception: - tasks.award_program_certificates.delay('nonexistent-username').get() + with mock.patch(TASKS_MODULE + ".LOGGER.exception") as mock_exception: + tasks.award_program_certificates.delay("nonexistent-username").get() assert mock_exception.called for mock_helper in mock_helpers: assert not mock_helper.called @@ -270,13 +302,13 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo assert not mock_get_certified_programs.called assert not mock_award_program_certificate.called - @mock.patch('openedx.core.djangoapps.site_configuration.helpers.get_value') + @mock.patch("openedx.core.djangoapps.site_configuration.helpers.get_value") def test_programs_without_certificates( self, mock_get_value, mock_get_completed_programs, mock_get_certified_programs, - mock_award_program_certificate + mock_award_program_certificate, ): """ Checks that the task will be aborted without further action if there exists a list @@ -289,22 +321,22 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo assert not mock_get_certified_programs.called assert not mock_award_program_certificate.called - @mock.patch(TASKS_MODULE + '.get_credentials_api_client') + @mock.patch(TASKS_MODULE + ".get_credentials_api_client") def test_failure_to_create_api_client_retries( self, mock_get_api_client, mock_get_completed_programs, mock_get_certified_programs, - mock_award_program_certificate + mock_award_program_certificate, ): """ Checks that we log an exception and retry if the API client isn't creating. """ - mock_get_api_client.side_effect = Exception('boom') + mock_get_api_client.side_effect = Exception("boom") mock_get_completed_programs.return_value = {1: 1, 2: 2} mock_get_certified_programs.return_value = [2] - with mock.patch(TASKS_MODULE + '.LOGGER.exception') as mock_exception: + with mock.patch(TASKS_MODULE + ".LOGGER.exception") as mock_exception: with pytest.raises(MaxRetriesExceededError): tasks.award_program_certificates.delay(self.student.username).get() @@ -347,25 +379,30 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo """ mock_get_completed_programs.return_value = {1: 1, 2: 2} mock_get_certified_programs.side_effect = [[], [2]] - mock_award_program_certificate.side_effect = self._make_side_effect([Exception('boom'), None]) + mock_award_program_certificate.side_effect = self._make_side_effect( + [Exception("boom"), None] + ) - with mock.patch(TASKS_MODULE + '.LOGGER.info') as mock_info, \ - mock.patch(TASKS_MODULE + '.LOGGER.exception') as mock_warning: + with mock.patch(TASKS_MODULE + ".LOGGER.info") as mock_info, mock.patch( + TASKS_MODULE + ".LOGGER.exception" + ) as mock_warning: tasks.award_program_certificates.delay(self.student.username).get() assert mock_award_program_certificate.call_count == 3 mock_warning.assert_called_once_with( - 'Failed to award certificate for program {uuid} to user {username}.'.format( - uuid=1, - username=self.student.username) + "Failed to award certificate for program {uuid} to user {username}.".format( + uuid=1, username=self.student.username + ) + ) + mock_info.assert_any_call( + f"Awarded certificate for program {1} to user {self.student.username}" + ) + mock_info.assert_any_call( + f"Awarded certificate for program {2} to user {self.student.username}" ) - mock_info.assert_any_call(f"Awarded certificate for program {1} to user {self.student.username}") - mock_info.assert_any_call(f"Awarded certificate for program {2} to user {self.student.username}") def test_retry_on_programs_api_errors( - self, - mock_get_completed_programs, - *_mock_helpers + self, mock_get_completed_programs, *_mock_helpers ): """ Ensures that any otherwise-unhandled errors that arise while trying @@ -373,7 +410,9 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo transient API errors) will cause the task to be failed and queued for retry. """ - mock_get_completed_programs.side_effect = self._make_side_effect([Exception('boom'), None]) + mock_get_completed_programs.side_effect = self._make_side_effect( + [Exception("boom"), None] + ) tasks.award_program_certificates.delay(self.student.username).get() assert mock_get_completed_programs.call_count == 3 @@ -391,7 +430,9 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo """ mock_get_completed_programs.return_value = {1: 1, 2: 2} mock_get_certified_programs.return_value = [1] - mock_get_certified_programs.side_effect = self._make_side_effect([Exception('boom'), None]) + mock_get_certified_programs.side_effect = self._make_side_effect( + [Exception("boom"), None] + ) tasks.award_program_certificates.delay(self.student.username).get() assert mock_get_certified_programs.call_count == 2 assert mock_award_program_certificate.call_count == 1 @@ -464,59 +505,68 @@ class PostCourseCertificateTestCase(TestCase): """ def setUp(self): # lint-amnesty, pylint: disable=super-method-not-called - self.student = UserFactory.create(username='test-student') + self.student = UserFactory.create(username="test-student") self.course = CourseOverviewFactory.create( self_paced=True # Any option to allow the certificate to be viewable for the course ) self.certificate = GeneratedCertificateFactory( user=self.student, - mode='verified', + mode="verified", course_id=self.course.id, - status='downloadable' + status="downloadable", ) @httpretty.activate - @mock.patch('openedx.core.djangoapps.programs.tasks.get_credentials_api_base_url') + @mock.patch("openedx.core.djangoapps.programs.tasks.get_credentials_api_base_url") def test_post_course_certificate(self, mock_get_api_base_url): """ Ensure the correct API call gets made """ - mock_get_api_base_url.return_value = 'http://test-server/' + mock_get_api_base_url.return_value = "http://test-server/" test_client = requests.Session() - test_client.auth = SuppliedJwtAuth('test-token') + test_client.auth = SuppliedJwtAuth("test-token") httpretty.register_uri( httpretty.POST, - 'http://test-server/credentials/', + "http://test-server/credentials/", ) visible_date = datetime.now() - tasks.post_course_certificate(test_client, self.student.username, self.certificate, visible_date) + tasks.post_course_certificate( + test_client, self.student.username, self.certificate, visible_date + ) expected_body = { - 'username': self.student.username, - 'status': 'awarded', - 'credential': { - 'course_run_key': str(self.certificate.course_id), - 'mode': self.certificate.mode, - 'type': tasks.COURSE_CERTIFICATE, + "username": self.student.username, + "status": "awarded", + "credential": { + "course_run_key": str(self.certificate.course_id), + "mode": self.certificate.mode, + "type": tasks.COURSE_CERTIFICATE, }, - 'date_override': None, - 'attributes': [{ - 'name': 'visible_date', - 'value': visible_date.strftime('%Y-%m-%dT%H:%M:%SZ') # text representation of date - }] + "date_override": None, + "attributes": [ + { + "name": "visible_date", + "value": visible_date.strftime( + "%Y-%m-%dT%H:%M:%SZ" + ), # text representation of date + } + ], } - last_request_body = httpretty.last_request().body.decode('utf-8') + last_request_body = httpretty.last_request().body.decode("utf-8") assert json.loads(last_request_body) == expected_body @skip_unless_lms @ddt.ddt -@mock.patch("lms.djangoapps.certificates.api.auto_certificate_generation_enabled", mock.Mock(return_value=True)) -@mock.patch(TASKS_MODULE + '.post_course_certificate') -@override_settings(CREDENTIALS_SERVICE_USERNAME='test-service-username') +@mock.patch( + "lms.djangoapps.certificates.api.auto_certificate_generation_enabled", + mock.Mock(return_value=True), +) +@mock.patch(TASKS_MODULE + ".post_course_certificate") +@override_settings(CREDENTIALS_SERVICE_USERNAME="test-service-username") class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase): """ Test the award_course_certificate celery task @@ -529,21 +579,21 @@ class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase): self.course = CourseOverviewFactory.create( self_paced=True, # Any option to allow the certificate to be viewable for the course certificate_available_date=self.available_date, - certificates_display_behavior=CertificatesDisplayBehaviors.END_WITH_DATE + certificates_display_behavior=CertificatesDisplayBehaviors.END_WITH_DATE, ) - self.student = UserFactory.create(username='test-student') + self.student = UserFactory.create(username="test-student") # Instantiate the Certificate first so that the config doesn't execute issuance self.certificate = GeneratedCertificateFactory.create( user=self.student, - mode='verified', + mode="verified", course_id=self.course.id, - status='downloadable' + status="downloadable", ) self.create_credentials_config() self.site = SiteFactory() - ApplicationFactory.create(name='credentials') + ApplicationFactory.create(name="credentials") UserFactory.create(username=settings.CREDENTIALS_SERVICE_USERNAME) def _add_certificate_date_override(self): @@ -552,12 +602,12 @@ class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase): """ self.certificate.date_override = CertificateDateOverrideFactory.create( generated_certificate=self.certificate, - overridden_by=UserFactory.create(username='test-admin'), + overridden_by=UserFactory.create(username="test-admin"), ) @ddt.data( - 'verified', - 'no-id-professional', + "verified", + "no-id-professional", ) def test_award_course_certificates(self, mode, mock_post_course_certificate): """ @@ -565,89 +615,119 @@ class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase): """ self.certificate.mode = mode self.certificate.save() - tasks.award_course_certificate.delay(self.student.username, str(self.course.id)).get() + tasks.award_course_certificate.delay( + self.student.username, str(self.course.id) + ).get() call_args, _ = mock_post_course_certificate.call_args assert call_args[1] == self.student.username assert call_args[2] == self.certificate assert call_args[3] == self.certificate.modified_date - def test_award_course_certificates_available_date(self, mock_post_course_certificate): + def test_award_course_certificates_available_date( + self, mock_post_course_certificate + ): """ Tests the API POST method is called with available date when the course is not self paced """ self.course.self_paced = False self.course.save() - tasks.award_course_certificate.delay(self.student.username, str(self.course.id)).get() + tasks.award_course_certificate.delay( + self.student.username, str(self.course.id) + ).get() call_args, _ = mock_post_course_certificate.call_args assert call_args[1] == self.student.username assert call_args[2] == self.certificate assert call_args[3] == self.available_date - def test_award_course_certificates_override_date(self, mock_post_course_certificate): + def test_award_course_certificates_override_date( + self, mock_post_course_certificate + ): """ Tests the API POST method is called with date override when present """ self._add_certificate_date_override() - tasks.award_course_certificate.delay(self.student.username, str(self.course.id)).get() + tasks.award_course_certificate.delay( + self.student.username, str(self.course.id) + ).get() call_args, _ = mock_post_course_certificate.call_args assert call_args[1] == self.student.username assert call_args[2] == self.certificate assert call_args[3] == self.certificate.modified_date assert call_args[4] == self.certificate.date_override.date - def test_award_course_cert_not_called_if_disabled(self, mock_post_course_certificate): + def test_award_course_cert_not_called_if_disabled( + self, mock_post_course_certificate + ): """ Test that the post method is never called if the config is disabled """ self.create_credentials_config(enabled=False) - with mock.patch(TASKS_MODULE + '.LOGGER.warning') as mock_warning: + with mock.patch(TASKS_MODULE + ".LOGGER.warning") as mock_warning: with pytest.raises(MaxRetriesExceededError): - tasks.award_course_certificate.delay(self.student.username, str(self.course.id)).get() + tasks.award_course_certificate.delay( + self.student.username, str(self.course.id) + ).get() assert mock_warning.called assert not mock_post_course_certificate.called - def test_award_course_cert_not_called_if_user_not_found(self, mock_post_course_certificate): + def test_award_course_cert_not_called_if_user_not_found( + self, mock_post_course_certificate + ): """ Test that the post method is never called if the user isn't found by username """ - with mock.patch(TASKS_MODULE + '.LOGGER.exception') as mock_exception: + with mock.patch(TASKS_MODULE + ".LOGGER.exception") as mock_exception: # Use a random username here since this user won't be found in the DB - tasks.award_course_certificate.delay('random_username', str(self.course.id)).get() + tasks.award_course_certificate.delay( + "random_username", str(self.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): + 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 """ self.certificate.delete() - with mock.patch(TASKS_MODULE + '.LOGGER.exception') as mock_exception: - tasks.award_course_certificate.delay(self.student.username, str(self.course.id)).get() + with mock.patch(TASKS_MODULE + ".LOGGER.exception") as mock_exception: + tasks.award_course_certificate.delay( + self.student.username, str(self.course.id) + ).get() 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_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.exception') as mock_exception: + with mock.patch(TASKS_MODULE + ".LOGGER.exception") 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() + 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_certificated_not_verified_mode(self, mock_post_course_certificate): + def test_award_course_cert_not_called_if_certificated_not_verified_mode( + self, mock_post_course_certificate + ): """ Test that the post method is never called if the GeneratedCertificate is an 'audit' cert """ # Temporarily disable the config so the signal isn't handled from .save self.create_credentials_config(enabled=False) - self.certificate.mode = 'audit' + self.certificate.mode = "audit" self.certificate.save() self.create_credentials_config() - tasks.award_course_certificate.delay(self.student.username, str(self.certificate.course_id)).get() + tasks.award_course_certificate.delay( + self.student.username, str(self.certificate.course_id) + ).get() assert not mock_post_course_certificate.called @@ -658,42 +738,44 @@ class RevokeProgramCertificateTestCase(TestCase): """ @httpretty.activate - @mock.patch('openedx.core.djangoapps.programs.tasks.get_credentials_api_base_url') + @mock.patch("openedx.core.djangoapps.programs.tasks.get_credentials_api_base_url") def test_revoke_program_certificate(self, mock_get_api_base_url): """ Ensure the correct API call gets made """ - mock_get_api_base_url.return_value = 'http://test-server/' - test_username = 'test-username' + mock_get_api_base_url.return_value = "http://test-server/" + test_username = "test-username" test_client = requests.Session() - test_client.auth = SuppliedJwtAuth('test-token') + test_client.auth = SuppliedJwtAuth("test-token") httpretty.register_uri( httpretty.POST, - 'http://test-server/credentials/', + "http://test-server/credentials/", ) tasks.revoke_program_certificate(test_client, test_username, 123) expected_body = { - 'username': test_username, - 'status': 'revoked', - 'credential': { - 'program_uuid': 123, - 'type': tasks.PROGRAM_CERTIFICATE, - } + "username": test_username, + "status": "revoked", + "credential": { + "program_uuid": 123, + "type": tasks.PROGRAM_CERTIFICATE, + }, } - last_request_body = httpretty.last_request().body.decode('utf-8') + last_request_body = httpretty.last_request().body.decode("utf-8") assert json.loads(last_request_body) == expected_body @skip_unless_lms @ddt.ddt -@mock.patch(TASKS_MODULE + '.revoke_program_certificate') -@mock.patch(TASKS_MODULE + '.get_certified_programs') -@mock.patch(TASKS_MODULE + '.get_inverted_programs') -@override_settings(CREDENTIALS_SERVICE_USERNAME='test-service-username') -class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiConfigMixin, TestCase): +@mock.patch(TASKS_MODULE + ".revoke_program_certificate") +@mock.patch(TASKS_MODULE + ".get_certified_programs") +@mock.patch(TASKS_MODULE + ".get_inverted_programs") +@override_settings(CREDENTIALS_SERVICE_USERNAME="test-service-username") +class RevokeProgramCertificatesTestCase( + CatalogIntegrationMixin, CredentialsApiConfigMixin, TestCase +): """ Tests for the 'revoke_program_certificates' celery task. """ @@ -701,15 +783,15 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC def setUp(self): super().setUp() - self.student = UserFactory.create(username='test-student') - self.course_key = 'course-v1:testX+test101+2T2020' + self.student = UserFactory.create(username="test-student") + self.course_key = "course-v1:testX+test101+2T2020" self.site = SiteFactory() self.site_configuration = SiteConfigurationFactory(site=self.site) - ApplicationFactory.create(name='credentials') + ApplicationFactory.create(name="credentials") UserFactory.create(username=settings.CREDENTIALS_SERVICE_USERNAME) self.create_credentials_config() - self.inverted_programs = {self.course_key: [{'uuid': 1}, {'uuid': 2}]} + self.inverted_programs = {self.course_key: [{"uuid": 1}, {"uuid": 2}]} def _make_side_effect(self, side_effects): """ @@ -742,7 +824,9 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC Checks that the Programs API is used correctly to determine completed programs. """ - tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + tasks.revoke_program_certificates.delay( + self.student.username, self.course_key + ).get() mock_get_inverted_programs.assert_any_call(self.student) def test_revoke_program_certificate( @@ -757,34 +841,37 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC """ expected_program_uuid = 1 mock_get_inverted_programs.return_value = { - self.course_key: [{'uuid': expected_program_uuid}] + self.course_key: [{"uuid": expected_program_uuid}] } mock_get_certified_programs.return_value = [expected_program_uuid] - tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + tasks.revoke_program_certificates.delay( + self.student.username, self.course_key + ).get() call_args, _ = mock_revoke_program_certificate.call_args assert call_args[1] == self.student.username assert call_args[2] == expected_program_uuid @ddt.data( - ('credentials', 'enable_learner_issuance'), + ("credentials", "enable_learner_issuance"), ) @ddt.unpack def test_retry_if_config_disabled( - self, - disabled_config_type, - disabled_config_attribute, - *mock_helpers + self, disabled_config_type, disabled_config_attribute, *mock_helpers ): """ Checks that the task is aborted if any relevant api configs are disabled. """ - getattr(self, f'create_{disabled_config_type}_config')(**{disabled_config_attribute: False}) - with mock.patch(TASKS_MODULE + '.LOGGER.warning') as mock_warning: + getattr(self, f"create_{disabled_config_type}_config")( + **{disabled_config_attribute: False} + ) + with mock.patch(TASKS_MODULE + ".LOGGER.warning") as mock_warning: with pytest.raises(MaxRetriesExceededError): - tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + tasks.revoke_program_certificates.delay( + self.student.username, self.course_key + ).get() assert mock_warning.called for mock_helper in mock_helpers: assert not mock_helper.called @@ -794,8 +881,10 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC Checks that the task will be aborted and not retried if the username passed was not found, and that an exception is logged. """ - with mock.patch(TASKS_MODULE + '.LOGGER.exception') as mock_exception: - tasks.revoke_program_certificates.delay('nonexistent-username', self.course_key).get() + with mock.patch(TASKS_MODULE + ".LOGGER.exception") as mock_exception: + tasks.revoke_program_certificates.delay( + "nonexistent-username", self.course_key + ).get() assert mock_exception.called for mock_helper in mock_helpers: assert not mock_helper.called @@ -811,7 +900,9 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC not part of any program. """ mock_get_inverted_programs.return_value = {} - tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + tasks.revoke_program_certificates.delay( + self.student.username, self.course_key + ).get() assert mock_get_inverted_programs.called assert not mock_get_certified_programs.called assert not mock_revoke_program_certificate.called @@ -830,20 +921,29 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC """ mock_get_inverted_programs.return_value = self.inverted_programs mock_get_certified_programs.side_effect = [[1], [1, 2]] - mock_revoke_program_certificate.side_effect = self._make_side_effect([Exception('boom'), None]) + mock_revoke_program_certificate.side_effect = self._make_side_effect( + [Exception("boom"), None] + ) - with mock.patch(TASKS_MODULE + '.LOGGER.info') as mock_info, \ - mock.patch(TASKS_MODULE + '.LOGGER.warning') as mock_warning: - tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + with mock.patch(TASKS_MODULE + ".LOGGER.info") as mock_info, mock.patch( + TASKS_MODULE + ".LOGGER.warning" + ) as mock_warning: + tasks.revoke_program_certificates.delay( + self.student.username, self.course_key + ).get() assert mock_revoke_program_certificate.call_count == 3 mock_warning.assert_called_once_with( - 'Failed to revoke certificate for program {uuid} of user {username}.'.format( - uuid=1, - username=self.student.username) + "Failed to revoke certificate for program {uuid} of user {username}.".format( + uuid=1, username=self.student.username + ) + ) + mock_info.assert_any_call( + f"Revoked certificate for program {1} for user {self.student.username}" + ) + mock_info.assert_any_call( + f"Revoked certificate for program {2} for user {self.student.username}" ) - mock_info.assert_any_call(f"Revoked certificate for program {1} for user {self.student.username}") - mock_info.assert_any_call(f"Revoked certificate for program {2} for user {self.student.username}") def test_retry_on_credentials_api_errors( self, @@ -859,8 +959,12 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC """ mock_get_inverted_programs.return_value = self.inverted_programs mock_get_certified_programs.return_value = [1] - mock_get_certified_programs.side_effect = self._make_side_effect([Exception('boom'), None]) - tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + mock_get_certified_programs.side_effect = self._make_side_effect( + [Exception("boom"), None] + ) + tasks.revoke_program_certificates.delay( + self.student.username, self.course_key + ).get() assert mock_get_certified_programs.call_count == 2 assert mock_revoke_program_certificate.call_count == 1 @@ -881,7 +985,9 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC [exception, None] ) - tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + tasks.revoke_program_certificates.delay( + self.student.username, self.course_key + ).get() assert mock_revoke_program_certificate.call_count == 3 @@ -902,7 +1008,9 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC [exception, None] ) - tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + tasks.revoke_program_certificates.delay( + self.student.username, self.course_key + ).get() assert mock_revoke_program_certificate.call_count == 2 @@ -923,7 +1031,9 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC [exception, None] ) - tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + tasks.revoke_program_certificates.delay( + self.student.username, self.course_key + ).get() assert mock_revoke_program_certificate.call_count == 2 @@ -942,47 +1052,52 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC with mock.patch( TASKS_MODULE + ".get_credentials_api_client" ) as mock_get_api_client, mock.patch( - TASKS_MODULE + '.LOGGER.exception' + TASKS_MODULE + ".LOGGER.exception" ) as mock_exception: mock_get_api_client.side_effect = Exception("boom") with pytest.raises(MaxRetriesExceededError): - tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() + 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 not mock_revoke_program_certificate.called @skip_unless_lms -@override_settings(CREDENTIALS_SERVICE_USERNAME='test-service-username') +@override_settings(CREDENTIALS_SERVICE_USERNAME="test-service-username") class UpdateCredentialsCourseCertificateConfigurationAvailableDateTestCase(TestCase): """ Tests for the update_credentials_course_certificate_configuration_available_date function """ + def setUp(self): super().setUp() self.course = CourseOverviewFactory.create( - certificate_available_date=datetime.now().strftime('%Y-%m-%dT%H:%M:%SZ') + certificate_available_date=datetime.now().strftime("%Y-%m-%dT%H:%M:%SZ") ) - CourseModeFactory.create(course_id=self.course.id, mode_slug='verified') - CourseModeFactory.create(course_id=self.course.id, mode_slug='audit') + CourseModeFactory.create(course_id=self.course.id, mode_slug="verified") + CourseModeFactory.create(course_id=self.course.id, mode_slug="audit") self.available_date = self.course.certificate_available_date self.course_id = self.course.id - self.credentials_worker = UserFactory(username='test-service-username') + self.credentials_worker = UserFactory(username="test-service-username") def test_update_course_cert_available_date(self): - with mock.patch(TASKS_MODULE + '.post_course_certificate_configuration') as update_posted: + with mock.patch( + TASKS_MODULE + ".post_course_certificate_configuration" + ) as update_posted: tasks.update_credentials_course_certificate_configuration_available_date( - self.course_id, - self.available_date + self.course_id, self.available_date ) update_posted.assert_called_once() def test_course_with_two_paid_modes(self): - CourseModeFactory.create(course_id=self.course.id, mode_slug='professional') - with mock.patch(TASKS_MODULE + '.post_course_certificate_configuration') as update_posted: + CourseModeFactory.create(course_id=self.course.id, mode_slug="professional") + with mock.patch( + TASKS_MODULE + ".post_course_certificate_configuration" + ) as update_posted: tasks.update_credentials_course_certificate_configuration_available_date( - self.course_id, - self.available_date + self.course_id, self.available_date ) update_posted.assert_not_called() @@ -992,74 +1107,80 @@ class PostCourseCertificateConfigurationTestCase(TestCase): """ Test the post_course_certificate_configuration function """ + def setUp(self): super().setUp() self.certificate = { - 'mode': 'verified', - 'course_id': 'testCourse', + "mode": "verified", + "course_id": "testCourse", } @httpretty.activate - @mock.patch('openedx.core.djangoapps.programs.tasks.get_credentials_api_base_url') + @mock.patch("openedx.core.djangoapps.programs.tasks.get_credentials_api_base_url") def test_post_course_certificate_configuration(self, mock_get_api_base_url): """ Ensure the correct API call gets made """ - mock_get_api_base_url.return_value = 'http://test-server/' + mock_get_api_base_url.return_value = "http://test-server/" test_client = requests.Session() - test_client.auth = SuppliedJwtAuth('test-token') + test_client.auth = SuppliedJwtAuth("test-token") httpretty.register_uri( httpretty.POST, - 'http://test-server/course_certificates/', + "http://test-server/course_certificates/", ) - available_date = datetime.now().strftime('%Y-%m-%dT%H:%M:%SZ') + available_date = datetime.now().strftime("%Y-%m-%dT%H:%M:%SZ") - tasks.post_course_certificate_configuration(test_client, self.certificate, available_date) + tasks.post_course_certificate_configuration( + test_client, self.certificate, available_date + ) expected_body = { - "course_id": 'testCourse', - "certificate_type": 'verified', + "course_id": "testCourse", + "certificate_type": "verified", "certificate_available_date": available_date, - "is_active": True + "is_active": True, } - last_request_body = httpretty.last_request().body.decode('utf-8') + last_request_body = httpretty.last_request().body.decode("utf-8") assert json.loads(last_request_body) == expected_body @skip_unless_lms -class UpdateCertificateVisibleDatesOnCourseUpdateTestCase(CredentialsApiConfigMixin, TestCase): +class UpdateCertificateVisibleDatesOnCourseUpdateTestCase( + CredentialsApiConfigMixin, TestCase +): """ Tests for the `update_certificate_visible_date_on_course_update` task. """ + def setUp(self): super().setUp() self.credentials_api_config = self.create_credentials_config(enabled=False) # setup course self.course = CourseOverviewFactory.create() # setup users - self.student1 = UserFactory.create(username='test-student1') - self.student2 = UserFactory.create(username='test-student2') - self.student3 = UserFactory.create(username='test-student3') + self.student1 = UserFactory.create(username="test-student1") + self.student2 = UserFactory.create(username="test-student2") + self.student3 = UserFactory.create(username="test-student3") # award certificates to users in course we created self.certificate_student1 = GeneratedCertificateFactory.create( user=self.student1, - mode='verified', + mode="verified", course_id=self.course.id, - status='downloadable' + status="downloadable", ) self.certificate_student2 = GeneratedCertificateFactory.create( user=self.student2, - mode='verified', + mode="verified", course_id=self.course.id, - status='downloadable' + status="downloadable", ) self.certificate_student3 = GeneratedCertificateFactory.create( user=self.student3, - mode='verified', + mode="verified", course_id=self.course.id, - status='downloadable' + status="downloadable", ) def tearDown(self): @@ -1075,7 +1196,9 @@ class UpdateCertificateVisibleDatesOnCourseUpdateTestCase(CredentialsApiConfigMi 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 + tasks.update_certificate_visible_date_on_course_update( + self.course.id + ) # pylint: disable=no-value-for-parameter def test_update_visible_dates(self): """ @@ -1089,17 +1212,24 @@ class UpdateCertificateVisibleDatesOnCourseUpdateTestCase(CredentialsApiConfigMi self.credentials_api_config.enabled = True 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 + 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 assert award_course_cert.call_count == 3 @skip_unless_lms -class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigMixin, TestCase): +class UpdateCertificateAvailableDateOnCourseUpdateTestCase( + CredentialsApiConfigMixin, TestCase +): """ Tests for the `update_certificate_available_date_on_course_update` task. """ + def setUp(self): super().setUp() self.credentials_api_config = self.create_credentials_config(enabled=False) @@ -1119,7 +1249,9 @@ class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigM course = CourseOverviewFactory.create() 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.id + ) # pylint: disable=no-value-for-parameter def test_update_certificate_available_date_with_self_paced_course(self): """ @@ -1133,16 +1265,19 @@ class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigM self.credentials_api_config.enable_learner_issuance = True course = CourseOverviewFactory.create( - self_paced=True, - certificate_available_date=None + 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 + 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) + update_credentials_course_cert_config.assert_called_once_with( + str(course.id), None + ) def test_update_certificate_available_date_with_instructor_paced_course(self): """ @@ -1160,15 +1295,19 @@ class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigM course = CourseOverviewFactory.create( self_paced=False, certificate_available_date=available_date, - certificates_display_behavior=CertificatesDisplayBehaviors.END_WITH_DATE + certificates_display_behavior=CertificatesDisplayBehaviors.END_WITH_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.id + ) # pylint: disable=no-value-for-parameter - update_credentials_course_cert_config.assert_called_once_with(str(course.id), str(available_date)) + 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): """ @@ -1183,7 +1322,7 @@ class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigM course = CourseOverviewFactory.create( self_paced=False, certificate_available_date=available_date, - certificates_display_behavior=CertificatesDisplayBehaviors.EARLY_NO_INFO + certificates_display_behavior=CertificatesDisplayBehaviors.EARLY_NO_INFO, ) expected_message = ( @@ -1192,7 +1331,9 @@ class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigM ) 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( + course.id + ) # pylint: disable=no-value-for-parameter assert len(log_capture.records) == 1 assert log_capture.records[0].getMessage() == expected_message diff --git a/openedx/core/lib/edx_api_utils.py b/openedx/core/lib/edx_api_utils.py index b34780f78b..90c45ce17e 100644 --- a/openedx/core/lib/edx_api_utils.py +++ b/openedx/core/lib/edx_api_utils.py @@ -2,12 +2,16 @@ import logging +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union from urllib.parse import urljoin from django.core.cache import cache from openedx.core.lib.cache_utils import zpickle, zunpickle +if TYPE_CHECKING: + from requests import session + log = logging.getLogger(__name__) @@ -19,9 +23,20 @@ def get_fields(fields, response): return results -def get_api_data(api_config, resource, api_client, base_api_url, resource_id=None, - querystring=None, cache_key=None, many=True, - traverse_pagination=True, fields=None, long_term_cache=False): +def get_api_data( + api_config: Any, + resource: str, + api_client: "session", + base_api_url: str, + resource_id: Optional[Union[int, str]] = None, + querystring: Optional[Dict[Any, Any]] = None, + cache_key: Optional[str] = None, + many: bool = True, + traverse_pagination: bool = True, + fields: Optional[List[Any]] = None, + long_term_cache: bool = False, + raise_on_error: bool = False, +) -> Union[List[Any], Dict[Any, Any]]: """ GET data from an edX REST API endpoint using the API client. @@ -43,21 +58,26 @@ def get_api_data(api_config, resource, api_client, base_api_url, resource_id=Non many (bool): Whether the resource requested is a collection of objects, or a single object. If false, an empty dict will be returned in cases of failure rather than the default empty list. traverse_pagination (bool): Whether to traverse pagination or return paginated response.. + fields (list): Return only specific fields from the response long_term_cache (bool): Whether to use the long term cache ttl or the standard cache ttl + raise_on_error (bool): Reraise errors back to the caller, instead if returning empty results. Returns: Data returned by the API. When hitting a list endpoint, extracts "results" (list of dict) - returned by DRF-powered APIs. + returned by DRF-powered APIs. By default, returns an empty result if the called API + returns an error. """ no_data = [] if many else {} if not api_config.enabled: - log.warning('%s configuration is disabled.', api_config.API_NAME) + log.warning("%s configuration is disabled.", api_config.API_NAME) return no_data if cache_key: - cache_key = f'{cache_key}.{resource_id}' if resource_id is not None else cache_key - cache_key += '.zpickled' + cache_key = ( + f"{cache_key}.{resource_id}" if resource_id is not None else cache_key + ) + cache_key += ".zpickled" cached = cache.get(cache_key) if cached: @@ -77,19 +97,23 @@ def get_api_data(api_config, resource, api_client, base_api_url, resource_id=Non querystring = querystring if querystring else {} api_url = urljoin( f"{base_api_url}/", - f"{resource}/{str(resource_id) + '/' if resource_id is not None else ''}" + f"{resource}/{str(resource_id) + '/' if resource_id is not None else ''}", ) response = api_client.get(api_url, params=querystring) response.raise_for_status() response = response.json() if resource_id is None and traverse_pagination: - results = _traverse_pagination(response, api_client, api_url, querystring, no_data) + results = _traverse_pagination( + response, api_client, api_url, querystring, no_data + ) else: results = response except: # pylint: disable=bare-except - log.exception('Failed to retrieve data from the %s API.', api_config.API_NAME) + log.exception("Failed to retrieve data from the %s API.", api_config.API_NAME) + if raise_on_error: + raise return no_data if cache_key: @@ -111,17 +135,17 @@ def _traverse_pagination(response, api_client, api_url, querystring, no_data): Extracts and concatenates "results" (list of dict) returned by DRF-powered APIs. """ - results = response.get('results', no_data) + results = response.get("results", no_data) page = 1 - next_page = response.get('next') + next_page = response.get("next") while next_page: page += 1 - querystring['page'] = page + querystring["page"] = page response = api_client.get(api_url, params=querystring) response.raise_for_status() response = response.json() - results += response.get('results', no_data) - next_page = response.get('next') + results += response.get("results", no_data) + next_page = response.get("next") return results diff --git a/openedx/core/lib/tests/test_edx_api_utils.py b/openedx/core/lib/tests/test_edx_api_utils.py index 5a71eeb1e7..86aa17213c 100644 --- a/openedx/core/lib/tests/test_edx_api_utils.py +++ b/openedx/core/lib/tests/test_edx_api_utils.py @@ -7,6 +7,7 @@ from urllib.parse import urljoin import httpretty from django.core.cache import cache +from requests.exceptions import HTTPError from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.catalog.models import CatalogIntegration @@ -16,29 +17,36 @@ from openedx.core.djangoapps.credentials.tests.mixins import CredentialsApiConfi from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms from openedx.core.lib.edx_api_utils import get_api_data -UTILITY_MODULE = 'openedx.core.lib.edx_api_utils' -TEST_API_URL = 'http://www-internal.example.com/api' +UTILITY_MODULE = "openedx.core.lib.edx_api_utils" +TEST_API_URL = "http://www-internal.example.com/api" @skip_unless_lms @httpretty.activate -class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, CacheIsolationTestCase): +class TestGetEdxApiData( + CatalogIntegrationMixin, CredentialsApiConfigMixin, CacheIsolationTestCase +): """ Tests for edX API data retrieval utility. """ - ENABLED_CACHES = ['default'] + + ENABLED_CACHES = ["default"] def setUp(self): super().setUp() self.user = UserFactory() - self.base_api_url = CatalogIntegration.current().get_internal_api_url().strip('/') + self.base_api_url = ( + CatalogIntegration.current().get_internal_api_url().strip("/") + ) httpretty.httpretty.reset() cache.clear() def _mock_catalog_api(self, responses, url=None): - assert httpretty.is_enabled(), 'httpretty must be enabled to mock Catalog API calls.' + assert ( + httpretty.is_enabled() + ), "httpretty must be enabled to mock Catalog API calls." url = url if url else urljoin(f"{self.base_api_url}/", "programs/") @@ -57,19 +65,22 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach catalog_integration = self.create_catalog_integration() api = get_catalog_api_client(self.user) - expected_collection = ['some', 'test', 'data'] + expected_collection = ["some", "test", "data"] data = { - 'next': None, - 'results': expected_collection, + "next": None, + "results": expected_collection, } self._mock_catalog_api( - [httpretty.Response(body=json.dumps(data), content_type='application/json')] + [httpretty.Response(body=json.dumps(data), content_type="application/json")] ) - with mock.patch('requests.Session') as mock_init: + with mock.patch("requests.Session") as mock_init: actual_collection = get_api_data( - catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url + catalog_integration, + "programs", + api_client=api, + base_api_url=self.base_api_url, ) # Verify that the helper function didn't initialize its own client. @@ -86,25 +97,30 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach catalog_integration = self.create_catalog_integration() api = get_catalog_api_client(self.user) - expected_collection = ['some', 'test', 'data'] + expected_collection = ["some", "test", "data"] url = urljoin(f"{self.base_api_url}/", "/programs/?page={}") responses = [] for page, record in enumerate(expected_collection, start=1): data = { - 'next': url.format(page + 1) if page < len(expected_collection) else None, - 'results': [record], + "next": url.format(page + 1) + if page < len(expected_collection) + else None, + "results": [record], } body = json.dumps(data) responses.append( - httpretty.Response(body=body, content_type='application/json') + httpretty.Response(body=body, content_type="application/json") ) self._mock_catalog_api(responses) actual_collection = get_api_data( - catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url + catalog_integration, + "programs", + api_client=api, + base_api_url=self.base_api_url, ) assert actual_collection == expected_collection @@ -120,22 +136,31 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach url = urljoin(f"{self.base_api_url}/", "/programs/?page={}") responses = [ { - 'next': url.format(2), - 'results': ['some'], + "next": url.format(2), + "results": ["some"], }, { - 'next': url.format(None), - 'results': ['test'], + "next": url.format(None), + "results": ["test"], }, ] expected_response = responses[0] self._mock_catalog_api( - [httpretty.Response(body=json.dumps(body), content_type='application/json') for body in responses] + [ + httpretty.Response( + body=json.dumps(body), content_type="application/json" + ) + for body in responses + ] ) actual_collection = get_api_data( - catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url, traverse_pagination=False + catalog_integration, + "programs", + api_client=api, + base_api_url=self.base_api_url, + traverse_pagination=False, ) assert actual_collection == expected_response self._assert_num_requests(1) @@ -150,15 +175,23 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach resource_id = 1 url = urljoin(f"{self.base_api_url}/", f"programs/{resource_id}/") - expected_resource = {'key': 'value'} + expected_resource = {"key": "value"} self._mock_catalog_api( - [httpretty.Response(body=json.dumps(expected_resource), content_type='application/json')], - url=url + [ + httpretty.Response( + body=json.dumps(expected_resource), content_type="application/json" + ) + ], + url=url, ) actual_resource = get_api_data( - catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url, resource_id=resource_id + catalog_integration, + "programs", + api_client=api, + base_api_url=self.base_api_url, + resource_id=resource_id, ) assert actual_resource == expected_resource @@ -180,15 +213,23 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach resource_id = 0 url = urljoin(f"{self.base_api_url}/", f"programs/{resource_id}/") - expected_resource = {'key': 'value', 'results': []} + expected_resource = {"key": "value", "results": []} self._mock_catalog_api( - [httpretty.Response(body=json.dumps(expected_resource), content_type='application/json')], - url=url + [ + httpretty.Response( + body=json.dumps(expected_resource), content_type="application/json" + ) + ], + url=url, ) actual_resource = get_api_data( - catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url, resource_id=resource_id + catalog_integration, + "programs", + api_client=api, + base_api_url=self.base_api_url, + resource_id=resource_id, ) assert actual_resource == expected_resource @@ -201,17 +242,21 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach catalog_integration = self.create_catalog_integration(cache_ttl=5) api = get_catalog_api_client(self.user) - response = {'lang': 'en', 'weeks_to_complete': '5'} + response = {"lang": "en", "weeks_to_complete": "5"} - resource_id = 'course-v1:testX+testABC+1T2019' + resource_id = "course-v1:testX+testABC+1T2019" url = urljoin(f"{self.base_api_url}/", f"course_runs/{resource_id}/") - expected_resource_for_lang = {'lang': 'en'} - expected_resource_for_weeks_to_complete = {'weeks_to_complete': '5'} + expected_resource_for_lang = {"lang": "en"} + expected_resource_for_weeks_to_complete = {"weeks_to_complete": "5"} self._mock_catalog_api( - [httpretty.Response(body=json.dumps(response), content_type='application/json')], - url=url + [ + httpretty.Response( + body=json.dumps(response), content_type="application/json" + ) + ], + url=url, ) cache_key = CatalogIntegration.current().CACHE_KEY @@ -219,24 +264,24 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach # get response and set the cache. actual_resource_for_lang = get_api_data( catalog_integration, - 'course_runs', + "course_runs", resource_id=resource_id, api_client=api, base_api_url=self.base_api_url, cache_key=cache_key, - fields=['lang'] + fields=["lang"], ) assert actual_resource_for_lang == expected_resource_for_lang # Hit the cache actual_resource = get_api_data( catalog_integration, - 'course_runs', + "course_runs", api_client=api, base_api_url=self.base_api_url, resource_id=resource_id, cache_key=cache_key, - fields=['weeks_to_complete'] + fields=["weeks_to_complete"], ) assert actual_resource == expected_resource_for_weeks_to_complete @@ -251,73 +296,94 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach catalog_integration = self.create_catalog_integration(cache_ttl=5) api = get_catalog_api_client(self.user) - expected_collection = ['some', 'test', 'data'] + expected_collection = ["some", "test", "data"] data = { - 'next': None, - 'results': expected_collection, + "next": None, + "results": expected_collection, } self._mock_catalog_api( - [httpretty.Response(body=json.dumps(data), content_type='application/json')], + [ + httpretty.Response( + body=json.dumps(data), content_type="application/json" + ) + ], ) resource_id = 1 url = urljoin(f"{self.base_api_url}/", f"programs/{resource_id}/") - expected_resource = {'key': 'value'} + expected_resource = {"key": "value"} self._mock_catalog_api( - [httpretty.Response(body=json.dumps(expected_resource), content_type='application/json')], - url=url + [ + httpretty.Response( + body=json.dumps(expected_resource), content_type="application/json" + ) + ], + url=url, ) cache_key = CatalogIntegration.current().CACHE_KEY # Warm up the cache. get_api_data( - catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url, cache_key=cache_key + catalog_integration, + "programs", + api_client=api, + base_api_url=self.base_api_url, + cache_key=cache_key, ) get_api_data( catalog_integration, - 'programs', + "programs", api_client=api, base_api_url=self.base_api_url, resource_id=resource_id, - cache_key=cache_key + cache_key=cache_key, ) # Hit the cache. actual_collection = get_api_data( - catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url, cache_key=cache_key + catalog_integration, + "programs", + api_client=api, + base_api_url=self.base_api_url, + cache_key=cache_key, ) assert actual_collection == expected_collection actual_resource = get_api_data( catalog_integration, - 'programs', + "programs", api_client=api, base_api_url=self.base_api_url, resource_id=resource_id, - cache_key=cache_key + cache_key=cache_key, ) assert actual_resource == expected_resource # Verify that only two requests were made, not four. self._assert_num_requests(2) - @mock.patch(UTILITY_MODULE + '.log.warning') + @mock.patch(UTILITY_MODULE + ".log.warning") def test_api_config_disabled(self, mock_warning): """ Verify that no data is retrieved if the provided config model is disabled. """ catalog_integration = self.create_catalog_integration(enabled=False) - actual = get_api_data(catalog_integration, 'programs', api_client=None, base_api_url=self.base_api_url) + actual = get_api_data( + catalog_integration, + "programs", + api_client=None, + base_api_url=self.base_api_url, + ) assert mock_warning.called assert actual == [] - @mock.patch(UTILITY_MODULE + '.log.exception') + @mock.patch(UTILITY_MODULE + ".log.exception") def test_data_retrieval_failure(self, mock_exception): """ Verify that an exception is logged when data can't be retrieved. @@ -326,15 +392,24 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach api = get_catalog_api_client(self.user) self._mock_catalog_api( - [httpretty.Response(body='clunk', content_type='application/json', status_code=500)] + [ + httpretty.Response( + body="clunk", content_type="application/json", status_code=500 + ) + ] ) - actual = get_api_data(catalog_integration, 'programs', api_client=api, base_api_url=self.base_api_url) + actual = get_api_data( + catalog_integration, + "programs", + api_client=api, + base_api_url=self.base_api_url, + ) assert mock_exception.called assert actual == [] - @mock.patch(UTILITY_MODULE + '.log.warning') + @mock.patch(UTILITY_MODULE + ".log.warning") def test_api_config_disabled_with_id_and_not_collection(self, mock_warning): """ Verify that no data is retrieved if the provided config model is disabled. @@ -343,17 +418,17 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach actual = get_api_data( catalog_integration, - 'programs', + "programs", api_client=None, base_api_url=self.base_api_url, resource_id=100, - many=False + many=False, ) assert mock_warning.called assert actual == {} - @mock.patch(UTILITY_MODULE + '.log.exception') + @mock.patch(UTILITY_MODULE + ".log.exception") def test_data_retrieval_failure_with_id(self, mock_exception): """ Verify that an exception is logged when data can't be retrieved. @@ -362,16 +437,52 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach api = get_catalog_api_client(self.user) self._mock_catalog_api( - [httpretty.Response(body='clunk', content_type='application/json', status_code=500)] + [ + httpretty.Response( + body="clunk", content_type="application/json", status_code=500 + ) + ] ) actual = get_api_data( catalog_integration, - 'programs', + "programs", api_client=api, base_api_url=self.base_api_url, resource_id=100, - many=False + many=False, ) assert mock_exception.called assert actual == {} + + def test_data_retrieval_raise_on_error(self): + """ + Verify that when raise_on_error is set and the API call gives an error response, + we raise HttpError. + """ + catalog_integration = self.create_catalog_integration() + api = get_catalog_api_client(self.user) + + url = urljoin(f"{self.base_api_url}/", "/programs/100") + + self._mock_catalog_api( + [ + httpretty.Response( + body="clunk", + content_type="application/json", + status_code=500, + url=url, + ) + ] + ) + + with self.assertRaises(Exception): + get_api_data( + catalog_integration, + "programs", + api_client=api, + base_api_url=self.base_api_url, + resource_id=100, + many=False, + raise_on_error=True, + )