Skip to content

feat: support adding custom editors to Studio [BB-4060] - #27466

Merged
ormsbee merged 1 commit into
openedx:masterfrom
open-craft:agrendalath/bb-4060-allow_adding_custom_editors_to_studio
Oct 27, 2021
Merged

feat: support adding custom editors to Studio [BB-4060]#27466
ormsbee merged 1 commit into
openedx:masterfrom
open-craft:agrendalath/bb-4060-allow_adding_custom_editors_to_studio

Conversation

@Agrendalath

@Agrendalath Agrendalath commented Apr 28, 2021

Copy link
Copy Markdown
Member

Description

This:

  1. Introduces a variable for the Course Outline view in Studio.
    A custom theme can override it to add new editors.
  2. Exports a function for creating new editor modals.
    A custom theme can use it to create editors without adding boilerplate code.
  3. Adds a pluggable override for XBlock fields that are passed to the Studio.
    A plugin can inject a custom field here.

An example custom editor can look like this. The fa-eye icon is a button for opening it.
image

Jira

OSPR-5764

Sandbox

https://pr27466.sandbox.opencraft.hosting

Prerequisites

  1. custom-unit-icons plugin.
  2. custom-unit-icons-theme.

Testing instructions

  1. Install the prerequisites and set up the comprehensive theme.
  2. Add the following to lms/envs/private.py:
    from .common import XBLOCK_MIXINS
    OVERRIDE_GET_UNIT_ICON = 'custom_unit_icons.icons.get_icon'
    XBLOCK_MIXINS += ('custom_unit_icons.icons.IconOverrideMixin',)
  3. Add the following to cms/envs/private.py:
    from .common import XBLOCK_MIXINS
    OVERRIDE_CREATE_XBLOCK_INFO = 'custom_unit_icons.icons.create_xblock_info'
    OVERRIDE_GET_UNIT_ICON = 'custom_unit_icons.icons.get_icon'
    XBLOCK_MIXINS += ('custom_unit_icons.icons.IconOverrideMixin',)
  4. Restart LMS and Studio.
  5. Go to the Course Outline page in Studio (sandbox link).
  6. Click on the fa-eye icon next to one of the verticals and change the icon. Visit this page in LMS to see that the icon has been changed.

Deadline

It would be great if this could make it to some iteration of Lilac.

Reviewers

Background

This is a follow-up to #21433. That PR initially:

  1. Introduced a way to add custom editors via a theme. This was a big theming change, which included splitting a template monolith, to achieve pluggability). This has been removed from the scope of that PR. We have revisited the approach and found a simpler way to do this, which is a part of this PR.
  2. Had an icon field added directly to the VerticalBlock class. We have changed the approach and decided to add it via a mixin. Unfortunately, this makes the fields write-only for Studio, because the custom field cannot be passed to the create_xblock_info function.

Settings

# Theme
EDXAPP_ENABLE_COMPREHENSIVE_THEMING: true
EDXAPP_COMPREHENSIVE_THEME_DIRS:
  - /edx/app/edxapp/themes
EDXAPP_DEFAULT_SITE_THEME: icons
edxapp_theme_name: "{{ EDXAPP_DEFAULT_SITE_THEME }}"
edxapp_theme_source_repo: https://github.com/open-craft/custom-unit-icons-theme.git

EDXAPP_EXTRA_REQUIREMENTS:
  - name: git+https://github.com/open-craft/custom-unit-icons.git@v0.4.0#egg=custom-unit-icons

EDXAPP_LMS_ENV_EXTRA:
  OVERRIDE_GET_UNIT_ICON: custom_unit_icons.icons.get_icon
  XBLOCK_EXTRA_MIXINS:
    - custom_unit_icons.icons.IconOverrideMixin

EDXAPP_CMS_ENV_EXTRA:
  OVERRIDE_CREATE_XBLOCK_INFO: custom_unit_icons.icons.create_xblock_info
  OVERRIDE_GET_UNIT_ICON: custom_unit_icons.icons.get_icon
  XBLOCK_EXTRA_MIXINS:
    - custom_unit_icons.icons.IconOverrideMixin

