-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor: Refactor & add required rapid response counterpart #275
Conversation
@@ -237,6 +239,10 @@ def instructor_dashboard_2(request, course_id): # lint-amnesty, pylint: disable | |||
|
|||
certificate_invalidations = CertificateInvalidation.get_certificate_invalidations(course_key) | |||
|
|||
# x block asides enabled, included rapid response tab | |||
if XBlockAsidesConfig.current().enabled: |
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.
@pdpinch We've been adding the Rapid Responses
tab in the instructor dashboard based on a condition if xBlockAside is added/enabled
. Should I keep it like that or should I do it like something similar as we did for canvas integration e.g. Show the canvas tab if the plugin is not installed - (We removed a feature flag there)
?
*Edit(17:56 GMT+5) Show the canvas tab if the plugin is installed
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 think that's a better pattern and it would be good to be consistent.
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. Updated my message above, I meant Show the canvas tab if the plugin is installed
. But you already got it right I believe.
691ae7c
to
f6c7cc6
Compare
@@ -237,6 +237,21 @@ def instructor_dashboard_2(request, course_id): # lint-amnesty, pylint: disable | |||
|
|||
certificate_invalidations = CertificateInvalidation.get_certificate_invalidations(course_key) | |||
|
|||
# x block asides enabled, included rapid response tab |
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 don't think this comment is necessary
if 'ol_openedx_rapid_response.app.RapidResponsePluginConfig' in settings.INSTALLED_APPS: | ||
from openedx.core.djangoapps.plugins.constants import ProjectType | ||
from edx_django_utils.plugins import get_plugins_view_context | ||
from ol_openedx_rapid_response.constants import RAPID_RESPONSE_PLUGIN_VIEW_NAME # pylint: disable=import-error |
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.
These inline imports aren't great for a few reasons. One reason is that if there's an import error, we wouldn't know until this function is called. I tried this locally and it seems to work.
Add this to the bottom of the imports at the top of the module:
rapid_response_enabled = 'ol_openedx_rapid_response.app.RapidResponsePluginConfig' in settings.INSTALLED_APPS
if rapid_response_enabled:
from openedx.core.djangoapps.plugins.constants import ProjectType
from edx_django_utils.plugins import get_plugins_view_context
from ol_openedx_rapid_response.constants import RAPID_RESPONSE_PLUGIN_VIEW_NAME # pylint: disable=import-error
Then update this block:
if 'ol_openedx_rapid_response.app.RapidResponsePluginConfig' in settings.INSTALLED_APPS: | |
from openedx.core.djangoapps.plugins.constants import ProjectType | |
from edx_django_utils.plugins import get_plugins_view_context | |
from ol_openedx_rapid_response.constants import RAPID_RESPONSE_PLUGIN_VIEW_NAME # pylint: disable=import-error | |
if rapid_response_enabled: |
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 gave it a cleaner look instead of checking the plugin installation and then fetching the context we'll just fetch the context which will be a dict/None
based on if the plugin is installed or not. Also, there has been an Upstream PR for making more general improvements and when that's merged we'll make our plugins context generic too.
1caed0e
to
f9b8045
Compare
@gsidebo This is ready for review again. |
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.
Looks great! Great job on finding get_plugins_view_context
. That's so much cleaner than the conditional imports
f9b8045
to
b65920c
Compare
b65920c
to
bcad850
Compare
feat: Add support for Rapid Response Reports plugin (#275)
Related Ticket:
#273
Description
We did some platform changes to use some of the rapid response functionality & later on we used to cherry-pick that with every Open edX release. Now we are trying to refactor and migrate all this code into a plugin in https://github.com/mitodl/open-edx-plugins.
What this PR does?
How to test?
This should be tested once we create a rapid response plugin in https://github.com/mitodl/open-edx-plugins.