Support for folderish objects

sneridagh commented 3 years ago

So, this one is ready, and it's a hard one, in terms of decissions to be taken :)

This branch enables the support for DX folderish content objects, which was pretty much broken all this years. We need this working for Plone 6, since all the content are folderish and can contain a whole tree on themselves which, we don't want to copy it over (as it worked until now) using Zope copy method on checkout, neither copy them back again on checkin.

This PR hooks into the adapter for folderish content types and tweaks the checkout/checkin workflow, so:

On checkout, the content is cloned into the working copy rather than copied over, so the children are not copied along the content
The user is able to add children on the working copy as usual (eg. images, etc)
On checkin, the working copy is cloned back into the baseline, and the children (if any) of the working copy are also copied back as children of the baseline. The ids are preserved, the ids are collision aware, so links, images, and references are kept.

The idea is that if some add-on wants to modify the behaviour of the folderish (and the non-folderish as well) it still can create a more specific adapter over the existing ones and make them work any specific purpose.

The first decision is if we want to make breaking change out of it, since the nature of behaviour.

The second, I named this branch "plone6", this is not really true, since it will still have AT support on it. If the first decision is affirmative, do we want to remove all traces of AT and Python 2 support while we are on it?

Next one, do we need to PLIP it?

It's unlikely that this package improves over time, so my answer would be yes to the first two, and maybe to the other...

jensens commented 3 years ago

Just talked with @2silver and he told me there's a branch mentioned in #50 solving this problem. Did you had a look at it?

sneridagh commented 3 years ago

@jensens Nope, although I saw the issue. Not surprising, because now it's completely unusable in the current shape in core. Due that it's an external branch created 4 years ago, no way to see what really was developed there related with the problem. A pity that the author didn't contribute back properly.

This implements the first approach, cloning (as opposite of Zope copy) so we only clone the object, not the entire tree. So the job is done anyways (nothing that it was that complex). Where we go from here is still unexplored, like:

Do we allow the users to add new objects inside the working copy?
If not, maybe just allow images that we sync on check-in (then we take care of name clashes).

Given the architecture of p.a.iterate, we can implement a default, then the add-on authors could implement their own adapters on what to check-out and how to check in back.

tisto commented 3 years ago

@jenkins-plone-org please run jobs

sneridagh commented 3 years ago

@jenkins-plone-org please run jobs

tisto commented 3 years ago

@jenkins-plone-org please run jobs

mauritsvanrees commented 3 years ago

I am still doubting whether we should include this in Plone 5 or not. The tests pass, which helps. It is a new feature, so maybe not. But you could argue that this fixes a regression, where dexterity missed a feature that Archetypes did have. At least, from reading the initial comment above, it sounds like iterate has always worked fine for Archetypes folders, but not for dexterity folders. Is that correct? I haven't tried it.

Can someone summarise what was broken before? Did you get a traceback on checkout? Or checkin? Something else?

tisto commented 3 years ago

@mauritsvanrees I guess @sneridagh can answer your question better. Bottom line is that folderish DX objects never worked in the first place, only AT. There was no traceback but the folderish object was just not copied with its subfolders. Therefore this is a bugfix IMHO because it fixes a broken functionality. Though, since this somehow changes the behavior, @jensens recommended going with a major release, which I just did. My recommendation would be to include this in Plone 5 as well, but that's up to you to decide. :)

Background of this work is that we are working on the working copy support in Volto and since Volto pages are folderish we need to fix this for Plone 6.

sneridagh commented 3 years ago

@mauritsvanrees yes, that is correct. For AT, it worked since always but for DX, the situation was that the object was Zope-copied (along with all the children trees) on check-out then on check-in, only the working copy object was cloned at a field level, and all changes made to the cloned children were left behind (and deleted). So the experience was quite bad, since you potentially lose all the changes in there, no traceback, only bad behaviour. Even the container tests for DX were disabled.

This PR addresses that, enabling the tests that failed before and giving full support for DX containers in the way I described in the description.

There's no harm in Plone 5, only now it will work "as expected" or as "per design". If some add-on need to keep and move along some inner content for working correctly into the working copy, it can still hook on the right adapters to override check-out/check-in behaviours.

mauritsvanrees commented 3 years ago

Thanks for the info. Sound nice if we could include this in 5.2 then.

I tried it just now in Plone 5.2 Python 3 with your code. I do see a problem.

I created a folder with one page and one link.
I checked out the folder, changed the title and added one link, then checked in.
Folder seems fine, contents as well: the page and two links.
But main navigation is broken. The page shows: "error while rendering plone.global_sections"

Traceback

