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

Search: allow to filter by project slugs #8149

Merged
merged 9 commits into from Jun 21, 2021
Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Apr 30, 2021

This is so we can override this from our views
instead of inside search and bring more closer .com.

We could also support searching only on projects the user owns in .org if we want.

@stsewd stsewd force-pushed the refactor-search-permissions branch 2 times, most recently from 63a75f8 to 1a9d07a Compare May 4, 2021 00:01
stsewd added 2 commits May 4, 2021 14:41
This is so we can override this from our views
instead of inside search and bring more closer .com.
@stsewd stsewd force-pushed the refactor-search-permissions branch from 1a9d07a to 3d4c306 Compare May 4, 2021 19:43
stsewd added a commit that referenced this pull request May 4, 2021
This was extracted from #8149
to make it easy to review.

- Separate this "shared" view into two classes, too many conditionals
  otherwise.
- Remove unused `index` query param.
@stsewd stsewd mentioned this pull request May 4, 2021
stsewd added a commit that referenced this pull request May 25, 2021
This was extracted from #8149
to make it easy to review.

- Separate this "shared" view into two classes, too many conditionals
  otherwise.
- Remove unused `index` query param.
fields = ('name^10', 'slug^5', 'description')
operators = ['and', 'or']
excludes = ['users', 'language']

def query(self, search, query):
Copy link
Member Author

Choose a reason for hiding this comment

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

This query method was moved from the parent to this class, as it has specifics about the contents of this class, and it's overridden by the other class that inherits it.


if isinstance(self.projects, dict):
versions_query = [
Bool(filter=[Term(project=project), Term(version=version)])
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed from must to filter since we don't want this to add to the score https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-bool-query.html

@stsewd stsewd marked this pull request as ready for review June 16, 2021 15:48
@stsewd stsewd requested a review from a team June 16, 2021 15:49
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks like a good change, and I like that it removes 2 overrides. I'm 👍 on making the default search default to only searching your projects, as long as we provide a UI element to expand that search to global.

readthedocs/search/faceted_search.py Outdated Show resolved Hide resolved
)

# Make sure we always have projects to filter by if we allow private projects.
if settings.ALLOW_PRIVATE_REPOS and not projects:
Copy link
Member

Choose a reason for hiding this comment

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

💯

Copy link
Member

Choose a reason for hiding this comment

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

This should log an error, as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about the error, basically this case will be hit by people that don't have projects in .com. Should I log that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added a test case for empty projects

Copy link
Member

Choose a reason for hiding this comment

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

Hrm, true. I guess it's fine, but I feel like we could hit this logic and not know why in some other code. I guess it's fine.

Maybe just a log.warning, so people can see it in dev, for example?

@stsewd stsewd force-pushed the refactor-search-permissions branch from 443afd6 to a4d1a29 Compare June 16, 2021 23:55
projects_query = Bool(filter=Terms(slug=self.projects))
bool_query = Bool(must=[bool_query, projects_query])
else:
raise ValueError('projects must be a list!')
Copy link
Member

Choose a reason for hiding this comment

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

Yea, this seems like the right approach. 👍

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Having tests makes this feel safer, and a bit more logic in the code. 👍

@stsewd stsewd merged commit d7382d7 into master Jun 21, 2021
@stsewd stsewd deleted the refactor-search-permissions branch June 21, 2021 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants