Skip to content
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

feat: external plugin integration in instructor dashboard #29376

Conversation

MAbdurrehman12
Copy link
Contributor

@MAbdurrehman12 MAbdurrehman12 commented Nov 22, 2021

Description

There are plugins which return context which is then embedded/used accordingly. So in instructor dashboard we need a flow/function which get/fetches the context from external plugins and updates the existing context with the context returned from external plugins. This has been done via a function get_plugins_view_context of edx_django_utils.plugins. A separate constant file has been created in instructor which consist of a constant: INSTRUCTOR_DASHBOARD_PLUGIN_VIEW_NAME. Any external plugin which uses this constant, will return context to instructor dashboard.

While installing an external plugin, all the settings should be derived first and add_plugins function should be called afterwards in lms -> envs -> production.py but currently add_plugins function is being called before derive_settings function, hence their order has been switched. Same goes for cms -> envs -> production.py

In lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html, we include the html files of sections from relative path, but in case of plugin, we need to give absolute path, as plugin's html file is inside plugin so in order to give absolute path we need to add a prefix "/".

Supporting information

None

Testing instructions

  • Any external plugin which uses the constant INSTRUCTOR_DASHBOARD_PLUGIN_VIEW_NAME to return any context, will be received in instructor dashboard.
  • If we call the add_plugins function before derive_settings function, then an exception can be seen while adding plugin's template in app_dirs.
  • If we call the add_plugins function after derive_settings function, then plugin's template is successfully added in app_dirs.
  • If we don't place a prefix "/" while including a html file from some other directory/app then an exception can be seen as it automatically adds relative path to it while including and the file can not be found on this path because plugin's html file is inside plugin templates.
  • If we place a prefix "/" while including a html file from some other directory/app then it includes the file successfully as now absolute path is provided by appending this prefix.

Deadline

None

Other information

None

@openedx-webhooks
Copy link

Thanks for the pull request, @MAbdurrehman12! I've created OSPR-6232 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 as you can:

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

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Nov 22, 2021
@MAbdurrehman12 MAbdurrehman12 force-pushed the Abdurrehman/external-plugin-integration-fixes branch 5 times, most recently from 9d3768a to f001232 Compare November 29, 2021 04:49
@MAbdurrehman12 MAbdurrehman12 force-pushed the Abdurrehman/external-plugin-integration-fixes branch 4 times, most recently from a7c8f83 to 85ad46c Compare December 1, 2021 06:15
@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@MAbdurrehman12 MAbdurrehman12 force-pushed the Abdurrehman/external-plugin-integration-fixes branch 5 times, most recently from e5d2f47 to 87f86ce Compare December 2, 2021 05:49
@ziafazal
Copy link
Contributor

ziafazal commented Dec 2, 2021

@natabene Can I review this PR?

@MAbdurrehman12 MAbdurrehman12 force-pushed the Abdurrehman/external-plugin-integration-fixes branch 2 times, most recently from bc655bf to 25748eb Compare December 3, 2021 07:03
@natabene
Copy link
Contributor

natabene commented Dec 3, 2021

@MAbdurrehman12 Thank you for your contribution. @ziafazal Thanks for offering, let me check with the owning team. I will post an update here.

@natabene
Copy link
Contributor

natabene commented Dec 3, 2021

@ziafazal Please review.

@ziafazal
Copy link
Contributor

ziafazal commented Dec 6, 2021

@MAbdurrehman12 Could you please add unit test to support you changes in instructor dashboard view?

@MAbdurrehman12
Copy link
Contributor Author

@MAbdurrehman12 Could you please add unit test to support you changes in instructor dashboard view?

Sure. I'll add it

@MAbdurrehman12 MAbdurrehman12 force-pushed the Abdurrehman/external-plugin-integration-fixes branch from 25748eb to 5bc4d4d Compare December 6, 2021 10:07
@MAbdurrehman12 MAbdurrehman12 force-pushed the Abdurrehman/external-plugin-integration-fixes branch from 55de451 to 90c74f7 Compare December 7, 2021 06:55
Copy link
Contributor

@ziafazal ziafazal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@MAbdurrehman12 MAbdurrehman12 changed the title fix: external plugin integration fixes feat: external plugin integration in instructor dashboard Dec 7, 2021
@ziafazal ziafazal merged commit 6c6cf0a into openedx:master Dec 7, 2021
@openedx-webhooks
Copy link

@MAbdurrehman12 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@MAbdurrehman12 MAbdurrehman12 deleted the Abdurrehman/external-plugin-integration-fixes branch December 7, 2021 11:20
@edx-status-bot
Copy link

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

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants