added function get_allow_discussion_value

[Akshat2Jain]

Description

This pr aims to add the function

def get_allow_discussion_value(context, request):
    return getMultiAdapter((context, request), name="conversation_view").enabled()

in dxcontent.py and site.py

Why

The Plone Site serializer and the Dexterity serializer return the same value for the "allow_discussion" field. Rather than duplicating the code, create a separate function that can be called by both serializers to set the value consistently.

This pr is related to the issue mentioned in the Volto project

fixes https://github.com/plone/volto/issues/4758

[mister-roboto]

@Akshat2Jain 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!

[netlify[bot]]

✅ Deploy Preview for plone-restapi canceled.

Name	Link
🔨 Latest commit	3db1034c9d36b5122c93ba1fef6e5e8cd34ab656
🔍 Latest deploy log	https://app.netlify.com/sites/plone-restapi/deploys/65b3a8b66b1b83000813f2ed

[wesleybl]

@davisagli @mauritsvanrees @tisto the tests failed in Plone 6 because the Plone Site does not have the Products.CMFCore.interfaces.IContentish interface:

https://github.com/plone/plone.restapi/actions/runs/5751692622/job/15591025795?pr=1674#step:10:242

Should it have? In Plone 5.2 the test does not fail. The interface is needed to access the view:

https://github.com/plone/plone.app.discussion/blob/c38fa500e929e51eb2eb289097054c85710db601/plone/app/discussion/browser/configure.zcml#L130

[Akshat2Jain]

Any update regarding this pr

[Akshat2Jain]

@wesleybl, do you suggest something to go ahead with?

[davisagli]

the tests failed in Plone 6 because the Plone Site does not have the Products.CMFCore.interfaces.IContentish interface

@wesleybl Are you sure that's why? It should inherit it from PortalContent (https://github.com/zopefoundation/Products.CMFCore/blob/master/src/Products/CMFCore/PortalContent.py#L32) via the plone.dexterity Container class. You can check portal.__provides__.__iro__ to see all provided interfaces.

Maybe the problem is instead that the IDiscussionLayer browser layer is missing in the test? Just a guess.

[wesleybl]

@wesleybl Are you sure that's why? It should inherit it from PortalContent (https://github.com/zopefoundation/Products.CMFCore/blob/master/src/Products/CMFCore/PortalContent.py#L32) via the plone.dexterity Container class. You can check portal.provides.iro to see all provided interfaces.

@davisagli yes, it does not have the interface:

>>> portal.__provides__.__iro__
(<InterfaceClass plone.base.interfaces.siteroot.IPloneSiteRoot>, <InterfaceClass plone.base.interfaces.siteroot.INavigationRoot>, <InterfaceClass Products.CMFCore.interfaces.ISiteRoot>, <InterfaceClass plone.base.interfaces.syndication.ISyndicatable>, <InterfaceClass Products.Five.component.interfaces.IObjectManagerSite>, <InterfaceClass zope.component.interfaces.ISite>, <InterfaceClass plone.dexterity.interfaces.IDexterityContainer>, <InterfaceClass Products.CMFDynamicViewFTI.interfaces.ISelectableBrowserDefault>, <InterfaceClass Products.CMFDynamicViewFTI.interfaces.IBrowserDefault>, <InterfaceClass Products.CMFCore.interfaces.ICatalogAware>, <InterfaceClass Products.CMFCore.interfaces.IWorkflowAware>, <InterfaceClass Products.CMFCore.interfaces.IOpaqueItemManager>, <InterfaceClass OFS.interfaces.IOrderedContainer>, <InterfaceClass plone.folder.interfaces.IOrderableFolder>, <InterfaceClass plone.folder.interfaces.IFolder>, <InterfaceClass plone.contentrules.engine.interfaces.IRuleAssignable>, <InterfaceClass plone.portlets.interfaces.ILocalPortletAssignable>, <InterfaceClass zope.annotation.interfaces.IAttributeAnnotatable>, <InterfaceClass zope.annotation.interfaces.IAnnotatable>, <InterfaceClass Products.CMFCore.interfaces.IFolderish>, <InterfaceClass Products.CMFCore.interfaces.IDynamicType>, <InterfaceClass OFS.interfaces.IFolder>, <InterfaceClass OFS.interfaces.IObjectManager>, <InterfaceClass zope.component.interfaces.IPossibleSite>, <InterfaceClass zope.container.interfaces.IContainer>, <InterfaceClass zope.container.interfaces.IReadContainer>, <InterfaceClass zope.container.interfaces.ISimpleReadContainer>, <InterfaceClass zope.container.interfaces.IItemContainer>, <InterfaceClass zope.interface.common.mapping.IEnumerableMapping>, <ABCInterfaceClass zope.interface.common.collections.ISized>, <InterfaceClass zope.interface.common.mapping.IReadMapping>, <ABCInterfaceClass zope.interface.common.collections.IContainer>, <ABCInterfaceClass zope.interface.common.ABCInterface>, <InterfaceClass zope.interface.common.mapping.IItemMapping>, <InterfaceClass zope.container.interfaces.IWriteContainer>, <InterfaceClass OFS.interfaces.ICopyContainer>, <InterfaceClass App.interfaces.INavigation>, <InterfaceClass webdav.interfaces.IDAVCollection>, <InterfaceClass webdav.interfaces.IDAVResource>, <InterfaceClass OFS.interfaces.IWriteLock>, <InterfaceClass OFS.EtagSupport.EtagBaseInterface>, <InterfaceClass OFS.interfaces.IPropertyManager>, <InterfaceClass OFS.interfaces.IFindSupport>, <InterfaceClass plone.dexterity.interfaces.IDexterityContent>, <InterfaceClass plone.uuid.interfaces.IAttributeUUID>, <InterfaceClass plone.uuid.interfaces.IUUIDAware>, <InterfaceClass Products.CMFCore.interfaces.ICatalogableDublinCore>, <InterfaceClass Products.CMFCore.interfaces.IMutableDublinCore>, <InterfaceClass Products.CMFCore.interfaces.IMutableMinimalDublinCore>, <InterfaceClass Products.CMFCore.interfaces.IDublinCore>, <InterfaceClass Products.CMFCore.interfaces.IMinimalDublinCore>, <InterfaceClass OFS.interfaces.ISimpleItem>, <InterfaceClass OFS.interfaces.IItem>, <InterfaceClass OFS.interfaces.IZopeObject>, <InterfaceClass OFS.interfaces.IManageable>, <InterfaceClass OFS.interfaces.ICopySource>, <InterfaceClass OFS.interfaces.ITraversable>, <InterfaceClass AccessControl.interfaces.IOwned>, <InterfaceClass persistent.interfaces.IPersistent>, <InterfaceClass Acquisition.interfaces.IAcquirer>, <InterfaceClass AccessControl.interfaces.IRoleManager>, <InterfaceClass AccessControl.interfaces.IPermissionMappingSupport>, <InterfaceClass zope.location.interfaces.IContained>, <InterfaceClass zope.location.interfaces.ILocation>, <InterfaceClass zope.interface.Interface>)

Maybe @implementer doesn't trigger inheritance? Or is it "overwritten" at some point?

But do you agree with me that the portal should have this interface?

[wesleybl]

@davisagli I found this:

https://github.com/plone/Products.CMFPlone/blob/016459fd9d023017e9dc0a0b635bd66099826db1/Products/CMFPlone/Portal.py#L214

@jaroel any opinions on this?

[wesleybl]

@davisagli would it be the case to release the p.a.d view for any content? Or for an interface that the portal has?

[davisagli]

I found this:

@wesleybl Oh, interesting. Hmm, I wonder what other things don't work on the site root because of this.

would it be the case to release the p.a.d view for any content?

What would you suggest? That is already the intent behind registering it for IContentish

Or for an interface that the portal has?

It would be fine to also register the same view for ISiteRoot

[jaroel]

@davisagli I found this:

https://github.com/plone/Products.CMFPlone/blob/016459fd9d023017e9dc0a0b635bd66099826db1/Products/CMFPlone/Portal.py#L214

@jaroel any opinions on this?

Yep, see https://github.com/plone/Products.CMFPlone/issues/3833 .

This specific issue exists as in plone.restapi we already have different serializers for the site root and regular DX content: this bug would've existed anyhow imho.

[wesleybl]

@davisagli @jaroel I understand that the best solution would be to add the IContentish interface to the Plone site. Would you be interested in fix this?