Traceback (most recent call last):
  File "/Users/maurits/shared-eggs/cp38/plone.app.viewletmanager-3.1.1-py3.8.egg/plone/app/viewletmanager/manager.py", line 110, in render
    html.append(viewlet.render())
  File "/Users/maurits/shared-eggs/cp38/plone.app.layout-3.4.6-py3.8.egg/plone/app/layout/viewlets/common.py", line 79, in render
    return self.index()
  File "/Users/maurits/shared-eggs/cp38/Zope-4.5.5-py3.8.egg/Products/Five/browser/pagetemplatefile.py", line 126, in __call__
    return self.__func__(__self__, *args, **kw)
  File "/Users/maurits/shared-eggs/cp38/Zope-4.5.5-py3.8.egg/Products/Five/browser/pagetemplatefile.py", line 58, in __call__
    s = self.pt_render(
  File "/Users/maurits/shared-eggs/cp38/zope.pagetemplate-4.5.0-py3.8.egg/zope/pagetemplate/pagetemplate.py", line 133, in pt_render
    return self._v_program(
  File "/Users/maurits/shared-eggs/cp38/Zope-4.5.5-py3.8.egg/Products/PageTemplates/engine.py", line 378, in __call__
    return template.render(**kwargs)
  File "/Users/maurits/shared-eggs/cp38/z3c.pt-3.3.0-py3.8.egg/z3c/pt/pagetemplate.py", line 176, in render
    return base_renderer(**context)
  File "/Users/maurits/shared-eggs/cp38/Chameleon-3.9.0-py3.8.egg/chameleon/zpt/template.py", line 302, in render
    return super(PageTemplate, self).render(**_kw)
  File "/Users/maurits/shared-eggs/cp38/Chameleon-3.9.0-py3.8.egg/chameleon/template.py", line 214, in render
    raise_with_traceback(exc, tb)
  File "/Users/maurits/shared-eggs/cp38/Chameleon-3.9.0-py3.8.egg/chameleon/utils.py", line 53, in raise_with_traceback
    raise exc
  File "/Users/maurits/shared-eggs/cp38/Chameleon-3.9.0-py3.8.egg/chameleon/template.py", line 192, in render
    self._render(stream, econtext, rcontext)
  File "/Users/maurits/community/plone-coredev/py3/var/cache/b74b3cb030d373673fef3a9ecd8fe459.py", line 224, in render
    __cache_4716882960 = _static_4657941232('python', 'view.render_globalnav()', econtext=econtext)(_static_4657941040(econtext, __zt_tmp))
  File "/Users/maurits/shared-eggs/cp38/zope.tales-5.1-py3.8.egg/zope/tales/pythonexpr.py", line 73, in __call__
    return eval(self._code, vars)
  File "<string>", line 1, in <module>
  File "/Users/maurits/shared-eggs/cp38/plone.app.layout-3.4.6-py3.8.egg/plone/app/layout/viewlets/common.py", line 390, in render_globalnav
    return self.build_tree(self.navtree_path)
  File "/Users/maurits/shared-eggs/cp38/plone.app.layout-3.4.6-py3.8.egg/plone/app/layout/viewlets/common.py", line 382, in build_tree
    for item in self.navtree.get(path, []):
  File "/Users/maurits/shared-eggs/cp38/plone.memoize-2.1.0-py3.8.egg/plone/memoize/view.py", line 59, in memogetter
    cache[key] = func(*args, **kwargs)
  File "/Users/maurits/shared-eggs/cp38/plone.app.layout-3.4.6-py3.8.egg/plone/app/layout/viewlets/common.py", line 323, in navtree
    brains = portal_catalog.searchResults(**query)
  File "/Users/maurits/community/plone-coredev/py3/src/Products.CMFPlone/Products/CMFPlone/CatalogTool.py", line 463, in searchResults
    return ZCatalog.searchResults(self, query, **kw)
  File "/Users/maurits/shared-eggs/cp38/Products.ZCatalog-5.2-py3.8.egg/Products/ZCatalog/ZCatalog.py", line 625, in searchResults
    return self._catalog.searchResults(query, **kw)
  File "/Users/maurits/shared-eggs/cp38/Products.ZCatalog-5.2-py3.8.egg/Products/ZCatalog/Catalog.py", line 1091, in searchResults
    return self.search(query, sort_indexes, reverse, sort_limit, _merge)
  File "/Users/maurits/shared-eggs/cp38/Products.ZCatalog-5.2-py3.8.egg/Products/ZCatalog/Catalog.py", line 702, in search
    result = self.sortResults(
  File "/Users/maurits/shared-eggs/cp38/Products.ZCatalog-5.2-py3.8.egg/Products/ZCatalog/Catalog.py", line 1007, in sortResults
    actual_result_count, length, result = sort_func(
  File "/Users/maurits/shared-eggs/cp38/Products.ZCatalog-5.2-py3.8.egg/Products/ZCatalog/Catalog.py", line 778, in _sort_iterate_resultset
    index_key_map = sort_index.documentToKeyMap()
  File "/Users/maurits/community/plone-coredev/py3/src/plone.folder/src/plone/folder/nogopip.py", line 121, in documentToKeyMap
    pos[rid] = container.getObjectPosition(id)
  File "/Users/maurits/community/plone-coredev/py3/src/plone.folder/src/plone/folder/ordered.py", line 110, in getObjectPosition
    return self.getOrdering().getObjectPosition(id)
  File "/Users/maurits/community/plone-coredev/py3/src/plone.folder/src/plone/folder/default.py", line 158, in getObjectPosition
    raise ValueError('No object with id "{0:s}" exists in "{1:s}".'.format(
ValueError: No object with id "pagina-1" exists in "/Plone2/map".

 - Expression: "python:view.render_globalnav()"
 - Filename:   ... ut-3.4.6-py3.8.egg/plone/app/layout/viewlets/sections.pt
 - Location:   (line 22: col 42)
 - Source:     ... eplace="structure python:view.render_globalnav()" />
                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Arguments:  template: <Products.Five.browser.pagetemplatefile.ViewPageTemplateFile object at 0x10f68cc10>
               options: {}
               args: ()
               nothing: None
               modules: <Products.PageTemplates.ZRPythonExpr._SecureModuleImporter object at 0x10c5fcdc0>
               request: <WSGIRequest, URL=http://localhost:8080/Plone2/front-page/document_view>
               view: <Products.Five.viewlet.metaconfigure.GlobalSectionsViewlet object at 0x112f62730>
               context: <Document at /Plone2/front-page>
               views: <Products.Five.browser.pagetemplatefile.ViewMapper object at 0x112f98550>
               here: <Document at /Plone2/front-page>
               container: <Document at /Plone2/front-page>
               root: <Application at >
               traverse_subpath: []
               user: <PropertiedUser 'admin'>
               default: <DEFAULT>
               repeat: <Products.PageTemplates.engine.RepeatDictWrapper object at 0x112fac900>
               loop: {}
               target_language: None
               translate: <function BaseTemplate.render.<locals>.translate at 0x112fa90d0>
               attrs: {}
               portal_tabs: [{'id': 'index_html', 'category': 'portal_tabs', 'title': 'Home', 'description': '', 'url': <Products.CMFCore.Expression.Expression object at 0x112768f90 oid 0x3300 in <Connection at 10e74caf0>>, 'link_target': None, 'icon': '', 'available': True, 'visible': True, 'allowed': True, 'name': 'Home'}, {'name': 'Nieuws', 'id': 'news', 'url': 'http://localhost:8080/Plone2/news', 'description': 'Nieuwsberichten op deze website.', 'review_state': 'published'}, {'name': 'Agenda', 'id': 'events', 'url': 'http://localhost:8080/Plone2/events', 'description': 'Agenda-items op deze website.', 'review_state': 'published'}, {'name': 'Gebruikers', 'id': 'Members', 'url': 'http://localhost:8080/Plone2/Members', 'description': 'Map voor de persoonlijke mappen van gebruikers.', 'review_state': 'private'}, {'name': 'Underscore', 'id': 'underscore-1', 'url': 'http://localhost:8080/Plone2/underscore-1', 'description': '', 'review_state': 'private'}, {'name': 'Plain text', 'id': 'all-plone-releases.txt', 'url': 'http://localhost:8080/Plone2/all-plone-releases.txt/view', 'description': '', 'review_state': Missing.Value}, {'name': 'file.pdf', 'id': 'file.pdf', 'url': 'http://localhost:8080/Plone2/file.pdf/view', 'description': '', 'review_state': Missing.Value}, {'name': 'whisky-as-we-get-it.jpg', 'id': 'whisky-as-we-get-it.jpg', 'url': 'http://localhost:8080/Plone2/whisky-as-we-get-it.jpg/view', 'description': '', 'review_state': Missing.Value}, {'name': '<script>console.error("hello from folder title")</script>', 'id': 'folder', 'url': 'http://localhost:8080/Plone2/folder', 'description': '<script>console.error("hello from folder description")</script>', 'review_state': 'published'}, {'name': 'Map 2', 'id': 'map', 'url': 'http://localhost:8080/Plone2/map', 'description': 'Testbeschrijving 2.', 'review_state': 'published'}]

A clear and rebuild of the catalog does not help.

Might be something for a new issue, but maybe you see what is wrong.

sneridagh commented 3 years ago

@mauritsvanrees yes, I can confirm the issue in classic :( Weird that it's not present in the Navigation component in Volto, since it's sharing (almost) the same code than the Viewlet one...

mauritsvanrees commented 3 years ago

I only now see that Timo has already released this in 4.0.0 a few days ago. I have copied my comment to a new issue: https://github.com/plone/plone.app.iterate/issues/93

mauritsvanrees commented 3 years ago

For the record, I finally tried the same in the original implementation, and then check-in already fails with a traceback for the added link object. When I remove this new item I can check-in again. So practically speaking you could only change the title and description of the folder. So: this PR is definitely an improvement, except for the navigation problem.

plone / plone.app.iterate

Support for folderish objects #84