-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
[BD-32] feat: add certificate creation Open edX Filter #29949
[BD-32] feat: add certificate creation Open edX Filter #29949
Conversation
Thanks for the pull request, @mariajgrimaldi! I've created BLENDED-1102 to keep track of it in Jira. More details are on the BD-32 project page. When this pull request is ready, tag your edX technical lead. |
c2ec8d1
to
ae87b7f
Compare
b3d7d86
to
4fad7c5
Compare
@mariajgrimaldi I started reviewing this and got blocked for the night trying to setup the different ways you can generate the certs in the application. However one thing that I found is this: self.user, self.course_id, self.mode, self.status = CertificateCreationRequested.run_filter(
user=self.user, course_id=self.course_id, mode=self.mode, status=self.status,
) We are passing user, course_id, mode and status as the 4 parameters that can be filtered and all of them come from the certificate object. Could we pass |
Hi @felipemontoya! but what might make more sense flexibility-wise is to use the whole object, so the developer has a broader range of information. I'm gonna run some tests for this suggestion :) |
@felipemontoya: the 1st concern I have about sending |
I was thinking along the lines of:
Something like: |
@@ -464,6 +473,15 @@ def save(self, *args, **kwargs): # pylint: disable=signature-differs | |||
The COURSE_CERT_AWARDED signal helps determine if a Program Certificate can be awarded to a learner in the | |||
Credentials IDA. | |||
""" | |||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I was able to configure my devstack correctly so that I could generate certs via:
- self generated by the user
- generated by the instructor because the learner had enough grades
- generate by the instructor based on the allowlist
When I did this and tried to load the progress page, it failed due to a recursion stack limit. I don't really think that it is due to this, because reverting the code back to the previous master did not fix it, however it got me thinking if a better place to trigger this filter would be
def _generate_certificate_task(user, course_key, enrollment_mode, course_grade, status=None, generation_mode=None, |
This way we are doing the filter while the task is still in the sync process, before it's sent to the async task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe what you experienced has anything to do with this PR? In fact, it's in my backlog for review.
I believe you're right! That would be a better place to trigger the filter to avoid any shenanigans with async tasks. I'll move it to there and run some tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested:
- self generated by the user
- generated by the instructor because the learner had a passing grade (through the command line, I'm not sure which method you used)
- generated by the instructor based on the allowlist
And 1st the error you mentioned happened but disappeared after applying the PR I said.
What do you think? When the generation fails, for method 1 we should modify how it's displayed in the template, and for method 3 it just doesn't say the "Certificate generated" date in the instructor panel as before I believe.
In the meantime, I'll be fixing how to display the error and our unit tests that will fail after these changes.
e0254dd
to
ca3524a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, PR#29792 was indeed it and applying here fixed the recursion error.
Now I was able to test the generation when using a filter that stops the certificate generation and every attempt was blocked. This is good.
We must then catch the exceptions that are generating a 500 error and make them conform to what the situation is expecting.
I found calls to the generate_certificate_task
function in:
lms/djangoapps/certificates/signals.py
lms/djangoapps/certificates/api.py
lms/djangoapps/certificates/management/commands/cert_generation.py
lms/djangoapps/certificates/views/support.py
lms/djangoapps/courseware/views/views.py
lms/djangoapps/instructor_task/tasks_helper/certs.py
This would also cover generating certs via the command line and certs being autogenerated when the progess
page loads and auto_certificate_generation_enabled()
returns true.
One thing that I was left wondering is if it makes sense to change the definition of the Exception to the api.py module so that importing the models is not obligatory every time, but usage changes among those files and sometimes the api wrapper is used while others is directly with the generation_handler.
ca3524a
to
9374f7e
Compare
Thanks for your suggestions @felipemontoya!
I moved it to the generation_handler because of a circular dependency issue in api.py |
I'll add tests for the interactions with different apps after this round of feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for all the updates @mariajgrimaldi .
I believe this is now a lot better connected to the application. The user message when self_requesting is working perfectly.
For the allowlist generation that is causing the console to , you can catch the exception around _generate_certificate_task
in generate_allowlist_certificate_task
and return false
. This makes the log a little better contained. The result is the same and there is no change in the notes. If we wanted to put a message there we need to modify a lot more things and for the moment I don't believe its necessary.
The signals connection also worked nicely for me.
Great work. I think we need to update the library version, get the QA tests passing and squash the changes.
try: | ||
generate_certificate_task(user, course_key) | ||
except CertificateGenerationNotAllowed as e: | ||
raise CommandError(str(e)) from e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, instead of re-raising I would suggest we log it with WARN or ERROR level and move on to the next users. The command allows for a long list of users and the other scenarios where it fails (eg not enough grade) move on after logging so we should respect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good! Thanks
We are at the point where we would very much need another look at this and based on your previous work on this files, I think that the most probable experts for this would be:
Your comments here would be very valuable. For context we are targeting to merge this before Nutmeg is cut. Thanks |
We are at the point where we would very much need another look at this and based on your previous work on this files, I think that the most probable experts for this would be: @bseverino Your comments here would be very valuable. For context we are targeting to merge this before Nutmeg is cut. Thanks ps. I messed up the formatting of the last message so reviewer were not properly tagged. Posting again |
@MichaelRoytman has been looking around in here a lot recently for Cosmonauts |
dccff32
to
be24db4
Compare
Hi there @ashultz0! I've taken into account your suggestions, what do you think? Hello there to you too @MichaelRoytman! How does it been testing out this particular filter? |
The basic structure of the calls is still weird - I bet none of the caller even check the return type since it was always True in the past. But that feels outside of this PR's scope to change so I'm ok with it. I think we really need @justinhynes or another member of Aperture to weigh in with the actual 👍 , they are the certificate owners. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ashultz0 for the review.
I also think that the PR is good to go, but if @justinhynes or the Aperture team want to chime in we are happy to get their review.
Thanks @mariajgrimaldi for all the hard work.
Hello @justinhynes, we are trying to get this merged before the cut of the next named release (nutmeg) is made on Monday. Would you approve us merging this before that? This PR will not alter the application for anyone not using or configuring filters so on our eyes is super safe to merge. |
534f0c8
to
473bcc7
Compare
eb59704
to
d18b4e1
Compare
- Add filters interactions with code that used generate_certificate_task - Add unit-testing for filters - Upgrade to latest library update
d18b4e1
to
e8fa890
Compare
@mariajgrimaldi 🎉 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 Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
EdX Release Notice: This PR has been deployed to the production environment. |
1 similar comment
EdX Release Notice: This PR has been deployed to the production environment. |
<!-- 🌰🌰 🌰🌰🌰🌰 🌰 Note: the Nutmeg master branch has been created. Please consider whether your change 🌰🌰🌰🌰 should also be applied to Nutmeg. If so, make another pull request against the 🌰🌰🌰🌰 open-release/nutmeg.master branch, or ping @nedbat for help or questions. 🌰🌰 Please give your pull request a short but descriptive title. Use conventional commits to separate and summarize commits logically: https://open-edx-proposals.readthedocs.io/en/latest/oep-0051-bp-conventional-commits.html Use this template as a guide. Omit sections that don't apply. You may link to information rather than copy it. More details about the template are at openedx/open-edx-proposals#180 (link will be updated when that document merges) --> ## Description Backport filters that didn't make it to nutmeg release: **Add filter before certificate creation starts** (cherry picked from commit e8fa890) **Add cohort change filter before moving users from cohorts** (cherry picked from commit 465e5c0) **Add filter before certificate rendering process starts** (cherry picked from commit 7f974d1) **Add filter before course dashboard rendering process starts** (cherry picked from commit 895a649) **Add filter before course about rendering process starts** (cherry picked from commit ccfa0b4) **Integrate cohort assignment filter definition to cohort model** (cherry picked from commit ec69659) ## Supporting information Refer to the BTR wg github issue for the rationale behind this PR: openedx/wg-build-test-release#187 ## Testing instructions 1. Install the needed library release: `openedx-filters==0.7.0` 2. Install the samples library: `pip install git+https://github.com/eduNEXT/openedx-filters-samples.git@master#egg=openedx_filters_samples` 3. Then, configure each filter. If you want to test all the filters simultaneously, use this configuration and try to do each operation the filter is related to; the filter sample step will stop the operation. ``` OPEN_EDX_FILTERS_CONFIG = { "org.openedx.learning.certificate.creation.requested.v1": { "fail_silently": False, "pipeline": [ "openedx_filters_samples.samples.pipeline.StopCertificateCreation" ] }, "org.openedx.learning.cohort.change.requested.v1": { "fail_silently": False, "pipeline": [ "openedx_filters_samples.samples.pipeline.StopCohortChange" ] }, "org.openedx.learning.certificate.render.started.v1": { "fail_silently": False, "pipeline": [ "openedx_filters_samples.samples.pipeline.RenderAlternativeCertificate", ] }, "org.openedx.learning.dashboard.render.started.v1": { "fail_silently": False, "pipeline": [ "openedx_filters_samples.samples.pipeline.RenderAlternativeDashboard", ] }, "org.openedx.learning.course_about.render.started.v1": { "fail_silently": False, "pipeline": [ "openedx_filters_samples.samples.pipeline.RenderAlternativeCourseAbout", ] }, "org.openedx.learning.cohort.assignment.requested.v1": { "fail_silently": False, "pipeline": [ "openedx_filters_samples.samples.pipeline.StopCohortAssignment" ] }, } ``` Please, for detailed instructions on how to test each filter, refer to each of these PR(s): Filter for certificate creation: #29949 Filter for cohort change: #29964 Filter for certificate rendering: #29976 Filter for dashboard rendering: #29994 Filter for course about rendering: #29996 Filter for cohort assignment: #30431 ## Deadline For the next nutmeg release.
Description
As part of the Hooks Extension Framework implementation plan, this PR adds a filter before the certificate generation process starts.
Supporting information
ADR(s) on:
Testing instructions
We're currently using this version because it has the newer implementation of the certificate creation filter.
2. Implement your pipeline steps in your favorite plugin. We created some as illustration in openedx-filters-samples. We'll be using those in this example.
3. Install openedx-filters-samples
With this configuration, you won't be able to:
org.openedx.learning.certificate.creation.requested.v1
And with this one, the certificate mode is changed