SE-2483 Unhide student-generated certificates toggle - #23735
SE-2483 Unhide student-generated certificates toggle#23735samuelallan72 wants to merge 1 commit into
Conversation
|
Thanks for the pull request, @swalladge! I've created OSPR-4389 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:
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. |
openedx-webhooks
added
needs triage
open-source-contribution
pomegranited
left a comment
pomegranited
left a comment
There was a problem hiding this comment.
👍
- I tested this on the sandbox via the convoluted process for enabling automatically-issued certificates using the
staff@example.comuser (elevated to superuser):- Created a Certificate course
- Enabled self-pacing on the course.
- Added an honor course mode
- Created an activated an honor certificate
- Added an enabled CertificateGenerationConfiguration
- Verified on the Instructor Dashboard Certificate tab that I could enable/disable student-generate certificates.
- I read through the code
- I checked for accessibility issues
-
Includes documentationAlready documented under 14.5.3.2. Allow On-Demand Certificates (Self-Paced Courses Only); this PR makes the documentation correct again. -
I made sure any change in configuration variables is reflected in the correspondingN/A
client'sconfiguration-securerepository.
|
Thanks @pomegranited ! @natabene this is ready for edX review. 😄 |
|
@swalladge Thanks, queueing it for our review. |
openedx-webhooks
added
awaiting prioritization
product review
|
@marcotuts Please take a look once you have a chance. |
|
This isn't something we'd like to expose / show to all instructors, but I can understand how some instances might want to not make the same choice here. I don't think this is something we would merge as is. Any other options to address your concerns without introducing this button to all instructors once again? |
I think some instances definitely need access to be able to toggle on student-generated certificates for these edge cases. The backend already allows users with the ENABLE_CERTIFICATE_GENERATION permission to toggle this setting, so would it be acceptable to conditionally show that toggle button only if the user has that permission? |
|
Hi @marcotuts , do you have time to address the most recent proposal on this OSPR? CC @natabene |
cf https://github.com/edx/edx-platform/pull/23735 Unhide student-generated certificates toggle Student-generated certificates are meant to be auto-enabled through signal for self-paced courses, but it has been observed that this behaviour is flaky. When this happens, we are left in a state where it is impossible to enable student-generated certificates because the section is hidden in the instructor dashboard. Also by unconditionally displaying the toggle button, this allows more flexibility for instructors. For example, it may be desirable for student-generated certificates to be disabled for a self-paced course. (cherry picked from commit 86acd244d964e6281f3564d14e370e5bc79c2fbb)
cf https://github.com/edx/edx-platform/pull/23735 Unhide student-generated certificates toggle Student-generated certificates are meant to be auto-enabled through signal for self-paced courses, but it has been observed that this behaviour is flaky. When this happens, we are left in a state where it is impossible to enable student-generated certificates because the section is hidden in the instructor dashboard. Also by unconditionally displaying the toggle button, this allows more flexibility for instructors. For example, it may be desirable for student-generated certificates to be disabled for a self-paced course. (cherry picked from commit 86acd244d964e6281f3564d14e370e5bc79c2fbb)
|
@marcotuts @natabene any updates on when this might get another review pass? Thanks. :) |
|
@swalladge Sorry, not yet. Will try next week. |
|
Hi @natabene @marcotuts , this has been blocked for a really long time, and it's at the top of our priority list. Do you have an idea of when it might be reviewed? |
|
+1 on @pomegranited's comment; we're also carrying this patch locally at the moment and would love to see it land upstream. |
|
@pomegranited Sorry about that, we will try to address this today. |
|
@natabene: it's been another month, any news on this one? |
86acd24 to
7ac2e68
Compare
|
Rebased on master to check tests. |
Student-generated certificates are meant to be auto-enabled through signal for self-paced courses, but it has been observed that this behaviour is flaky. When this happens, we are left in a state where it is impossible to enable student-generated certificates because the section is hidden in the instructor dashboard. Also by unconditionally displaying the toggle button, this allows more flexibility for instructors. For example, it may be desirable for student-generated certificates to be disabled for a self-paced course.
|
Your PR has finished running tests. There were no failures. |
|
@natabene @marcotuts any updates on this? 🙂 |
|
@swalladge No, not yet, but it is lined to be reviewed in the coming weeks hopefully. Do you have a timeline you need to meet? |
|
@natabene thanks for the update. No, we don't have a timeline; we were just running routine checks on PRs that have been open for a while. :) |
|
Update: we are still looking into this from product perspective, we need to reach out to a couple more people internally. Stay tuned. |
|
@pomegranited UPDATE: we will need to hold until the team has the time to review, unless this is causing CAT-1/2 issues for your clients. We are currently reviewing the learner-generated certificate functionality overall, so we'll need a bit more time. |
|
I apologize for the major delays in this PR. My team and I are just coming up to speed on this. I am sorry if these questions have already been addressed in the past. As @marcotuts mentioned we would like to avoid exposing this option if possible. We are trying to move away from manual certificate generation. We have numerous auto certificate generation improvements coming. Would it be possible to articulate the flakiness issues that you are seeing that lead to the need to open this option up to others? One of the lines in your description of the PR concerns me when I look at the PR that originally added this check: https://github.com/edx/edx-platform/pull/12210
It seems that the intent of the PR that added this check was to make it so that we default to automatically generating certificates in new self-paced courses. I am still investigating the current state because the PR in question was made 5 years ago and much has changed since then. (https://github.com/edx/edx-platform/pull/12210
I will check in with Product on this goal. I currently believe that auto-generation is enabled by default on Master and we would like all of the courses to be that way.
I am going to look into this permission you are mentioning to see how it is applied and if it will work. The last question for now, is certificate auto generation enabled on the deployments that are running into this flakiness? The answer to this question will help to provide suggestions on moving forward. |
This reverts commit 8910ccf. Our clients are no longer using this feature, and edX won't accept this upstream as-is currently. See https://github.com/edx/edx-platform/pull/23735 for more information. Reverting to reduce code drift from upstream.
|
@staubina thanks for the update. Our clients are no longer using this, so we don't have the budget to investigate further or users to test the changes with, so we'll stop development on this. |
openedx-webhooks
added
rejected
and removed
product review
|
@swalladge Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
|
@swalladge Thank you for the update and for your time working on this. |
|
@staubina Please see the linked doc PR. I believe the fact that the documentation still pretends that this is a supported feature might potentially lead other contributors to believe that its non-functionality is a bug to be fixed. As evident from this discussion that is not the case, so it's probably best to explain that in the docs. |
This reverts commit 8910ccf. Our clients are no longer using this feature, and edX won't accept this upstream as-is currently. See https://github.com/edx/edx-platform/pull/23735 for more information. Reverting to reduce code drift from upstream.
* [SE-4101] fix: address VisibleBlocks caching race condition (openedx#27359) (#349) * fix: address VisibleBlocks caching race condition * sets visual block creation in an atomic transaction * refactor: add logging statement to bulk create Co-authored-by: Raul Gallegos <raul@opencraft.com> Co-authored-by: Raul Gallegos <raul@opencraft.com> * BB-3954 Add toggle for enrollment behavior (#351) Adds toggle REDIRECT_UNAUTHENTICATED_USER_TO_LOGIN_ON_ENROLL for enrollment behaviour for unauthenticated user. If true, the user will be redirected to 'signin_user' route. Co-authored-by: Arjun Singh Yadav <arjun@opencraft.com> * [BB-3622] feat:Restrict user create course (#319) (#352) * Add course creation condition for organization The condition added to ensure that if the feature is enabled user will not be able to create the course outside of the organization in which they belong. Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com> * fix: update the xblock-lti-consumer commit * Added new setting CERTIFICATE_DATE_FORMAT for easy customization of (#354) certificate issued date (cherry picked from commit 421e661) * fix: Produce grade report when subsections have future start dates When getting a subsection grade for a user, instead of failing if the user can't access that subsection, fallback to the collected structure. * Revert "Unhide student-generated certificates toggle" This reverts commit 8910ccf. Our clients are no longer using this feature, and edX won't accept this upstream as-is currently. See https://github.com/edx/edx-platform/pull/23735 for more information. Reverting to reduce code drift from upstream. * chore: update Arabic translations * fix:Fix function call to check MFE (#359) Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com> * fix: don't cache ajax request * feat: add site language config Cherry-picked from: https://github.com/edx/edx-platform/pull/27696 * Revert "[FAL-1813] fix: codejail issue when using matplotlib (#343)" (#365) This reverts commit 2143f84. * Update celery routing for celery 4+ (openedx#25567) * Update celery routing - Used routing function instead of class - Move task queues dictionary to Django settings - Removed routing_key parameter - Refactored routing for singleton celery instantiation Co-authored-by: Awais Qureshi <awais.qureshi@arbisoft.com> (cherry picked from commit e3b4d23) * fix: add missing set_code_owner_attribute imports (cherry picked from commit d1060d2) * chore: bump edx-django-utils to 3.12.0 The required `set_code_owner_attribute` decorator was introduced in v3.12.0, therefore we need to bump the dependency. (cherry picked from commit f52b84e) * [SE-4482] Allow delete course content in Studio only for admin users (#360) Co-authored-by: Nizar Mahmoud <nizarmah@hotmail.com> * fix: Password reset page throwing not found error (#364) Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com> * fix: change buttons in wiki modal to anchor tags The action buttons in wiki modal have href attribute but does not have an event listener for click. This PR changes the buttons to anchor tags so that they work as expected when clicked. * fix: use high priority queue for celery heartbeat check Makes HIGH_PRIORITY_QUEUE a derived setting, which allows HEARTBEAT_CELERY_ROUTING_KEY to use the correct config variant default. (cherry picked from commit 2a9067a) * fix: prevent invalidation of allowlisted certificates The allowlisted certificates were getting invalidated upon visiting the Course Progress page by users. This is a rough backport of the Lilac fix (edx#26356). In Lilac, this is gated by the `certificates_revamp.use_allowlist` Waffle flag. In post-Lilac branches, this is working out of the box (the flag has been removed in edx#27576). Jira ticket: BB-4287 * feat: add celery beat configuration Co-authored-by: Raul Gallegos <raul@opencraft.com> Co-authored-by: Farhaan Bukhsh <farhaan@opencraft.com> Co-authored-by: Arjun Singh Yadav <arjun@opencraft.com> Co-authored-by: pkulkark <pooja@opencraft.com> Co-authored-by: Shimul Chowdhury <shimul@opencraft.com> Co-authored-by: João Cabrita <joao.cabrita@opencraft.com> Co-authored-by: Samuel Walladge <samuel@opencraft.com> Co-authored-by: Giovanni Cimolin da Silva <giovannicimolin@gmail.com> Co-authored-by: 0x29a <demid@opencraft.com> Co-authored-by: Dmitry Gamanenko <dmitry.gamanenko@raccoongang.com> Co-authored-by: Jillian Vogel <jill@opencraft.com> Co-authored-by: Muhammad Soban Javed <58461728+iamsobanjaved@users.noreply.github.com> Co-authored-by: Sandeep Choudhary <sandeep@opencraft.com> Co-authored-by: Nizar Mahmoud <nizarmah@hotmail.com> Co-authored-by: Agrendalath <piotr@surowiec.it>
8 participants
Student-generated certificates are meant to be auto-enabled through
signal for self-paced courses, but it has been observed that this
behaviour is flaky. When this happens, we are left in a state where it
is impossible to enable student-generated certificates because the
section is hidden in the instructor dashboard.
Also by unconditionally displaying the toggle button, this allows more
flexibility for instructors. For example, it may be desirable for
student-generated certificates to be disabled for a self-paced course.
JIRA tickets: OSPR-4389
External discussions:
Others appear to be having similar issues with certificates, that may be helped by the changes in this PR: https://discuss.openedx.org/t/certificate-link-is-not-appearing-on-instructor-dashboard/936/11
Dependencies: None
Screenshots:
Sandbox URL:
Merge deadline: None
Test instructions:
Reviewers:
Settings