undo icontentish interface #3836

Akshat2Jain · 2023-09-03T14:27:36Z

also related to plone/plone.restapi#1674

mister-roboto · 2023-09-03T14:27:41Z

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

Products/CMFPlone/Portal.py

davisagli · 2023-09-04T05:19:11Z

@jenkins-plone-org please run jobs

wesleybl · 2023-09-05T00:55:41Z

The tests are failing with the error:

File "/home/jenkins/.buildout/eggs/cp311/Products.CMFUid-4.0-py3.11.egg/Products/CMFUid/UniqueIdAnnotationTool.py", line 86, in handleUidAnnotationEvent
    uidtool.unregister(ob)
'NoneType' object has no attribute 'unregister'

in this line of this subscriber:

https://github.com/zopefoundation/Products.CMFUid/blob/8ac5c69df3d8d008561b9ace1cb5fc43d3d3bf1d/src/Products/CMFUid/UniqueIdAnnotationTool.py#L86

The error occurs when creating a Plone Site. It looks like the subscriber wants to use the portal_uidhandler tool, but it hasn't been created yet. Then this line could test if it exists, looking like this:

if anno_tool is not None and uidtool is not None:

Akshat2Jain · 2023-09-11T15:12:29Z

The tests are failing with the error:
File "/home/jenkins/.buildout/eggs/cp311/Products.CMFUid-4.0-py3.11.egg/Products/CMFUid/UniqueIdAnnotationTool.py", line 86, in handleUidAnnotationEvent
    uidtool.unregister(ob)
'NoneType' object has no attribute 'unregister'
in this line of this subscriber:

https://github.com/zopefoundation/Products.CMFUid/blob/8ac5c69df3d8d008561b9ace1cb5fc43d3d3bf1d/src/Products/CMFUid/UniqueIdAnnotationTool.py#L86

The error occurs when creating a Plone Site. It looks like the subscriber wants to use the portal_uidhandler tool, but it hasn't been created yet. Then this line could test if it exists, looking like this:
if anno_tool is not None and uidtool is not None:

@wesleybl what do I have to do in this?

wesleybl · 2023-09-11T15:50:09Z

@polyester @icemac

@Akshat2Jain have already signed the Plone contributor agreement. Could you add it to zopefoundation, so I can fix this error?

Akshat2Jain · 2023-09-16T03:52:53Z

any progress?

polyester · 2023-09-18T08:02:54Z

I'm not too sure what my role here is. For historical reasons, we need a (separate) contributor agreement for the Zopefoundation repos; that can be signed here: https://sign.plone.org/d/FTU6XN6qrGtoni

Is the question if @icemac should put that in there for him? Or if @Akshat2Jain does it there himself? I'm not at all familiar with the Zope codebase, testing infrastructure, and customs, although @jaroel is I think so maybe he knows.

wesleybl · 2023-09-18T12:42:55Z

@Akshat2Jain please sign the Zopefoundation contributor agreement. This way you can make a PR for Products.CMFUid

Akshat2Jain · 2023-09-18T13:40:36Z

@Akshat2Jain please sign the Zopefoundation contributor agreement. This way you can make a PR for Products.CMFUid

Done

Akshat2Jain · 2023-09-19T14:07:23Z

@wesleybl I have been added to the Zope Foundation. Please guide what I have to do next.

wesleybl · 2023-09-19T15:28:27Z

@Akshat2Jain you need to make a PR on Products.CMFUid, with what was explained in: #3836 (comment)

Akshat2Jain · 2023-09-28T05:21:58Z

@wesleybl can you run jobs again?

wesleybl · 2023-09-28T14:41:17Z

@Akshat2Jain It's no use running the jobs again. We need a new release of Products.CMFUid with the modifications made in zopefoundation/Products.CMFUid#23

wesleybl · 2023-10-02T15:14:10Z

@Akshat2Jain a new 4.1 release of Products.CMFUid is available. You now need to do a PR on buildout.coredev updating the version:

https://github.com/plone/buildout.coredev/blob/54aaf0f60c79f1e0038f832180b506a71c4498bb/versions.cfg#L151

wesleybl · 2023-10-09T23:47:45Z

@jenkins-plone-org please run jobs

wesleybl · 2023-10-10T19:38:50Z

@davisagli @jaroel @Akshat2Jain the errors with Products.CMFUid are gone.

We now have two tests failing in plone.api. Are they:

These tests count the number of objects returned by searching the catalog through the object_provides index, using the IContentish interface. As Plone Site now also implements this interface, the number of objects has increased by one. Therefore, I think it is only necessary to fix the number of objects in the test asserts.

@Akshat2Jain , can you make a PR on plone.api fxing these tests?

wesleybl · 2023-10-11T12:43:43Z

@Akshat2Jain can you please make a PR like this in branch 6.0.x?

https://github.com/plone/Products.CMFPlone/tree/6.0.x

Akshat2Jain · 2023-10-11T18:22:48Z

@davisagli @jaroel @Akshat2Jain the errors with Products.CMFUid are gone.

We now have two tests failing in plone.api. Are they:

test_find_interface_dict

test_find_interface_dict__include_not_query

These tests count the number of objects returned by searching the catalog through the object_provides index, using the IContentish interface. As Plone Site now also implements this interface, the number of objects has increased by one. Therefore, I think it is only necessary to fix the number of objects in the test asserts.

@Akshat2Jain , can you make a PR on plone.api fxing these tests?

this pr would be on the master branch? @wesleybl

Akshat2Jain · 2023-10-11T18:23:47Z

@Akshat2Jain can you please make a PR like this in branch 6.0.x?

https://github.com/plone/Products.CMFPlone/tree/6.0.x

and this pr would be on 6.0x branch? @wesleybl

wesleybl · 2023-10-11T19:27:36Z

this pr would be on the master branch?

in master of https://github.com/plone/plone.api

and this pr would be on 6.0x branch?

in branch 6.0.x of https://github.com/plone/Products.CMFPlone

wesleybl · 2023-10-11T19:32:20Z

@Akshat2Jain can you please make a PR like this in branch 6.0.x?

@Akshat2Jain the "like this" is the PR we are in now #3836, but in branch 6.0.x.

wesleybl · 2023-10-19T20:03:00Z

@jaroel did you manage to make this test fail locally? What did you do?

…ndo_icontentish

jaroel · 2023-10-19T20:59:17Z

@jenkins-plone-org please run jobs

jaroel · 2023-10-19T21:05:15Z

@wesleybl Sorry, I had a left over change. My buildout.coredev only checks out Products.CMFPlone branch=undo_icontentish and the test passes.

I fetched origin and then merged origin/master in. That should correctly disable that thingy I do in

Products.CMFPlone/Products/CMFPlone/__init__.py

Lines 12 to 21 in 621edb0

    
           if __version__ < "7": 
        
               # This sets SKIP_PTA to skip the check for 
        
               # Publication through acquisition in Plone 6. 
        
               # Please remove this code block when can. 
        
               import Products.CMFCore.explicitacquisition 
        
               from Products.CMFCore.explicitacquisition import PTA_ENV_KEY 
        
               os.environ[PTA_ENV_KEY] = os.environ.get(PTA_ENV_KEY, "false") 
        
               # Importing (from) the module sets SKIP_PTA. We need to override that too. 
        
               Products.CMFCore.explicitacquisition.SKIP_PTA = os.environ[PTA_ENV_KEY] == "false"

wesleybl · 2023-10-19T21:12:56Z

@jaroel anyway, this doesn't explain why the test fails in Jenkins and works locally. Are any environment variables set in Jenkins?

@gforcada can you help here?

…ndo_icontentish

jaroel · 2023-10-20T13:10:54Z

I'm setting SKIP_PTA in https://github.com/plone/plone.rest/blob/5f9898970f871008e84b254bf73ff13f48218bbc/src/plone/rest/tests/test_explicitacquisition.py#L67-L118, which then leaks into our test_site_serializer.py.

I can reproduce with ./bin/test -t test_portal_root -t test_site_root_get_request. Order is important here

wesleybl · 2023-10-20T13:41:19Z

@jaroel so you need to test the environment variable at the end of these tests and set the SKIP_PTA variable again, to restore the default behavior. Can you take care of it?

jaroel · 2023-10-20T13:43:25Z

@wesleybl please look here: plone/plone.rest#168

wesleybl · 2023-10-20T13:55:11Z

@jaroel anyway, it would be good to create a test similar to test_site_root_get_request, but setting SKIP_PTA to false. In the end, it was a good thing the test failed.

jaroel · 2023-10-20T13:57:21Z

@wesleybl No, there should be no difference.
There is a bug in plone.rest though, but that's out of scope for this PR.

wesleybl · 2023-10-20T14:02:42Z

No, there should be no difference.

But if someone uses SKIP_PTA = false, restapi will not work. That's why I think it's worth creating the test.

There is a bug in plone.rest though

Was it this bug that caused the test to fail? Is there already an issue about this? Can you point?

jaroel · 2023-10-20T14:29:05Z

@jenkins-plone-org please run jobs

note to self: This is gonna be useless because the plone.rest stuff isn't merge yet, isn't it?

jaroel · 2023-10-20T14:56:00Z

There is a bug in plone.rest though

Was it this bug that caused the test to fail? Is there already an issue about this? Can you point?

plone/plone.rest#169

wesleybl · 2023-10-20T15:53:43Z

@jenkins-plone-org please run jobs

@jaroel I think the tests will pass now. But I don't know if we should merge this before fix plone/plone.rest#169. If someone uses SKIP_PTA = false, restapi will not work.

jaroel · 2023-10-20T15:58:22Z

@wesleybl No worries, nobody uses that. It's just there so we don't ship it as enabled by default.

Products/CMFPlone/Portal.py

wesleybl · 2023-10-20T16:11:15Z

@jaroel can you please take care of #3856?

jaroel · 2023-10-20T16:16:50Z

@jaroel can you please take care of #3856?

amanhã, amanhã (I think I used that correctly)

