Fix certificate generation without persistent grades by dyudyunov · Pull Request #29792 · openedx/openedx-platform

dyudyunov · 2022-01-20T12:06:49Z

Description

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

Related PR to open-release/maple.master:

Fix certificate generation without persistent grades #29793

openedx-webhooks · 2022-01-20T12:06:54Z

Thanks for the pull request, @dyudyunov! I've created OSPR-6408 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

natabene · 2022-01-24T03:23:40Z

@dyudyunov Thank you for your contribution. Is this ready for our review?

mariajgrimaldi · 2022-01-27T21:55:50Z

Hi there @dyudyunov 😄 I hope you're great!!
I'm curious about the effects of the recursion on the platform 🤔 what did you notice to make these changes?

dyudyunov · 2022-01-28T08:24:59Z

Hi @mariajgrimaldi!
How to reproduce the issue:

Be sure that Waffle Swith "certificates.auto_certificate_generation" flag is enabled
Be sure the persistent grades are disabled
Change the course certificate settings to be able to generate the certificate ASAP
Achieve minimal progress for generating certificate (50% by default)

As a result, you'll see the same logs in the loop ending with the max recursion depth error leading to 500 (certificate is not generated)

My fix relates to the certificate generation flow only, so any functionality that depends on the course grade factory was not affected.

shadinaif · 2022-02-07T16:11:32Z

Thank you @dyudyunov . I tried to add inline reviews but there is something stuck with me in this particular PR. I just have one suggestion:

I believe we can remove the try-.catch thing in the new test method

dyudyunov · 2022-02-08T08:16:16Z

Thank you @dyudyunov . I tried to add inline reviews but there is something stuck with me in this particular PR. I just have one suggestion:

I believe we can remove the try-.catch thing in the new test method

Hi @shadinaif
I wouldn't want to do that because that is the way to catch the issue fixed in this PR and guarantee it will not appear in the future. I would like to leave it as it is if you don't mind )

felipemontoya · 2022-03-19T00:22:33Z

Thanks @dyudyunov, this changes fixed the recursion issue I was having when my grades where enough to pass a course and I rendered the progress page.

When I turned Persistent Grades on, everything kept working correctly.

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

dyudyunov · 2022-03-21T10:40:19Z

Rebased on the latest master, please, review

mariajgrimaldi · 2022-03-21T13:27:54Z

Hi there @dyudyunov and @natabene. I'll be reviewing this :)

mariajgrimaldi · 2022-03-21T14:21:52Z

lms/djangoapps/grades/course_grade_factory.py

+        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


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

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

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?

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

mariajgrimaldi · 2022-03-21T15:55:08Z

lms/djangoapps/grades/course_grade_factory.py

+                COURSE_GRADE_NOW_PASSED.send(
+                    sender=CourseGradeFactory,
+                    user=user,
+                    course_id=course_data.course_key,


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

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

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)

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

Thanks for testing that out!

lms/djangoapps/certificates/generation_handler.py

mariajgrimaldi · 2022-03-23T20:11:11Z

I believe this was the change that introduced this bug 🤔

dyudyunov · 2022-03-24T08:41:44Z

I believe #28196 was the change that introduced this bug 🤔

Yes.
And I kind of understand why it was missing there.
Disabling the Persistent Grades is used rarely and AFAIK will be deprecated in the future

mariajgrimaldi

This is what I've tested until now with/without persistent grades, and with/without certificates.auto_certificate_generation.

Certificate generation through the progress page
Using cert_generation command
Generation through allowlist
And this it's working correctly.

I'm OK with this change since it avoids sending the grade signals when getting them in the certificate generation process, as it was before the changes from last year.

mariajgrimaldi · 2022-03-24T16:52:41Z

I'm interested to know what do you think about this approach? @felipemontoya

We explored another solution besides using the flag to send the grades signals, but it didn't work in all cases. Besides, PR #28196 explicitly added getting the course grade while generating the certificate, where the crucial thing is to get the grade not to send the signals.

felipemontoya

