Skip to content

Fix certificate generation without persistent grades #29792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mariajgrimaldi merged 3 commits into from
Apr 8, 2022
Merged

Fix certificate generation without persistent grades #29792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c4bc6e2
fix: certificate generation without Persistent Grades
dyudyunov Jan 19, 2022
956c985
test: :ambulance: add test to catch recursion when persistent grades …
dyudyunov Jan 20, 2022
aa501de
refactor: change the send_grade_signals to send_course_grade_signals
dyudyunov Mar 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions lms/djangoapps/certificates/generation_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_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,
Expand All @@ -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_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,
Expand Down Expand Up @@ -377,11 +377,14 @@ def _get_grade_value(course_grade):
return ''


def _get_course_grade(user, course_key):
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_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)
return CourseGradeFactory().read(user, course_key=course_key, send_course_grade_signals=send_course_grade_signals)


def _get_enrollment_mode(user, course_key):
Expand Down
55 changes: 30 additions & 25 deletions lms/djangoapps/grades/course_grade_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def read(
course_structure=None,
course_key=None,
create_if_needed=True,
send_course_grade_signals=True,
):
"""
Returns the CourseGrade for the given user in the course.
Expand All @@ -51,7 +52,7 @@ def read(
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_course_grade_signals=send_course_grade_signals)
else:
return None

Expand Down Expand Up @@ -160,13 +161,16 @@ def _read(user, course_data):
)

@staticmethod
def _update(user, course_data, force_update_subsections=False):
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.
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_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
COURSE_GRADE_NOW_PASSED if learner has passed course or
COURSE_GRADE_NOW_FAILED if learner is now failing course

@mariajgrimaldi mariajgrimaldi Mar 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help me list the cases where we want to send the signals? At least in this PR

@dyudyunov dyudyunov Mar 21, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably any place where the CourseGradeFactory().read() used (because it calls the _update() if there is no persistent grade present).

Some examples of the signal receivers:

  • evaluation of any milestone relationships which are attached
    to the course grade;
  • Notifies the Credentials IDA about certain grades it needs for its records, when a grade changes;
  • update minimum grade requirement status;
  • generate a certificate task;
  • etc

@mariajgrimaldi mariajgrimaldi Mar 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

evaluation of any milestone relationships which are attached
to the course grade;
Notifies the Credentials IDA about certain grades it needs for its records, when a grade changes;
update minimum grade requirement status;
generate a certificate task;
etc

But the whole is to not send the course grade related signals when generating the certificate task right?

@dyudyunov dyudyunov Mar 24, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's the main goal to break the recursive grade recalculation when the certificate is generated

"""
should_persist = should_persist_grades(course_data.course_key)
if should_persist and force_update_subsections:
Expand All @@ -193,26 +197,27 @@ def _update(user, course_data, force_update_subsections=False):
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,
user=user,
course_id=course_data.course_key,
)
else:
COURSE_GRADE_NOW_FAILED.send(
sender=CourseGradeFactory,
if send_course_grade_signals:

@felipemontoya felipemontoya Mar 25, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the case, that every time that send_course_grade_signals is false, should_persist_grades is also false?

I'm thinking that if signals should not be sent when persistent grades is disabled, then we could make the if statement dependant on that variable and avoid creating a new control variable that needs to be accounted for and passed by the generation_handler.

@dyudyunov dyudyunov Mar 29, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should send signals 1 time regardless of the persistent grade status.

The flow is:

  • user answers the question in the course;
  • recalculation of course grade is starting;
  • course grade now passing -> send the COURSE_GRADE_NOW_PASSED signal to start certificate generation;

here we should not break the signal sending

  • certificate generation process would like to receive the course grades so it calls the course_grade_factory again

here we should avoid sending the signals

@felipemontoya felipemontoya Mar 29, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. we should not send the signal within the processing of the same signal.

Since this was the last pending thing, I'll leave my approve

COURSE_GRADE_CHANGED.send_robust(
sender=None,
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,

@mariajgrimaldi mariajgrimaldi Mar 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is using send_grade_signals=False better than sending the course_grade in the signal? and removing course_grade = _get_course_grade(user, course_key, send_grade_signals=False)? 🤔

NOTE: I haven't tested this, I'm just curious if you have

@dyudyunov dyudyunov Mar 21, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice question!
I didn’t try it, but now I want to 🙂
I will give it a try ASAP

@dyudyunov dyudyunov Mar 22, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried this and can say that:

  • this works perfectly for certificates generated automatically
  • this will not work for certificates generated manually (this case requires to call _get_course_grade 'cause there is no other way to get grades)

@dyudyunov dyudyunov Mar 22, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And calling it will lead to sending the signal and checking the certificate status again

@mariajgrimaldi mariajgrimaldi Mar 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing that out!

)
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',
Expand Down
34 changes: 33 additions & 1 deletion lms/djangoapps/grades/tests/test_course_grade_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -73,6 +76,35 @@ def test_course_grade_feature_gating(self, feature_flag, course_setting):
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()

Expand Down