Code review - Issue 5373080: Make /all?closed=1 param in URL show only closed issueshttps://codereview.appspot.com/2011-11-24T13:09:36+00:00rietveld
Message from unknown
2011-11-12T13:02:32+00:00techtonikurn:md5:f33b8f4c5f68d3d25127a0334dbdebb5
Message from techtonik@gmail.com
2011-11-12T13:02:36+00:00techtonikurn:md5:8bf0fd3a94e6a2a1bcbd0e62d628499e
Message from unknown
2011-11-12T13:17:30+00:00techtonikurn:md5:aee78df2a5c5cdfeed53bff077f4d4af
Message from techtonik@gmail.com
2011-11-12T13:17:32+00:00techtonikurn:md5:f15f0202406b2d39aef63ebd3675f1cd
Message from unknown
2011-11-12T13:20:15+00:00techtonikurn:md5:2a59eff2569df348b059e29e7bc4e507
Message from techtonik@gmail.com
2011-11-12T13:20:17+00:00techtonikurn:md5:6f17acdc929881d431d309a8e88f7d2b
Message from unknown
2011-11-12T13:27:16+00:00techtonikurn:md5:13bea2fd944945a1a63aa511f69c9ac0
Message from techtonik@gmail.com
2011-11-12T13:27:19+00:00techtonikurn:md5:d389c6198901772a1cd38ec661b0cc2d
Message from imirkin@alum.mit.edu
2011-11-12T18:00:58+00:00imirkin_alum.mit.eduurn:md5:e8a4e526f2761151628cf42040c3bd2e
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.
>
>
Message from techtonik@gmail.com
2011-11-12T22:14:04+00:00techtonikurn:md5:234df89072703be15597dcb12f2a7100
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.
Message from imirkin@alum.mit.edu
2011-11-12T22:26:03+00:00imirkin_alum.mit.eduurn:md5:73a880078e2c312753b3a512510a280d
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.
Message from albrecht.andi@googlemail.com
2011-11-14T13:37:27+00:00Andi Albrechturn:md5:a1d7ca57a6c040089cc8d31a5f616d72
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.
Message from unknown
2011-11-14T17:01:24+00:00techtonikurn:md5:f42ed0159b53be363f199d49fd3757e3
Message from techtonik@gmail.com
2011-11-14T17:01:25+00:00techtonikurn:md5:e1de2770ed93d5172b81e96e4fe173ac
Message from techtonik@gmail.com
2011-11-14T17:13:21+00:00techtonikurn:md5:e26b782f670e5c52b3411eaf4b3de4fc
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#newcode30
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'?
Message from albrecht.andi@googlemail.com
2011-11-15T07:13:44+00:00Andi Albrechturn:md5:0e6f8aeaf03e17186d7e998691cd2895
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#newcode30
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.
Message from techtonik@gmail.com
2011-11-15T08:40:34+00:00techtonikurn:md5:190a78cda2b71c53b151c744c8c076fd
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.
Message from techtonik@gmail.com
2011-11-15T09:18:25+00:00techtonikurn:md5:012455591cc8b312acfd47d4eb3e09c0
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.
Message from techtonik@gmail.com
2011-11-15T09:19:07+00:00techtonikurn:md5:b595c8c83d99cdd10929df75da7a571e
s/now shown/not shown/
Message from unknown
2011-11-15T14:56:22+00:00techtonikurn:md5:f8b06d4e9e10cf23a9a713a3721c4b67
Message from techtonik@gmail.com
2011-11-15T14:56:26+00:00techtonikurn:md5:2db761d17e77a26f6154e0277a02f368
Message from techtonik@gmail.com
2011-11-15T15:02:12+00:00techtonikurn:md5:2ab7a271fa72a3d8593b9aaa8049a194
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.
Message from albrecht.andi@googlemail.com
2011-11-15T20:20:01+00:00Andi Albrechturn:md5:536c7512f2a87d7c64d25a273d32bcd4
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/
>
Message from unknown
2011-11-23T16:37:45+00:00techtonikurn:md5:f4bca624364b4f45de8b3871d84a7719
Message from techtonik@gmail.com
2011-11-23T16:37:48+00:00techtonikurn:md5:73cb63d4041ffad7e343bd43e37fc9a6
Message from techtonik@gmail.com
2011-11-23T16:42:08+00:00techtonikurn:md5:a3f8b099abf0dec806265558b4ee1e0c
New patchset and demo at http://71.retreview.appspot.com/ makes open issues appear by default.
Message from albrecht.andi@googlemail.com
2011-11-23T19:05:08+00:00Andi Albrechturn:md5:a86210a3c1d433df3cd3c05f9246c635
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?
Message from unknown
2011-11-23T23:03:38+00:00techtonikurn:md5:94ff0f3870a209a875a5bb0608e65ee2
Message from techtonik@gmail.com
2011-11-23T23:03:39+00:00techtonikurn:md5:e90d4e014a2beafe5673de7d45630337
Message from techtonik@gmail.com
2011-11-23T23:06:49+00:00techtonikurn:md5:f6c6792a271a0ee6004adecab2072036
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/
Message from albrecht.andi@googlemail.com
2011-11-24T11:26:58+00:00Andi Albrechturn:md5:241c61a1cd714aed658d806b20ee893a
LGTM
Message from techtonik@gmail.com
2011-11-24T12:59:34+00:00techtonikurn:md5:49c31a7174d903f86dbe724572caa204
Thanks! Committed as rd71c50dd25b6.
Message from albrecht.andi@googlemail.com
2011-11-24T13:09:36+00:00Andi Albrechturn:md5:4e2607875af62d24b4cb30b78a6a6516
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/
>