plone / plone.app.iterate

Checkin/ checkout procedure for content editing (working copy) for Plone
https://pypi.python.org/pypi/plone.app.iterate
3 stars 12 forks source link

Cancelling a check out from the original deletes the original #54

Closed cdw9 closed 6 years ago

cdw9 commented 7 years ago

What I did:

What I expect to happen:

I expect this would delete the working copy of the page

What actually happened:

The original is deleted. Viewing the copy has some KeyErrors since the original has been deleted.

What version of Plone/ Addons I am using:

Plone 5.0.7, plone.app.iterate 3.2.4 and 3.3.5

jensens commented 6 years ago

I can confirm this in a fresh latest Plone 5.1 coredev buildout. This is a serious bug.

jensens commented 6 years ago

further, if after this one now clicks on the working copy, the toolbar is broken for this item. This is because the working copy can not find its baseline.

pbauer commented 6 years ago

Since this also happens in 5.0.7 (what about 4.3.x?) I do not consider this be a blocker for 5.1. Anyway, I can take a look at it tomorrow.

jensens commented 6 years ago

I think this one is really harmful. I can also look at it, a customer of mine depends on the feature. I hope it is easy to fix.

pbauer commented 6 years ago

I see two options:

  1. Do not display the action "Cancel check-out" on the baseline (which is the original object). This might be as easy as comparing the UID of the original with self.context in plone.app.iterate.browser.Control.cancel_allowed:
    @memoize
    def cancel_allowed(self):
        """Check to see if the user can cancel the checkout on the
        given working copy
        """
        policy = ICheckinCheckoutPolicy(self.context, None)
        if policy is None:
            return False
        original = policy.getBaseline()
        if original is not None:
            return IUUID(original) != IUUID(self.context)
        return False
  1. Display the action but cancel the checkout from the working copy. This would require a change in plone.app.iterate.browser.cancel.Cancel.__call__.

I vote for option 1 which requires to navigate to the working copy before canceling it. This way users are more likely to notice if they are deleting valuable work.

@jensens: I'll hold still when you implement. Ping me if you don't find the time. It would also be nice to have a non-doctest (with DX) that tests this.

pbauer commented 6 years ago

I just checked in Plone 4.3 and the action was not there on the orignal. So option 1 it is I guess.

jensens commented 6 years ago

I just look at it, now, first I write a test to show the wrong behavior...

jensens commented 6 years ago

Ok, the problem only occurs on the DX implementation. It does not raise the correct exception. See my commit on branch fix-54

agschaid commented 6 years ago

thank you @jensens for your time!

I see the problem in Plone 5.1b4 and plone.app.iterate-3.3.3 for now option 1 as proposed by @pbauer works nicely and we'll patch it like that until it is fixed

jensens commented 6 years ago

I found the real problem: it is much deeper in the dexterity implementation. Stay tuned, I will provide a PR in some minutes.

rileydog commented 6 years ago

I installed a new instance of Plone 5.1rc1 (5108).
tested the iterate and had the same problem as above. Using the info from above, I updated the iterate to 3.3.6 (I've done this successfully in the past, can't remember if it was 5.1 or 5.0.x) I got these errors when running buildout.

[testsite@wf-207-38-86-114 zinstance]$ bin/buildout Updating instance. Getting distribution for 'plone.app.iterate==3.3.6'. Using unpack_wheel() shim over the deprecated wheel_to_egg() hook. Please update your wheel extension implementation for one that installs a .whl handler in zc.buildout.easy_install.UNPACKERS While: Updating instance. Getting distribution for 'plone.app.iterate==3.3.6'. Error: Wheels are not supported [testsite@wf-207-38-86-114 zinstance]$

does anyone have any idea what this means?
more importantly (this is a test site so I can recreate) I know the iterate issue is fixed in 5.0.x ; however, is it fixed in 5.1rc1?

thanks for any help

agschaid commented 6 years ago

Well, I don't know about your setup, am not a plone guru and don't know what wheel is.

But it says Please update your wheel extension implementation for one that installs a .whl And that is what I would suggest you to do ;)

Besides: this issue is closed and doesn't really have something to do with your problem (which appears to be a setup problem). So I'd rather open another one than resurrect this one.

rileydog commented 6 years ago

Hi, Sorry, you’re right, I shouldn’t have bothered you with this unrelated post. At the time it seemed related, but it wasn’t.

Sorry and thanks for helping with iterate