I think that the approach is solid and it looks like it's working correctly. However I think it can be simplified a little as I left in the inline comment.
If that is not the case I would be fine with merging as is.

felipemontoya · 2022-03-25T17:57:47Z

lms/djangoapps/grades/course_grade_factory.py

-        else:
-            COURSE_GRADE_NOW_FAILED.send(
-                sender=CourseGradeFactory,
+        if send_course_grade_signals:


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.

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

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

openedx-webhooks · 2022-04-08T16:02:59Z

@dyudyunov 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2022-04-08T17:08:23Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2022-04-08T17:43:57Z

EdX Release Notice: This PR has been deployed to the production environment.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jan 20, 2022

dyudyunov mentioned this pull request Jan 20, 2022

Fix certificate generation without persistent grades #29793

Closed

dyudyunov force-pushed the dyudyunov/fix-certificate-generation-without-persistent-grades branch from 8c1cd1b to f385591 Compare January 20, 2022 12:23

openedx-webhooks added awaiting prioritization and removed needs triage labels Jan 24, 2022

shadinaif approved these changes Feb 7, 2022

View reviewed changes

mariajgrimaldi mentioned this pull request Mar 18, 2022

[BD-32] feat: add certificate creation Open edX Filter #29949

Merged

dyudyunov force-pushed the dyudyunov/fix-certificate-generation-without-persistent-grades branch from f385591 to 9f8a1cd Compare March 21, 2022 10:38

test: 🚑 add test to catch recursion when persistent grades disabled

956c985

dyudyunov force-pushed the dyudyunov/fix-certificate-generation-without-persistent-grades branch from 9f8a1cd to 956c985 Compare March 21, 2022 11:05

mariajgrimaldi reviewed Mar 21, 2022

View reviewed changes

mariajgrimaldi reviewed Mar 23, 2022

View reviewed changes

lms/djangoapps/certificates/generation_handler.py Outdated Show resolved Hide resolved

refactor: change the send_grade_signals to send_course_grade_signals

aa501de

mariajgrimaldi approved these changes Mar 24, 2022

View reviewed changes

felipemontoya reviewed Mar 25, 2022

View reviewed changes

felipemontoya approved these changes Mar 29, 2022

View reviewed changes

mariajgrimaldi merged commit 7dc60db into openedx:master Apr 8, 2022

openedx-webhooks added merged and removed awaiting prioritization labels Apr 8, 2022

dyudyunov deleted the dyudyunov/fix-certificate-generation-without-persistent-grades branch April 11, 2022 07:36

Cup0fCoffee mentioned this pull request Nov 9, 2022

fix: ytp_api package in maple open-craft/openedx-platform#507

Merged

Conversation

dyudyunov commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

openedx-webhooks commented Jan 20, 2022

Uh oh!

natabene commented Jan 24, 2022

Uh oh!

mariajgrimaldi commented Jan 27, 2022

Uh oh!

dyudyunov commented Jan 28, 2022

Uh oh!

shadinaif commented Feb 7, 2022

Uh oh!

dyudyunov commented Feb 8, 2022

Uh oh!

felipemontoya commented Mar 19, 2022

Uh oh!

dyudyunov commented Mar 21, 2022

Uh oh!

mariajgrimaldi commented Mar 21, 2022

Uh oh!

mariajgrimaldi Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariajgrimaldi Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mariajgrimaldi commented Mar 23, 2022

Uh oh!

dyudyunov commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariajgrimaldi left a comment

Choose a reason for hiding this comment

Uh oh!

mariajgrimaldi commented Mar 24, 2022

Uh oh!

felipemontoya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openedx-webhooks commented Apr 8, 2022

Uh oh!

edx-pipeline-bot commented Apr 8, 2022

Uh oh!

edx-pipeline-bot commented Apr 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dyudyunov commented Jan 20, 2022 •

edited

Loading

mariajgrimaldi Mar 21, 2022 •

edited

Loading

mariajgrimaldi Mar 21, 2022 •

edited

Loading

dyudyunov commented Mar 24, 2022 •

edited

Loading