From ef6de11467276a81fe13681fd822ca9ca67423b8 Mon Sep 17 00:00:00 2001 From: Justin Hynes Date: Wed, 14 Sep 2022 15:19:21 -0400 Subject: [PATCH] feat: Send `modified` data of a learner's grade to the Credentials IDA [APER-1968] We don't have a good way to understand if grade data in Credentials is out of sync with the LMS. Grades are sent to Credentials via a REST API call originating from an asynchronous Celery task on the LMS side. This PR updates our Celery task `send_grade_to_credentials` to include sending the `modified` DateTime value of a grade record to the Credentials IDA. Updates will be made on the Credentials side to accept and store this data as part of the UserGrade instance. * Updates the `send_grade_to_credentials` task to include passing the grade's `modified` DateTime info as part of the request data to Credentials * Updates the `CourseGradeBase` class to include an optional `last_updated` field. This will store the `modified` date of a PersistentCourseGrade instance when a grade is read through the CourseGradeFactory. * Update existing log statement to use format strings where possible. --- lms/djangoapps/grades/course_grade.py | 13 ++- lms/djangoapps/grades/course_grade_factory.py | 3 +- lms/djangoapps/grades/tests/utils.py | 3 +- .../djangoapps/credentials/tasks/v1/tasks.py | 83 ++++++++++--------- .../credentials/tests/test_tasks.py | 21 ++++- 5 files changed, 78 insertions(+), 45 deletions(-) diff --git a/lms/djangoapps/grades/course_grade.py b/lms/djangoapps/grades/course_grade.py index a4e3c49e8a..fa1288547b 100644 --- a/lms/djangoapps/grades/course_grade.py +++ b/lms/djangoapps/grades/course_grade.py @@ -22,7 +22,16 @@ class CourseGradeBase: """ Base class for Course Grades. """ - def __init__(self, user, course_data, percent=0.0, letter_grade=None, passed=False, force_update_subsections=False): + def __init__( + self, + user, + course_data, + percent=0.0, + letter_grade=None, + passed=False, + force_update_subsections=False, + last_updated=None + ): self.user = user self.course_data = course_data @@ -33,6 +42,8 @@ class CourseGradeBase: self.letter_grade = letter_grade or None self.force_update_subsections = force_update_subsections + self.last_updated = last_updated + def __str__(self): return 'Course Grade: percent: {}, letter_grade: {}, passed: {}'.format( str(self.percent), diff --git a/lms/djangoapps/grades/course_grade_factory.py b/lms/djangoapps/grades/course_grade_factory.py index 513cb5d88e..a6665bf67b 100644 --- a/lms/djangoapps/grades/course_grade_factory.py +++ b/lms/djangoapps/grades/course_grade_factory.py @@ -143,7 +143,8 @@ class CourseGradeFactory: course_data, persistent_grade.percent_grade, persistent_grade.letter_grade, - persistent_grade.letter_grade != '' + persistent_grade.letter_grade != '', + last_updated=persistent_grade.modified ) @staticmethod diff --git a/lms/djangoapps/grades/tests/utils.py b/lms/djangoapps/grades/tests/utils.py index 3a98c16070..82edebbc31 100644 --- a/lms/djangoapps/grades/tests/utils.py +++ b/lms/djangoapps/grades/tests/utils.py @@ -15,7 +15,7 @@ from xmodule.graders import ProblemScore # lint-amnesty, pylint: disable=wrong- @contextmanager -def mock_passing_grade(letter_grade='Pass', percent=0.75, ): +def mock_passing_grade(letter_grade='Pass', percent=0.75, last_updated=None): """ Mock the grading function to always return a passing grade. """ @@ -24,6 +24,7 @@ def mock_passing_grade(letter_grade='Pass', percent=0.75, ): percent=percent, passed=letter_grade is not None, attempted=True, + last_updated=last_updated, ) with patch('lms.djangoapps.grades.course_grade_factory.CourseGradeFactory.read') as mock_grade_read: mock_grade_read.return_value = MagicMock(**passing_grade_fields) diff --git a/openedx/core/djangoapps/credentials/tasks/v1/tasks.py b/openedx/core/djangoapps/credentials/tasks/v1/tasks.py index f2f84554a6..d86204e036 100644 --- a/openedx/core/djangoapps/credentials/tasks/v1/tasks.py +++ b/openedx/core/djangoapps/credentials/tasks/v1/tasks.py @@ -48,7 +48,15 @@ MAX_RETRIES = 11 @shared_task(bind=True, ignore_result=True) @set_code_owner_attribute -def send_grade_to_credentials(self, username, course_run_key, verified, letter_grade, percent_grade): +def send_grade_to_credentials( + self, + username, + course_run_key, + verified, + letter_grade, + percent_grade, + grade_last_updated +): """ Celery task to notify the Credentials IDA of a grade change via POST. """ @@ -66,10 +74,11 @@ def send_grade_to_credentials(self, username, course_run_key, verified, letter_g api_url, data={ 'username': username, - 'course_run': str(course_key), + 'course_run': course_run_key, 'letter_grade': letter_grade, 'percent_grade': percent_grade, 'verified': verified, + 'lms_last_updated_at': grade_last_updated } ) response.raise_for_status() @@ -197,6 +206,7 @@ def send_notifications( status, grade.letter_grade, grade.percent_grade, + grade_last_updated=grade.modified, verbose=verbose ) @@ -263,28 +273,26 @@ def gradestr(grade): # 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, mode, status, letter_grade, percent_grade, verbose=False): +def send_grade_if_interesting( + user, + course_run_key, + mode, + status, + letter_grade, + percent_grade, + grade_last_updated=None, + verbose=False +): """ Checks if grade is interesting to Credentials and schedules a Celery task if so. """ if verbose: - msg = "Starting send_grade_if_interesting with params: "\ - "user [{username}], "\ - "course_run_key [{key}], "\ - "mode [{mode}], "\ - "status [{status}], "\ - "letter_grade [{letter_grade}], "\ - "percent_grade [{percent_grade}], "\ - "verbose [{verbose}]"\ - .format( - username=getattr(user, 'username', None), - key=str(course_run_key), - mode=mode, - status=status, - letter_grade=letter_grade, - percent_grade=percent_grade, - verbose=verbose - ) + msg = ( + f"Starting send_grade_if_interesting with_params: user [{getattr(user, 'username', None)}], " + f"course_run_key [{course_run_key}], mode [{mode}], status [{status}], letter_grade [{letter_grade}], " + f"percent_grade [{percent_grade}], grade_last_updated [{grade_last_updated}], verbose [{verbose}]" + ) logger.info(msg) + # Avoid scheduling new tasks if certification is disabled. (Grades are a part of the records/cert story) if not CredentialsApiConfig.current().is_learner_issuance_enabled: if verbose: @@ -311,10 +319,8 @@ def send_grade_if_interesting(user, course_run_key, mode, status, letter_grade, # We only care about grades for which there is a certificate. if verbose: logger.info( - "Skipping send grade: no cert for user [{username}] & course_id [{course_id}]".format( - username=getattr(user, 'username', None), - course_id=str(course_run_key) - ) + f"Skipping send grade: no cert for user [{getattr(user, 'username', None)}] & course_id " + f"[{course_run_key}]" ) return @@ -323,12 +329,7 @@ def send_grade_if_interesting(user, course_run_key, mode, status, letter_grade, # those too. if mode not in INTERESTING_MODES or status not in INTERESTING_STATUSES: if verbose: - logger.info( - "Skipping send grade: mode/status uninteresting for mode [{mode}] & status [{status}]".format( - mode=mode, - status=status - ) - ) + logger.info(f"Skipping send grade: mode/status uninteresting for mode [{mode}] & status [{status}]") return # If the course isn't in any program, don't bother telling Credentials about it. When Credentials grows support @@ -336,26 +337,32 @@ def send_grade_if_interesting(user, course_run_key, mode, status, letter_grade, if not is_course_run_in_a_program(course_run_key): if verbose: logger.info( - f"Skipping send grade: course run not in a program. [{str(course_run_key)}]" + f"Skipping send grade: course run not in a program. [{course_run_key}]" ) return - # Grab grades if we don't have them in hand - if letter_grade is None or percent_grade is None: + # Grab grade data if we don't have them in hand + if letter_grade is None or percent_grade is None or grade_last_updated is None: grade = CourseGradeFactory().read(user, course_key=course_run_key, create_if_needed=False) if grade is None: if verbose: logger.info( - "Skipping send grade: No grade found for user [{username}] & course_id [{course_id}]".format( - username=getattr(user, 'username', None), - course_id=str(course_run_key) - ) + f"Skipping send grade: No grade found for user [{getattr(user, 'username', None)}] & course_id " + f"[{course_run_key}]" ) return letter_grade = grade.letter_grade percent_grade = grade.percent + grade_last_updated = grade.last_updated - send_grade_to_credentials.delay(user.username, str(course_run_key), True, letter_grade, percent_grade) + send_grade_to_credentials.delay( + user.username, + str(course_run_key), + True, + letter_grade, + percent_grade, + grade_last_updated + ) def is_course_run_in_a_program(course_run_key): diff --git a/openedx/core/djangoapps/credentials/tests/test_tasks.py b/openedx/core/djangoapps/credentials/tests/test_tasks.py index 02edd17f07..f7117eddda 100644 --- a/openedx/core/djangoapps/credentials/tests/test_tasks.py +++ b/openedx/core/djangoapps/credentials/tests/test_tasks.py @@ -51,7 +51,16 @@ class TestSendGradeToCredentialTask(TestCase): api_client = mock.MagicMock() mock_get_api_client.return_value = api_client - tasks.send_grade_to_credentials.delay('user', 'course-v1:org+course+run', True, 'A', 1.0).get() + last_updated = datetime.now() + + tasks.send_grade_to_credentials.delay( + 'user', + 'course-v1:org+course+run', + True, + 'A', + 1.0, + last_updated + ).get() assert mock_get_api_client.call_count == 1 assert mock_get_api_client.call_args[0] == (self.user,) @@ -63,6 +72,7 @@ class TestSendGradeToCredentialTask(TestCase): 'letter_grade': 'A', 'percent_grade': 1.0, 'verified': True, + 'lms_last_updated_at': last_updated.isoformat(), }) def test_retry(self, mock_get_api_client): @@ -71,7 +81,7 @@ class TestSendGradeToCredentialTask(TestCase): """ mock_get_api_client.side_effect = boom - task = tasks.send_grade_to_credentials.delay('user', 'course-v1:org+course+run', True, 'A', 1.0) + task = tasks.send_grade_to_credentials.delay('user', 'course-v1:org+course+run', True, 'A', 1.0, None) pytest.raises(Exception, task.get) assert mock_get_api_client.call_count == (tasks.MAX_RETRIES + 1) @@ -482,10 +492,13 @@ class TestSendGradeIfInteresting(TestCase): _mock_is_learner_issuance_enabled): mock_is_course_run_in_a_program.return_value = True - with mock_passing_grade('B', 0.81): + last_updated = datetime.now() + with mock_passing_grade('B', 0.81, last_updated): tasks.send_grade_if_interesting(self.user, self.key, 'verified', 'downloadable', None, None) assert mock_send_grade_to_credentials.delay.called - assert mock_send_grade_to_credentials.delay.call_args[0] == (self.user.username, str(self.key), True, 'B', 0.81) + assert mock_send_grade_to_credentials.delay.call_args[0] == ( + self.user.username, str(self.key), True, 'B', 0.81, last_updated + ) mock_send_grade_to_credentials.delay.reset_mock() def test_send_grade_without_issuance_enabled(self, _mock_is_course_run_in_a_program,