Make /all?closed=1 param in URL show only closed issues

Demo: http://69.retreview.appspot.com/

See also issue 298 for further enhancements.

[imirkin_alum.mit.edu, 2011-11-12 18:00:58]

What's the motivation behind removing the ability to view all issues?

On Sat, Nov 12, 2011 at 8:02 AM,  <techtonik@gmail.com> wrote:
> Reviewers: Andi Albrecht, M-A,
>
>
>
> Please review this at http://codereview.appspot.com/5373080/
>
> Affected files:
>  M codereview/views.py
>
>
> Index: codereview/views.py
> ===================================================================
> --- codereview/views.py
> +++ codereview/views.py
> @@ -1063,19 +1063,18 @@
>
>  def all(request):
>   """/all - Show a list of up to DEFAULT_LIMIT recent issues."""
> -  closed = request.GET.get('closed') or ''
> +  closed = request.GET.get('closed', '')
>   nav_parameters = {}
> -  if closed:
> -    nav_parameters['closed'] = '1'
> -
> -  if closed:
> -    query = db.GqlQuery('SELECT * FROM Issue '
> -                        'WHERE private = FALSE '
> -                        'ORDER BY modified DESC')
> -  else:
> -    query = db.GqlQuery('SELECT * FROM Issue '
> -                        'WHERE closed = FALSE AND private = FALSE '
> -                        'ORDER BY modified DESC')
> +  if closed != '':
> +    nav_parameters['closed'] = closed  # [ ] is escaping necessary?
> +
> +  query = models.Issue.all().filter('private =', False)
> +  if closed != '':
> +    if closed in ('0', 'false'):  # return only opened issue
> +      query.filter('closed =', False)
> +    else:
> +      query.filter('closed =', True)
> +  query.order('-modified')
>
>   return _paginate_issues(reverse(all),
>                           request,
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "codereview-discuss" group.
> To post to this group, send email to codereview-discuss@googlegroups.com.
> To unsubscribe from this group, send email to
> codereview-discuss+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/codereview-discuss?hl=en.
>
>

[techtonik, 2011-11-12 22:14:04]

On 2011/11/12 18:00:58, imirkin_alum.mit.edu wrote:
> What's the motivation behind removing the ability to view all issues?

Nothing is removed. You still can list all issues and only opened issues. But
now you can also page through closed issues only - see
http://69.retreview.appspot.com/all - as a side effect the breadcrumb is now
slightly cleaner IMO.

[imirkin_alum.mit.edu, 2011-11-12 22:26:03]

On Sat, Nov 12, 2011 at 5:14 PM,  <techtonik@gmail.com> wrote:
> On 2011/11/12 18:00:58, imirkin_alum.mit.edu wrote:
>>
>> What's the motivation behind removing the ability to view all issues?
>
> Nothing is removed. You still can list all issues and only opened
> issues. But now you can also page through closed issues only - see
> http://69.retreview.appspot.com/all - as a side effect the breadcrumb is
> now slightly cleaner IMO.

Oh, right. I misread the patch. Nevermind.

[Andi Albrecht, 2011-11-14 13:37:27]

http://codereview.appspot.com/5373080/diff/3/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5373080/diff/3/codereview/views.py#newcode1069
codereview/views.py:1069: nav_parameters['closed'] = closed  # [ ] is escaping
necessary?
I'm not sure about the side-effects of not validating closed as user input. To
me it looks safer if we validate the input and only allow known parameters here.

http://codereview.appspot.com/5373080/diff/3/codereview/views.py#newcode1072
codereview/views.py:1072: if closed != '':
The code would be cleaner if we remove one level of nesting: if closed in ('0',
'false'): ...; elif closed in ('1', 'true');

http://codereview.appspot.com/5373080/diff/3/codereview/views.py#newcode1075
codereview/views.py:1075: else:
Why do you change the default behavior? ATM we're showing only open issues by
default which sounds reasonable to me. Or is the indentation wrong for the
"else"?

http://codereview.appspot.com/5373080/diff/3/templates/all.html
File templates/all.html (right):

http://codereview.appspot.com/5373080/diff/3/templates/all.html#newcode4
templates/all.html:4: {%if closed == '0' or closed == 'false' %}
IMO we should unify this in the views.py when validating user input.

[techtonik, 2011-11-14 17:13:21]

Uploaded new patchset. Comments are below.

http://codereview.appspot.com/5373080/diff/3/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5373080/diff/3/codereview/views.py#newcode1069
codereview/views.py:1069: nav_parameters['closed'] = closed  # [ ] is escaping
necessary?
On 2011/11/14 13:37:27, Andi Albrecht wrote:
> I'm not sure about the side-effects of not validating closed as user input. To
> me it looks safer if we validate the input and only allow known parameters
here.

Done.

http://codereview.appspot.com/5373080/diff/3/codereview/views.py#newcode1072
codereview/views.py:1072: if closed != '':
On 2011/11/14 13:37:27, Andi Albrecht wrote:
> The code would be cleaner if we remove one level of nesting: if closed in
('0',
> 'false'): ...; elif closed in ('1', 'true');

Done.

http://codereview.appspot.com/5373080/diff/3/codereview/views.py#newcode1075
codereview/views.py:1075: else:
On 2011/11/14 13:37:27, Andi Albrecht wrote:
> Why do you change the default behavior? ATM we're showing only open issues by
> default which sounds reasonable to me. Or is the indentation wrong for the
> "else"?

I didn't notice that index() also uses all() handler to show default page with
open issue. I thought only that URL http://codereview.appspot.com/all should
return all issues by default.

Do we have any best practice way on how to pass 'closed' param from index()
handler to all() method properly? AppEngine `request` object doesn't have .set()
method.

http://codereview.appspot.com/5373080/diff/3/templates/all.html
File templates/all.html (right):

http://codereview.appspot.com/5373080/diff/3/templates/all.html#newcode4
templates/all.html:4: {%if closed == '0' or closed == 'false' %}
On 2011/11/14 13:37:27, Andi Albrecht wrote:
> IMO we should unify this in the views.py when validating user input.

Done.

I've run into two 'interesting' bugs while fixing this. that's why there is
'lower' modifier on the left side.

Django templating is weird.

https://code.djangoproject.com/ticket/17229
https://code.djangoproject.com/ticket/17230

http://codereview.appspot.com/5373080/diff/7002/templates/issue_base.html
File templates/issue_base.html (right):

http://codereview.appspot.com/5373080/diff/7002/templates/issue_base.html#new...
templates/issue_base.html:30: <a class="novisit" href="{%url
codereview.views.index%}?closed=0">Opened Issues</a>
Is it better to label link 'Open Issues' instead of 'Opened Issues'?

