[BB-933] Allow overriding xblock icons with plugins by Agrendalath · Pull Request #21433 · openedx/openedx-platform

Agrendalath · 2019-08-23T12:36:23Z

This is a follow-up to #19772, which allowed setting custom icons for each unit in Studio. By default the icon would be derived from unit's type. The suggestion was to do move it to a plugin. This PR introduces some changes required to move it to external Django application

Most of this diff is about extracting modals from monolithic cms/static/js/views/modals/course_outline_modals.js to make them reusable. We also extracted CourseOutlineView class into a field in cms/static/js/views/pages/course_outline.js to make this view easily extendable.

For overriding implementation of getting icon, we implemented pluggable_override decorator.

JIRA tickets: OSPR-3801

Merge deadline: None

Sandbox: https://pr21433.sandbox.opencraft.hosting/ (provisioning)

Testing instructions:

Go to this page and see that the first unit icon is fa-book.
Install the master version of edx-django-utils in LMS (make lms-shell):
pip install -e git+https://github.com/edx/edx-django-utils.git#egg=edx-django-utils
Install custom-unit-icons for LMS (make lms-shell):
pip install -e git+https://github.com/open-craft/custom-unit-icons.git@agrendalath/bb-933-address_override_changes#egg=custom-unit-icons

Set the following in lms/envs/private.py and run make lms-restart:

from .common import XBLOCK_MIXINS
OVERRIDE_GET_UNIT_ICON = 'custom_unit_icons.icons.get_icon'
XBLOCK_MIXINS += ('custom_unit_icons.icons.IconOverrideMixin',)

Go to this page and see that the first unit icon is still fa-book.

Run the following snippet in LMS Django shell:

from xmodule.modulestore.django import modulestore
from opaque_keys.edx.keys import UsageKey

usage_key = UsageKey.from_string('block-v1:edX+DemoX+Demo_Course+type@vertical+block@867dddb6f55d410caaa9c1eb9c6743ec')
item = modulestore().get_item(usage_key)
item.icon_override = 'video'
modulestore().update_item(item, 1)

Go to this page and see that the first unit icon is now fa-film.

Reviewers

@viadanna
@bradenmacdonald
edX reviewer[s] TBD

Settings

EDXAPP_EXTRA_REQUIREMENTS:
  - name: git+https://github.com/open-craft/custom-unit-icons.git@agrendalath/bb-933-address_override_changes#egg=custom-unit-icons
  - name: git+https://github.com/edx/edx-django-utils.git@master#egg=edx-django-utils

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

TODO:

Provision the sandbox.
Update the docs from Document the various integration points for platform extensions #24057.

openedx-webhooks · 2019-08-23T12:36:30Z

Thanks for the pull request, @Agrendalath! I've created OSPR-3801 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 · 2019-08-23T19:31:05Z

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

viadanna

Good to go 👍

I tested this locally on my devstack.
I read through the code.
~~I checked for accessibility issues.~~
~~Includes documentation.~~

bradenmacdonald · 2019-08-30T19:20:07Z

