-
Notifications
You must be signed in to change notification settings - Fork 4.3k
[BB-1674] Template plugins #21968
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
[BB-1674] Template plugins #21968
Changes from all commits
b7c4d82
5ece183
aaf6e79
539623d
656ede0
acca5c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,8 @@ class CoursewareIndex(View): | |
| View class for the Courseware page. | ||
| """ | ||
|
|
||
| slot_namespace = "courseware:index" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another sample usage to show how it will work for classes. |
||
|
|
||
| @cached_property | ||
| def enable_unenrolled_access(self): | ||
| return COURSE_ENABLE_UNENROLLED_ACCESS_FLAG.is_enabled(self.course_key) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| from typing import Callable | ||
|
|
||
| from functools import wraps | ||
|
|
||
|
|
||
| def view_namespace(slot_namespace: str) -> Callable: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this won't work for class-based views. Will look into that. |
||
| """ | ||
| Adds the "slot_namespace" attribute to the decorated view. | ||
|
|
||
| This is used by the template slots plugin mechanism to find which view to decorate. | ||
| :param slot_namespace: The namespace for the slots rendered by this view. | ||
| """ | ||
| def decorator(view): | ||
| @wraps(view) | ||
| def wrapper(*args, **kwargs): | ||
| return view(*args, **kwargs) | ||
| setattr(wrapper, 'slot_namespace', slot_namespace) | ||
| return wrapper | ||
| return decorator | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| Plugin Slots | ||
| ------------ | ||
|
|
||
| Status | ||
| ====== | ||
| Draft | ||
|
|
||
| Context | ||
| ======= | ||
| edx-platform contains a plugin system (https://github.com/edx/edx-platform/tree/master/openedx/core/djangoapps/plugins) | ||
| which allows new Django apps to be installed inside the LMS and Studio without | ||
| requiring the LMS/Studio to know about them. This is what enables us to move to | ||
| a small and extensible core. While it's possible to extend the content of pages | ||
| rendered by the platform using templates, via certain extension points that allow | ||
| injecting content into 'head-extra', 'body-initial' etc slots in the base template, | ||
| it isn't possible for plugins to inject content at all. | ||
|
|
||
| Decisions | ||
| ========= | ||
| We have added the ability for plugins to render content into existing pages. To | ||
| support this, we have decided: | ||
|
|
||
| * A template can how declare slots into which a plugin can inject content, by | ||
|
xitij2000 marked this conversation as resolved.
Outdated
|
||
| using the `plugin_slot` template tag (for Django template) or function (for | ||
| Mako templates). | ||
| * Plugins can define a callable function that the LMS or Studio can import and | ||
| call. This function will be called with minimal context, and in turn supports | ||
| pluggable contexts. | ||
| * The callable function should return direct HTML content as text that can be | ||
| rendered on page. | ||
| * Each view can provide an list of what context data should be made available to | ||
| all plugins by adding it to the context itself in a list called | ||
| `context_allow_list`. This will need to be maintained across releases so | ||
| should be kept to a bare minimum. | ||
| * A plugin will need to specify the namespace in which that slot should be active. | ||
| Different views can be under different namespaces, such as: | ||
| - 'course_home' | ||
| - 'learner_dashboard' | ||
| - 'instructor_dashboard' | ||
| * All templates/pages will support three slots: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just want to clarify that this is "three and only three" slots. My worry is that we'll have a rapid proliferation of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tuchfarber My original goal for this work was simply to allow plugins to plug into the same three slots that you get via a theme, head-extra, body-initial, and body-extra. I'm starting there, and leaving the potential to hook into more places in the future. This need not proliferate, any additions will have to go through edX review, so they can be stopped at that point. I also don't imagine that there should be hundreds of slots, just a few minimal slots in a few key places.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The MFE world will have a similar concept, with the idea being that we can be very intentional about where we choose to add more slots. We can choose to allow particular UIs/pages to be extensible in particular places, this way. But yeah, we don't want them added willy nilly; maybe a sidebar here, additional headers there, etc. |
||
| + ``head-extra``: This slot exists near the end of the header tag for each page | ||
| and can be used to add scripts, metadata, stylesheets or other header content. | ||
| It is equivalent to adding a 'head-extra.html' template file. | ||
| + ``body-initial``: This slot exists at the start of the page, right after the | ||
| opening of the body tag. It is equivalent to adding a 'body-initial.html' | ||
| template file. | ||
| + ``body-extra``: This slot exists at the end of the page near the closing of | ||
| the body tag. It is equivalent to adding a 'body-extra.html' template file. | ||
|
|
||
| Implementation | ||
| ============== | ||
|
|
||
| In the plugin app | ||
| ~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Config | ||
| ++++++ | ||
|
|
||
| Inside of the AppConfig of your new plugin app, add a "slots_config" item like below. | ||
|
|
||
| * The format will be ``{"slot_name": "function_inside_plugin_app"}`` | ||
| * The function name & path don't need to be named anything specific, so long as they work | ||
| * These functions will be called on **every** render of that view, so keep them | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ormsbee I'm curious what you think of this approach, in terms of sandboxing of misbehaving plugins. Is there a safer way we could allow plugins to render HTML content to be injected into the page? Or is the warning on efficiency/memoization here the best we can do? I'm thinking about your presentation about misbehaving
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As is, this would make our response rendering performance directly beholden to the efficiency of our plugins. I don't see a great way around it while still enabling this sort of behavior, though.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was also a worry on |
||
| efficient or memoize them if they aren't user specific. | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| class MyAppConfig(AppConfig): | ||
| name = "my_app" | ||
|
|
||
| plugin_app = { | ||
| "slots_config": { | ||
| "lms.djangoapp": { | ||
| "view_namespace": { | ||
| "body-initial": "my_app.slots_api.get_body_initial_content" | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Function | ||
| ++++++++ | ||
| The function that will be called by the plugin system should accept a single | ||
| parameter which will be the context for that slot. It should then return an | ||
| HTML string that will be injected in the specified slot. | ||
|
|
||
| Example: | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| def my_slot_function(context): | ||
| return render_to_string('my_app/template.html', context=context) | ||
|
|
||
|
|
||
| In the core (LMS / Studio) | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| The view you wish to add slots to should have the following pieces enabled: | ||
|
|
||
| * A constant defined inside the apps for the slot name. | ||
| * Decorate the view with `@view_namespace("namespace")` to make it work with | ||
| slots in that namespace. (This should be the very first decorator on that | ||
| view function). | ||
| * The view can add an entry called `context_allow_list` to its context. This | ||
| should either be equal to '*', or be a list of context entries that are | ||
| allowed to be passed to plugin slots. If omitted, only the current request | ||
| and url are passed through. | ||
|
Comment on lines
103
to
106
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of a I don't have a good solution to this, but assuming this gets merged in roughly as-is, it might be worth pointing to what happens when you need to make changes to either |
||
| * The template can include a line like the following to declare a new slot. | ||
|
|
||
| ``${plugin_slot(context, 'lms.djangoapp', 'slot_name') | n}`` | ||
|
|
||
| Here ``lms.djangoapp`` or ``studio.djangoapp`` can be used to specify if this | ||
| is an slot in the LMS or Studio. The slot name should be unique for each | ||
| project. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| from django.utils.deprecation import MiddlewareMixin | ||
|
|
||
|
|
||
| class ViewNameAndSlotMiddleware(MiddlewareMixin): | ||
| """ | ||
| Django middleware object to inject view name into request context | ||
| """ | ||
| def process_view(self, request, view_func, view_args, view_kwargs): | ||
| """ | ||
| Injects the view name value into the request context | ||
| """ | ||
| request.view_name = view_func.__name__ | ||
| # For class-based views the view function will have a `view_class` attribute | ||
| # and we can get the slot_namespace from that | ||
| view = getattr(view_func, 'view_class', view_func) | ||
| request.slot_namespace = None | ||
| if hasattr(view, 'slot_namespace'): | ||
| assert isinstance(view.slot_namespace, str) | ||
| request.slot_namespace = view.slot_namespace |
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 is just a sample usage, not a suggestion or recommendation for how the namespaces should be named.