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

Hierarchical search #731

Merged
merged 5 commits into from Dec 14, 2017
Merged

Conversation

duvholt
Copy link
Member

@duvholt duvholt commented Nov 20, 2017

Fixes #580

Allows searching below a certain article path. Uses urls like /test/_search?q=test.
Otherwise it works just like the normal search.

I'll take a look at adding some tests for this tomorrow.

@codecov
Copy link

codecov bot commented Nov 20, 2017

Codecov Report

Merging #731 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #731      +/-   ##
==========================================
+ Coverage   69.98%   70.06%   +0.07%     
==========================================
  Files          89       89              
  Lines        4418     4429      +11     
==========================================
+ Hits         3092     3103      +11     
  Misses       1326     1326
Impacted Files Coverage Δ
src/wiki/urls.py 97.4% <ø> (ø) ⬆️
src/wiki/views/article.py 66.2% <100%> (+0.65%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ce67cc...55753e8. Read the comment docs.

Copy link
Member

@benjaoming benjaoming left a comment

Choose a reason for hiding this comment

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

Really great idea, but it's also a change that removes doing "full search" and adds some implicit functionality, so would be nice with something that guides the user!

{% url 'wiki:search' %}
{% endif %}
{% endspaceless %}"
>
Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting! From a user perspective though, this might get confusing because you will be searching in the current hierarchy but without an option to search the full wiki.

This will also be a behavioral change for anyone used to using this for searching the full wiki.

Could we fix this by editing the placeholder text for the input field? For instance Search in '{{ article.title }}' vs Search full wiki ?

Also, all of this behavior should maybe be conditioned by whether or not there actually exist any nested articles? Because if not, we might as well default to searching globally?

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 agree. I think we should clearly label both the search input and the search view.

Also what about adding a checkbox to the search view to force full search? Reddit does it like this:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, all of this behavior should maybe be conditioned by whether or not there actually exist any nested articles? Because if not, we might as well default to searching globally?

Right now the search also includes the article you searched from, so I think this would seem odd to the user. It think it makes sense to allow searching for text in just one article, even though it's probably easier to just do a CTRL+F search.

Copy link
Member

Choose a reason for hiding this comment

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

Right now the search also includes the article you searched from

That's cool, I mean at least not catastrophic in any way..

Also what about adding a checkbox to the search view to force full search? Reddit does it like this:

To me, it's not mandatory to "expand" the task, I think as long as we make sure the user can easily see where she is searching, it's all fine. But it would be nice with some additional features for the search area, just keep in mind that there's limited space in the topbar, and it has to be responsive.. so I don't know? Perhaps a separate PR?

@benjaoming
Copy link
Member

Seems like a great idea, and I agree.. if you have already clicked some title, it's typical that you want to search only in the current hierarchy.

article_ids = self.urlpath.get_descendants(
include_self=True).values_list('article_id')
articles = articles.filter(id__in=article_ids)
except (NoRootURL, models.URLPath.DoesNotExist):
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 raise Http404, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it probably should. I'm not sure how to trigger that from a get_queryset method though.

Copy link
Member

Choose a reason for hiding this comment

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

Oh that's fine, if you raise Http404 anywhere in a view class, it'll get caught in the middleware. So to raise it here is okay.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, neat!

@duvholt duvholt force-pushed the hierarchical-search branch 5 times, most recently from 15e8bef to 77b6349 Compare November 23, 2017 00:05
@benjaoming
Copy link
Member

@duvholt have you had a chance to look at the test failure in this one? :)

@duvholt
Copy link
Member Author

duvholt commented Dec 14, 2017

@benjaoming should be good now!

Not really sure what I did wrong last time as I tried several things, but now it works at least.

@benjaoming benjaoming merged commit 81a9073 into django-wiki:master Dec 14, 2017
@benjaoming
Copy link
Member

Awesome work, thanks!!! :)

Will report back when a release is up!

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