[SE-3381] Allows adding new tinymce plugins through platform configuration #25695

nizarmah · 2020-11-29T16:35:51Z

This makes it possible to install new tinymce plugins through the environment settings.

This PR is responsible for adding the tinymce plugin settings to the tinymce javascript configuration.

JIRA tickets: SE-3381, SE-3247, OSPR-5263, OSPR-5264

Dependencies:

https://github.com/edx/configuration/pull/6179

Sandbox URL:

Testing instructions:

Login to Studio using staff@example.com/edx
Create a new Unit with an HTML Text
Make sure that the ADSK Link shows in the toolbar
Create a link and make sure you can preview it, and see it on the LMS

Author Notes and Concerns:

This is a limited implementation, it does not support custom stylesheets in the TinyMCE plugins.
The reason behind that is that no plugin currently requires any stylesheets to be included into Studio and LMS.

Reviewers

@pkulkark
@bradenmacdonald
edX reviewer[s] TBD

Settings

EDXAPP_CMS_ENV_EXTRA:
  JS_ENV_EXTRA_CONFIG:
    TINYMCE_ADDITIONAL_PLUGINS:
    - name: adsklink
      toolbar: true
      extra_settings:
        linktypes:
        - download
        - offer
        filetypes:
        - PDF
        - zip
        - Video
        - Design
        orientations:
        - Vertical
        - Horizontal
        styles:
        - Primary
        - Normal
        - Secondary

TINYMCE_ADDITIONAL_PLUGINS_LIST:
- repo: https://gitlab.com/nizarmah/tinymce-adsk-plugin
  name: adsklink
  plugin_path: "/adsklink"

edx_ansible_source_repo: "https://github.com/open-craft/configuration.git"
configuration_version: "nizar/tinymce_plugins_role"
edx_platform_commit: "nizar/tinymce_plugins_combined_dependencies_master"

openedx-webhooks · 2020-11-29T16:35:56Z

Thanks for the pull request, @nizarmah! I've created OSPR-5264 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 · 2020-12-01T15:33:25Z

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

nizarmah · 2020-12-01T15:35:18Z

@natabene small note by the way, I know I have a bunch of pull requests open at the moment. I just wanted to let you know that even if I haven't made any changes on a pull request, I haven't abandoned any of them.

I'm keeping good track of them, and switching between them depending on the work's priority internally. 👍

common/lib/xmodule/xmodule/js/src/html/edit.js

nizarmah · 2020-12-05T21:09:56Z

Apparently, all it took to fix the python build was 2 commits, rasied to the power of 3 🙂

pkulkark

LGTM 👍

I tested this: verified the new tinymce plugin is installed correctly as described in the testing instructions
I read through the code

bradenmacdonald · 2020-12-12T00:46:37Z

@nizarmah Could you please include documentation about this new extension point in extension_points.rst ? That will also notify people watching for changes that improve platform extensibility.

common/lib/xmodule/xmodule/js/src/html/edit.js

nizarmah · 2020-12-14T11:38:15Z

Alright, so what has been addressed is the following:

Added information about the limitation of the solution in the pull request description
Changed the method of passing the additional tinymce plugins configuration:
- Now each plugin is passed as a dictionary, instead of a key
- Each plugin has a toolbar key in order to indicate whether it should be added to the toolbar
- Each plugin has extra_settings in order to hold its extra settings
Also @bradenmacdonald I have added a brief mention of the TinyMCE plugins in the extension_points.rst. But I have not created an actual doc to include in edX's documentation. Would you prefer that I create one and link to it, instead of linking to this pull request?

nizarmah · 2020-12-15T16:25:46Z

@bradenmacdonald sorry, I'll need you to hold slightly on reviewing this ticket. There's something that I should've checked for, and if this PR would break Multiple App Servers.

The reason is, that we're doing changes to edit.js which would be reflected in the bundled common.js, and if there's a slight sorting issue due to passing the JS_ENV_EXTRA_CONFIG, then they'll break.

So I'll be building multiple app servers to verify that the hash is identical 👍🏼

bradenmacdonald · 2020-12-15T22:41:19Z

No problem @nizarmah, just let me know.

@natabene can I take this as a core committer?

natabene · 2020-12-17T03:16:23Z

@bradenmacdonald Sure.

nizarmah · 2020-12-18T02:27:49Z

@bradenmacdonald thanks a lot for being patient with me 😄 I ended up creating 5 different servers and made sure that the bundled commons.js is generated consistently among the different servers, without any single change (content hashes were identical on all machines 🥳)

So now that I've verified the change doesn't affect multiple app server support, this is ready for your review again 👍🏼

