[WIP] SE-1406 pluggable instructor dashboard

[samuelallan72]

Work in progress!

This PR includes fixes and features to support a cohort importer prototype that
we are building at https://github.com/open-craft/cohort-manager

Things currently modified in this PR:

• prototype support for adding a plugin page to the instructor dashboard via
  package resources / entry points
• temporary fix for 'CohortHandler' should either include a 'queryset' attribute, or override the 'get_queryset()' method error on accessing the
  cohorts list api.

JIRA tickets: OSPR-3804

Dependencies: None

Sandbox URL:

• LMS: https://pr21443.sandbox.opencraft.hosting/
• Studio: https://studio.pr21443.sandbox.opencraft.hosting/

Merge deadline: None

Testing instructions:

• install https://github.com/open-craft/cohort-manager in the lms environment
• view the instructor dashboard for a course and verify that the new cohort manager tab is displayed

Author notes and concerns:

• There is currently no api method to bulk-create cohorts. This may be a useful method to consider implementing (or expanding the existing cohort create endpoint) in the future (though beyond the scope of this PR).

Reviewers

• @clemente
• @bradenmacdonald
• edX reviewer[s] TBD

Settings

EDXAPP_EXTRA_REQUIREMENTS:
  - name: git+https://github.com/open-craft/cohort-manager.git@master#egg=cohort_manager

[openedx-webhooks]

Thanks for the pull request, @swalladge! I've created OSPR-3804 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• edx-code email 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 still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

[natabene]

@swalladge Thank you for your contribution. Please let me know once it is ready for our review.

[samuelallan72]

@natabene this is still a work in progress, but we consider this complete as a prototype now. We would be interested in hearing thoughts from edX regarding this idea of tab plugins in the instructor dashboard, and comments on how it should be implemented to be acceptable. Thanks.

[edx-status-bot]

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

[natabene]

@marcotuts Can you give this an early look?

[marcotuts]

This seems like a behind the scene change that @ormsbee (or someone else in engineering) can help move forward. Thanks for pushing forward the cause of pluggability! I want to also make sure @nasthagiri has visibility into this contribution as she and the Arch team are looking into pluggability more broadly.

[samuelallan72]

@natabene CC @marcotuts Any updates on when this might be looked at? Thanks. 🙂

[natabene]

@swalladge No, no updates yet.

[pomegranited]

@swalladge Since this is a prototype/proposal, could you submit a Plaform Development Proposal and link to this PR?

[samuelallan72]

@pomegranited done. :)

[bradenmacdonald]

@swalladge I discussed this with @nasthagiri yesterday and she is wondering if we can convert all the existing instructor dashboard tabs to be plugin-based. Would that be a big change?

[samuelallan72]

@bradenmacdonald This should be feasible. It wouldn't be a massive change in the backend, because already the code in instructor_dashboard.py is fairly modular. We already have a dynamic sections list: https://github.com/edx/edx-platform/blob/8882b8c60ddfa0a5247c5d09223b499123f770c6/lms/djangoapps/instructor/views/instructor_dashboard.py#L131-L138

Each _section_* function just takes the course and access variables and does most (if not all) of its processing based on the content of those vars.

Some tabs are conditionally added, but we can invert the logic there and include a method on the plugin like should_render to implement conditional inclusion of tabs.

We would also need to analyse the existing sections to determine any other common things used that would need to either be part of the plugin api (to pass extra required data) or that would need to be implemented differently from the external plugin.

The method of including templates, etc. would also need to be standardized. Unsure how much work that would be on the frontend and templates side since it's fairly complex and the instructor dashboard html template is quite complex.

[nasthagiri]

Thanks for taking a stab at the pluggability of tabs! 😄

Here are some of the advantages of refactoring the current tabs to be pluggable:

• It will be less likely that we inadvertently break the plugin functionality if the platform dog-foods the framework.
• More consistent implementation leads to better readability and future maintenance of the codebase.

