[BB-6151] Update TinyMCE Editor static file references to upgraded version 5.x #1869

tecoholic · 2022-07-01T06:32:48Z

TL;DR - Replaces the version 4.x references of TinyMCE static files with version 5.x and removes the workaround required for TMCE 4 from oa_editor_tinymce.js

JIRA: - NA

What changed?

The edx-platform's TinyMCE is being upgraded from version 4.x to 5.x (v5.5.1) at openedx/edx-platform#30335. The upgrade breaks the ORA WYSIWYG editor as the TinyMCE files referenced in the oa_editor_tinymce.js are hard-coded to v4.x file paths.

In this PR these paths are update to reference the v5.x files
Corresponding changes are made in the TinyMCE init function
Removes a TinyMCE 4.x specific workaround
Updates all the static files using make static

Developer Checklist

Reviewed the release process
Translations and JS/SASS compiled
Bumped version number in setup.py and package.json

Testing Instructions

Checkout to the PR branch of [BB-6151] feat: upgrades tinyMCE v4.0.20 to tinyMCE v5.5.1 edx-platform#30335
Add a ORA block in a course and set the editor to WYSIWYG
Open the Unit in the LMS and notice that the Editor doesn't load for input
Install the open-craft:tecoholic/BB-6151-upgrade-tinymce-to-v5 branch of the ORA in to lms
Restart the LMS
Revisit the test unit in LMS and verify that the Editor is functioning as expected

Reviewer Checklist

Collectively, these should be completed by reviewers of this PR:

I've done a visual code review
I've tested the new functionality

FYI: @openedx/content-aurora

openedx-webhooks · 2022-07-01T06:32:53Z

Thanks for the pull request, @tecoholic! 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.

Agrendalath · 2022-07-01T08:10:21Z

@natabene, would you mind approving the CI on this PR?

Agrendalath

@tecoholic, while the LMS is working for me, I cannot use it with Studio on my devstack. When I switch the "Response Editor" to "WYSIWYG Editor", I'm getting the following warning:

Source map error: Error: request failed with status 404
Resource URL: http://localhost:18000/static/dist/openassessment-lms.d67e4d76d91bec14c4f5.js
Source Map URL: openassessment-lms.d67e4d76d91bec14c4f5.js.map

Then, I'm getting a 404 on http://localhost:18010/static/studio/bundles/js/factories/themes/silver/theme.js. The editor doesn't load at all there.

I'm running make dev.static.studio after installing this version of XBlock. Restarting the Studio container doesn't help too. Am I doing something incorrectly?

Also, please bump the version in __init__.py and in package.json.

tecoholic · 2022-07-04T07:00:18Z

@Agrendalath The source file missing error seems to because of them being gitignored. So you will have to do a npm build locally if you want then to load I guess. However, the theme file missing is an issue. Let me figure that out and let you know.

tecoholic · 2022-07-05T15:55:59Z

@Agrendalath The issue is now fixed with 2c3dea8.

Agrendalath · 2022-07-06T20:00:59Z

@tecoholic, the editor in the main Studio view is working now, but I'm getting the 404 on http://localhost:18010/static/studio/js/vendor/tinymce/js/tinymce/themes/modern/theme.js when I go to Edit -> Prompt (the first tab in the editor). This view should contain editors as well.

I don't know if this is only a devstack-specific issue - I have spawned a new sandbox to test it there. However, I believe we'll need to fix it anyway if it's broken in the devstack.

tecoholic · 2022-07-07T04:31:55Z

@Agrendalath I see. I am guessing the theme is not getting properly set for the openassessment-studio.js file. I will find and fix and get back to you.

tecoholic · 2022-07-07T07:17:29Z

I found another file initializing the TinyMCE with its own configuration. Now the editor should work in:

LMS
Studio preview of the Block
Edit window of the Studio

Agrendalath · 2022-07-07T10:27:01Z

@tecoholic,

I'm now getting 404 for https://studio.bb-6151-sandbox.opencraft.hosting/static/studio/bundles/js/factories/themes/silver/theme.js. This seems to be breaking the JS when the Simple Text Editor is enabled - the simple editor is not displayed for prompts. Please see the editor of the second XBlock on this page.
Nit: we should remove the textcolor plugin from openassessment/xblock/static/js/src/studio/oa_tiny_mce.js, as it's already built into the core editor.

tecoholic · 2022-07-07T11:52:14Z