+        An API for returning a css class identifying this module in the context of an icon.
+        It can be overridden by setting `GET_UNIT_ICON_IMPL` to an alternative implementation.
+        """
+        return self.get_icon_class()


If this is defined on XModuleMixin, it will only apply to XModule instances, not XBlock ones, which is the newer class. Since this is where get_icon_class is defined too, that's probably fine, but I don't think this needs to be an XModuleMixin method at all... I think it could just be a top-level method in some runtime/xblock utils file, e.g.

@pluggable_override('GET_UNIT_ICON_IMPL') def get_icon_for_block(block): return block.get_icon_class()

No need to change now, I'm not an official reviewer here, just pointing out options.

I was wondering about the correct place for this, but wasn't sure how this could be applied to XBlock, as it doesn't have a method for getting an icon. Should we just raise NotImplementedError when we don't find block.get_icon_class method?

I was wondering about the correct place for this

I think it should be a top level function in xblock_utils/__init__.py. See my example that I'm replying to here.

Should we just raise NotImplementedError when we don't find block.get_icon_class method?

Return 'other' if it doesn't have a get_icon_class method; that's what get_icon_class does if it isn't overridden anyways.

Sounds good, I'll move this.

Agrendalath · 2019-09-17T00:50:10Z

@bradenmacdonald, thank you for your review! And sorry for addressing it late, I missed this email.

Agrendalath · 2019-09-17T00:50:14Z

@natabene, this is ready for the review.

natabene · 2019-09-17T17:06:34Z

@marcotuts Is this something you want to review before it moves to engineering?

bradenmacdonald · 2019-09-17T17:13:07Z

-            mod = import_module(module)
-            func = getattr(mod, function)
-            return func(*args, **kwargs)
+            prev_fn = f  # The base method in `edx-platform`.


This might need to be partial(f, *args, **kwargs) because it becomes the prev_fn arg later, and the other prev_fn args already have the parameters applied?

That's a good point. Thank you for catching this!

Agrendalath · 2019-10-08T15:11:16Z

@marcotuts, @natabene, any news on this?

natabene · 2019-10-08T16:43:24Z

@Agrendalath Sorry, no news yet.

openedx-webhooks · 2020-02-12T07:51:50Z

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

marcotuts · 2020-02-12T13:18:44Z

@ormsbee - This is ok to move to engineering review.

@Agrendalath - One question I have is that this seems like a large change to edx-platform for this plugin, is this largely a file relocation change, or what makes the change seem (at a glance) as large out of curiosity? Thanks!

Agrendalath · 2020-02-13T08:45:39Z

@marcotuts, we divided monolithic cms/static/js/views/modals/course_outline_modals.js into smaller chunks to be able to override specific parts with a theme. This modular approach makes the theme much easier to maintain.

Btw. does the new courseware experience include these icons or is the navigation part altered in some way?

bradenmacdonald · 2020-03-01T00:14:33Z

@marcotuts Does the new courseware experience include these icons?

marcotuts · 2020-03-01T18:07:08Z

@bradenmacdonald, @Agrendalath - The icons are used in the new learning sequence bar / MFE: https://github.com/edx/frontend-app-learning

Note: We haven not yet release an initial version of this to production on edX.org, still in early testing, but included link as FYI for MFE focused on the course learner experience

bradenmacdonald · 2020-03-19T19:15:58Z

@natabene Per @marcotuts's comment above (thanks!), could you please update this to "Engineering Review"? I think the status got lost when we mistakenly closed and re-opened the PR.

This is ok to move to engineering review.

natabene · 2020-03-23T01:11:47Z

@bradenmacdonald The status is currently set to "Awaiting prioritization" and will be changed to "Engineering review" once we start reviewing it.

bradenmacdonald · 2020-05-15T18:25:51Z

@Agrendalath Since the current courseware code (that you've modified here) will eventually be replaced by https://github.com/edx/frontend-app-learning , I think it might make sense to rebase this PR and strip out all the JavaScript/frontend changes, then test it with the frontend-app-learning MFE. That will make it easier to review and forwards-compatible.

Also I think it's worth calling more attention to the fact that this introduces a generic decorator for overriding any python function and as such provides a new model for platform extensibility - @natabene could we get some light engineering feedback on that approach soon?

Agrendalath · 2020-05-18T14:27:28Z

@bradenmacdonald, sure, I'll set aside some time for the next sprint to change this.

natabene · 2021-03-26T14:13:19Z

@Agrendalath Is this good to review now? openedx/edx-django-utils#64 has been merged

Agrendalath · 2021-03-26T16:13:45Z

@natabene, yes, this is ready for review. We still need to push the new version of edx/edx-django-utils to PyPi (cf. my last comment on openedx/edx-django-utils#109), but this is not a blocker for reviewing this.

natabene · 2021-03-29T15:31:24Z

@bradenmacdonald Do you still want to review this as a Core Committer?

bradenmacdonald · 2021-03-29T17:42:32Z

@natabene Yes, I'm hoping to get to it soon. Thanks!

bradenmacdonald

👍 Looks great, thanks!

This is a conditional approval; before we merge, please:

Get the version bump of edx-django-utils completed, and rebase this
Squash down to a single commit following conventional commits

I tested this: on my devstack, as described
I read through the code
I checked for accessibility issues
Includes documentation

bradenmacdonald · 2021-04-02T16:28:30Z

following conventional commits

Don't forget this part ;)

This: 1. Introduces a new override using the `pluggable_override` decorator. It is now possible to specify a custom way of getting XBlock's icon by defining `GET_UNIT_ICON_IMPL` in settings. 2. Introduces a way to add custom `XBLOCK_MIXINS` by defining `XBLOCK_EXTRA_MIXINS` in settings. This allows, e.g. to add new fields to XBlocks.

edx-status-bot · 2021-04-03T12:29:30Z

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

openedx-webhooks · 2021-04-07T13:42:16Z

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

kdmccormick · 2021-04-07T13:44:13Z

Thanks Braden and @Agrendalath . FYI, when squashing, I hard-wrapped your commit message body at ~80 chars. Git doesn't auto-wrap messages very well :)

edx-pipeline-bot · 2021-04-07T14:59:47Z

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

edx-pipeline-bot · 2021-04-07T15:28:19Z

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

edx-pipeline-bot · 2021-04-07T15:44:56Z

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

edx-pipeline-bot · 2021-04-07T16:41:01Z

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

Agrendalath requested a review from a team August 23, 2019 12:36

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Aug 23, 2019

Agrendalath mentioned this pull request Aug 23, 2019

[BB-933] Add alternative implementation for retrieving XBlock icon open-craft/custom-unit-icons#1

Merged

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Aug 23, 2019

viadanna approved these changes Aug 26, 2019

View reviewed changes

bradenmacdonald reviewed Aug 30, 2019

View reviewed changes

openedx-webhooks added product review PR requires product review before merging and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Sep 17, 2019

bradenmacdonald reviewed Sep 17, 2019

View reviewed changes

Agrendalath closed this Feb 12, 2020

Agrendalath reopened this Feb 13, 2020

openedx-webhooks added the awaiting prioritization label Feb 13, 2020

Agrendalath commented Mar 25, 2021

View reviewed changes

Comment thread requirements/edx/testing.txt Outdated

openedx-webhooks added awaiting prioritization engineering review and removed blocked by other work PR cannot be finished until other work is complete awaiting prioritization labels Mar 29, 2021

bradenmacdonald approved these changes Mar 31, 2021

View reviewed changes

Comment thread common/lib/xmodule/xmodule/seq_module.py Outdated

Agrendalath force-pushed the agrendalath/custom-icons branch 2 times, most recently from 67dd0f3 to bfef952 Compare April 2, 2021 12:53

Agrendalath force-pushed the agrendalath/custom-icons branch from bfef952 to 01cc05a Compare April 3, 2021 11:45

Agrendalath force-pushed the agrendalath/custom-icons branch from 01cc05a to c4c7197 Compare April 3, 2021 11:48

kdmccormick merged commit bc1e9af into openedx:master Apr 7, 2021

openedx-webhooks added merged and removed engineering review labels Apr 7, 2021

Agrendalath deleted the agrendalath/custom-icons branch April 7, 2021 18:42

Agrendalath restored the agrendalath/custom-icons branch April 8, 2021 18:34

Agrendalath deleted the agrendalath/custom-icons branch April 8, 2021 18:38

Agrendalath mentioned this pull request Apr 28, 2021

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

Merged

2 tasks

Uh oh!

Conversation

Agrendalath commented Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natabene commented Aug 23, 2019

Uh oh!

viadanna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bradenmacdonald Aug 30, 2019

Choose a reason for hiding this comment

Uh oh!

Agrendalath Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Agrendalath Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Agrendalath commented Sep 17, 2019

Uh oh!

Agrendalath commented Sep 17, 2019

Uh oh!

natabene commented Sep 17, 2019

Uh oh!

bradenmacdonald Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

Agrendalath Sep 18, 2019

Choose a reason for hiding this comment

Uh oh!

Agrendalath commented Oct 8, 2019

Uh oh!

natabene commented Oct 8, 2019

Uh oh!

openedx-webhooks commented Feb 12, 2020

Uh oh!

marcotuts commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Agrendalath commented Feb 13, 2020

Uh oh!

bradenmacdonald commented Mar 1, 2020

Uh oh!

marcotuts commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradenmacdonald commented Mar 19, 2020

Uh oh!

natabene commented Mar 23, 2020

Uh oh!

bradenmacdonald commented May 15, 2020

Uh oh!

Agrendalath commented May 18, 2020

Uh oh!

Uh oh!

natabene commented Mar 26, 2021

Uh oh!

Agrendalath commented Mar 26, 2021

Uh oh!

natabene commented Mar 29, 2021

Uh oh!

bradenmacdonald commented Mar 29, 2021

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bradenmacdonald commented Apr 2, 2021

Uh oh!

edx-status-bot commented Apr 3, 2021

Agrendalath commented Aug 23, 2019 •

edited

Loading

openedx-webhooks commented Aug 23, 2019 •

edited

Loading

bradenmacdonald Sep 14, 2020 •

edited

Loading

marcotuts commented Feb 12, 2020 •

edited

Loading

marcotuts commented Mar 1, 2020 •

edited

Loading