@openedx-webhooks

openedx-webhooks commented Apr 28, 2021

Copy link
Copy Markdown

Thanks for the pull request, @Agrendalath! I've created OSPR-5764 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 open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Apr 28, 2021
@Agrendalath
Agrendalath marked this pull request as ready for review April 28, 2021 17:41
@openedx-webhooks openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Apr 28, 2021
@natabene

Copy link
Copy Markdown
Contributor

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

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Apr 29, 2021
@Agrendalath
Agrendalath force-pushed the agrendalath/bb-4060-allow_adding_custom_editors_to_studio branch from eee964a to 2ca90fd Compare April 29, 2021 09:51

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

👍

  • I checked that sandbox deployed using commit from this PR, has theme and custom editor installed, and tested that icon can be changed from studio.
  • I read through the code.

@Agrendalath

Copy link
Copy Markdown
Member Author

@natabene, this is ready for your review.

@Agrendalath
Agrendalath force-pushed the agrendalath/bb-4060-allow_adding_custom_editors_to_studio branch from 2ca90fd to 26267ce Compare May 3, 2021 18:46
@openedx-webhooks openedx-webhooks added awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 5, 2021
@bradenmacdonald

bradenmacdonald commented May 12, 2021

Copy link
Copy Markdown
Contributor

@natabene Can I review this?

@Agrendalath The PR is relatively simple, but I find the PR description here a bit complex. Could you perhaps include an example screenshot of the "custom editors" that this allows, and explain why someone might want a "custom editor" ? The "Background" part can be removed or moved to the end.

Comment thread cms/static/js/views/modals/course_outline_modals.js Outdated
Comment thread cms/djangoapps/contentstore/views/item.py Outdated
@natabene

Copy link
Copy Markdown
Contributor

@Agrendalath @bradenmacdonald Thank you both. I would prefer for someone from TNL to also have a look.

@arch-bom-gocd-alerts

Copy link
Copy Markdown

📣 💥 Heads-up: You must either rebase onto master or merge master into your branch to avoid breaking the build.

We recently removed diff-quality and introduced lint-amnesty. This means that the automated quality check that has run on your branch doesn't work the same way it will on master. If you have introduced any quality failures, they might pass on the PR but then break the build on master.

This branch has been detected to not have commit 2e33565 as an ancestor. Here's how to see for yourself:

git merge-base --is-ancestor 2e335653 agrendalath/bb-4060-allow_adding_custom_editors_to_studio && echo "You're all set" || echo "Please rebase onto master or merge master to your branch"

