diff --git a/openedx/core/djangoapps/credentials/tasks/v1/tasks.py b/openedx/core/djangoapps/credentials/tasks/v1/tasks.py index d86204e036..bc0c90c1bc 100644 --- a/openedx/core/djangoapps/credentials/tasks/v1/tasks.py +++ b/openedx/core/djangoapps/credentials/tasks/v1/tasks.py @@ -1,7 +1,6 @@ """ -This file contains celery tasks for credentials-related functionality. +This file contains Celery tasks and utility functions supporting the Credentials IDA. """ - import math import time from urllib.parse import urljoin @@ -11,7 +10,7 @@ from celery.exceptions import MaxRetriesExceededError from celery.utils.log import get_task_logger from celery_utils.logged_task import LoggedTask from django.conf import settings -from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user +from django.contrib.auth import get_user_model from django.contrib.sites.models import Site from edx_django_utils.monitoring import set_code_owner_attribute from MySQLdb import OperationalError @@ -27,22 +26,25 @@ from openedx.core.djangoapps.content.course_overviews.models import CourseOvervi from openedx.core.djangoapps.credentials.helpers import is_learner_records_enabled_for_org from openedx.core.djangoapps.credentials.models import CredentialsApiConfig from openedx.core.djangoapps.credentials.utils import get_credentials_api_base_url, get_credentials_api_client -from openedx.core.djangoapps.programs.signals import handle_course_cert_awarded, handle_course_cert_changed +from openedx.core.djangoapps.programs.signals import ( + handle_course_cert_awarded, + handle_course_cert_changed, + handle_course_cert_revoked, +) from openedx.core.djangoapps.site_configuration.models import SiteConfiguration +User = get_user_model() logger = get_task_logger(__name__) -# "interesting" here means "credentials will want to know about it" +# "Interesting" means "The Credentials IDA will want to know about it" INTERESTING_MODES = CourseMode.CERTIFICATE_RELEVANT_MODES INTERESTING_STATUSES = [ CertificateStatuses.notpassing, CertificateStatuses.downloadable, ] -# Maximum number of retries before giving up. -# For reference, 11 retries with exponential backoff yields a maximum waiting -# time of 2047 seconds (about 30 minutes). Setting this to None could yield -# unwanted behavior: infinite retries. +# Maximum number of retries before giving up. For reference, 11 retries with exponential backoff yields a maximum +# waiting time of 2047 seconds (about 30 minutes). Setting this to None could yield unwanted behavior: infinite retries. MAX_RETRIES = 11 @@ -58,7 +60,15 @@ def send_grade_to_credentials( grade_last_updated ): """ - Celery task to notify the Credentials IDA of a grade change via POST. + Celery task to notify the Credentials IDA of an "interesting" grade change via an API call. + + Args: + username (string): The username of the learner we are currently processing + course_run_key (string): String identifier of the course run associated with the grade update + verified (bool): Boolean determining if the course run is in a "verified" track + letter_grade (string): String identifier describing the "letter" grade the learner has earned + percent_grade (float): Number representing the learner's grade in this course run + grade_last_updated (string): String describing the last time this grade was modified in the LMS """ logger.info(f"Running task send_grade_to_credentials for username {username} and course {course_run_key}") @@ -82,16 +92,12 @@ def send_grade_to_credentials( } ) response.raise_for_status() - - logger.info(f"Sent grade for course {course_run_key} to user {username}") - + logger.info(f"Sent grade for course {course_run_key} for user {username}") except Exception: # lint-amnesty, pylint: disable=W0703 grade_str = f'(percent: {percent_grade} letter: {letter_grade})' - error_msg = f'Failed to send grade{grade_str} for course {course_run_key} to user {username}.' + error_msg = f'Failed to send grade {grade_str} for course {course_run_key} for user {username}.' logger.exception(error_msg) - exception = MaxRetriesExceededError( - f"Failed to send grade to credentials. Reason: {error_msg}" - ) + exception = MaxRetriesExceededError(f"Failed to send grade to credentials. Reason: {error_msg}") raise self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) # pylint: disable=raise-missing-from @@ -99,21 +105,26 @@ def send_grade_to_credentials( @set_code_owner_attribute def handle_notify_credentials(options, course_keys): """ - Celery task to handle the notify_credentials management command. Finds the - relevant cert and grade records, then starts other celery tasks to send the - data. - """ + Celery task to handle the notify_credentials management command. Finds the relevant cert and grade records, then + starts other celery tasks to send the data. + Args: + options (dict): Dictionary containing settings for this run of the `handle_notify_credentials` task + course_keys (list[string]): List of course run keys for this run of the `handle_notify_credentials` task + """ try: site_config = SiteConfiguration.objects.get(site__domain=options['site']) if options['site'] else None except SiteConfiguration.DoesNotExist: logger.exception('No site configuration found for site %s', options['site']) return - # If a start_date or end_date are passed, this will include certificates - # with a CertificateDateOverride modified within the time range + # If a `start_date` or `end_date` are included in the options we should also include certificates with a + # CertificateDateOveride modified within the time range certs = get_recently_modified_certificates( - course_keys, options['start_date'], options['end_date'], options['user_ids'] + course_keys, + options['start_date'], + options['end_date'], + options['user_ids'] ) users = None @@ -121,14 +132,15 @@ def handle_notify_credentials(options, course_keys): users = User.objects.filter(id__in=options['user_ids']) grades = get_recently_modified_grades( - course_keys, options['start_date'], options['end_date'], users + course_keys, + options['start_date'], + options['end_date'], + users ) - logger.info('notify_credentials Sending notifications for {certs} certificates and {grades} grades'.format( - certs=certs.count(), - grades=grades.count() - )) - + logger.info( + f"[notify_credentials] Sending notifications for {certs.count()} certificates and {grades.count()} grades" + ) if options['dry_run']: log_dry_run(certs, grades) else: @@ -142,13 +154,38 @@ def handle_notify_credentials(options, course_keys): notify_programs=options['notify_programs'] ) - logger.info('notify_credentials finished') - def send_notifications( - certs, grades, site_config=None, delay=0, page_size=100, verbose=False, notify_programs=False + certs, + grades, + site_config=None, + delay=0, + page_size=100, + verbose=False, + notify_programs=False, + revoke_program_certs=False ): - """ Run actual handler commands for the provided certs and grades. """ + """ + Utility function responsible for bootstrapping certificate and grade updates to the Credentials IDA. We do this by + invoking existing code paths that should force the LMS to (re-)send data to Credentials. + + First, we directly call our Django signal handlers responsible for updating the status of certificates throughout + the system. This will trigger attempts to send certificate status information to the Credentials IDA. + + Then, we call a utility function (`send_grade_if_interesting`) that is responsible for determining if the + Credentials IDA cares about this particular grade update for a learner. + + Args: + certs (QuerySet): A collection of course certificates associated with the users we need to update + grades (QuerySet): A collection of grades associated with the users we need to update + site_config (SiteConfiguration): Optional, may be None. The site associated with the users we need to update. + delay (int): Delay (in seconds) to wait between queries + page_size (int): Number of records to process from the certs or grades QuerySet at once. Used to handle load. + verbose (bool): Used to determine level of logging output during grade updates + notify_programs (bool): Used to determine if an update should be sent to Credentials + revoke_program_certs (bool): Used to determine if the system should attempt revoking program certificates during + this run of the `notify_credentials` management command + """ course_cert_info = {} # First, do certs for i, cert in paged_query(certs, delay, page_size): @@ -156,11 +193,7 @@ def send_notifications( logger.info("Skipping credential changes %d for certificate %s", i, certstr(cert)) continue - logger.info( - "Handling credential changes %d for certificate %s", - i, certstr(cert), - ) - + logger.info(f"Handling credential change {i} for certificate {certstr(cert)}") signal_args = { 'sender': None, 'user': cert.user, @@ -174,23 +207,23 @@ def send_notifications( 'mode': cert.mode, 'status': cert.status } - course_cert_info[(cert.user.id, str(cert.course_id))] = data + # handles awarding course certificates in Credentials handle_course_cert_changed(**signal_args) + # handles awarding program certificates in Credentials if notify_programs and CertificateStatuses.is_passing_status(cert.status): handle_course_cert_awarded(**signal_args) + # handles revoking program certificates in Credentials + if revoke_program_certs and notify_programs and not CertificateStatuses.is_passing_status(cert.status): + handle_course_cert_revoked(**signal_args) # Then do grades for i, grade in paged_query(grades, delay, page_size): if site_config and not site_config.has_org(grade.course_id.org): - logger.info("Skipping grade changes %d for grade %s", i, gradestr(grade)) + logger.info(f"Skipping grade change {i} for grade in {gradestr(grade)}") continue - logger.info( - "Handling grade changes %d for grade %s", - i, gradestr(grade), - ) - + logger.info(f"Handling grade change {i} for grade in {gradestr(grade)}") user = User.objects.get(id=grade.user_id) # Grab mode/status from cert call @@ -252,15 +285,15 @@ def log_dry_run(certs, grades): logger.info(f"{certs.count()} Certificates:") for cert in certs[:ITEMS_TO_SHOW]: - logger.info(f" {certstr(cert)}") + logger.info(f"\t{certstr(cert)}") if certs.count() > ITEMS_TO_SHOW: - logger.info(f" (+ {certs.count() - ITEMS_TO_SHOW} more)") + logger.info(f"\t(+ {certs.count() - ITEMS_TO_SHOW} more)") logger.info(f"{grades.count()} Grades:") for grade in grades[:ITEMS_TO_SHOW]: - logger.info(f" {gradestr(grade)}") + logger.info(f"\t{gradestr(grade)}") if grades.count() > ITEMS_TO_SHOW: - logger.info(f" (+ {grades.count() - ITEMS_TO_SHOW} more)") + logger.info(f"\t(+ {grades.count() - ITEMS_TO_SHOW} more)") def certstr(cert): @@ -271,8 +304,6 @@ def gradestr(grade): return f'{grade.course_id} for user {grade.user_id}' -# This has Credentials business logic that has bled into the LMS. But we want to filter here in order to -# not flood our task queue with a bunch of signals. So we put up with it. def send_grade_if_interesting( user, course_run_key, @@ -283,8 +314,22 @@ def send_grade_if_interesting( grade_last_updated=None, verbose=False ): - """ Checks if grade is interesting to Credentials and schedules a Celery task if so. """ + """ + Checks if a grade is interesting to Credentials and schedules a Celery task if so. This is Credentials business + logic that has bled into the LMS. We want to filter here in order to not flood our task queue with a bunch of + signals, so we put up with it. + Args: + user (User): User associated with this grade update + course_run_key (CourseLocator): The course run key associated with this grade update + mode (string): The "mode" for the specific course run (e.g. "verified", "audit", etc.) + status (string): The status of the Certificate associated with this grade update + letter_grade (string): The letter grade associated with this grade update (e.g. "A", "B", or "pass" or "fail") + percent_grade (float): A number representing the learner's grade in this course run + grade_last_updated (DateTime): DateTime object representing the last time the (percent) grade was updated in the + LMS. + verbose (bool): A value determining the logging level desired for this grade update + """ if verbose: msg = ( f"Starting send_grade_if_interesting with_params: user [{getattr(user, 'username', None)}], " @@ -366,9 +411,17 @@ def send_grade_if_interesting( def is_course_run_in_a_program(course_run_key): - """ Returns true if the given course key is in any program at all. """ + """ + Returns true if the given course key is in any program at all. This functionality depends on data to be present in + the program cache. We don't have an easy way to determine if a course run is in a program, so we must search through + each program (on a site-by-site basis). - # We don't have an easy way to go from course_run_key to a specific site that owns it. So just search each site. + Args: + course_run_key (CourseLocator): The course run key used we are using to verify program membership + + Returns: + A boolean describing if the course run is part of a program + """ sites = Site.objects.all() str_key = str(course_run_key) for site in sites: @@ -384,10 +437,10 @@ def is_course_run_in_a_program(course_run_key): @set_code_owner_attribute def backfill_date_for_all_course_runs(): """ - This task will update the course certificate configuration's certificate_available_date - in credentials for all course runs. This is different from the "visable_date" attribute. - This date will always either be the available date that is set in studio for a given course, or it will be None. - This will exclude any course runs that do not have a certificate_available_date or are self paced. + This task will update the course certificate configuration's certificate_available_date in credentials for all + course runs. This is different from the "visable_date" attribute. This date will either be the available date that + is set in studio for a given course, or it will be None. This will exclude any course runs that do not have a + certificate_available_date or are self paced. """ course_run_list = CourseOverview.objects.exclude(self_paced=True).exclude(certificate_available_date=None) for index, course_run in enumerate(course_run_list): @@ -435,10 +488,9 @@ def backfill_date_for_all_course_runs(): @set_code_owner_attribute def clean_certificate_available_date(): """ - This task will clean out the misconfigured certificate available date. When courses Change their - certificates_display_behavior, the certificate_available_date was not updating properly. This is - command is meant to be ran one time to clean up any courses that were not supposed to have - certificate_available_date + Historically, when courses changed their certificates_display_behavior, the certificate_available_date was not + updating properly. This task will clean out course runs that have a misconfigured certificate available date in the + Credentials IDA. """ course_run_list = CourseOverview.objects.exclude( self_paced=0, @@ -446,17 +498,13 @@ def clean_certificate_available_date(): certificate_available_date__isnull=False ) for index, course_run in enumerate(course_run_list): - logger.info( - f"removing certificate_available_date for course {course_run.id}" - ) + logger.info(f"removing certificate_available_date for course {course_run.id}") course_key = str(course_run.id) course_modes = CourseMode.objects.filter(course_id=course_key) # There should only ever be one certificate relevant mode per course run modes = [mode.slug for mode in course_modes if mode.slug in CourseMode.CERTIFICATE_RELEVANT_MODES] if len(modes) != 1: - logger.exception( - f'Either course {course_key} has no certificate mode or multiple modes. Task failed.' - ) + logger.exception(f'Either course {course_key} has no certificate mode or multiple modes. Task failed.') # if there is only one relevant mode, post to credentials else: try: diff --git a/openedx/core/djangoapps/credentials/tests/test_tasks.py b/openedx/core/djangoapps/credentials/tests/test_tasks.py index f7117eddda..1afc6ef51f 100644 --- a/openedx/core/djangoapps/credentials/tests/test_tasks.py +++ b/openedx/core/djangoapps/credentials/tests/test_tasks.py @@ -8,15 +8,18 @@ from datetime import datetime import ddt import pytest from django.conf import settings +from django.contrib.auth import get_user_model from django.db import connection, reset_queries from django.test import TestCase, override_settings from freezegun import freeze_time from opaque_keys.edx.keys import CourseKey from common.djangoapps.student.tests.factories import UserFactory +from lms.djangoapps.certificates.api import get_recently_modified_certificates from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.certificates.models import GeneratedCertificate from lms.djangoapps.grades.models import PersistentCourseGrade +from lms.djangoapps.grades.models_api import get_recently_modified_grades from lms.djangoapps.grades.tests.utils import mock_passing_grade from lms.djangoapps.certificates.tests.factories import CertificateDateOverrideFactory, GeneratedCertificateFactory from openedx.core.djangoapps.catalog.tests.factories import CourseFactory, CourseRunFactory, ProgramFactory @@ -26,6 +29,8 @@ from openedx.core.djangolib.testing.utils import skip_unless_lms from openedx.core.djangoapps.credentials.tasks.v1 import tasks + +User = get_user_model() TASKS_MODULE = 'openedx.core.djangoapps.credentials.tasks.v1.tasks' @@ -107,8 +112,6 @@ class TestHandleNotifyCredentialsTask(TestCase): self.cert4 = GeneratedCertificateFactory( user=self.user2, course_id='course-v1:edX+Test+4', status=CertificateStatuses.downloadable ) - print(('self.cert1.modified_date', self.cert1.modified_date)) - # No factory for these with freeze_time(datetime(2017, 1, 1)): self.grade1 = PersistentCourseGrade.objects.create(user_id=self.user.id, course_id='course-v1:edX+Test+1', @@ -122,8 +125,6 @@ class TestHandleNotifyCredentialsTask(TestCase): with freeze_time(datetime(2017, 2, 1, 5)): self.grade4 = PersistentCourseGrade.objects.create(user_id=self.user2.id, course_id='course-v1:edX+Test+4', percent_grade=1) - print(('self.grade1.modified', self.grade1.modified)) - self.options = { 'args_from_database': False, 'auto': False, @@ -557,3 +558,187 @@ class TestIsCourseRunInAProgramUtil(TestCase): mock_get_programs.return_value = self.data course_run2 = CourseRunFactory() assert not tasks.is_course_run_in_a_program(course_run2['key']) + + +@ddt.ddt +@skip_unless_lms +@mock.patch('openedx.core.djangoapps.credentials.tasks.v1.tasks.handle_course_cert_changed') +@mock.patch('openedx.core.djangoapps.credentials.tasks.v1.tasks.handle_course_cert_awarded') +@mock.patch('openedx.core.djangoapps.credentials.tasks.v1.tasks.handle_course_cert_revoked') +@mock.patch('openedx.core.djangoapps.credentials.tasks.v1.tasks.send_grade_if_interesting') +class TestSendNotifications(TestCase): + """ + Unit Tests for the `send_notifications` function in the `tasks.py` file. + """ + def setUp(self): + super().setUp() + self.user = UserFactory() + self.course_run = CourseRunFactory() + self.course_key = CourseKey.from_string(self.course_run['key']) + self.course = CourseFactory(course_runs=[self.course_run]) + self.site = SiteConfigurationFactory.create( + site_values={'course_org_filter': [self.course_key.org]}, + ) + self.certificate = GeneratedCertificateFactory( + user=self.user, + course_id=self.course_run['key'], + status=CertificateStatuses.downloadable + ) + self.grade = PersistentCourseGrade.objects.create( + user_id=self.user.id, + course_id=self.course_run['key'], + percent_grade=1.0 + ) + + def _get_certs_qs(self): + """ + The function under test expects a QuerySet containing certificate information, this retrieves the data needed + to invoke the `send_notifications` function in the form that is expected. We leverage a utility function that + the product code uses. + """ + return get_recently_modified_certificates([self.course_run['key']], None, None, [self.user.id]) + + def _get_grades_qs(self): + """ + The function under test expects a QuerySet containing grade information, this retrieves the data needed + to invoke the `send_notifications` function in the form that is expected. We leverage a utility function that + the product code uses. + """ + users = User.objects.filter(id__in=[self.user.id]) + return get_recently_modified_grades([self.course_run['key']], None, None, users) + + def _build_expected_signal_args(self): + return { + 'sender': None, + 'user': self.user, + 'course_key': self.certificate.course_id, + 'mode': self.certificate.mode, + 'status': self.certificate.status, + 'verbose': False + } + + def test_send_notifications(self, mock_interesting, mock_revoked, mock_awarded, mock_changed): + """ + A test case that verifies the `send_notifications` functions behavior when the `notify_programs` flag is False. + """ + certs = self._get_certs_qs() + grades = self._get_grades_qs() + + tasks.send_notifications( + certs, + grades, + self.site + ) + + expected_signal_args = self._build_expected_signal_args() + mock_changed.assert_called_once_with(**expected_signal_args) + mock_interesting.assert_called_once_with( + self.user, + self.grade.course_id, + self.certificate.mode, + self.certificate.status, + self.grade.letter_grade, + self.grade.percent_grade, + grade_last_updated=self.grade.modified, + verbose=False + ) + mock_revoked.assert_not_called() + mock_awarded.assert_not_called() + + @ddt.data( + ['downloadable', True], + ['unavailable', False] + ) + @ddt.unpack + def test_send_notifications_notify_programs( + self, + cert_status, + award_expected, + mock_interesting, + mock_revoked, + mock_awarded, + mock_changed + ): + """ + Test cases that verify the `send_notifications` functions behavior when the `notify_programs` flag is True. + """ + certs = self._get_certs_qs() + grades = self._get_grades_qs() + + self.certificate.status = cert_status + self.certificate.save() + + tasks.send_notifications( + certs, + grades, + self.site, + notify_programs=True + ) + + expected_signal_args = self._build_expected_signal_args() + mock_changed.assert_called_once_with(**expected_signal_args) + mock_interesting.assert_called_once_with( + self.user, + self.grade.course_id, + self.certificate.mode, + self.certificate.status, + self.grade.letter_grade, + self.grade.percent_grade, + grade_last_updated=self.grade.modified, + verbose=False + ) + mock_revoked.assert_not_called() + if award_expected: + mock_awarded.assert_called_once_with(**expected_signal_args) + else: + mock_awarded.assert_not_called() + + @ddt.data( + ['downloadable', False], + ['unavailable', True] + ) + @ddt.unpack + def test_send_notifications_revoke_programs( + self, + cert_status, + revoke_expected, + mock_interesting, + mock_revoked, + mock_awarded, + mock_changed + ): + """ + Test cases that verify the `send_notifications` functions behavior when the `revoke_program_certs` flag is True. + """ + certs = self._get_certs_qs() + grades = self._get_grades_qs() + + self.certificate.status = cert_status + self.certificate.save() + + tasks.send_notifications( + certs, + grades, + self.site, + notify_programs=True, + revoke_program_certs=True + ) + + expected_signal_args = self._build_expected_signal_args() + mock_changed.assert_called_once_with(**expected_signal_args) + mock_interesting.assert_called_once_with( + self.user, + self.grade.course_id, + self.certificate.mode, + self.certificate.status, + self.grade.letter_grade, + self.grade.percent_grade, + grade_last_updated=self.grade.modified, + verbose=False + ) + if revoke_expected: + mock_revoked.assert_called_once_with(**expected_signal_args) + mock_awarded.mock_not_called() + else: + mock_revoked.assert_not_called() + mock_awarded.assert_called_once_with(**expected_signal_args)