[Andi Albrecht, 2011-11-15 07:13:44]

LGTM

http://codereview.appspot.com/5373080/diff/3/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5373080/diff/3/codereview/views.py#newcode1075
codereview/views.py:1075: else:
On 2011/11/14 17:13:21, techtonik wrote:
> On 2011/11/14 13:37:27, Andi Albrecht wrote:
> > Why do you change the default behavior? ATM we're showing only open issues
by
> > default which sounds reasonable to me. Or is the indentation wrong for the
> > "else"?
> 
> I didn't notice that index() also uses all() handler to show default page with
> open issue. I thought only that URL http://codereview.appspot.com/all should
> return all issues by default.
> 
> Do we have any best practice way on how to pass 'closed' param from index()
> handler to all() method properly? AppEngine `request` object doesn't have
.set()
> method.

Why do you need to modify the request (which isn't possible IIRC)? index()
should allow the same URL parameters as all() in that case and since the request
object is passed to all() in the index() function everything should be fine.

http://codereview.appspot.com/5373080/diff/7002/templates/issue_base.html
File templates/issue_base.html (right):

http://codereview.appspot.com/5373080/diff/7002/templates/issue_base.html#new...
templates/issue_base.html:30: <a class="novisit" href="{%url
codereview.views.index%}?closed=0">Opened Issues</a>
On 2011/11/14 17:13:21, techtonik wrote:
> Is it better to label link 'Open Issues' instead of 'Opened Issues'?
IMO "Open Issues" sounds better.

