Issue 312: unrestricted find #522
Conversation
|
@gogobd thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: 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! |
src/plone/api/tests/test_content.py
Outdated
| @@ -912,6 +912,13 @@ def test_find(self): | |||
| documents = api.content.find(portal_type="Document") | |||
| self.assertEqual(len(documents), 2) | |||
|
|
|||
| def test_find(self): | |||
There was a problem hiding this comment.
Hi @gogobd, good idea :)
This test masks the one defined in line 908. It should have another name. In addition, it would be good to verify we find a different number of results in case we ask for an unrestricted search :)
There was a problem hiding this comment.
The function was renamed, which is good. Well, I see a typo in the name: test_unTrestricted_find.
But it still finds the same number of results with or without unrestricted=True, so it does not tell us much. Can you add a document that is not found in test_find, but is found in test_unrestricted_find? I guess set an expiry date.
There was a problem hiding this comment.
@mauritsvanrees An expiry date in the past did't do it; I don't know how to create an object that is only found with "unrestricted=True"...
There was a problem hiding this comment.
I tried .start and .end and .effective and .expires; none of those made a difference...
There was a problem hiding this comment.
@gogobd The catalog's searchResults method does not filter by effective/expires if the current user has the "Access inactive portal content" permission (see https://github.com/plone/Products.CMFPlone/blob/master/Products/CMFPlone/CatalogTool.py#L405), so it won't work if your test is running with the Manager role.
I'd suggest:
- create a document that you do not publish
- use setRoles to switch current roles to Anonymous
- now the document should be missing with unrestricted=False, because it's filtered out based on the allowedRolesAndUsers index (https://github.com/plone/Products.CMFPlone/blob/master/Products/CMFPlone/CatalogTool.py#L403)
There was a problem hiding this comment.
I just logged out and that did the trick!
|
@gogobd thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: 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! |
|
@jenkins-plone-org please run jobs |
news/312.implementation
Outdated
There was a problem hiding this comment.
@gogobd This file's extension needs to match one of the predefined categories from https://github.com/plone/plone.api/blob/master/pyproject.toml
Use .feature for this one.
|
@jenkins-plone-org please run jobs |
|
I have no idea what this "tox linkcheck" wants to do, please help. |
|
Argh, it's that thing again. I'll push a commit to this PR that will fix it. |
|
Actually, I'll push a separate PR so it has its own changelog, and you can cherry pick it. |
|
Forget that, it already was fixed two weeks ago in 4b91ad8, which was released in plone.api Hmm, for some reason https://github.com/plone/plone.api/actions/runs/7981002081/job/21791817987?pr=522#step:6:14 says Later that gets clobbered in https://github.com/plone/plone.api/actions/runs/7981002081/job/21791817987?pr=522#step:6:23 with This is above my pay grade for how to resolve. |
|
The branch needs to be updated. |
mauritsvanrees
dismissed
davisagli’s stale review
Requested change (rename news snippet) has been done.
|
YAY! Thanks, every 01! |
Maybe we want this; unrestricted find is necessary because proxy roles are not being handled correctly atm.