Skip to content

Document the various integration points for platform extensions - #24057

Merged
nasthagiri merged 3 commits into
openedx:masterfrom
open-craft:extension-points-doc
Jun 10, 2020
Merged

Document the various integration points for platform extensions#24057
nasthagiri merged 3 commits into
openedx:masterfrom
open-craft:extension-points-doc

Conversation

@bradenmacdonald

Copy link
Copy Markdown
Contributor

As far as I know, we're missing a comprehensive guide to all the different ways that plugins or applications can integrate with Open edX. Here's my attempt to fix that :)

Now that we have Automatic notifications of ADR changes, my thought is that we can use the ADR watcher bot to watch this document as well, so that anyone who has asked to be notified of architectural decisions will get pinged when someone opens a PR that proposes a new extension point (provided their PR includes a change to this doc file).

Open question: is https://github.com/edx/edx-platform/blob/master/docs/guides/index.rst published somewhere in compiled form? It contains Sphinx directives that GitHub doesn't render, but I couldn't find a compiled version of it. If we're just using GitHub to view it (which I think is great), we should probably drop the toctree etc.

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @bradenmacdonald! I've created OSPR-4580 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.

@nedbat

nedbat commented May 26, 2020

Copy link
Copy Markdown
Contributor

The ADR bot didn't send a notification about this. It isn't watching docs/*.rst: https://github.com/edx/pr_watcher_configuration/blob/master/config.yml#L4-L9

  patterns:
    - "decisions/*.rst"
    - "decisions/*.md"
    - "*/decisions/*.rst"
    - "*/decisions/*.md"
    - "oeps/*.rst"
    - "oeps/*.md"

@pomegranited pomegranited left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bradenmacdonald Thank you for this! Left some nits, but that's it.

FYI, if you could hold off addressing these nits until openedx/pr_watcher_configuration#12 is merged and deployed, then we can test that the ADR alerts happen as expected.

Comment thread docs/guides/extension_points.rst Outdated
Comment thread docs/guides/extension_points.rst Outdated
| | | contexts. However, Blockstore-based content libraries are. |
| | | See |learning_context.py|_ to learn more. |
+---------------------+------------------------------------------+-------------------------------------------------------------+
| User partition | ``openedx.user_partition_scheme`` and | A user partition scheme is a named way for dividing users |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oo cool, I didn't know about this one!

Comment thread docs/guides/extension_points.rst Outdated
Comment thread docs/guides/extension_points.rst Outdated
@natabene

Copy link
Copy Markdown
Contributor

@bradenmacdonald Thank you for your contribution. @nasthagiri Will you review this?

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

@nedbat

The ADR bot didn't send a notification about this. It isn't watching docs/*.rst: https://github.com/edx/pr_watcher_configuration/blob/master/config.yml#L4-L9

Right, I should have mentioned that I was going to open a PR to address that after this one had been reviewed, but now @pomegranited has done that for me - thanks :)

@nasthagiri

Copy link
Copy Markdown
Contributor

@natabene @bradenmacdonald Reviewing this PR is now at the top of my backlog. I will prioritize this tomorrow. Thanks!

@@ -0,0 +1,165 @@
Options for Extending the edX Platform

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bradenmacdonald Thank you for taking the time to put this together.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nasthagiri Thanks for the quick and helpful review! I've addressed all your comments.

.. contents:: **Integration Methods Overview**


REST API Integration with External Applications

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we add something like Support Level to each of these?

While the xBlock and Django App Plugin interfaces are intended for versioned support, we have yet to invest that time on our REST APIs. By listing it in this doc (and also as the 1st item), I am concerned folks will expect them to be stable APIs. At this time, only a few of our REST APIs are expected to be versioned and maintained for compatibility. Once we invest in standardizing our API practices (listed in Needed Architecture Investments), we will have a better foundation to support more stable REST APIs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can we add something like Support Level to each of these?

Once again, perhaps a field or some-such to indicate Support level would be useful.
In Tech Radar speak, we have the following categories: Adopt, Trial (or Invest), Assess, and Hold. See their definitions in this glossary. I believe a similar rating for each of the extensions in this doc would be useful to let people know the support level of its usage.

Sure, in my latest commit, I've proposed a new Status (Adoption and Completion) scheme, and added my best guess for each one. Please take a look!

Comment thread docs/guides/extension_points.rst Outdated
Comment thread docs/guides/extension_points.rst Outdated
Comment thread docs/guides/extension_points.rst Outdated
Comment thread docs/guides/extension_points.rst
Comment thread docs/guides/extension_points.rst Outdated
Comment thread docs/guides/extension_points.rst
Comment thread docs/guides/extension_points.rst Outdated
.. |UserPartition docstring| replace:: ``UserPartition`` docstring
.. _UserPartition docstring: https://github.com/edx/edx-platform/blob/f8cc58618a39c9f7b8e9e1001eb2d7a10395797e/common/lib/xmodule/xmodule/partitions/partitions.py#L105-L120

Platform Look & Feel

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@davidjoy FYI on this section

@nedbat

nedbat commented Jun 4, 2020

Copy link
Copy Markdown
Contributor

I've taken Nimisha's suggestion, and added "guides" as a directory the bot watches.

@edx-status-bot

Copy link
Copy Markdown

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

@nasthagiri nasthagiri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great @bradenmacdonald!

Thanks for adding the status (adoption and completion) values. I'm hoping those will clarify our recommendations for folks.

I'll go ahead and squash before merging.

@nasthagiri
nasthagiri merged commit 831a90e into openedx:master Jun 10, 2020
@openedx-webhooks

Copy link
Copy Markdown

@bradenmacdonald 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@bradenmacdonald
bradenmacdonald deleted the extension-points-doc branch June 11, 2020 00:41
@edx-pipeline-bot

Copy link
Copy Markdown
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
Copy Markdown
Contributor

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

Comment on lines +105 to +107
- A "Django app plugin" is a self-contained Django `Application`_ that can define models (MySQL tables), new REST APIs, signal listeners, asynchronous tasks, and more. Even some parts of the core platform are implemented as Django app plugins, for better separation of concerns (``announcements``, ``credentials``, ``grades``, etc.) Read the `Django app plugin documentation`_ to learn more.

Plugins can also inject custom data into django template contexts, to affect standard pages delivered by the core platform. See `Plugin Contexts`_ to learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI: We are in process of moving this to edx-django-utils. See openedx/edx-django-utils#48

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

FYI: We are in process of moving this to edx-django-utils. See openedx/edx-django-utils#48

OK, cool. Could you please update this doc at the same time, to point to that new home for the functionality?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oops. I thought this hadn't landed yet because I got a 404 on: https://github.com/open-craft/edx-platform/blob/master/docs/guides/extension_points.rst
I didn't realize I was searching in the wrong org. We can update the following accordingly:
https://github.com/edx/edx-platform/blob/master/docs/guides/extension_points.rst
Thanks.

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.

9 participants