-
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
feat: allow instructors to manage certificates #31265
feat: allow instructors to manage certificates #31265
Conversation
Thanks for the pull request, @dcoa! 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:
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. |
305196d
to
4d4483d
Compare
I think this is a good feature to take into consideration, several of our clients request it. What do you think @jmakowski1123 @santiagosuarezedunext? |
@jmakowski1123 @ProductRyan - flagging for you as it looks like this is awaiting Product approval. |
Putting this back on the radar @jmakowski1123 and @ProductRyan. |
Hi @dcoa - just checking in on this to see if the tests will be run? Looks like they may need to be re-run. |
4d4483d
to
58bde8b
Compare
Hi @mphilbrick211 yes it's okay re-run the test |
@mphilbrick211 tests running. |
Hi @dcoa ! Thanks for the contribution! As this is a feature that will impact end-users, it will need to undergo product review. To start that process, I've created a Feature Ticket, which contains a checklist of the information needed to help the reviewing product manager evaluate and make an approval decision. We'd like to get some more information from you regarding the potential impact of this permissions change for staff users, which as you note, can be a dangerous set of permissions. For example, does the decision to extend course instructors this new role still lie with superusers and/or course staff? Are there unintended risks, such as the Instructor getting access to additional permissions that they shouldn't? How would this risk be mitigated? Can this role expansion be managed at both the course and instance level? If you could provide additional information along these lines directly in the Feature Ticket, that will enable us to start the product review! We'll use the Feature Ticket to manage all product-related communications and decisions as well. Thanks! cc @mphilbrick211 |
Hi @dcoa! While this item is in product review, would you mind re-running the tests on this pull request? We've had some new ones pop up (Shellcheck) that need to be run. Thanks! |
Hello @mphilbrick211, any update with this PR? |
Hi @MaferMazu! Please check out this thread. This is on hold for now., though I've pinged Product again to see if they have an update. |
Hi @openedx/2u-aperture @jsnwesson! Could you please review / merge this for us? Product review is complete. Thanks! |
added to our prioritization queue, @mphilbrick211, hopefully we will get to it next week. In the meantime if someone gets a chance to look at the conflicts that would be great. |
Thank you, @deborahgu! @dcoa @MaferMazu would it be possible to resolve the branch conflicts so Aperture can review? |
We resolve the conflicts, but we need to look at this comment: openedx/platform-roadmap#230 (comment) because I'm not sure we are fulfilling that. |
@@ -15,6 +15,11 @@ | |||
ENABLE_CERTIFICATE_GENERATION = 'instructor.enable_certificate_generation' | |||
GENERATE_CERTIFICATE_EXCEPTIONS = 'instructor.generate_certificate_exceptions' | |||
GENERATE_BULK_CERTIFICATE_EXCEPTIONS = 'instructor.generate_bulk_certificate_exceptions' | |||
GENERATE_EXAMPLE_CERTIFICATES = 'instructor.generate_example_certificates' |
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.
Is GENERATE_EXAMPLE_CERTIFICATES
still viable these days? I thought generating example certificates were related to PDF certificates, and this functionality has been gutted from edx-platform for quite some time.
@dcoa I may be missing it, but according to the discussion here: openedx/platform-roadmap#230, my assumption was that there would be a waffle or configuration switch to enable this on a deployment. I don't think we want this functionality at 2U. This PR appears like it would be enabled and give access to this functionality in the instructor dashboard by default to all members of the course team. Can it be gated and configurable? |
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.
Is there any configuration/setting that gates this configuration for Open edX instances where we don't want course teams to have access to this functionality?
@@ -499,7 +499,6 @@ | |||
} | |||
|
|||
// hide the fancy | |||
.accomplishment-signatories .signatory-signature, |
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.
What is the purpose of this change? Was this supposed to be included?
Yes, the requirement in the product review was to implement behind a feature flag. Maybe it was simply overlooked and can be added now? |
Hi @justinhynes, As we explained here, we haven't applied the change that PWG asked for but we will work on that.
|
634aec2
to
4594dfc
Compare
hi, @justinhynes, I create the flag. |
2d81e4a
to
7a8cbf7
Compare
Define permissions using bridgekeeper for views related to certificates. Add support to instructor certificates generation. If user has instructor permissions over a course, the staff permissions are also granted.
7a8cbf7
to
417d2bc
Compare
thanks, @dcoa, we'll review now that the flag exists. |
I plan on taking another look at this today, thanks for the heads up @dcoa :) |
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.
This looks good! I'll plan on merging this today too.
@dcoa 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
Currently only super admin and staff are able to configure the certificates for the course.
This PR allows course instructors (admin course permissions) to manage certificates through all the actions available in the tab instructor > certificates, in this way, each instructor will be able to manage their certificates, which allows more granularity on the permissions granted to users. The staff permission is a somewhat dangerous permission, since it grants access to a wide variety of functions, with this change the instructors (who already manage the users, cohorts, and others) will also be able to manage the certificates.
This feature is used for eduNEXT due to client requests and we think is useful for the community, we put this into consideration for product reviewers (@santiagosuarezedunext @jmakowski1123 ).
Testing instructions
FEATURES
in your environment (to activate all certificate options):C.C @MaferMazu @Alec4r @felipemontoya @Ian2012