[Fix #2328 #2013] Refresh search index and test for case insensitive search #4277
Conversation
|
|
||
| It tests with uppercase, lowercase and camelcase | ||
| """ | ||
| query = get_search_query_from_project_file(project_slug=project.slug)\ |
There was a problem hiding this comment.
Stray slash at the end of this line.
| """ | ||
| query = get_search_query_from_project_file(project_slug=project.slug)\ | ||
|
|
||
| cased_query = getattr(query, case) |
There was a problem hiding this comment.
This feels really magical. I think there is a better way to write this code so that it's more obvious what it is doing. Perhaps by calling the function reference that is returned here, instead of just passing it in below.
There was a problem hiding this comment.
Perhaps by calling the function reference that is returned here, instead of just passing it in below.
Do you mean something like
if case == 'lower':
query = query.lower()There was a problem hiding this comment.
Or even just:
case_function = getattr(query, case)
query = case_function()
Something that shows that the return is a function & we're calling it.
safwanrahman
changed the title
safwanrahman
force-pushed
the
insensitive
branch
from
cc4da57 to
49f4287
Compare
readthedocs/search/documents.py
Outdated
| # Its a model instance. | ||
| queryset = self.get_queryset() | ||
| obj = queryset.filter(pk=thing.pk) | ||
| if not obj.exists(): | ||
| return None | ||
|
|
||
| return super(PageDocument, self).update(thing=thing, refresh=None, action='index', **kwargs) | ||
| return super(PageDocument, self).update(thing=thing, refresh=refresh, action=action, **kwargs) |
There was a problem hiding this comment.
My fault while fixing lint! 😢
| if isinstance(thing, HTMLFile): | ||
| # Moreover, do not need to check if its a delete action | ||
| # Because while delete action, the object is already remove from database | ||
| if isinstance(thing, HTMLFile) and action != 'delete': |
There was a problem hiding this comment.
I did not consider delete case. While a object gets deleted, it calls update with delete action.
|
@ericholscher I have fixed the issue #2013 . My assumption was wrong as there was a edge case which I did not handle in the |
safwanrahman
force-pushed
the
insensitive
branch
from
49f4287 to
c1b2770
Compare
| # Its a model instance. | ||
| queryset = self.get_queryset() | ||
| obj = queryset.filter(pk=thing.pk) | ||
| if not obj.exists(): | ||
| return None | ||
|
|
||
| return super(PageDocument, self).update(thing=thing, refresh=None, action='index', **kwargs) | ||
| return super(PageDocument, self).update(thing=thing, refresh=refresh, |
There was a problem hiding this comment.
My fault while fixing lint! 😢
| def test_page_search_not_return_removed_page(self, client, project): | ||
| """Check removed page are not in the search index""" | ||
| query = get_search_query_from_project_file(project_slug=project.slug) | ||
| # Delete all the HTML files of the project |
There was a problem hiding this comment.
We should confirm that the query returns a result here, so that we know that then it doesn't return later it's a direct result of the deletion of the file.
There was a problem hiding this comment.
@ericholscher Good point. Fixed it in latest commit
|
@ericholscher Fixed! |
[Fix readthedocs#2328 readthedocs#2013] Refresh search index and test for case insensitive search
[Fix readthedocs#2328 readthedocs#2013] Refresh search index and test for case insensitive search
#4211 made the search case insensitive. This PR adds tests for it so it does not get broken while implementing #4266
This fix #2328 #2013
@ericholscher r?