Rietveld Search box

[jiayao, 2008-08-14 07:59:37]

Hi Guido and Andi,
This is my first attempt on the search box for rietveld. Could you have a look
and let me know what you think? 
Thanks,
Jiayao

[GvR, 2008-08-14 14:26:58]

Ooooh, drool, drool.

The solution of having separate searchable issue entities is clever.

I wonder about the performance impact though -- writing a searchable entity can
be very expensive (have you read the source for Searchable?)

Another worry is result ranking.  The problem is that we've found out that you
can't really combine a search query with any other search or order criterion,
since this causes a problem with exploding indexes (the size of the required
indexes increases exponentially with the number of words per issue).

So we're stuck with no ranking at all, which is not very satisfying.

There's also the issue of conversion of the existing db, but I suppose we can
deal with this once we've decided whether to go for it or not.

How about implementing a version that only shows the search box when you're an
admin, so we can experiment a bit without committing?

http://codereview.appspot.com/2918/diff/1/2
File codereview/views.py (right):

http://codereview.appspot.com/2918/diff/1/2#newcode721
Line 721: descript=issue.description)
typo, descript -> description

http://codereview.appspot.com/2918/diff/1/2#newcode1827
Line 1827: return HttpResponse('Method not allowed.', status=405)
Why? It's a query, so it *should* really use GET...

[Andi Albrecht, 2008-08-14 17:59:06]

http://codereview.appspot.com/2918/diff/1/2
File codereview/views.py (right):

http://codereview.appspot.com/2918/diff/1/2#newcode1827
Line 1827: return HttpResponse('Method not allowed.', status=405)
On 2008/08/14 14:26:58, GvR wrote:
> Why? It's a query, so it *should* really use GET...

Using GET would even allow us to provide a very simple 'save query' feature by
just storing the URL somewhere.

http://codereview.appspot.com/2918/diff/1/5
File templates/search_results.html (right):

http://codereview.appspot.com/2918/diff/1/5#newcode1
Line 1: {%extends "issue_base.html"%}
Extend "base.html" instead of "issue_base.html" which is only for pages that
display a single issue.

http://codereview.appspot.com/2918/diff/1/5#newcode2
Line 2: {%block title1%}SerachResults -{%endblock%}
Typo: Search Results

http://codereview.appspot.com/2918/diff/1/5#newcode6
Line 6: 
IMO a query form, maybe with some extended features like additional filter
(created by me, reviewable by me, etc.), should appear here. At least the search
term should be repeated somewhere on this page.

http://codereview.appspot.com/2918/diff/1/5#newcode10
Line 10: <tr><td colspan="7"><span class="disabled">(None)</span></td></tr>
Maybe "No results" instead of "None"?

[Andi Albrecht, 2008-08-14 18:19:16]

http://codereview.appspot.com/2918/diff/1/5
File templates/search_results.html (right):

http://codereview.appspot.com/2918/diff/1/5#newcode1
Line 1: {%extends "issue_base.html"%}
On 2008/08/14 17:59:07, andi.albrecht wrote:
> Extend "base.html" instead of "issue_base.html" which is only for pages that
> display a single issue.

Forget this comment, I'm wrong! Sorry...

[jiayao, 2008-08-16 23:51:31]

http://codereview.appspot.com/2918/diff/1/2
File codereview/views.py (right):

http://codereview.appspot.com/2918/diff/1/2#newcode721
Line 721: descript=issue.description)
On 2008/08/14 14:26:58, GvR wrote:
> typo, descript -> description

Done.

http://codereview.appspot.com/2918/diff/1/2#newcode1827
Line 1827: return HttpResponse('Method not allowed.', status=405)
On 2008/08/14 14:26:58, GvR wrote:
> Why? It's a query, so it *should* really use GET...

Done.

http://codereview.appspot.com/2918/diff/1/2#newcode1827
Line 1827: return HttpResponse('Method not allowed.', status=405)
On 2008/08/14 17:59:07, andi.albrecht wrote:
> On 2008/08/14 14:26:58, GvR wrote:
> > Why? It's a query, so it *should* really use GET...
> 
> Using GET would even allow us to provide a very simple 'save query' feature by
> just storing the URL somewhere.

Good point!

http://codereview.appspot.com/2918/diff/1/5
File templates/search_results.html (right):

http://codereview.appspot.com/2918/diff/1/5#newcode2
Line 2: {%block title1%}SerachResults -{%endblock%}
On 2008/08/14 17:59:07, andi.albrecht wrote:
> Typo: Search Results

Done.

http://codereview.appspot.com/2918/diff/1/5#newcode6
Line 6: 
On 2008/08/14 17:59:07, andi.albrecht wrote:
> IMO a query form, maybe with some extended features like additional filter
> (created by me, reviewable by me, etc.), should appear here. At least the
search
> term should be repeated somewhere on this page.

I've added a line "showing x o of y results for {{key_words}}"
I agree the format of the results should be nicer, I will think about it for a
bit.

http://codereview.appspot.com/2918/diff/1/5#newcode10
Line 10: <tr><td colspan="7"><span class="disabled">(None)</span></td></tr>
On 2008/08/14 17:59:07, andi.albrecht wrote:
> Maybe "No results" instead of "None"?

Done.

[Andi Albrecht, 2008-08-17 06:31:02]

http://codereview.appspot.com/2918/diff/1/5
File templates/search_results.html (right):

http://codereview.appspot.com/2918/diff/1/5#newcode6
Line 6: 
On 2008/08/16 23:51:31, jiayao wrote:
> On 2008/08/14 17:59:07, andi.albrecht wrote:
> > IMO a query form, maybe with some extended features like additional filter
> > (created by me, reviewable by me, etc.), should appear here. At least the
> search
> > term should be repeated somewhere on this page.
> 
> I've added a line "showing x o of y results for {{key_words}}"
> I agree the format of the results should be nicer, I will think about it for a
> bit. 

Of course, that'll be a future enhancement... ;-)