From c4bc6e29eae24fbc614feae9836cfa97d16ec140 Mon Sep 17 00:00:00 2001 From: dyudyunov Date: Wed, 19 Jan 2022 15:55:29 +0200 Subject: [PATCH 1/3] fix: certificate generation without Persistent Grades Fix for an error related to endless recursion Dev Notes: The student gets the passing grade -> CourseGradeFactory sends the `COURSE_GRADE_NOW_PASSED` signal after the grade calculation -> `listen_for_passing_grade` receives the signal and calls `generate_certificate_task` -> `generate_certificate_task` calls `_generate_regular_certificate_task` -> `_generate_regular_certificate_task` trying to get grade by calling the `_get_course_grade` -> `_get_course_grade` calls `CourseGradeFactory().read()` but I have persistent grades off, so actually that ends with `CourseGradeFactory().update()` -> `CourseGradeFactory().update()` sends the `COURSE_GRADE_NOW_PASSED` And so on --- .../certificates/generation_handler.py | 6 +- lms/djangoapps/grades/course_grade_factory.py | 55 ++++++++++--------- 2 files changed, 33 insertions(+), 28 deletions(-) diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index d2641e3abf..b05b0520a7 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -72,7 +72,7 @@ def _generate_regular_certificate_task(user, course_key, generation_mode=None, d eligible and a certificate can be generated. """ enrollment_mode = _get_enrollment_mode(user, course_key) - course_grade = _get_course_grade(user, course_key) + course_grade = _get_course_grade(user, course_key, send_grade_signals=False) if _can_generate_regular_certificate(user, course_key, enrollment_mode, course_grade): return _generate_certificate_task(user=user, course_key=course_key, enrollment_mode=enrollment_mode, course_grade=course_grade, generation_mode=generation_mode, @@ -377,11 +377,11 @@ def _get_grade_value(course_grade): return '' -def _get_course_grade(user, course_key): +def _get_course_grade(user, course_key, send_grade_signals=False): """ Get the user's course grade in this course run. Note that this may be None. """ - return CourseGradeFactory().read(user, course_key=course_key) + return CourseGradeFactory().read(user, course_key=course_key, send_grade_signals=send_grade_signals) def _get_enrollment_mode(user, course_key): diff --git a/lms/djangoapps/grades/course_grade_factory.py b/lms/djangoapps/grades/course_grade_factory.py index 1c64f97589..5a86a060c0 100644 --- a/lms/djangoapps/grades/course_grade_factory.py +++ b/lms/djangoapps/grades/course_grade_factory.py @@ -33,6 +33,7 @@ class CourseGradeFactory: course_structure=None, course_key=None, create_if_needed=True, + send_grade_signals=True, ): """ Returns the CourseGrade for the given user in the course. @@ -51,7 +52,7 @@ class CourseGradeFactory: if assume_zero_if_absent(course_data.course_key): return self._create_zero(user, course_data) elif create_if_needed: - return self._update(user, course_data) + return self._update(user, course_data, send_grade_signals=send_grade_signals) else: return None @@ -160,13 +161,16 @@ class CourseGradeFactory: ) @staticmethod - def _update(user, course_data, force_update_subsections=False): + def _update(user, course_data, force_update_subsections=False, send_grade_signals=True): """ - Computes, saves, and returns a CourseGrade object for the - given user and course. - Sends a COURSE_GRADE_CHANGED signal to listeners and - COURSE_GRADE_NOW_PASSED if learner has passed course or - COURSE_GRADE_NOW_FAILED if learner is now failing course + Computes, saves, and returns a CourseGrade object for the given user and course. + + send_grade_signals defines if signals should be sent. Use it to avoid recursion issues in + cases when the signal listener trying to get grades but Persistent Grades are disabled. + If True - sends: + COURSE_GRADE_CHANGED signal to listeners and + COURSE_GRADE_NOW_PASSED if learner has passed course or + COURSE_GRADE_NOW_FAILED if learner is now failing course """ should_persist = should_persist_grades(course_data.course_key) if should_persist and force_update_subsections: @@ -193,26 +197,27 @@ class CourseGradeFactory: passed=course_grade.passed, ) - COURSE_GRADE_CHANGED.send_robust( - sender=None, - user=user, - course_grade=course_grade, - course_key=course_data.course_key, - deadline=course_data.course.end, - ) - if course_grade.passed: - COURSE_GRADE_NOW_PASSED.send( - sender=CourseGradeFactory, + if send_grade_signals: + COURSE_GRADE_CHANGED.send_robust( + sender=None, user=user, - course_id=course_data.course_key, - ) - else: - COURSE_GRADE_NOW_FAILED.send( - sender=CourseGradeFactory, - user=user, - course_id=course_data.course_key, - grade=course_grade, + course_grade=course_grade, + course_key=course_data.course_key, + deadline=course_data.course.end, ) + if course_grade.passed: + COURSE_GRADE_NOW_PASSED.send( + sender=CourseGradeFactory, + user=user, + course_id=course_data.course_key, + ) + else: + COURSE_GRADE_NOW_FAILED.send( + sender=CourseGradeFactory, + user=user, + course_id=course_data.course_key, + grade=course_grade, + ) log.info( 'Grades: Update, %s, User: %s, %s, persisted: %s', From 956c9855080d77c07278f925960d64322e1554d7 Mon Sep 17 00:00:00 2001 From: dyudyunov Date: Thu, 20 Jan 2022 12:45:49 +0200 Subject: [PATCH 2/3] test: :ambulance: add test to catch recursion when persistent grades disabled --- .../certificates/generation_handler.py | 7 ++-- .../grades/tests/test_course_grade_factory.py | 34 ++++++++++++++++++- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index b05b0520a7..95b6c825a6 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -53,7 +53,7 @@ def generate_allowlist_certificate_task(user, course_key, generation_mode=None, Create a task to generate an allowlist certificate for this user in this course run. """ enrollment_mode = _get_enrollment_mode(user, course_key) - course_grade = _get_course_grade(user, course_key) + course_grade = _get_course_grade(user, course_key, send_grade_signals=False) if _can_generate_allowlist_certificate(user, course_key, enrollment_mode): return _generate_certificate_task(user=user, course_key=course_key, enrollment_mode=enrollment_mode, course_grade=course_grade, generation_mode=generation_mode, @@ -377,9 +377,12 @@ def _get_grade_value(course_grade): return '' -def _get_course_grade(user, course_key, send_grade_signals=False): +def _get_course_grade(user, course_key, send_grade_signals=True): """ Get the user's course grade in this course run. Note that this may be None. + + Use send_grade_signals=False to avoid firing the course grade signals recursively. + See details in lms/djangoapps/grades/course_grade_factory.py _update method. """ return CourseGradeFactory().read(user, course_key=course_key, send_grade_signals=send_grade_signals) diff --git a/lms/djangoapps/grades/tests/test_course_grade_factory.py b/lms/djangoapps/grades/tests/test_course_grade_factory.py index 91fea2f1f5..880b31f0d2 100644 --- a/lms/djangoapps/grades/tests/test_course_grade_factory.py +++ b/lms/djangoapps/grades/tests/test_course_grade_factory.py @@ -2,16 +2,19 @@ Tests for the CourseGradeFactory class. """ import itertools -from unittest.mock import patch +from unittest.mock import patch, Mock import ddt from django.conf import settings from edx_toggles.toggles.testutils import override_waffle_switch +import pytest from common.djangoapps.student.tests.factories import UserFactory +from lms.djangoapps.certificates.config import AUTO_CERTIFICATE_GENERATION from lms.djangoapps.courseware.access import has_access from lms.djangoapps.grades.config.tests.utils import persistent_grades_feature_flags from openedx.core.djangoapps.content.block_structure.factory import BlockStructureFactory +from openedx.core.djangoapps.signals.signals import COURSE_GRADE_NOW_PASSED from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order @@ -73,6 +76,35 @@ class TestCourseGradeFactory(GradeTestBase): grade_factory.read(self.request.user, self.course) assert mock_read_grade.called == (feature_flag and course_setting) + @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False}) + def test_no_recursion_without_persistent_grades(self): + """ + Course grade signals should not be fired recursively when persistent grades are disabled. + """ + self.mock_process_signal = Mock() # pylint: disable=attribute-defined-outside-init + + def handler(**kwargs): + """ + Mock signal receiver. + """ + self.mock_process_signal() + + with persistent_grades_feature_flags( + global_flag=False, + enabled_for_all_courses=False, + course_id=self.course.id, + enabled_for_course=False + ): + with override_waffle_switch(AUTO_CERTIFICATE_GENERATION, active=True), mock_get_score(2, 2): + COURSE_GRADE_NOW_PASSED.connect(handler) + try: + CourseGradeFactory().update(self.request.user, self.course) + except RecursionError: + pytest.fail("The COURSE_GRADE_NOW_PASSED signal fired recursively.") + + self.mock_process_signal.assert_called_once() + COURSE_GRADE_NOW_PASSED.disconnect(handler) + def test_read_and_update(self): grade_factory = CourseGradeFactory() From aa501dea662bafd788777ee386fd7fa3481e082a Mon Sep 17 00:00:00 2001 From: Eugene Dyudyunov Date: Thu, 24 Mar 2022 10:53:30 +0200 Subject: [PATCH 3/3] refactor: change the send_grade_signals to send_course_grade_signals --- lms/djangoapps/certificates/generation_handler.py | 10 +++++----- lms/djangoapps/grades/course_grade_factory.py | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index 95b6c825a6..8b3834f528 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -53,7 +53,7 @@ def generate_allowlist_certificate_task(user, course_key, generation_mode=None, Create a task to generate an allowlist certificate for this user in this course run. """ enrollment_mode = _get_enrollment_mode(user, course_key) - course_grade = _get_course_grade(user, course_key, send_grade_signals=False) + course_grade = _get_course_grade(user, course_key, send_course_grade_signals=False) if _can_generate_allowlist_certificate(user, course_key, enrollment_mode): return _generate_certificate_task(user=user, course_key=course_key, enrollment_mode=enrollment_mode, course_grade=course_grade, generation_mode=generation_mode, @@ -72,7 +72,7 @@ def _generate_regular_certificate_task(user, course_key, generation_mode=None, d eligible and a certificate can be generated. """ enrollment_mode = _get_enrollment_mode(user, course_key) - course_grade = _get_course_grade(user, course_key, send_grade_signals=False) + course_grade = _get_course_grade(user, course_key, send_course_grade_signals=False) if _can_generate_regular_certificate(user, course_key, enrollment_mode, course_grade): return _generate_certificate_task(user=user, course_key=course_key, enrollment_mode=enrollment_mode, course_grade=course_grade, generation_mode=generation_mode, @@ -377,14 +377,14 @@ def _get_grade_value(course_grade): return '' -def _get_course_grade(user, course_key, send_grade_signals=True): +def _get_course_grade(user, course_key, send_course_grade_signals=True): """ Get the user's course grade in this course run. Note that this may be None. - Use send_grade_signals=False to avoid firing the course grade signals recursively. + Use send_course_grade_signals=False to avoid firing the course grade signals recursively. See details in lms/djangoapps/grades/course_grade_factory.py _update method. """ - return CourseGradeFactory().read(user, course_key=course_key, send_grade_signals=send_grade_signals) + return CourseGradeFactory().read(user, course_key=course_key, send_course_grade_signals=send_course_grade_signals) def _get_enrollment_mode(user, course_key): diff --git a/lms/djangoapps/grades/course_grade_factory.py b/lms/djangoapps/grades/course_grade_factory.py index 5a86a060c0..65f3a9c4cc 100644 --- a/lms/djangoapps/grades/course_grade_factory.py +++ b/lms/djangoapps/grades/course_grade_factory.py @@ -33,7 +33,7 @@ class CourseGradeFactory: course_structure=None, course_key=None, create_if_needed=True, - send_grade_signals=True, + send_course_grade_signals=True, ): """ Returns the CourseGrade for the given user in the course. @@ -52,7 +52,7 @@ class CourseGradeFactory: if assume_zero_if_absent(course_data.course_key): return self._create_zero(user, course_data) elif create_if_needed: - return self._update(user, course_data, send_grade_signals=send_grade_signals) + return self._update(user, course_data, send_course_grade_signals=send_course_grade_signals) else: return None @@ -161,11 +161,11 @@ class CourseGradeFactory: ) @staticmethod - def _update(user, course_data, force_update_subsections=False, send_grade_signals=True): + def _update(user, course_data, force_update_subsections=False, send_course_grade_signals=True): """ Computes, saves, and returns a CourseGrade object for the given user and course. - send_grade_signals defines if signals should be sent. Use it to avoid recursion issues in + send_course_grade_signals defines if signals should be sent. Use it to avoid recursion issues in cases when the signal listener trying to get grades but Persistent Grades are disabled. If True - sends: COURSE_GRADE_CHANGED signal to listeners and @@ -197,7 +197,7 @@ class CourseGradeFactory: passed=course_grade.passed, ) - if send_grade_signals: + if send_course_grade_signals: COURSE_GRADE_CHANGED.send_robust( sender=None, user=user,