search

plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
243 stars 186 forks source link

undo icontentish interface #3836

Closed Akshat2Jain closed 9 months ago

Akshat2Jain commented 11 months ago

fixes #3833

also related to https://github.com/plone/plone.restapi/pull/1674

mister-roboto commented 11 months ago

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

davisagli commented 11 months ago

@jenkins-plone-org please run jobs

wesleybl commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

@polyester @icemac

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

Akshat2Jain commented 11 months ago

any progress?

polyester commented 11 months ago

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 commented 11 months ago

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

Akshat2Jain commented 11 months ago

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

Done

Akshat2Jain commented 11 months ago

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

wesleybl commented 11 months ago

@Akshat2Jain you need to make a PR on Products.CMFUid, with what was explained in: https://github.com/plone/Products.CMFPlone/pull/3836#issuecomment-1705806117

Akshat2Jain commented 10 months ago

@wesleybl can you run jobs again?

wesleybl commented 10 months ago

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

wesleybl commented 10 months ago

@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 commented 10 months ago

@jenkins-plone-org please run jobs

wesleybl commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

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

this pr would be on the master branch? @wesleybl

Akshat2Jain commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

@jenkins-plone-org please run jobs

wesleybl commented 10 months ago

@jaroel @Akshat2Jain we still have a test failing in plone.restapi:

'@id'

  File "/srv/python3.12/lib/python3.12/unittest/case.py", line 58, in testPartExecutor
    yield
  File "/srv/python3.12/lib/python3.12/unittest/case.py", line 634, in run
    self._callTestMethod(testMethod)
  File "/srv/python3.12/lib/python3.12/unittest/case.py", line 589, in _callTestMethod
    if method() is not None:
       ^^^^^^^^
  File "/home/jenkins/.buildout/eggs/cp312/plone.restapi-9.1.0-py3.12.egg/plone/restapi/tests/test_site_serializer.py", line 122, in test_site_root_get_request
    self.assertEqual(response.json()["@id"], self.portal.absolute_url())
                     ~~~~~~~~~~~~~~~^^^^^^^

See: https://jenkins.plone.org/job/pull-request-6.1-3.12/26/testReport/junit/plone.restapi.tests.test_site_serializer/TestSiteSerializationFunctional/test_site_root_get_request/

The strange thing is that I ran this test locally on Plone 6.1 coredev and it worked. So it's difficult to know what it is.

jaroel commented 10 months ago

@jaroel @Akshat2Jain we still have a test failing in plone.restapi: [...] The strange thing is that I ran this test locally on Plone 6.1 coredev and it worked. So it's difficult to know what it is.

Yep. https://github.com/zopefoundation/Products.CMFCore/blob/be1c7590677b978bb9ccf6050b7de84b0d86235d/src/Products/CMFCore/explicitacquisition.py#L29 is triggering the NotFound.

Turns out that the aq_chain contains the Plone site twice in plone.rest. If you know what causes that, I'd be really happy.

wesleybl commented 10 months ago

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

jaroel commented 10 months ago

@jenkins-plone-org please run jobs

jaroel commented 10 months ago

@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 https://github.com/plone/Products.CMFPlone/blob/621edb0851967f033c40dae89ead14c8f4d4250d/Products/CMFPlone/__init__.py#L12-L21

wesleybl commented 10 months ago

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

jaroel commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

@wesleybl please look here: https://github.com/plone/plone.rest/pull/168

wesleybl commented 10 months ago

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

jaroel commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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?

https://github.com/plone/plone.rest/issues/169

wesleybl commented 10 months ago

@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 https://github.com/plone/plone.rest/issues/169. If someone uses SKIP_PTA = false, restapi will not work.

jaroel commented 10 months ago

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

wesleybl commented 10 months ago

@jaroel can you please take care of https://github.com/plone/Products.CMFPlone/pull/3856?

jaroel commented 10 months ago

@jaroel can you please take care of #3856?

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

wesleybl commented 10 months ago

@jenkins-plone-org please run jobs

jaroel commented 10 months ago

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

wesleybl commented 9 months ago

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 https://github.com/plone/plone.restapi/pull/1674 in an easy way, since we don't have a version of restapi just for Plone 6.1

wesleybl commented 9 months ago

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