-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[BD-32] feat: add unenrollment Open edX Filter #29948
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
Conversation
|
Thanks for the pull request, @mariajgrimaldi! I've created BLENDED-1101 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. |
a3321f6 to
52955ad
Compare
9c5a53e to
3a67223
Compare
|
How exactly am I suppose to change the filter settings, should I create settings variable for both cases above?, should the |
|
Hi there @ghassanmas! Thank you for taking the time to review this. We appreciate it. Well, or |
|
Hey @mariajgrimaldi, I reviewed this and I found that on the backend it works just as I would expect. In your example, using Using Now, what I think it could still be improved, is that We would need to catch the exception at https://github.com/openedx/edx-platform/blob/db32ff2cdf678fa8edd12c9da76a76eef0478614/common/djangoapps/student/views/management.py#L413 to pass the str as the response message and make the js view support it at https://github.com/openedx/edx-platform/blob/db32ff2cdf678fa8edd12c9da76a76eef0478614/lms/static/js/learner_dashboard/views/unenroll_view.js#L70 |
|
Hi there, @felipemontoya. Well spotted! I already fixed the testing instructions 😄 I'll try adding the message to the UI, and let you know! Thank you again. |
|
Hello again @felipemontoya, what do you think about this? 😎 I'm currently implementing some unit tests for the new changes. Stay tuned! |
b452bbe to
b33e1a6
Compare
|
It looks great @mariajgrimaldi. Thanks! For anyone reviewing this, the message "You can't un-enroll from this site" is returned by the plugin, so the actual message will vary depending on the filter that is implemented. From my end this looks good and ready to be merged. Based on your previous work on this files, I think that the most likely reviewers for this would be:
Your comments here would be priceless. Thanks |
|
I got same result as what @felipemontoya had metioned above: Anyone can try that at http://edx.gsgapp.io the settings to cancel or stop unrollment is active |
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 believe the dashboard is the only spot you can unenroll from, yeah?
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.
Yes, I believe so! At least using this view
a4517ea to
6e408f0
Compare
6e408f0 to
73533f0
Compare
|
@mariajgrimaldi can we go ahead and merge this? we have 2 approvals now. |
Yes @felipemontoya, I'll do it first thing in the morning. Thanks for the reminder. |
|
@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 production environment. |
1 similar comment
|
EdX Release Notice: This PR has been deployed to the production environment. |


Description
As part of the Hooks Extension Framework implementation plan, this PR adds a filter before the unenrollment process starts.
Supporting information
ADR(s) on:
Testing instructions
With this configuration, you won't be able to:
org.openedx.learning.course.unenrollment.started.v1And with this one, the user's profile will be modified before the unenrollment process starts
Straightforward implementations as a way of illustrating how they work. More complex steps are coming.