@Agrendalath

Fix it by adding the base_url to the oa_tiny_mce.js configuration. It was working without it when the response editor was set to WYSIWYG editor because window.tinymce was already defined and the resources were available. In case of the "Simple Text Editor", the TinyMCE resources had to be fetched and it didn't have the right base url to load things.
Removed it from the configuration.

Agrendalath

@tecoholic, please bump the version in __init__.py and package.json.

👍

I tested this: tested the XBlock on the sandbox and locally
I read through the code
I checked for accessibility issues: n/a
Includes documentation: n/a
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

Agrendalath · 2022-07-08T18:42:33Z

@natabene, this is ready for your review. It's a dependency for openedx/edx-platform#30335.

This is a Lilac backport of openedx#1869 Replaces the version 4.x references of TinyMCE static files with version 5.x and removes the workaround required for TMCE 4 from oa_editor_tinymce.js Related platform PR: openedx/edx-platform#30335

tecoholic · 2022-07-12T09:13:17Z

Version bumped to 4.5.0

natabene · 2022-07-20T19:50:05Z

@tecoholic Thank you for your contribution. @Agrendalath Got it, thanks.

pomegranited

👍 Works great @tecoholic ! And thank you for providing such complete testing instructions.

I noticed one issue which would warrant a follow-up ticket: Using the WYSIWYG editor + Allowing LaTeX responses doesn't work well -- there's no way to get a good LaTeX preview with the TinyMCE editor.

I tested this on my devstack using the PR testing instructions.
I read through the code.
I checked for accessibility issues by following the TinyMCE accessibility instructions.
~~Includes documentation~~ N/A
Commit structure follows OEP-0051

pomegranited · 2022-07-28T04:01:40Z

@tecoholic Could you resolve the conflicts here to prepare this for merge?

Once that's done, I can merge and tag this if that's ok with you @Agrendalath ?

natabene · 2022-07-28T14:52:44Z

@pomegranited Before you merge, can you please give heads up or coordinate in openedx-cc-core-applications Slack channel?

tecoholic · 2022-07-29T02:52:11Z

@tecoholic Sure. I will resolve the conflicts and let you know. 👍

tecoholic · 2022-08-05T12:32:52Z

@pomegranited I have rebased the changes on to the master and resolved the conflicts. :)

pomegranited · 2022-08-08T00:19:58Z

Thanks @tecoholic :)

Hi @openedx/content-aurora -- does anyone have any objections to merging this change?
If not, I'll merge & tag it after midnight 9 Aug UTC.

tecoholic · 2022-08-08T11:28:24Z

@pomegranited @Agrendalath The CI tests are failing when installing JS dependencies as the @edx/paragon depends on React 16.x while edx-ora2 depends on React 17.x.

npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR! 
npm ERR! While resolving: @edx/[email protected]
npm ERR! Found: [email protected]
npm ERR! node_modules/react
npm ERR!   react@"^17.0.2" from the root project
npm ERR!   peer react@">=16.x" from @fortawesome/[email protected]
npm ERR!   node_modules/@fortawesome/react-fontawesome
npm ERR!     @fortawesome/react-fontawesome@"^0.1.11" from @edx/[email protected]
npm ERR!     node_modules/@edx/paragon
npm ERR!       @edx/paragon@"^12.8.0" from the root project
npm ERR!   18 more (@restart/context, @restart/hooks, ...)
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer react@"^16.8.6" from @edx/[email protected]
npm ERR! node_modules/@edx/paragon
npm ERR!   @edx/paragon@"^12.8.0" from the root project
npm ERR! 
npm ERR! Conflicting peer dependency: [email protected]
npm ERR! node_modules/react
npm ERR!   peer react@"^16.8.6" from @edx/[email protected]
npm ERR!   node_modules/@edx/paragon
npm ERR!     @edx/paragon@"^12.8.0" from the root project
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

The current version of @edx/paragon is 20.8.0. Can we update the version of paragon in ORA2 to a more recent version with React 17 support? I am not aware of the impact of upgrading Paragon. Kindly advise.

Edit: The @edx/paragon version with React 17 support seems to be 20.x. So we will have to upgrade from 12.8.0 to 20.8.0

pomegranited · 2022-08-09T02:00:01Z

Sorry @tecoholic, I wasn't aware of this build issue, but I do think that upgrading paragon to the latest possible version is the way to go here.