bradenmacdonald · 2020-12-18T21:14:55Z

@nizarmah Unfortunately there seems to be an error on the sandbox - the visual editor is not displaying:

cms/envs/common.py

docs/guides/extension_points.rst

bradenmacdonald · 2020-12-18T21:23:19Z

pavelib/assets.py

        static_root_lms=static_root_lms,
-        static_root_cms=static_root_cms
+        static_root_cms=static_root_cms,
+        js_env_extra_config=js_env_extra_config


Nit: please add a trailing comma, to get nicer diffs in the future.

Yeap, sorry about missing that 👍🏼 This should be addressed now 😃

webpack.dev.config.js

nizarmah · 2021-01-01T04:36:09Z

@bradenmacdonald this is ready for your review again.

Sorry about the problem you faced earlier! I added some explanation about it in the changes you requested 👍🏼

Please note that there seems to be a problem with one of the builds on the whole edX platform, so the currently failing python test isn't a result of the changes I did in this pull request.

bradenmacdonald · 2021-01-07T23:41:29Z

pavelib/utils/envs.py

Does this return a string or a decoded JSON object?

This returns a string, since it changes the True values to true which would be invalid in a Python object.

I'll update the docstring to mention it in a better way. 👍🏼

Addressed 👍🏼

Updated to

:return: json string value of the django setting

bradenmacdonald · 2021-01-07T23:43:23Z

pavelib/assets.py

I think that here js_env_extra_config_setting is a string right? So "string" or {} seems like inconsistent typing - should it be js_env_extra_config_setting or "{}" ? Or is js_env_extra_config_setting an object already?

js_env_extra_config_setting is a string, yes.

You're right, it is inconsistent. I'll update this to js_env_extra_config_setting or "{}". In addition, I'll update the tests too, I added comments to address those changes 👍🏼

Addressed 👍🏼

webpack.dev.config.js

bradenmacdonald

Thanks for your patience and diligence here, refining this approach and documenting it!

👍 if you address these last questions, rebase and squash into a single commit with a clear commit message.

Please also do a quick test to make sure that this doesn't introduce any problems when the setting is not set (i.e. default values everywhere).

I tested this: on the sandbox
I read through the code
I checked for accessibility issues
Includes documentation

pavelib/utils/envs.py

nizarmah · 2021-01-08T03:36:00Z

pavelib/utils/envs.py

This returns a string, since it changes the True values to true which would be invalid in a Python object.

I'll update the docstring to mention it in a better way. 👍🏼

pavelib/paver_tests/test_servers.py

nizarmah · 2021-01-08T03:38:02Z

pavelib/assets.py

js_env_extra_config_setting is a string, yes.

You're right, it is inconsistent. I'll update this to js_env_extra_config_setting or "{}". In addition, I'll update the tests too, I added comments to address those changes 👍🏼

edx-status-bot · 2021-01-08T05:15:50Z

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

nizarmah · 2021-01-08T09:22:40Z

@bradenmacdonald It's my pleasure! Your comments are something I look forward to, because I always end up learning something new, whether about the edX platform or on a personal level 😃

I addressed the comments you raised, and I also squashed the commits.

Regarding testings, I deployed two new sandboxes:

No TinyMCE plugins (default settings/configuration)
- LMS: https://configpr6179.sandbox.opencraft.hosting/
- Studio: https://studio.configpr6179.sandbox.opencraft.hosting/
Custom TinyMCE plugin
- LMS: https://pr25695.sandbox.opencraft.hosting/
- Studio: https://studio.pr25695.sandbox.opencraft.hosting/

I have tested both sandboxes, and the TinyMCE editors are running great on both 👍🏼
Please note that the sandboxes are running the following:

open-craft/edx-platform:nizar/tinymce_plugins_combined_dependencies_master
- Identical to this branch, but cherry-picks the change from https://github.com/edx/edx-platform/pull/25324
edx/configuration:master
- Includes the changes from https://github.com/edx/configuration/pull/6179

openedx-webhooks · 2021-01-11T16:28:46Z

@nizarmah 🎉 Your pull request was merged!

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

edx-pipeline-bot · 2021-01-11T18:04:18Z

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

edx-pipeline-bot · 2021-01-11T18:41:42Z

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