The method of including templates, etc. would also need to be standardized. Unsure how much work that would be on the frontend and templates side since it's fairly complex and the instructor dashboard html template is quite complex.

Is it necessary to go through a standardization refactoring of these templates? Is that an additional nice-to-have or is it required in order to make the existing tabs pluggable?

Question: Once your refactoring is completed, how easy would it be for someone to plugin a React-based implementation of a new tab?

Thanks again!

[samuelallan72]

@nasthagiri

Is it necessary to go through a standardization refactoring of these templates? Is that an additional nice-to-have or is it required in order to make the existing tabs pluggable?

To be honest, probably not. It's fairly neat is it is. I hesitate in saying this though, because there's a lot of code going on there, so we may find some things difficult to work with plugins without some refactoring. Especially in the instructor_dashboard_2.html template.

Question: Once your refactoring is completed, how easy would it be for someone to plugin a React-based implementation of a new tab?

Fairly easily. I would say about the same amount of work as building a SPA plugin today. See https://github.com/open-craft/cohort-manager for an example that we have been using with this branch as an instructor dashboard plugin. The glue to actually be picked up as a plugin looks like this.

A hurdle that will be an initial blocker to including a full SPA as a tab in the dashboard is that there is a huge amount of other javascript on the dashboard page, which will conflict with javascript bundles used in the page unless extreme care is taken (we didn't get that far). With cohort-manager, we initially tried to build in into the dashboard, but gave up in the end and implemented it as a simple static page with a link to the actual SPA (the existing separate url spa plugin that is).

[nasthagiri]

@swalladge Thanks for the additional context.

A hurdle that will be an initial blocker to including a full SPA as a tab in the dashboard is that there is a huge amount of other javascript on the dashboard page, which will conflict with javascript bundles used in the page unless extreme care is taken (we didn't get that far). With cohort-manager, we initially tried to build in into the dashboard, but gave up in the end and implemented it as a simple static page with a link to the actual SPA (the existing separate url spa plugin that is).

I see and yes this has been a usual problem with making frontend changes in the legacy monolith. I see a few different paths here:

• Separate static page that is linked from the dashboard (either when clicking a tab or a link within a tab). This is the approach that our Masters team took with their gradebook feature, and looks like you did the same for cohort-manager.
• IFrame the plugin within the current Instructor Dashboard using resizing techniques as implemented in LabXChange and will possibly be adopted elsewhere in the platform. This sandboxes the plugin and prevents JS/CSS/etc conflicts.
• Develop a new Pluggable Instructor Dashboard using latest design thoughts on React-based Frontend Plugins from the Arch-FED team. This would be a higher effort, but would be aligned with our future direction of splitting the Frontend from the backend and will have a more consistent frontend plugin framework with our upcoming rewrites of the Courseware pages.

FYI @davidjoy and @abutterworth who are spending their thought cycles on these matters.

[samuelallan72]

@nasthagiri @marcotuts @natabene We noticed this still has the 'waiting on author' tag. Is there something we can do to move this forward?

Specifically, what changes will be required to this PR to be considered to merge? This doesn't convert all dashboard tabs to plugins, but it does provide a start that can be worked on in the future.

[natabene]

@nasthagiri Is there something you expect from @swalladge or can this be moved to review?

[natabene]

Team, @ormsbee is reviewing this, stay tuned.

[pomegranited]

Hi @ormsbee , have you had a chance to review this proposal yet?

[marcotuts]

This is something that @davidjoy will be taking for initial review, thanks for the ping on this @pomegranited!

[natabene]

@davidjoy Can you give this a look when you have a chance?

[nasthagiri]

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

[davidjoy]

Hi all -

As @nasthagiri mentioned above - @tuchfarber recently did some work to extend the plugin framework to allow plugins to add their context data to specific view contexts. See the PR here: https://github.com/edx/edx-platform/pull/23330

It seems like this might be applicable to this PR. Rather than the instructor dashboard needing to be aware of a plugin manager, the plugin itself could use the PluginContexts to inject its data into the instructor dashboard view - a nice inversion of control.

From the plugins README.rst:

# Configuration setting for Plugin Contexts for this app.
PluginContexts.CONFIG: {

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

        # Key is the view that the app wishes to add context to and the value
        # is the function within the app that will return additional context
        # when called with the original context
        u'course_dashboard': u'my_app.context_api.get_dashboard_context'
    }
}

What do we think about reorienting the approach to use this new PluginContexts mechanism? Technically, I think it'd mean a PR isn't necessary anymore. 😄

[samuelallan72]

@davidjoy This new PluginContexts is really cool!

I think it will be somewhat suitable for this use case. It would still require a PR, because the context updating code needs to be added for the instructor page, copying what was done for the student dashboard:

https://github.com/edx/edx-platform/blob/0a8b06a122a2b81e6fe85284ac68f005da335fa4/common/djangoapps/student/views/dashboard.py#L886-L891

The only other thing that concerns me is that the context dictionary is only added to under a plugins.my_plugin_name namespace, so it's impossible to update top level existing context:

https://github.com/edx/edx-platform/blob/0a8b06a122a2b81e6fe85284ac68f005da335fa4/openedx/core/djangoapps/plugins/plugin_contexts.py#L46

This was required here to add a new tab to the context["sections"] list. It would still be possible without, but would require overriding the instructor_dashboard_2.html template from a custom theme.

[davidjoy]

I think it will be somewhat suitable for this use case. It would still require a PR, because the context updating code needs to be added for the instructor page, copying what was done for the student dashboard:

Good point! I'm with you. 👍

The only other thing that concerns me is that the context dictionary is only added to under a plugins.my_plugin_name namespace, so it's impossible to update top level existing context:

https://github.com/edx/edx-platform/blob/0a8b06a122a2b81e6fe85284ac68f005da335fa4/openedx/core/djangoapps/plugins/plugin_contexts.py#L46

This was required here to add a new tab to the context["sections"] list. It would still be possible without, but would require overriding the instructor_dashboard_2.html template from a custom theme.

This is interesting... I get what you're saying. Let me propose an alternate approach and you can tell me if it makes sense for your use case. If the Tabs really need to go in the "sections" list so they look just like any other section, then you're right, we'd have to come up with some extra mechanism to do that.

That said - rather than overriding instructor_dashboard_2.html from a custom theme, what if that template had a "slot" where your plugin was allowed to render a template, configured in a way similar to PluginContexts? This is an approach we're discussing in https://github.com/edx/edx-platform/pull/21968. The plugin would register with the instructor dashboard saying that it had content to render at a slot that the view provides in its HTML DOM (see WIP code in that PR). When the view renders, it would load the template from the plugin and pass it the context it needs to do its job (an "allow list" of context data from the page, as well as any context data the plugin provided itself).

Again, assuming that you were adding the data into sections just because it was a convenient way to get it rendered without a custom template, I'm hoping that this is actually preferable in the end. What do you think?

Let me know if any of that doesn't make sense and I'll try to explain better. :)

[samuelallan72]

@davidjoy

If the Tabs really need to go in the "sections" list so they look just like any other section, then you're right, we'd have to come up with some extra mechanism to do that.

Yes, the original proposal was to be able to add another tab seamlessly. I like the idea you are proposing, but it would make it more work on the plugin side to integrate with the tabs layout. This may be preferable though, since it means less work in edx-platform.

There would need to be a slot added in the tabs navbar (ul.instructor-nav) as well as just before </main>, so the plugin would have a chance of building the tab page content (the section with id) and the tab button in the navbar.

I think this could work well. 👍

[pomegranited]

Glad to see there's been some agreement here on a way forward!
@swalladge , do you need anything more from @davidjoy to proceed with this work? Just adjusting OSPR priorities.

[samuelallan72]

@pomegranited I believe this PR can be closed, since it's only reinventing the wheel now that we have PluginContexts and slots.

[pomegranited]

Cool, thanks @swalladge. Feel free to close this, and link to it on any later PRs.

[openedx-webhooks]

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