-
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 2nd batch of Open edX Filters #29840
Conversation
Thanks for the pull request, @mariajgrimaldi! I've created BLENDED-1080 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. |
try: | ||
context = DashboardRenderStarted.run_filter(context=context) | ||
except DashboardRenderStarted.PreventDashboardRender as exc: | ||
raise DashboardRenderNotAllowed(reverse(exc.redirect_to or 'account_settings')) from exc |
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 should happen when interacting with templates?
As it is right now, when filters that interact with templates (i.e they interact with template contexts) fail they raise an exception with the intent to redirect to some constant view (e.g if course about filter fails, then redirect to dashboard). If there's a handler that interprets this kind of exception, then the redirection occurs (as it happens when using CourseAccessRedirect here where this handler catches the exception and manages the redirection). If not, like in this case, then error 500 is thrown. So we can replicate this behavior we should implement a handler that manages exceptions in the student's dashboard, but that's probably out of the scope of this PR.
Either way, should be this way the standard when interacting with templates?
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 like the idea that we should have a consistent way of interacting with templates when filters are involved. Having the ability to redirect somewhere and changing the template_name that should be rendered and naturally updating the context before rendering are the three that come to my mind now.
Awesome! 🚀 |
6c3a77b
to
8bd19c6
Compare
8bd19c6
to
aaf1789
Compare
I tested some filters and make the BD-32 Open edx Filters document with the results of the testing from description. I hope it can help!! |
Knowing that we are modifying multiple applications and this can be difficult to review all at once, I'll be dividing this PR into smaller chunks! |
PR closed in favor of: PR 29948: PR 29949: PR 29976: PR 29964: PR 29996: PR 29995: PR 29994: |
@mariajgrimaldi 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. |
Description
This PR adds the 2nd Open edX Filters batch as part of the implementation plan of Hooks Extension Framework:
Supporting information
ADR(s) on:
Testing instructions
With this configuration, you won't be able to:
org.openedx.learning.certificate.creation.requested.v1
org.openedx.learning.certificate.render.started.v1
org.openedx.learning.course.unenrollment.started.v1
org.openedx.learning.course_about.render.started.v1
org.openedx.learning.course_home.render.started.v1
org.openedx.learning.course_home.render.started.v1
org.openedx.learning.cohort.change.requested.v1
To change the behavior you can replace
Stop<process>
withNoopFilter
or remove it completely.Notes about the filters steps:
The other ones halt the app execution.
Simple and straightforward implementations as a way of illustrating how they work. More complex steps are coming.