…tor (openedx#25695) (cherry picked from commit 1e872d4)

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Nov 29, 2020

nizarmah mentioned this pull request Nov 29, 2020

[SE-3381] Allows adding new tinymce plugins through platform configuration open-craft/openedx-platform#295

Merged

2 tasks

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

bradenmacdonald reviewed Dec 3, 2020

View reviewed changes

common/lib/xmodule/xmodule/js/src/html/edit.js Outdated Show resolved Hide resolved

nizarmah force-pushed the nizar/tinymce-easy-plugin-modification branch 8 times, most recently from 1b56061 to e6fc4de Compare December 5, 2020 19:14

pkulkark approved these changes Dec 11, 2020

View reviewed changes

nizarmah commented Dec 14, 2020

View reviewed changes

common/lib/xmodule/xmodule/js/src/html/edit.js Outdated Show resolved Hide resolved

natabene added the cc-reviewer label Dec 17, 2020

openedx-webhooks added the open edx community review label Dec 17, 2020

bradenmacdonald reviewed Dec 18, 2020

View reviewed changes

openedx-webhooks removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 27, 2020

nizarmah mentioned this pull request Dec 28, 2020

[SE-3381] Allows adding new tinymce plugins through platform configuration openedx-unsupported/configuration#6179

Merged

10 tasks

nizarmah requested a review from bradenmacdonald January 1, 2021 04:41

This was referenced Jan 6, 2021

[SE-3381] [JUNIPER BACKPORT] Allows adding new tinymce plugins through platform configuration open-craft/openedx-platform#307

Merged

[SE-3401] Updates JakePackage.zip to make JS minification consistent across different machines #25324

Merged

nizarmah force-pushed the nizar/tinymce-easy-plugin-modification branch from 1aca766 to 6360185 Compare January 7, 2021 08:11

bradenmacdonald reviewed Jan 7, 2021

View reviewed changes

bradenmacdonald approved these changes Jan 7, 2021

View reviewed changes

nizarmah commented Jan 8, 2021

View reviewed changes

Adds support for enabling custom tinymce plugins in Studio's HTML editor

1c3b8bf

nizarmah force-pushed the nizar/tinymce-easy-plugin-modification branch from 6360185 to 1c3b8bf Compare January 8, 2021 04:43

ormsbee merged commit 1e872d4 into openedx:master Jan 11, 2021

openedx-webhooks added merged and removed open edx community review labels Jan 11, 2021

nizarmah deleted the nizar/tinymce-easy-plugin-modification branch January 11, 2021 17:00

nizarmah added a commit to open-craft/openedx-platform that referenced this pull request Jan 20, 2021

Adds support for enabling custom tinymce plugins in Studio's HTML edi…

af94ed8

…tor (openedx#25695) (cherry picked from commit 1e872d4)

0x29a pushed a commit to open-craft/openedx-platform that referenced this pull request Feb 15, 2021

Adds support for enabling custom tinymce plugins in Studio's HTML edi…

c769af6

…tor (openedx#25695) (cherry picked from commit 1e872d4)

0x29a pushed a commit to open-craft/openedx-platform that referenced this pull request Apr 20, 2021

Adds support for enabling custom tinymce plugins in Studio's HTML edi…

b2be77d

…tor (openedx#25695) (cherry picked from commit 1e872d4)

nizarmah mentioned this pull request May 7, 2021

SE-3247 Allow custom node env variables for plugins #25022

Closed

2 tasks

tecoholic mentioned this pull request Nov 18, 2022

Allow overriding TinyMCE config #31313

Merged

[SE-3381] Allows adding new tinymce plugins through platform configuration #25695

[SE-3381] Allows adding new tinymce plugins through platform configuration #25695

Uh oh!

Conversation

nizarmah commented Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Nov 29, 2020

Uh oh!

natabene commented Dec 1, 2020

Uh oh!

nizarmah commented Dec 1, 2020

Uh oh!

Uh oh!

nizarmah commented Dec 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkulkark left a comment

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald commented Dec 12, 2020

Uh oh!

Uh oh!

nizarmah commented Dec 14, 2020

Uh oh!

nizarmah commented Dec 15, 2020

Uh oh!

bradenmacdonald commented Dec 15, 2020

Uh oh!

natabene commented Dec 17, 2020

Uh oh!

nizarmah commented Dec 18, 2020

Uh oh!

bradenmacdonald commented Dec 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nizarmah commented Jan 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edx-status-bot commented Jan 8, 2021

Uh oh!

nizarmah commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Jan 11, 2021

Uh oh!

edx-pipeline-bot commented Jan 11, 2021

Uh oh!

edx-pipeline-bot commented Jan 11, 2021

Uh oh!

nizarmah commented Nov 29, 2020 •

edited

Loading

nizarmah commented Dec 5, 2020 •

edited

Loading

nizarmah commented Jan 8, 2021 •

edited

Loading