[BB-1674] Template plugins

[xitij2000]

Implements a system that allows plugins to inject static content into all pages.

JIRA tickets: OSPR-3878

Sandbox URL: https://pr21968.sandbox.opencraft.hosting/

Testing instructions:
TBD

Reviewers

• @giovannicimolin
• edX reviewer[s] TBD

Settings

EDXAPP_FEATURES:
  ENABLE_COMBINED_LOGIN_REGISTRATION: true

EDXAPP_EXTRA_REQUIREMENTS:
  - name: git+https://github.com/xitij2000/openedx-plugin-template-test.git@master#egg=test_template_plugin==0.1

[openedx-webhooks]

Thanks for the pull request, @xitij2000! I've created OSPR-3878 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:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

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.

[natabene]

@xitij2000 Thank you for your contribution. Please let me know once it is ready to be reviewed.

[xitij2000]

@natabene Sorry for not answering here since this was discussed elsewhere.

I'm currently hoping to get feedback on the general approach taken here. If the approach is OK I will finish up and polish this PR.

[nasthagiri]

FYI on this somewhat related PR: https://github.com/edx/edx-platform/pull/23330

[bradenmacdonald]

@nasthagiri Thanks for the ping about that other PR. Do you think we could get some feedback on the approach in this PR soon? I think they might go hand-in-hand and offer us a lot more flexibility, but we've been keeping this work on hold until we get some input from you.

[nasthagiri]

@bradenmacdonald I completely agree that this PR and the other PR can go hand-in-hand and can provide an excellent fuller story for frontend pluggability through Django. It would be great to have the changes in this PR incorporated into our existing backend plugins framework.

Thanks for proposing these changes.

@davidjoy will be adding more context and comments on this PR later this afternoon.

[davidjoy]

Hi all -

To expand on what @nasthagiri said above, I'm wondering if there are a few tweaks we can make to further incorporate this functionality into the existing plugin framework.

The plugin_includes function is effectively a way of creating what we've coined a "slot" - i.e., a pre-defined mount point where a plugin could choose to display some HTML in a template. To my knowledge, the plugin framework doesn't have a first-class way of doing this today. Rather than having that code live in theming, I'm wondering if it can live in the plugin framework.

Templates could have pre-defined slots for plugins to add content, and the plugin could register for slots in their plugin configuration.

Something like:

# Configuration setting for Plugin Slots for this app.
PluginSlots.CONFIG: {

    # Configure the Plugin Settings for each Project Type, as needed.
    ProjectType.LMS: {

        # Configure each Slot, as needed.
        'head-extra': <reference to view to include in slot>,
        'body-initial': <reference to view to include in slot>
    }
},

This is modeled after the rest of the existing plugin configuration - I'm not 100% sure on the best way to represent those "" comments above. We can probably use the existing PluginContexts configuration to make sure the plugin templates have necessary context from the plugin when they're mounted. And - getting into the weeds a bit - we could have a context filter/adapter for additional context provided by the platform itself so that the containing view/platform can choose what context is appropriate to hand down to the plugin.

Thoughts on trying to bring some of this into the framework?

[xitij2000]

Hi @davidjoy

Sorry for the very delayed response. It seems I composed a reply early but it either didn't go through or I forgot to actually submit it.

I've used the same "slot" terminology in my prototype, and I think the approach you've outlined for configuring them makes a lot of sense. I will adapt my prototype to use that approach.

I definitely think that the platform should restrict what's passed in the context to templates, and that bare-minimal context should be kept compatible across releases. For the the use case for which I developed this, simply having access to the request object is enough, so I was planning to start there.

I can follow a similar implementation as the contexts PR. When passing a context to template plugins, the context can be default include only a whitelisted set of data, like the requests. This whitelist can be overridden if needed, all other context will be stripped out.

If a template plugin needs additional context, it can always include a context plugin as well that adds back the context it needs. The context passed to the context plugin will always include the full context.

Does that approach sound OK?

[davidjoy]

I definitely think that the platform should restrict what's passed in the context to templates, and that bare-minimal context should be kept compatible across releases. For the the use case for which I developed this, simply having access to the request object is enough, so I was planning to start there.

Yeah, I agree that we should be mindful about what context is exposed to the plugin. I'm not 100% sure what that looks like, but I imagine that it's some sort of filter that lets the containing page decide what context gets passed to a given plugin slot. I'm inclined to only pass through what we know we need today for the use cases we know about (in the spirit of YAGNI). As new use cases emerge, we can revisit passing further context through to the plugins.

I can follow a similar implementation as the contexts PR. When passing a context to template plugins, the context can be default include only a whitelisted set of data, like the requests. This whitelist can be overridden if needed, all other context will be stripped out.

This sounds great. I'm expecting this whitelist will not be plugin specific, but will be dictated by the code that exposes the plugin slots, so it can keep plugins from accessing sensitive data (though I couldn't say exactly what that'd be right now! Probably some PII)

If a template plugin needs additional context, it can always include a context plugin as well that adds back the context it needs. The context passed to the context plugin will always include the full context.