You'll have to go through the paragon release notes to find out the impact of upgrading from 12.8.0 to 20.x.x. E.g., v20.0.0 has breaking changes, which look involved but well-documented.

tecoholic · 2022-08-15T11:23:03Z

The upgrade of Paragon to 20.x is blocked by openedx/paragon#1559

cc: @Agrendalath @pomegranited

UsamaSadiq · 2022-09-06T11:59:26Z

Created PR #1889 to add a new command in Makefile for installing the latest Transifex client locally to run translation commands.

UsamaSadiq · 2022-09-06T13:45:06Z

HI @pomegranited,
I've created final PR #1890 to make the tx command available on local with the new transifex client.
Since your PR is from a fork, so I couldn't check it out on local to debug. After investigating, I'd suggest you to perform following steps:

Rebase your PR to get the new make target in your branch.
Run the make install_transifex_client command which will download the latest GO Transifex client and make it accessible across the repo.
Run make pull_translations command to fetch the latest translations from Transifex in case any of the translations need to be updated.
Run tox -e quality to verify if the check passes now with the updated translations.

I'm hoping that this'll resolve the issue with the translations failure but I'll be able to help further after knowing the result of these instructions.
[Note] remember to delete the tx executable before pushing the changes in the branch.

pomegranited · 2022-09-07T08:39:06Z

Thank you @UsamaSadiq !
@tecoholic could you update this PR as described above?

tecoholic · 2022-09-09T06:48:02Z

@pomegranited I was getting the unsupported protocol scheme errors after following the steps as well. Turns out, my Transifex client configuration was incompatible with the Go Client and I needed to migrate my old transifex client config using the tx migrate command before make pull_translations could start working.

However I have run into another issue. All the files that are being pulled from transifex just has the following:

