Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect types_not_searched field for search #862

Closed
wants to merge 13 commits into from

Conversation

rodfersou
Copy link
Member

closes #861

@mister-roboto
Copy link

@rodfersou thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@rodfersou rodfersou changed the title Add a test explaining problem Respect types_not_searched field for search Jan 27, 2020
types = query.get('portal_type', [])
if 'query' in types:
types = types['query']
query['portal_type'] = self.filter_types(types)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plone_utils = getToolByName(self.context, 'plone_utils')
if not isinstance(types, list):
types = [types]
return plone_utils.getUserFriendlyTypes(types)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def test_types_not_searched(self):
registry = getUtility(IRegistry)
search_settings = registry.forInterface(ISearchSchema, prefix="plone")
search_settings.types_not_searched = ('Folder',)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, this configuration is changing when running the search.

Need help to figure out it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test is failing, and when debugging it, realized that the types_not_searched configuration is missing before add the filter in the search ( Folder type is being listed)

Copy link
Member

@sneridagh sneridagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far, however, I would include only the types filter if the user hasn't specified explicitly a query containing portal types.

def test_types_not_searched(self):
registry = getUtility(IRegistry)
search_settings = registry.forInterface(ISearchSchema, prefix="plone")
search_settings.types_not_searched = ('Folder',)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

@rodfersou
Copy link
Member Author

@sneridagh added your suggestion to filter just if portal_types is not set in the query.

@coveralls
Copy link

coveralls commented Mar 30, 2020

Coverage Status

Coverage decreased (-0.03%) to 95.543% when pulling 39fc48d on respect-types-not-searched into 864d566 on master.

@rodfersou
Copy link
Member Author

@jenkins-plone-org please run jobs

@rodfersou
Copy link
Member Author

@jenkins-plone-org please run jobs

@rodfersou
Copy link
Member Author

@jenkins-plone-org please run jobs

@rodfersou
Copy link
Member Author

@sneridagh I don't get why it is failing since the field is there...

https://github.com/plone/plone.app.controlpanel/blob/2.3.11/plone/app/controlpanel/search.py#L59-L65

Error in test test_types_not_searched (plone.restapi.tests.test_search.TestSearchFunctional)
Traceback (most recent call last):
  File "/Users/rodrigo/.asdf/installs/python/2.7.17/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/Users/rodrigo/.projects/plone/plone.restapi/src/plone/restapi/tests/test_search.py", line 139, in test_types_not_searched
    search_settings = registry.forInterface(ISearchSchema, prefix="plone")
  File "/Users/rodrigo/.projects/cache/eggs/plone.registry-1.0.5-py2.7.egg/plone/registry/registry.py", line 78, in forInterface
    name
KeyError: 'Interface `plone.app.controlpanel.search.ISearchSchema` defines a field `sort_on`, for which there is no record.'

@cekk
Copy link
Member

cekk commented Apr 8, 2020

See #904

cekk and others added 2 commits April 9, 2020 15:19
* add commit to make registry setting permanent

* add commit to make registry setting permanent

* enable plone.restapi in development

* handle plone <5 settings
@rodfersou
Copy link
Member Author

@jenkins-plone-org please run jobs

@sneridagh
Copy link
Member

Sorry, but disn’t we decided to not go for this one?

@tisto ?

TL;DR of this is: the @search endpoint is not the Plone search view...

@cekk
Copy link
Member

cekk commented Apr 9, 2020

uh..that's not nice ;)

@jensens
Copy link
Sponsor Member

jensens commented Apr 9, 2020

Sorry, but disn’t we decided to not go for this one?

I just looked at the technical POV.

@tisto
Copy link
Sponsor Member

tisto commented Apr 13, 2020

Sorry folks, this PR was supposed to be closed when we stopped working on it. This is a complex topic that touches REST API design decisions and requires careful consideration. Closing the PR now.

@tisto tisto closed this Apr 13, 2020
@tisto tisto deleted the respect-types-not-searched branch April 13, 2020 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respect types_not_searched field for search
7 participants