plone / plone.base

Plone Interface contracts, plus basic features and utilities
https://pypi.org/project/plone.base
2 stars 0 forks source link

Make the TinyMCE help plugin available as an option #59

Closed rber474 closed 7 months ago

rber474 commented 7 months ago

Fix issue #41

mister-roboto commented 7 months ago

@rber474 thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

rber474 commented 7 months ago

@jenkins-plone-org please run jobs

rber474 commented 7 months ago

@petschki, the review is needed or can this pull request be merged? Sometimes I am not sure about the workflow.

petschki commented 7 months ago

Sorry but as mentioned in the original PR here https://github.com/plone/plone.base/pull/42 this needs an upgrade step for existing installations, otherwise the vocabulary stored in the registry doesn't know the new token.

All in all @rber474 I wonder why you've once again created a new PRs instead of reviewing the existent one which was created before by @ewohnlich ?

rber474 commented 7 months ago

I tried, but the tests did not pass due to the verification of the signed agreement. Initially, I thought it was failing because of @ewohnlich's commits. However, it turned out that the failure was due to my email not being set up correctly on the company laptop.

Therefore, I recreated PR without any other goal or attempt to appropriate its work. If it could be, forget this PR and get back to @ewohnlich's

Sorry again!! 😞

Regarding the upgrade-step, thought that adding a new value to a source won't need it. At least, no exception was launched when I test in my local. It is not adding a new field, it just a new value that never has been stored. Anyway, I can add it to original PR.

Sorry @petschki

petschki commented 7 months ago

No worries. Steps to reproduce the issue without upgrade step:

  1. create Classic Plone Site with plone.base#main (or current release)
  2. upgrade to plone.base with help plugin
  3. go to @@tinymce-controlpanel, check the help plugin and click save

now you get a traceback:

Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 181, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 391, in publish_module
  Module ZPublisher.WSGIPublisher, line 285, in publish
  Module ZPublisher.mapply, line 98, in mapply
  Module ZPublisher.WSGIPublisher, line 68, in call_object
  Module plone.z3cform.layout, line 61, in __call__
  Module plone.z3cform.layout, line 45, in update
  Module plone.z3cform.fieldsets.extensible, line 62, in update
  Module plone.z3cform.patch, line 31, in GroupForm_update
  Module z3c.form.group, line 145, in update
  Module plone.app.z3cform.csrf, line 21, in execute
  Module z3c.form.action, line 98, in execute
  Module z3c.form.button, line 301, in __call__
  Module z3c.form.button, line 159, in __call__
  Module plone.app.registry.browser.controlpanel, line 60, in handleSave
  Module z3c.form.group, line 116, in applyChanges
  Module z3c.form.group, line 82, in applyChanges
  Module z3c.form.form, line 51, in applyChanges
  Module z3c.form.datamanager, line 91, in set
  Module plone.registry.recordsproxy, line 48, in __setattr__
  Module plone.registry.registry, line 45, in __setitem__
  Module plone.registry.record, line 79, in _set_value
  Module zope.schema._bootstrapfields, line 295, in validate
  Module zope.schema._field, line 772, in _validate
zope.schema._bootstrapinterfaces.WrongContainedType: ([ConstraintNotSatisfied('help', '')], 'value')

This is due to the registry's recordproxy which stores the original vocabulary to the database ... maybe the recordproxy should be more intelligent but unfortunately it isn't ...

rber474 commented 7 months ago

True.

Which repository would be the right one to place this upgrade_step? Should be an upgradeStep included in CMFPlone, shouldn't it?

    loadMigrationProfile(
        context,
        "profile-Products.CMFPlone:dependencies",
    )
petschki commented 7 months ago

Upgrades regarding CMFPlone are in repostiory plone.app.upgrade

petschki commented 7 months ago

btw. does this help plugin work for you ? in a vanilla classic plone I get a JS error: Uncaught (in promise) Script at URL "http://localhost:8080/Plone2/++webresource++ddd4552f-a679-5d91-a252-4b2fc079ddc2/++plone++static/bundle-plone/chunks/plugins/help/js/i18n/keynav/en.js" failed to load ... I think there are some tweaks in mockup necessary to make this plugin work? Maybe that's why it was not implemented?

rber474 commented 7 months ago

It worked, a help page is shown with the info for the hotkeys. Let me check again.

rber474 commented 7 months ago

Please, revert the PR. I am gonna make it work and check everything twice. Now I am getting same console error.

rber474 commented 7 months ago

@petschki I am working in the mockup help plugin for tinymce, to fix URL fail. Currently working in Navigation translations and cleaning up some unused code:

image

mauritsvanrees commented 5 months ago

An upgrade step was still needed, but @1letter meanwhile added this in https://github.com/plone/plone.app.upgrade/issues/324 and https://github.com/plone/plone.app.upgrade/pull/326 It was needed because he added the accordion plugin.

mauritsvanrees commented 5 months ago

Just checking if I understand the current status correctly. After I enable the help plugin, I see a Help menu in tinymce:

Screenshot 2024-04-24 at 18 02 31

But clicking it does nothing. Near the bottom of the tinymce widget I see "Press ⌥0 for help" but that does nothing either.

So the help module only really does something once https://github.com/plone/mockup/pull/1376 is working and merged. Is that correct?

petschki commented 5 months ago

I'm wondering why the mockup PR is needed because the help plugin is available in current TinyMCE 6 and 7 (https://www.tiny.cloud/docs/tinymce/latest/help/) ... maybe as backport for TinyMCE 5 ? but in that case I recommend to move on to Tiny 6+

petschki commented 5 months ago

NOTE: If Tiny ships with the plugin and it is activated in the controlpanel, the pattern tries to look it up with an import here: https://github.com/plone/mockup/blob/master/src/pat/tinymce/tinymce--implementation.js#L162 ... to get the log message in case something went wrong, you could add ?loglevel=DEBUG to the browser then you see them in the console.

stevepiercy commented 5 months ago

FYI I started a page in Plone 6 docs to upgrade to Plone 6.1. https://github.com/plone/documentation/pull/1655