added function get_allow_discussion_value #1674

Akshat2Jain · 2023-08-03T13:56:39Z

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 plone/volto#4758

mister-roboto · 2023-08-03T13:56:45Z

@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 · 2023-08-03T13:56:52Z

✅ Deploy Preview for plone-restapi canceled.

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

wesleybl · 2023-08-03T15:26:57Z

@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 · 2023-08-10T12:41:12Z

Any update regarding this pr

Akshat2Jain · 2023-08-13T10:13:25Z

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

news/1674.bugfix

src/plone/restapi/serializer/dxcontent.py

src/plone/restapi/tests/test_dxcontent_serializer.py

davisagli · 2023-08-15T05:13:23Z

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 · 2023-08-16T12:58:33Z

@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 · 2023-08-16T13:15:26Z

@davisagli I found this:

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

@jaroel any opinions on this?

wesleybl · 2023-08-16T14:42:58Z

@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 · 2023-08-16T14:47:41Z

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 · 2023-08-16T15:43:32Z

@davisagli I found this:

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

@jaroel any opinions on this?

Yep, see plone/Products.CMFPlone#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 · 2023-08-16T15:56:48Z

@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 · 2023-08-16T16:08:12Z

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 · 2023-08-16T16:11:45Z

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 · 2023-08-16T16:11:56Z

Would you be interested in fix this?

I would not.

jaroel · 2023-08-16T16:13:20Z

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 · 2023-08-18T11:11:59Z

@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 · 2023-09-03T14:31:27Z

@wesleybl, please check

Akshat2Jain · 2023-10-29T11:52:02Z

@wesleybl should we move forward?

wesleybl · 2023-10-30T13:59:14Z

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 · 2023-10-30T18:12:18Z

@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.

wesleybl

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:

plone.restapi/src/plone/restapi/tests/test_dxcontent_serializer.py

Line 449 in 04e6982

def test_allow_discussion_by_default(self):

but using self.portal.

Akshat2Jain · 2023-11-30T15:10:27Z

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:

plone.restapi/src/plone/restapi/tests/test_dxcontent_serializer.py

Line 449 in 04e6982

def test_allow_discussion_by_default(self):

but using self.portal.

where do I have to make this pr?

wesleybl · 2023-11-30T17:35:54Z

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

Akshat2Jain · 2023-12-05T17:34:58Z

@wesleybl

src/plone/restapi/tests/test_dxcontent_serializer.py

Akshat2Jain · 2023-12-11T16:33:07Z

@wesleybl

src/plone/restapi/tests/test_dxcontent_serializer.py

Akshat2Jain · 2023-12-14T17:12:56Z

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

src/plone/restapi/tests/test_dxcontent_serializer.py

wesleybl · 2023-12-19T17:33:59Z

@jenkins-plone-org please run jobs

wesleybl · 2024-01-26T12:41:42Z

@Akshat2Jain please rebase this branch.

wesleybl · 2024-01-26T12:56:55Z

@jenkins-plone-org please run jobs

wesleybl

LGTM.

davisagli

@wesleybl @Akshat2Jain Thanks for your patience on this one.

added allow_discussion funtion

af6afe3

added log

08213f2

Merge branch 'master' into allow_discussion

3719cc3

davisagli requested changes Aug 15, 2023

View reviewed changes

news/1674.bugfix Outdated Show resolved Hide resolved

src/plone/restapi/serializer/dxcontent.py Show resolved Hide resolved

src/plone/restapi/tests/test_dxcontent_serializer.py Outdated Show resolved Hide resolved

jaroel mentioned this pull request Aug 16, 2023

Un-remove IContentish from site root class plone/Products.CMFPlone#3833

Closed

Akshat2Jain and others added 2 commits August 18, 2023 15:51

Merge branch 'master' into allow_discussion

c9eb2cf

added else condition

e3c0960

added back test

bcfe006

Akshat2Jain mentioned this pull request Sep 3, 2023

undo icontentish interface plone/Products.CMFPlone#3836

Merged

wesleybl requested changes Nov 29, 2023

View reviewed changes

stevepiercy changed the title ~~added funtion get_allow_discussion_value~~ added function get_allow_discussion_value Nov 30, 2023

Akshat2Jain and others added 6 commits December 4, 2023 18:54

Merge branch 'main' into allow_discussion

0d92612

added discussion test to portal

f1a9ef9

added testSerializer

e87153c

added function

065b42c

formatted

658858d

test name changes to prevent flake8 fail

45ed392

wesleybl requested changes Dec 6, 2023

View reviewed changes

src/plone/restapi/tests/test_dxcontent_serializer.py Outdated Show resolved Hide resolved

src/plone/restapi/tests/test_dxcontent_serializer.py Outdated Show resolved Hide resolved