This one will be interesting to figure out. I expect we'll want a "context plugin" to have to adhere to the same whitelists, to the extent that the whitelist is enforcing security and/or performance considerations. If that's the case, then it may be that our only recourse for adding in more context is to update the whitelist. I don't feel strongly about this yet; that's more of a gut reaction.

Does that approach sound OK?

Sure does!

[bradenmacdonald]

@xitij2000 I'm working to document all of the platform's extension points in https://github.com/edx/edx-platform/pull/24057 . If that PR merges before this one, can you please update this one to add an explanation of the new extension mechanism into the extension_points.rst doc file? And if yours merges first, I'll do that :)

[xitij2000]

@bradenmacdonald Sure, will do!

[xitij2000]

This is just a sample usage, not a suggestion or recommendation for how the namespaces should be named.

[xitij2000]

@davidjoy This is good for another review pass.

CC: @nasthagiri

[tuchfarber]

This was also a worry on plugin_contexts since they get ran on every view. My thoughts on it are that with or without the this feature, if this code needs to be ran, it needs to be ran. So the poorly performing code would just be in edx-platform instead of a separate plugin. Due to the isolated nature of plugins, you should be able to disable a poorly performing one easier than ripping out the code in platform since nothing else will depend on it.

[tuchfarber]

Just want to clarify that this is "three and only three" slots. My worry is that we'll have a rapid proliferation of slots for things like "Well I want to show something below the X, but above the Y" Which is something I'd like to support in a future world, but I don't think this system is right for it. Likely we'll have to do something in our MFE framework for that.

[xitij2000]

@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.

[davidjoy]

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.

[tuchfarber]

I like the idea of a context_allow_list and felt the need for something similar when I did the plugin_contexts work. I'm struggling a bit with the idea that you can have a wild card, but we also want the context keys to be "maintained across releases". Template context just doesn't feel like something to be considered a stable API.

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 context_allow_list or the context if it's context_allow_list="*".

[tuchfarber]

On some views this will call the plugin's context function twice since they are already called in the view. I understand the goal of "My plugin should be able to add context, and use that context in new HTML/slots without touching edx-platform" though. So that part is fine.

The get_plugins_view_context parameters are (project_type, view_name, existing_context) though and this is passing in a plugin_name instead of view_name. So I don't think this will work. If we're going to do something like this I would like for us to be able to pass in a plugin_name parameter so that we go from
Allow plugins to extend the context for other plugins
to
Allow plugins to extends the context for themselves

I.e. The only additional context a plugin gets here is bits that they've created. I don't think it makes sense for plugin's to rely on each other. (That's a larger argument though).

[xitij2000]

Passing in the plugin name is intentional, and aims to allow plugins to extend their own context by using their own name, of the context of another plugin.

If plugina has supports extending the context of pluginb, then in the case that plugina isn't installed, that part of the code just won't run. So, it's not really causing a reliance between plugins.

That said, I do think this isn't really very useful and it makes sense to just drop it, in favour if just passing the whole context unfiltered context to the plugin. The goal of filtering the context was to pass bare minimum guaranteed context, so that the template context doesn't become something that can break plugins across releases. However, now we have context plugins which are going to break anyway if the context changes, and they rely on a specific context shape.

So we can just pass the entire context to the plugin and then it doesn't need to plug into the platform twice for one view.

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[xitij2000]

@davidjoy I guess I should move this PR to https://github.com/edx/edx-django-utils?

Would you like to finish and approve the review here before I move it, or should I move it already?

[bradenmacdonald]

@nasthagiri @davidjoy We don't seem to have any active projects that need this anymore, and with the move to MFEs I imagine it won't be useful for too long anyways. Is it something you're still interested in, and if so, would you like to take over finishing it up, or perhaps have us complete it under some existing scope of work we're doing for you? It may otherwise languish on the backburner given our other priorities.

[davidjoy]

Hi @bradenmacdonald and @xitij2000 ,

Admittedly, I've struggled with this PR for a while. I think the functionality has value, but I worry that some of the details of the approach make view rendering tightly coupled/dependent on functions in third-party plugin code, which seems like a dangerous road to walk down, and doesn't give us very much isolation and graceful degradation for bad-acting plugins.

I've been having conversations and asking for some other folks to weigh in and review it with me, and these sorts of concerns kept coming up, though it's taken me a while to figure out exactly what we were nervous about and how to verbalize it. Frankly, I haven't been able to come up with a suggestion on how to mitigate those concerns, and have been hesitant to invest too much brain power in the effort, since it seemed like something much more 'do-able' via frontend-based JS plugins.

I agree, I think, that given the continuing shift to MFEs, the value here is dropping pretty fast. If you all don't have a use case anymore, I'd say (with abashed sadness) that maybe we close it out.

[xitij2000]

@davidjoy I think what you're saying makes sense. It is also why I had essentially paused work on this PR for a while as I was reevaluating a different approach for what we wanted. I will close this PR now. It in any case no longer makes sense in this repo.

[openedx-webhooks]

@xitij2000 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.