wesleybl · 2023-10-20T16:25:02Z

@jenkins-plone-org please run jobs

news/3833.bugfix

jaroel · 2023-10-23T13:05:44Z

@davisagli how do you feel about landing this IContentish branch?

davisagli

Looks good to me. I also looked through the zopefoundation and plone github organizations for references to IContentish, just to see if there was anything that might be surprising, and I think it should be okay (as also indicated by the state of the tests).

I think I would keep it for 6.1 (master) only though and not make this change for 6.0 at this point.

Special thanks to @Akshat2Jain and @wesleybl for your persistence and patience with this one!

We will keep it at bugfix instead of feature.

wesleybl · 2023-10-24T18:02:39Z

I think I would keep it for 6.1 (master) only though and not make this change for 6.0 at this point.

If we don't have this in Plone 6.0, it won't be possible to fix plone/plone.restapi#1674 in an easy way, since we don't have a version of restapi just for Plone 6.1

wesleybl · 2023-10-24T18:12:39Z

I think it would be good to have an upgrade step to reindex the Plone Site, since we have indexes that have interfaces.

undo icontentish interface

57ed4a9

added log file

d563b34

jaroel reviewed Sep 3, 2023

View reviewed changes

Products/CMFPlone/Portal.py Outdated Show resolved Hide resolved

removed the class

23b2022

Akshat2Jain mentioned this pull request Sep 20, 2023

added uidtool zopefoundation/Products.CMFUid#23

Merged

Merge branch 'master' into undo_icontentish

6613208

Akshat2Jain mentioned this pull request Oct 3, 2023

updated new version of the Products.CMFUid plone/buildout.coredev#888

Merged

Merge branch 'master' into undo_icontentish

bdc4fbd

Akshat2Jain mentioned this pull request Oct 13, 2023

undo icontentish interface in 6.0.x #3856

Closed

Merge remote-tracking branch 'remotes/origin/undo_icontentish' into u…

64ae4cb

…ndo_icontentish

jaroel added 2 commits October 20, 2023 14:33

Update 3833.bugfix

c91e44b

Merge remote-tracking branch 'remotes/origin/undo_icontentish' into u…

3cc2229

…ndo_icontentish

wesleybl requested changes Oct 20, 2023

View reviewed changes

Products/CMFPlone/Portal.py Show resolved Hide resolved

cleanup

8a08ea3

wesleybl previously requested changes Oct 20, 2023

View reviewed changes

news/3833.bugfix Show resolved Hide resolved

davisagli approved these changes Oct 24, 2023

View reviewed changes

mauritsvanrees merged commit 6a8eb55 into master Oct 24, 2023

mauritsvanrees deleted the undo_icontentish branch October 24, 2023 09:16

undo icontentish interface #3836

undo icontentish interface #3836

Conversation

Akshat2Jain commented Sep 3, 2023 • edited Loading

mister-roboto commented Sep 3, 2023

davisagli commented Sep 4, 2023

wesleybl commented Sep 5, 2023 • edited Loading

Akshat2Jain commented Sep 11, 2023

wesleybl commented Sep 11, 2023

Akshat2Jain commented Sep 16, 2023

polyester commented Sep 18, 2023

wesleybl commented Sep 18, 2023

Akshat2Jain commented Sep 18, 2023

Akshat2Jain commented Sep 19, 2023

wesleybl commented Sep 19, 2023

Akshat2Jain commented Sep 28, 2023

wesleybl commented Sep 28, 2023

wesleybl commented Oct 2, 2023

wesleybl commented Oct 9, 2023

wesleybl commented Oct 10, 2023

wesleybl commented Oct 11, 2023

Akshat2Jain commented Oct 11, 2023

Akshat2Jain commented Oct 11, 2023

wesleybl commented Oct 11, 2023

wesleybl commented Oct 11, 2023 • edited Loading

wesleybl commented Oct 19, 2023

jaroel commented Oct 19, 2023

jaroel commented Oct 19, 2023

wesleybl commented Oct 19, 2023

jaroel commented Oct 20, 2023

wesleybl commented Oct 20, 2023

jaroel commented Oct 20, 2023 • edited Loading

wesleybl commented Oct 20, 2023

jaroel commented Oct 20, 2023

wesleybl commented Oct 20, 2023

jaroel commented Oct 20, 2023 • edited Loading

jaroel commented Oct 20, 2023

wesleybl commented Oct 20, 2023

jaroel commented Oct 20, 2023

wesleybl commented Oct 20, 2023

jaroel commented Oct 20, 2023

wesleybl commented Oct 20, 2023

jaroel commented Oct 23, 2023

davisagli left a comment

Choose a reason for hiding this comment

wesleybl commented Oct 24, 2023

wesleybl commented Oct 24, 2023

Akshat2Jain commented Sep 3, 2023 •

edited

Loading

wesleybl commented Sep 5, 2023 •

edited

Loading

wesleybl commented Oct 11, 2023 •

edited

Loading

jaroel commented Oct 20, 2023 •

edited

Loading

jaroel commented Oct 20, 2023 •

edited

Loading