If you have any questions, please reach out to the Architecture team (either #edx-shared-architecture on Open edX Slack or #architecture on edX internal).

@Agrendalath
Agrendalath force-pushed the agrendalath/bb-4060-allow_adding_custom_editors_to_studio branch 2 times, most recently from 338873b to ac42390 Compare June 15, 2021 12:44
@openedx-webhooks openedx-webhooks added product review PR requires product review before merging and removed awaiting prioritization labels Jul 6, 2021
@Agrendalath

Copy link
Copy Markdown
Member Author

@natabene, are there any updates on this?
cc: @bradenmacdonald

@natabene

Copy link
Copy Markdown
Contributor

@Agrendalath No, no updates.

@pomegranited

Copy link
Copy Markdown
Contributor

Hi @natabene , have TNL had a chance to look at this OSPR? CC @Agrendalath @bradenmacdonald

@natabene

Copy link
Copy Markdown
Contributor

I am afraid not.

@marcotuts

Copy link
Copy Markdown
Contributor

Can someone help me understand the implications of this in terms of what studio would now support with this change?

The ability to add actions to the elements o the course outline powered exclusively by theme level resources?

Are there any specific examples of when this would be valuable to use? The modal example seems to be for an icon switcher at the unit level?

@Agrendalath

Copy link
Copy Markdown
Member Author

@marcotuts,

Can someone help me understand the implications of this in terms of what studio would now support with this change?
The ability to add actions to the elements o the course outline powered exclusively by theme level resources?

We have three changes here:

  1. Extracting CourseOutlineView to a variable. This is a trivial change, which greatly increases the extendability of the Course Outline page in the Studio with comprehensive themes. Themes can add/modify functionalities on this page. New functionalities can be powered only with the theme, but we can also use plugins for more advanced customizations (I've included an example at the end of my comment).
  2. Exporting a function for creating new editor modals. This is related to code reusability. If a theme adds a custom editor (using the way introduced in the previous point), it basically needs to copy all of this code. This change simply exposes SettingsXBlockModal, so that other JS files (e.g. the ones from the theme) can reuse it.
  3. Adding a pluggable override for Studio's XBlock fields. This is a one-liner extension point that adds more flexibility to what is being passed to the Studio. It is already possible to add extra fields to all XBlocks through XBLOCK_EXTRA_MIXINS. At the moment these fields can only be read through the LMS, though. The Studio has write-only access. This change allows passing these custom fields to the Studio so that the custom themes can read them.

Are there any specific examples of when this would be valuable to use? The modal example seems to be for an icon switcher at the unit level?

Yes, that's correct. We're using these extension points and a simple plugin to add unit-level icon editors with a theme.
To sum up, this gives more power to the comprehensive themes for Studio by allowing them to:

  • Override the Course Outline page.
  • Reuse the existing code to create section/subsection/unit editors.
  • Read additional XBlock fields.

@ormsbee

ormsbee commented Oct 21, 2021

Copy link
Copy Markdown
Contributor

I had not been following the @pluggable_override inclusion. That's a super-powerful and mildly terrifying extension mechanism. Does it have an ADR by any chance? I'm not looking to re-open the discussion on it or anything, but I would like to understand what kinds of best practices have been established around it as context for this review.

@Agrendalath

Copy link
Copy Markdown
Member Author

@ormsbee, unfortunately, we don't have an ADR for them. For now, we have added them to the platform's extension points as a "Trial" plugin type. In openedx/edx-django-utils#64, we were discussing a proper way of documenting them but concluded that we can decide about their final form once we start using them for more scenarios and change their status to "Adopted".
We have started by using them for overriding the algorithm that determines XBlock's icon (#21433), which is a small functionality, so having more use cases is going to give us a better idea of how we want to use them. Since then, we have also added one override to customize the certificate view (#21147).

@ormsbee

ormsbee commented Oct 26, 2021

Copy link
Copy Markdown
Contributor

jenkins run django-3.2/python

@ormsbee

ormsbee commented Oct 26, 2021

Copy link
Copy Markdown
Contributor

jenkins run all

@ormsbee

ormsbee commented Oct 26, 2021

Copy link
Copy Markdown
Contributor

Can you try rebasing please?

@Agrendalath
Agrendalath force-pushed the agrendalath/bb-4060-allow_adding_custom_editors_to_studio branch from 166789f to 1e4a4eb Compare October 27, 2021 11:27
@openedx-webhooks openedx-webhooks removed the product review PR requires product review before merging label Oct 27, 2021
@Agrendalath

Copy link
Copy Markdown
Member Author

@ormsbee, the tests are currently failing on the master branch. I'll rebase this again once it's resolved.

@Agrendalath
Agrendalath force-pushed the agrendalath/bb-4060-allow_adding_custom_editors_to_studio branch from 1e4a4eb to 26f5f5c Compare October 27, 2021 14:46
This:
1. Introduces a variable for the Course Outline view in Studio.
   A custom theme can override it to add new editors.
2. Exports a function for creating new editor modals.
   A custom theme can use it to create editors without adding boilerplate code.
3. Adds a pluggable override for XBlock fields that are passed to the Studio.
   Without this, custom editors in Studio cannot retrieve values of XBlock fields.
@edx-status-bot

Copy link
Copy Markdown

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

@Agrendalath

Copy link
Copy Markdown
Member Author

@ormsbee, the tests are green now.

@ormsbee
ormsbee merged commit e633cc9 into openedx:master Oct 27, 2021
@openedx-webhooks

Copy link
Copy Markdown

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

@Agrendalath
Agrendalath deleted the agrendalath/bb-4060-allow_adding_custom_editors_to_studio branch October 27, 2021 17: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 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.