{"errors":[{"status":401,"code":"unauthorized","title":"Unauthorized","detail":"Invalid JWT token"}]

I was using the OC account and even created a new API token to double check it is not due to some invalid token.

@UsamaSadiq Do you have any idea why this is happening?

Also I had to change the Makefile command for pull_translations as it was creating a different directory openassessment/conf/ and kept downloading the files into it.

pull_translations: ## pull translations from Transifex
- 	cd ./openassessment/ && tx pull -a -f --mode reviewed --minimum-perc=1
+	tx pull -a -f --mode reviewed --minimum-perc=1

Agrendalath · 2022-09-09T08:01:26Z

@tecoholic, this is working correctly for me on your branch. Please make sure that your ~/.transifexrc looks like this:

[https://www.transifex.com]
api_hostname = https://api.transifex.com
hostname = https://www.transifex.com
password = <TOKEN>
rest_hostname = https://rest.api.transifex.com
token = <TOKEN>
username = API

I've shared the token with you through a different channel (though it's just a standard API token).

I'm using TX Client, version=1.3.0, downloaded directly from its release page.

tecoholic · 2022-09-09T10:15:32Z

@Agrendalath Thank you. I also have the same version of Transifex. And everything looks the same in my transifex file except the username which is in lowercase api instead of the uppercase API as you have mentioned. I think this might be issue. Let me give it another try.

tecoholic · 2022-09-09T10:54:20Z

@Agrendalath I tried with the uppercase username and the API key you shared. I get the same results as before. Are you able to run make pull_translations and make compile_translations without errors? I get a number of these

Execution of msgfmt failed: /src/edx-ora2/openassessment/conf/locale/el/LC_MESSAGES/django.po:1:2: syntax error
msgfmt: found 1 fatal error
Execution of msgfmt failed: /src/edx-ora2/openassessment/conf/locale/fr_CA/LC_MESSAGES/djangojs.po:1:2: syntax error
msgfmt: found 1 fatal error
Execution of msgfmt failed: /src/edx-ora2/openassessment/conf/locale/lv/LC_MESSAGES/django.po:1:2: syntax error
msgfmt: found 1 fatal error
Execution of msgfmt failed: /src/edx-ora2/openassessment/conf/locale/lv/LC_MESSAGES/djangojs.po:1:2: syntax error
msgfmt: found 1 fatal error
Execution of msgfmt failed: /src/edx-ora2/openassessment/conf/locale/fr/LC_MESSAGES/django.po:1:2: syntax error
msgfmt: found 1 fatal error
Execution of msgfmt failed: /src/edx-ora2/openassessment/conf/locale/fr/LC_MESSAGES/djangojs.po:1:2: syntax error
msgfmt: found 1 fatal error

And when I open one of the mentioned files in the errors, I see this

{"errors":[{"status":401,"code":"unauthorized","title":"Unauthorized","detail":"Invalid JWT token"}]}

Agrendalath · 2022-09-09T12:51:25Z

@tecoholic, huh, that's odd. I tried this again and could reproduce it for some translations without an underscore (_) in their names. However, after I tested it with tx version 1.0.3 and switched back to the newer one, it worked fine. I've allowed myself to push them to your branch.

There were some errors in the it_IT translation:

INFO:i18n.validate:

WARNING:i18n.validate:
it_IT/LC_MESSAGES/django.po:1411: 'msgid' and 'msgstr' entries do not both begin with '\n'
it_IT/LC_MESSAGES/django.po:1435: 'msgid' and 'msgstr' entries do not both begin with '\n'
it_IT/LC_MESSAGES/django.po:1499: 'msgid' and 'msgstr' entries do not both begin with '\n'
msgfmt: found 3 fatal errors

I've added translation suggestions to Transifex, but I don't have permissions to accept them (cc: @UsamaSadiq). As a workaround, I've manually fixed these strings in the translation files.

@UsamaSadiq, many languages appear to be duplicated in Transifex. E.g. in the resources we can see pl and pl_PL, which are the same language. This applies to most languages. When I tried using an older Transifex client, all of them were being downloaded. Is it something we could fix to avoid confusion?

Agrendalath · 2022-09-09T13:20:25Z

Update: since the CI was failing, I've also rebuilt statics (just in case) and updated source translations. However, I don't have permission to update them in Transifex, as make push_translations is returning this:

tx push -s
# Getting info about resources

edx-platform.openassessment - Error while fetching stats, 404, not_found: No `Project` resource found with the identifier `o:open-edx:p:edx-platform`
edx-platform.openassessment-js - Error while fetching stats, 404, not_found: No `Project` resource found with the identifier `o:open-edx:p:edx-platform`
[##############################] (2 / 2)
Aborted
make: *** [Makefile:102: push_translations] Error 1

cc: @UsamaSadiq

tecoholic · 2022-09-12T12:59:32Z

@Agrendalath Thanks you for finding the solution for this. Hopefully we will be able to wrap up this PR with this.

pomegranited · 2022-09-13T03:06:29Z

@tecoholic @Agrendalath I don't have push rights on the Transifex projects either, but it looks like we have a bot to take care of that: https://github.com/openedx/edx-ora2/pulls?q=is%3Apr+author%3Aedx-transifex-bot+

@tecoholic There's a couple conflicts remaining, but once they're resolved, I can merge this. Can we aim for first thing UTC Thu 15 Sep?

Replaces the version 4.x references of TinyMCE static files with version 5.x and removes the workaround required for TMCE 4 from oa_editor_tinymce.js Related platform PR: openedx/edx-platform#30335

tecoholic · 2022-09-13T08:07:22Z

@pomegranited I have rebased the PR branch on master, resolved conflicts, rebuilt the static files and translations and pushed the changes. I have also run tox -e quality to verify there aren't any quality issues. Kindly approve the workflows and we should be able to merge this.

openedx-webhooks · 2022-09-14T02:02:44Z

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

…rsion 5.x (openedx#1869) * feat: replace references to tinymce v4 with v5 Replaces the version 4.x references of TinyMCE static files with version 5.x and removes the workaround required for TMCE 4 from oa_editor_tinymce.js Related platform PR: openedx/edx-platform#30335 Related changes: * fix: set tinymce base url so resources are loaded in studio * fix: theme files not loading on studio edit window * chore: bumped minor version to 4.5.0 * chore: update edx paragon to latest version, and fix failing tests due to paragon api change * fix: fix and update translations Co-authored-by: Agrendalath <[email protected]> (cherry picked from commit 6072196)

tecoholic requested a review from a team as a code owner July 1, 2022 06:32

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 1, 2022

tecoholic mentioned this pull request Jul 1, 2022

[BB-6151] feat: upgrades tinyMCE v4.0.20 to tinyMCE v5.5.1 openedx/edx-platform#30335

Merged

Agrendalath reviewed Jul 1, 2022

View reviewed changes

Agrendalath approved these changes Jul 8, 2022

View reviewed changes

pomegranited self-requested a review July 28, 2022 01:35

pomegranited approved these changes Jul 28, 2022

View reviewed changes

tecoholic force-pushed the tecoholic/BB-6151-upgrade-tinymce-to-v5 branch from 6d1beb5 to ad1aec1 Compare August 5, 2022 12:26

tecoholic mentioned this pull request Aug 15, 2022

Importing core.scss is broken due to missing Bootstrap Variables openedx/paragon#1559

Closed

tecoholic and others added 12 commits September 13, 2022 13:25

feat: replace references to tinymce v4 with v5

32c4516

Replaces the version 4.x references of TinyMCE static files with version 5.x and removes the workaround required for TMCE 4 from oa_editor_tinymce.js Related platform PR: openedx/edx-platform#30335

fix: set tinymce base url so resources are loaded in studio

8d1e329

fix: theme files not loading on studio edit window

beaf227

fix: tinymce editor not loading in edit window for simple editor setting

00caaaa

chore: bumped minor version to 4.5.0

cb5d604

chore: update edx paragon to latest version

e80eecf

fix: failing tests due to paragon api change

f6dbe3d

fix: use correct path for pulling translations

dcf56d3

chore: update translations

34e3b01

chore: build static files after updating translations

a3847e8

chore: rebuild source translations

b0c2cae

chore: rebuild static & translations after conflict resolution

e968175

tecoholic force-pushed the tecoholic/BB-6151-upgrade-tinymce-to-v5 branch from 1840e35 to e968175 Compare September 13, 2022 08:04

pomegranited merged commit 6072196 into openedx:master Sep 14, 2022

pomegranited deleted the tecoholic/BB-6151-upgrade-tinymce-to-v5 branch September 14, 2022 02:02

Agrendalath mentioned this pull request Jun 6, 2023

fix!: use TinyMCE 5 as HTML editor openedx-unsupported/xblock-utils#207

Merged

[BB-6151] Update TinyMCE Editor static file references to upgraded version 5.x #1869

[BB-6151] Update TinyMCE Editor static file references to upgraded version 5.x #1869

Conversation

tecoholic commented Jul 1, 2022 • edited Loading

openedx-webhooks commented Jul 1, 2022

Agrendalath commented Jul 1, 2022

Agrendalath left a comment

Choose a reason for hiding this comment

tecoholic commented Jul 4, 2022

tecoholic commented Jul 5, 2022

Agrendalath commented Jul 6, 2022

tecoholic commented Jul 7, 2022

tecoholic commented Jul 7, 2022

Agrendalath commented Jul 7, 2022

tecoholic commented Jul 7, 2022

Agrendalath left a comment

Choose a reason for hiding this comment

Agrendalath commented Jul 8, 2022

tecoholic commented Jul 12, 2022

natabene commented Jul 20, 2022

pomegranited left a comment

Choose a reason for hiding this comment

pomegranited commented Jul 28, 2022

natabene commented Jul 28, 2022 • edited Loading

tecoholic commented Jul 29, 2022

tecoholic commented Aug 5, 2022

pomegranited commented Aug 8, 2022

tecoholic commented Aug 8, 2022 • edited Loading

pomegranited commented Aug 9, 2022

tecoholic commented Aug 15, 2022

UsamaSadiq commented Sep 6, 2022 • edited Loading

UsamaSadiq commented Sep 6, 2022 • edited Loading

pomegranited commented Sep 7, 2022

tecoholic commented Sep 9, 2022

Agrendalath commented Sep 9, 2022 • edited Loading

tecoholic commented Sep 9, 2022

tecoholic commented Sep 9, 2022

Agrendalath commented Sep 9, 2022 • edited Loading

Agrendalath commented Sep 9, 2022

tecoholic commented Sep 12, 2022

pomegranited commented Sep 13, 2022

tecoholic commented Sep 13, 2022

openedx-webhooks commented Sep 14, 2022

tecoholic commented Jul 1, 2022 •

edited

Loading

natabene commented Jul 28, 2022 •

edited

Loading

tecoholic commented Aug 8, 2022 •

edited

Loading

UsamaSadiq commented Sep 6, 2022 •

edited

Loading

UsamaSadiq commented Sep 6, 2022 •

edited

Loading

Agrendalath commented Sep 9, 2022 •

edited

Loading

Agrendalath commented Sep 9, 2022 •

edited

Loading