I don't know if @Akshat2Jain would be interested in fix that out. @Akshat2Jain, could you do a PR on Products.CMFPlone removing the interface removal, at least so we can see the tests that break?

[jaroel]

Personally I'd rather not have the indirection of the function, but see if we can have the site root serializer extend from the DX content one. Then we need to handle this in one place only. Still need tests for both serializers though.

Still we need to either have IContentish or register conversation_view for IPloneSiteRoot also and move on.

[davisagli]

Personally I'd rather not have the indirection of the function, but see if we can have the site root serializer extend from the DX content one.

We can't do this in the 8.x branch of plone.restapi, which is still compatible with Plone 5.2

[jaroel]

Would you be interested in fix this?

I would not.

[jaroel]

Personally I'd rather not have the indirection of the function, but see if we can have the site root serializer extend from the DX content one.

We can't do this in the 8.x branch of plone.restapi, which is still compatible with Plone 5.2

Fair enough.

[Akshat2Jain]

@davisagli @jaroel I understand that the best solution would be to add the IContentish interface to the Plone site. Would you be interested in fix this?

I don't know if @Akshat2Jain would be interested in fix that out. @Akshat2Jain, could you do a PR on Products.CMFPlone removing the interface removal, at least so we can see the tests that break?

Yeah Sure! I will try

[Akshat2Jain]

@wesleybl, please check

[Akshat2Jain]

@wesleybl should we move forward?

[wesleybl]

Since we won't have IContentish for Plone Site in Plone 6.0, we would need to test whether the context implements IContentish before trying to use conversation_view. This would cause test results to be different in Plone 6.0 and Plone 6.1. Maybe we would have to have different tests for each version of Plone.

@davisagli , what's your opinion here?

[davisagli]

@wesleybl I would focus on fixing it for Plone 6.1, and if it's difficult to fix for Plone 6.0 as well, you can just configure the test to be skipped there.

[davisagli]

@wesleybl Or is the problem that there are many tests where this key appears in the serialization even though it's not the main thing being tested?

[wesleybl]

@davisagli we can skip tests in Plone 6.0 and 5.2 that don't work. Then we can see if many needed to be skipped.

[wesleybl]

@Akshat2Jain please test whether the IContentish interface is providedBy context before trying to use conversation_view. This would only be true in Plone 6.1.

[Akshat2Jain]

@Akshat2Jain please test whether the IContentish interface is providedBy context before trying to use conversation_view. This would only be true in Plone 6.1.

@wesleybl can you explain to me a little bit? Have we not checked it previously when tests were failing?

[wesleybl]

@Akshat2Jain to use conversation_view, the context must implement the IContentish interface. But in the case of Plone Site, only in Plone 6.1 it will implement. So we need to test before trying to use conversation_view, since the version of plone.restapi is the same for Plone 6.1 and 6.0. It would be something like:

if IContentish.providedBy(context):
    result["allow_discussion"] = getMultiAdapter(
        (context, request), name="conversation_view"
    ).enabled()

[Akshat2Jain]

@wesleybl

[wesleybl]

@Akshat2Jain please check the github action jobs. They are failing.

[Akshat2Jain]

@wesleybl

[wesleybl]

@Akshat2Jain see my comment in https://github.com/plone/plone.restapi/pull/1674#discussion_r1376698814

[wesleybl]

@Akshat2Jain see this comment, to run Jenkins jobs: https://github.com/plone/plone.restapi/pull/1674#issuecomment-1664032275

[Akshat2Jain]

@jenkins-plone-org please run jobs

[Akshat2Jain]

Strangely the tests passed in Plone 6.1. This means that we do not have allow_discussion tests to portal. We have to create one, and skip it in Plone < 6.1. It would be a similar test:

https://github.com/plone/plone.restapi/blob/04e69827e9f8f935b0091a50096f780d2363b13c/src/plone/restapi/tests/test_dxcontent_serializer.py#L449

but using self.portal.

where do I have to make this pr?

[wesleybl]

@Akshat2Jain you can add the test in this same PR.

[Akshat2Jain]

@wesleybl

[Akshat2Jain]

@wesleybl

[Akshat2Jain]

why KeyError: 'portal' is this error coming in test 3.12

[wesleybl]

@jenkins-plone-org please run jobs

[wesleybl]

@Akshat2Jain please rebase this branch.

[wesleybl]

@jenkins-plone-org please run jobs