[techtonik, 2011-11-15 08:40:34]

On 2011/11/15 07:13:44, Andi Albrecht wrote:
> > > Why do you change the default behavior? ATM we're showing only open issues
> by
> > > default which sounds reasonable to me. Or is the indentation wrong for the
> > > "else"?
> > 
> > I didn't notice that index() also uses all() handler to show default page
with
> > open issue. I thought only that URL http://codereview.appspot.com/all should
> > return all issues by default.
> > 
> > Do we have any best practice way on how to pass 'closed' param from index()
> > handler to all() method properly? AppEngine `request` object doesn't have
> .set()
> > method.
> 
> Why do you need to modify the request (which isn't possible IIRC)? index()
> should allow the same URL parameters as all() in that case and since the
request
> object is passed to all() in the index() function everything should be fine.

The trick is to let /all URL show all issues by default and /index URL only open
issues. http://code.google.com/p/rietveld/source/browse/codereview/urls.py
doesn't allow to pass any additional parameters in request (or override existing
ones). So the only way I see is to add a keyword param to all() handler which
will only be set from index() function.

[techtonik, 2011-11-15 09:18:25]

On the second thought the logic to show all issues by default (including closed)
for non-logged users is not that bad. We show title "Recent Issues" and people
might be confused that that recent issue that is closed is now shown.

[techtonik, 2011-11-15 09:19:07]

s/now shown/not shown/

[techtonik, 2011-11-15 15:02:12]

Demo updated: http://69.retreview.appspot.com/

So if it is ok that we now display all Recent issues by default including
closed, I commit this.

[Andi Albrecht, 2011-11-15 20:20:01]

On Tue, Nov 15, 2011 at 4:02 PM,  <techtonik@gmail.com> wrote:
> Demo updated: http://69.retreview.appspot.com/

I'm not really happy with closed issues showing up as default. When
toggling between "Open" and "All" on codereview.appspot.com I got the
impression that "All" (including closed) is slightly slower than
"Open". But I could be wrong. Appstats reports slightly better
performance without closed issues, but we need to collect some more
numbers.

Are there any other opinions on showing closed issues on the entry page?

--Ando

>
> So if it is ok that we now display all Recent issues by default
> including closed, I commit this.
>
> http://codereview.appspot.com/5373080/
>

[techtonik, 2011-11-23 16:42:08]

New patchset and demo at http://71.retreview.appspot.com/ makes open issues
appear by default.

[Andi Albrecht, 2011-11-23 19:05:08]

http://codereview.appspot.com/5373080/diff/8002/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5373080/diff/8002/codereview/views.py#newcode890
codereview/views.py:890: return HttpResponseRedirect(reverse(all)+'?closed=0')
hm, let's not start with a redirect. Isn't it possible to call all() with an
keyword argument?

[techtonik, 2011-11-23 23:06:49]

http://codereview.appspot.com/5373080/diff/8002/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5373080/diff/8002/codereview/views.py#newcode890
codereview/views.py:890: return HttpResponseRedirect(reverse(all)+'?closed=0')
On 2011/11/23 19:05:08, Andi Albrecht wrote:
> hm, let's not start with a redirect. Isn't it possible to call all() with an
> keyword argument?

It's possible, but complicates logic a bit. See the patchset no.8 Demo
http://72.retreview.appspot.com/

[Andi Albrecht, 2011-11-24 11:26:58]

LGTM

[techtonik, 2011-11-24 12:59:34]

Thanks! Committed as rd71c50dd25b6.

[Andi Albrecht, 2011-11-24 13:09:36]

On Thu, Nov 24, 2011 at 1:59 PM,  <techtonik@gmail.com> wrote:
> Thanks! Committed as rd71c50dd25b6.

...and live!

Thanks, Anatoly!

--Andi

>
> http://codereview.appspot.com/5373080/
>