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

[Fix #2328 #2013] Refresh search index and test for case insensitive search #4277

Merged
merged 4 commits into from Jun 21, 2018

Conversation

2 participants
@safwanrahman
Member

safwanrahman commented Jun 20, 2018

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

@safwanrahman safwanrahman requested a review from ericholscher Jun 20, 2018

@safwanrahman safwanrahman self-assigned this Jun 20, 2018

@safwanrahman safwanrahman added this to Backlog in Search update via automation Jun 20, 2018

@safwanrahman safwanrahman moved this from Backlog to In progress in Search update Jun 20, 2018

@ericholscher

Looks good, but I'd like to make the test a bit easier to read. It took me a couple minutes to realize what it was doing!

It tests with uppercase, lowercase and camelcase
"""
query = get_search_query_from_project_file(project_slug=project.slug)\

This comment has been minimized.

@ericholscher

ericholscher Jun 20, 2018

Member

Stray slash at the end of this line.

"""
query = get_search_query_from_project_file(project_slug=project.slug)\
cased_query = getattr(query, case)

This comment has been minimized.

@ericholscher

ericholscher Jun 20, 2018

Member

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.

This comment has been minimized.

@safwanrahman

safwanrahman Jun 20, 2018

Member

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()

This comment has been minimized.

@ericholscher

ericholscher Jun 20, 2018

Member

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 safwanrahman changed the title from [Fix #2328] Adding Test for case insensitive search to [Fix #2328 #2013] Refresh search index and test for case insensitive search Jun 20, 2018

# 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)

This comment has been minimized.

@safwanrahman

safwanrahman Jun 20, 2018

Member

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':

This comment has been minimized.

@safwanrahman

safwanrahman Jun 20, 2018

Member

I did not consider delete case. While a object gets deleted, it calls update with delete action.

@safwanrahman

This comment has been minimized.

Member

safwanrahman commented Jun 20, 2018

@ericholscher I have fixed the issue #2013 . My assumption was wrong as there was a edge case which I did not handle in the update method overwrite.
r?

# 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,

This comment has been minimized.

@safwanrahman

safwanrahman Jun 20, 2018

Member

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

This comment has been minimized.

@ericholscher

ericholscher Jun 20, 2018

Member

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.

This comment has been minimized.

@safwanrahman

safwanrahman Jun 20, 2018

Member

@ericholscher Good point. Fixed it in latest commit

@safwanrahman

This comment has been minimized.

Member

safwanrahman commented Jun 20, 2018

@ericholscher Fixed!

@ericholscher

Looks good!

@ericholscher ericholscher merged commit d3c64c1 into rtfd:search_upgrade Jun 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Search update automation moved this from In progress to Done Jun 21, 2018

@safwanrahman safwanrahman deleted the safwanrahman:insensitive branch Jul 7, 2018

safwanrahman added a commit to safwanrahman/readthedocs.org that referenced this pull request Jul 16, 2018

Merge pull request rtfd#4277 from safwanrahman/insensitive
[Fix rtfd#2328 rtfd#2013] Refresh search index and test for case insensitive search

safwanrahman added a commit to safwanrahman/readthedocs.org that referenced this pull request Jul 16, 2018

Merge pull request rtfd#4277 from safwanrahman/insensitive
[Fix rtfd#2328 rtfd#2013] Refresh search index and test for case insensitive search
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment