Fix Plone 4.3 with p.a.widgets 1.x edit modals #720

petschki · 2017-05-30T09:49:09Z

see issue #719

…to issue_719

petschki · 2017-05-30T10:20:42Z

bah ... sorry for the mess in here ... tried to squash some commits, but gave up ... nevermind.

hvelarde · 2017-05-30T12:24:31Z

src/collective/cover/static/js/compose.js

@@ -96,7 +96,9 @@ $(document).ready(function() {

  TitleMarkupSetup();

-  if ($.fn.prepOverlay !== undefined) {


changes on JS usually require an upgrade step to cook the resources; do you mind to check if we already have one in place?

hvelarde · 2017-05-30T12:27:34Z

src/collective/cover/widgets/richtext.py

@@ -0,0 +1,50 @@
+import pkg_resources
+
+try:


I think this is not necessary as this code will only be execute if plone.app.widgets is installed and you already are checking for that on configure.zcml.

also, for me is pretty important to document what you're doing here, so we can remove this in the future when we drop compatibility with Plone 4. that said, please add docstrings at the module and class levels explaining why this is needed..

sure .. you're absolutely right ... i'll remove that

we're still missing the docstrings.

and make flake8 happy

I'll do it in a separate pull request ...

petschki · 2017-05-31T06:06:00Z

I'll try to fix the tests today ...

petschki · 2017-06-01T05:29:24Z

@hvelarde everything is green except one test in DEXTERITY_ONLY setup ... and I really do not know whats going wrong there. Can you give me some pointers? Could I add this test to the noncritical list (like issue_637 already is)?

implemented suggestions

hvelarde · 2017-06-01T12:02:39Z

@petschki those are mandelbugs; I restarted the builds for you; what happened with the idea of working on this directly in plone.app.widgets? I don't see the point on adding something that we must remove later.

petschki · 2017-06-01T12:18:32Z

@hvelarde ah ... thanks for restarting. Now we're green. But you're right about RichTextWidget... let me check the pull request plone/plone.app.widgets#160 before merging this...

petschki · 2017-06-02T06:05:29Z

WIP: plone/plone.app.widgets#163

petschki · 2017-06-07T12:40:55Z

plone/plone.app.widgets#163 was merged right now ... I'll request a release of p.a.widgets and cleanup the unused code here.

hvelarde · 2017-06-07T16:41:58Z

do we still to add something in collective.cover, or just using the new widgets solve your problem?

petschki · 2017-06-08T16:36:56Z

@hvelarde unfortunately we still have to map a custom FieldWidget adapter for the RichTextField (like its done in plone.app.widgets.dx_bbb) ... see plone/plone.app.widgets#164 ... my latest pull request would fix this, so everything would work supermagically plone/plone.app.widgets#165

hvelarde · 2017-06-08T18:42:11Z

ok, just keep me updated :-)

hvelarde · 2017-06-23T11:29:11Z

@petschki do you mind to make a rebase? also, do you mind to update me the status on this after the changes you made on plone.app.widgets?

petschki · 2017-06-26T06:25:37Z

@hvelarde sure ... I'll rebase later today. the plone.app.widgets progress made the integration a bit easier, so we do not need to wire any widgets in collective.cover anymore. right now, it depends on the latest 1.x branch but I think we can have a p.a.widgets release 1.10 this week. I've also setup a testing environment for the widgets implementation but got stuck in robots test, not finding elements within the plone-modal ... need to do more investigations on that this week. I'll commit the 4.3.x-widgets travis-environment later today and put it to allow_failures ...

hvelarde · 2017-06-26T12:08:21Z

we're having some mandelbugs with RF tests right now and I need to restart failing builds from time to time.

I think the problem on the new build you just created seems to be related with a feature removed from zc.buildout, but I don't remember very well and I'm not sure.

FWIW, buildout previously was able to downgrade itself and I think that's not the case anymore; that's why you're having:

VersionConflict: (zc.buildout 2.9.4 (/home/travis/build/collective/collective.cover/eggs/zc.buildout-2.9.4-py2.7.egg), Requirement.parse('zc.buildout==1.7.1'))

probably @idgserpro knows what is happening here.

idgserpro · 2017-06-27T14:54:07Z

We have good news and bad news. Let's start with the good ones, the reason (and a possible workaround) of the error.

When you use bootstrap.py, it downloads the latest zc.buildout (that now is 2.9.4). In bin/buildout it will point to /home/travis/build/collective/collective.cover/eggs/zc.buildout-2.9.4-py2.7.egg.

Here is the insanity: after bootstraping, when running bin/buildout, when downloading and extracting zc.buildout 1.7.1 for the first time when it's a direct dependency (install_requires) in an extensions section, it will give you the error: but if you already have a /home/travis/build/collective/collective.cover/eggs/zc.buildout-1.7.1-py2.7.egg in place, the buildout continues. That's why the error was happening only in the new job with the mr.developer in extensions, the old ones don't have mr.developer and had a cache that the new job didn't.

You can simulate it in your machine instead of travis, in a clean virtualenv environment (remember to comment a eggs-directory in ~/.buildout/default.cfg if you have it in place). Funny thing is: the first time you run it will break (giving the error), but the second time won't, because you now have a zc.buildout-1.7.1-py2.7.egg in your environment.

The bad news: It seems that it started in zc.buildout 2.9.x branch (https://travis-ci.org/collective/collective.cover/builds/247259422) but we don't have time to check exactly what happened to start having this issue. As miohtama used to say, life is hard and Plone is harder™.

setuptools have the same issue as zc.buildout above: travis decides to automatically install some packages and setuptools is a direct dependency from some of them (the specific versions can be checked here).

There are two immediate workarounds:

Pin zc.buildout and setuptools to the last version in versions-4.3.x-widgets.cfg (https://travis-ci.org/collective/collective.cover/jobs/247501030);

# VersionConflict: (zc.buildout 2.9.4 (/home/travis/build/collective/collective.cover/eggs/zc.buildout-2.9.4-py2.7.egg), Requirement.parse('zc.buildout==1.7.1'))
# We still don't know why this happens. It seems that when using extensions with a package that
# requires zc.buildout (like mr.developer).
zc.buildout = 
# XXX: Same as above.
# VersionConflict: (setuptools 12.0.5 (/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages), Requirement.parse('setuptools==20.1.1'))
# travis has some packages installed by default in it's virtualenv and setuptools is installed.
# https://docs.travis-ci.com/user/languages/python/#Pre-installed-packages
setuptools =

Run bootstrap with Plone 4.3 pins of zc.buildout and setuptools, bootstrap.py --buildout-version=1.7.1 --setuptools-version=20.1.1 in .travis.yml (https://travis-ci.org/collective/collective.cover/jobs/247520681)

# XXX:
# Trying to pin in versions-4.3.x-widgets to the latest version it works but then all tests break.
# So we're going to use the same versions that Plone uses in it's versions.
- PLONE_VERSION=4.3 WIDGETS=true BOOTSTRAP_PARAMS="--buildout-version=1.7.1 --setuptools-version=20.1.1"
(...)
python bootstrap.py $BOOTSTRAP_PARAMS

Bad news part 2: now you have 20 failed tests after solving the VersionConflict. @petschki, can you try to run locally and see if this happens only in Travis?

Another question: why are you pinning plone.app.widgets = since you're already checking it's branch in sources?

petschki · 2017-06-27T16:16:02Z

@idgserpro thanks for the details ... pinned everything down and removed unneeded version pin of plone.app.widgets. not the buildout runs smooth ... I'm aware of those 20 errors. thats why I moved the environment to allow_failures ..

idgserpro · 2017-06-27T17:13:31Z

You don't need the two pins, only one of them. It's a good practice as well to document the pins in the cfg itself, so in the future if you try to remove them its less work to know why it was pinned. Our suggestion to doc above the pins:

# XXX: https://github.com/collective/collective.cover/pull/720#issuecomment-311383938

hvelarde · 2017-06-27T17:38:37Z

I'm not sure which one would be best practice here; I guess we should ask.

idgserpro · 2017-06-27T18:06:19Z

I think we should open an issue at zc.buildout exposing the problem and, from there, we can know which one is the best approach.

From a Plone perspective, since they are planning to get rid of bootstrap.py, they will probably be in favour of changing cfgs.

twitter bootstrap sets its .hide class to `display:none !important` so we do not get any configure modal at all when assigning hide class in templates ... use the strategy to set `.modal { display:none }` like bootstrap does...

rodfersou · 2017-07-07T02:23:43Z

src/collective/cover/static/js/compose.js

+    if($("body").hasClass("pat-plone-widgets")) {
+      // XXX: reload tile content asynchronously here when destroying the modal
+      //      need to find the right event where to hook in
+      return  // exit here


miss semicolon here

hvelarde

the branch is outdated; a review is not possible in this state.

Fix Plone 4.3 with p.a.widgets 1.x edit modals

fe0520e

see issue #719

petschki added the in progress label May 30, 2017

petschki mentioned this pull request May 30, 2017

Tile edit overlay shows up twice with Plone 4.3-latest and p.a.widgets-1.x installed #719

Open

petschki added 3 commits May 30, 2017 11:55

update CHANGES.rst

22010e5

Fix Plone 4.3 with p.a.widgets 1.x edit modals

fced329

see issue #719

Merge branch 'issue_719' of github.com:collective/collective.cover in…

b47a472

…to issue_719

hvelarde requested a review from rodfersou May 30, 2017 12:27

hvelarde previously requested changes May 30, 2017

View reviewed changes

petschki added 4 commits May 30, 2017 15:00

add upgrade step (cook JS resources) and remove unused import check

b34755b

pep8

a57a70f

add tests for Phone 4.3 + p.a.widgets 1.x

67258bf

and make flake8 happy

remove tests for p.a.widgets

6600a39

I'll do it in a separate pull request ...

remove p.a.widgets test-egg

a56519a

Merge branch 'master' into issue_719

fab5f8d

fix HTML markup for modal

1d814c4

merge latest master and add p.a.widgets testing environment

6443c05

pin zc.buildout and setuptools for test

5670fcf

cleanup richtext widget

8fa0340

petschki added 6 commits June 29, 2017 15:56

fix JS check for plone.app.widgets

4b8b308

fix buildout

aa05c04

fix conflicting bootstrap classes

e0f067f

twitter bootstrap sets its .hide class to `display:none !important` so we do not get any configure modal at all when assigning hide class in templates ... use the strategy to set `.modal { display:none }` like bootstrap does...

restructure check for installed pat-plone-widgets

5357699

edit link initialization when p.a.widgets is installed

5c92132

failsafe compose.js check for installed plone.app.widgets

668a7db

rodfersou reviewed Jul 7, 2017

View reviewed changes

petschki added 3 commits July 13, 2017 08:57

fix move icon in tile configure

affa9e6

update tile_url to make "external rendered calls" possible

9c6a17e

fix tile_url

5ef1d4b

ale-rt approved these changes Oct 28, 2020

View reviewed changes

hvelarde requested changes Nov 12, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Plone 4.3 with p.a.widgets 1.x edit modals #720

Fix Plone 4.3 with p.a.widgets 1.x edit modals #720

petschki commented May 30, 2017

petschki commented May 30, 2017

hvelarde May 30, 2017

petschki Jun 1, 2017

hvelarde May 30, 2017 •

edited

Loading

petschki May 30, 2017

hvelarde May 30, 2017

petschki commented May 31, 2017

petschki commented Jun 1, 2017

hvelarde commented Jun 1, 2017 •

edited

Loading

petschki commented Jun 1, 2017 •

edited

Loading

petschki commented Jun 2, 2017

petschki commented Jun 7, 2017

hvelarde commented Jun 7, 2017

petschki commented Jun 8, 2017

hvelarde commented Jun 8, 2017

hvelarde commented Jun 23, 2017

petschki commented Jun 26, 2017

hvelarde commented Jun 26, 2017

idgserpro commented Jun 27, 2017 •

edited

Loading

petschki commented Jun 27, 2017

idgserpro commented Jun 27, 2017

hvelarde commented Jun 27, 2017 •

edited

Loading

idgserpro commented Jun 27, 2017 •

edited

Loading

rodfersou Jul 7, 2017

hvelarde left a comment

		@@ -96,7 +96,9 @@ $(document).ready(function() {

		TitleMarkupSetup();

		if ($.fn.prepOverlay !== undefined) {

Fix Plone 4.3 with p.a.widgets 1.x edit modals #720

Are you sure you want to change the base?

Fix Plone 4.3 with p.a.widgets 1.x edit modals #720

Conversation

petschki commented May 30, 2017

petschki commented May 30, 2017

hvelarde May 30, 2017

Choose a reason for hiding this comment

petschki Jun 1, 2017

Choose a reason for hiding this comment

hvelarde May 30, 2017 • edited Loading

Choose a reason for hiding this comment

petschki May 30, 2017

Choose a reason for hiding this comment

hvelarde May 30, 2017

Choose a reason for hiding this comment

petschki commented May 31, 2017

petschki commented Jun 1, 2017

hvelarde commented Jun 1, 2017 • edited Loading

petschki commented Jun 1, 2017 • edited Loading

petschki commented Jun 2, 2017

petschki commented Jun 7, 2017

hvelarde commented Jun 7, 2017

petschki commented Jun 8, 2017

hvelarde commented Jun 8, 2017

hvelarde commented Jun 23, 2017

petschki commented Jun 26, 2017

hvelarde commented Jun 26, 2017

idgserpro commented Jun 27, 2017 • edited Loading

petschki commented Jun 27, 2017

idgserpro commented Jun 27, 2017

hvelarde commented Jun 27, 2017 • edited Loading

idgserpro commented Jun 27, 2017 • edited Loading

rodfersou Jul 7, 2017

Choose a reason for hiding this comment

hvelarde left a comment

Choose a reason for hiding this comment

hvelarde May 30, 2017 •

edited

Loading

hvelarde commented Jun 1, 2017 •

edited

Loading

petschki commented Jun 1, 2017 •

edited

Loading

idgserpro commented Jun 27, 2017 •

edited

Loading

hvelarde commented Jun 27, 2017 •

edited

Loading

idgserpro commented Jun 27, 2017 •

edited

Loading