src/plone/restapi/tests/test_dxcontent_serializer.py Outdated Show resolved Hide resolved

Akshat2Jain added 2 commits December 7, 2023 21:58

made changes allow_discussion_portal_function

8e4a9cc

removed flake 8 error

0926d71

wesleybl requested changes Dec 12, 2023

View reviewed changes

src/plone/restapi/tests/test_dxcontent_serializer.py Outdated Show resolved Hide resolved

src/plone/restapi/tests/test_dxcontent_serializer.py Outdated Show resolved Hide resolved

src/plone/restapi/tests/test_dxcontent_serializer.py Outdated Show resolved Hide resolved

wesleybl reviewed Dec 12, 2023

View reviewed changes

src/plone/restapi/tests/test_dxcontent_serializer.py Outdated Show resolved Hide resolved

update the requested version

eab8bef

wesleybl requested changes Dec 15, 2023

View reviewed changes

src/plone/restapi/tests/test_dxcontent_serializer.py Outdated Show resolved Hide resolved

removed the portal error

90b9e09

Merge branch 'main' into allow_discussion

3db1034

wesleybl requested a review from davisagli January 26, 2024 13:40

wesleybl approved these changes Jan 26, 2024

View reviewed changes

davisagli approved these changes Feb 12, 2024

View reviewed changes

davisagli merged commit 7833026 into main Feb 12, 2024
25 checks passed

davisagli deleted the allow_discussion branch February 12, 2024 02:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added function get_allow_discussion_value #1674

added function get_allow_discussion_value #1674

Akshat2Jain commented Aug 3, 2023

mister-roboto commented Aug 3, 2023

netlify bot commented Aug 3, 2023 •

edited

Loading

wesleybl commented Aug 3, 2023

Akshat2Jain commented Aug 10, 2023

Akshat2Jain commented Aug 13, 2023

davisagli commented Aug 15, 2023

wesleybl commented Aug 16, 2023

wesleybl commented Aug 16, 2023

wesleybl commented Aug 16, 2023

davisagli commented Aug 16, 2023

jaroel commented Aug 16, 2023

wesleybl commented Aug 16, 2023 •

edited

Loading

jaroel commented Aug 16, 2023

davisagli commented Aug 16, 2023

jaroel commented Aug 16, 2023

jaroel commented Aug 16, 2023

Akshat2Jain commented Aug 18, 2023

Akshat2Jain commented Sep 3, 2023

Akshat2Jain commented Oct 29, 2023

wesleybl commented Oct 30, 2023

davisagli commented Oct 30, 2023

wesleybl left a comment

Akshat2Jain commented Nov 30, 2023

wesleybl commented Nov 30, 2023

Akshat2Jain commented Dec 5, 2023

Akshat2Jain commented Dec 11, 2023

Akshat2Jain commented Dec 14, 2023

wesleybl commented Dec 19, 2023

wesleybl commented Jan 26, 2024

wesleybl commented Jan 26, 2024

wesleybl left a comment

davisagli left a comment

added function get_allow_discussion_value #1674

added function get_allow_discussion_value #1674

Conversation

Akshat2Jain commented Aug 3, 2023

Description

Why

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

mister-roboto commented Aug 3, 2023

netlify bot commented Aug 3, 2023 • edited Loading

✅ Deploy Preview for plone-restapi canceled.

wesleybl commented Aug 3, 2023

Akshat2Jain commented Aug 10, 2023

Akshat2Jain commented Aug 13, 2023

davisagli commented Aug 15, 2023

wesleybl commented Aug 16, 2023

wesleybl commented Aug 16, 2023

wesleybl commented Aug 16, 2023

davisagli commented Aug 16, 2023

jaroel commented Aug 16, 2023

wesleybl commented Aug 16, 2023 • edited Loading

jaroel commented Aug 16, 2023

davisagli commented Aug 16, 2023

jaroel commented Aug 16, 2023

jaroel commented Aug 16, 2023

Akshat2Jain commented Aug 18, 2023

Akshat2Jain commented Sep 3, 2023

Akshat2Jain commented Oct 29, 2023

wesleybl commented Oct 30, 2023

davisagli commented Oct 30, 2023

wesleybl left a comment

Choose a reason for hiding this comment

Akshat2Jain commented Nov 30, 2023

wesleybl commented Nov 30, 2023

Akshat2Jain commented Dec 5, 2023

Akshat2Jain commented Dec 11, 2023

Akshat2Jain commented Dec 14, 2023

wesleybl commented Dec 19, 2023

wesleybl commented Jan 26, 2024

wesleybl commented Jan 26, 2024

wesleybl left a comment

Choose a reason for hiding this comment

davisagli left a comment

Choose a reason for hiding this comment

netlify bot commented Aug 3, 2023 •

edited

Loading

wesleybl commented Aug 16, 2023 •

edited

Loading