Good start! What do you think about changing the UI for seeing closed issues? Most ...
15 years, 10 months ago
(2008-05-13 17:05:31 UTC)
#2
Good start! What do you think about changing the UI for seeing closed issues?
Most bug trackers don't show closed issues unless you specifically search for
them. Most access to a closed issue is probably based on a URL from a recent
email anyway.
We also need a one-time command that adds 'active=True' to all issues, since
queries like "WHERE active=TRUE" don't return records that don't have the active
property in the datastore, and updating the model doesn't automatically add it
to the datastore...
http://codereview.appspot.com/970/diff/21/41
File codereview/views.py (right):
http://codereview.appspot.com/970/diff/21/41#newcode344
Line 344: query = db.GqlQuery('SELECT * FROM Issue WHERE active=True ORDER BY
modified DESC')
Many lines containing queries are now > 80 chars. Please wrap these properly.
http://codereview.appspot.com/970/diff/21/41#newcode385
Line 385: logging.info("not found")
Either be more specific with the message or remove it. :)
http://codereview.appspot.com/970/diff/21/41#newcode393
Line 393: closed_issues = list(db.GqlQuery(
I don't think we should list the closed issues by default. Maybe we can add an
option to the URL to include (or only show) closed issues, both here and on the
"recent issues" page; this would add "?closed=true" or "?closed=both" (the
default being "?closed=false").
Or, perhaps, we can limit this to the 10 most recently closed issues owned by
this user? That might be helpful.
http://codereview.appspot.com/970/diff/21/45
File templates/issue.html (right):
http://codereview.appspot.com/970/diff/21/45#newcode26
Line 26: <a href="{%url codereview.views.close issue.key.id%}"><b>Close
Issue</b></a>
You're missing a '|' before this.
But IMO "close" needs to be a POST command like edit. Maybe we can just make it
a check box on the edit form?
http://codereview.appspot.com/970/diff/21/44
File templates/mine.html (right):
http://codereview.appspot.com/970/diff/21/44#newcode66
Line 66: <tr><td colspan="7"><h3>Issues Closed</h3></td></tr>
I don't think we need this -- over time the list would become intolerably long.
Unless you make it a "recently closed" issues which actually makes some sense.
Hi Guido, I've addressed your comments inline and with another patch. And I've added a ...
15 years, 10 months ago
(2008-05-13 23:00:48 UTC)
#3
Hi Guido,
I've addressed your comments inline and with another patch. And I've added a
simple admin page to bulk edit entities. I tried to make it generic so we can
use it for similar purposes in the future. But I haven't learned enough about
python introspection. Could you show me how you would approach this?
http://codereview.appspot.com/970/diff/21/41
File codereview/views.py (right):
http://codereview.appspot.com/970/diff/21/41#newcode344
Line 344: query = db.GqlQuery('SELECT * FROM Issue WHERE active=True ORDER BY
modified DESC')
On 2008/05/13 17:05:32, GvR wrote:
> Many lines containing queries are now > 80 chars. Please wrap these properly.
Done.
http://codereview.appspot.com/970/diff/21/41#newcode385
Line 385: logging.info("not found")
On 2008/05/13 17:05:32, GvR wrote:
> Either be more specific with the message or remove it. :)
Done.
http://codereview.appspot.com/970/diff/21/41#newcode393
Line 393: closed_issues = list(db.GqlQuery(
I like the recently closed idea, done.
http://codereview.appspot.com/970/diff/21/45
File templates/issue.html (right):
http://codereview.appspot.com/970/diff/21/45#newcode26
Line 26: <a href="{%url codereview.views.close issue.key.id%}"><b>Close
Issue</b></a>
On 2008/05/13 17:05:32, GvR wrote:
> You're missing a '|' before this.
>
> But IMO "close" needs to be a POST command like edit. Maybe we can just make
it
> a check box on the edit form?
I agree, close should definitely be a POST. I prefer having a button in the edit
page though.
Good start. Some comments inline... http://codereview.appspot.com/970/diff/9/102 File codereview/views.py (right): http://codereview.appspot.com/970/diff/9/102#newcode205 Line 205: max_length=30, Please line ...
15 years, 10 months ago
(2008-05-14 00:30:30 UTC)
#4
Good start. Some comments inline...
http://codereview.appspot.com/970/diff/9/102
File codereview/views.py (right):
http://codereview.appspot.com/970/diff/9/102#newcode205
Line 205: max_length=30,
Please line up arguments under the first one.
http://codereview.appspot.com/970/diff/9/102#newcode354
Line 354: query = db.GqlQuery('SELECT * FROM Issue WHERE active=True '
make that
WHERE active = TRUE
since TRUE is a gql constant. (And I want spaces around my operators -- also
below. ;-)
http://codereview.appspot.com/970/diff/9/102#newcode399
Line 399: 'AND owner = :1 ORDER BY modified DESC',
If you can, break before 'WHERE' -- I think the whole WHERE clause will fit on a
line, and that way the syntax is slightly clearer.
http://codereview.appspot.com/970/diff/9/102#newcode407
Line 407: 'AND owner = :1 ORDER BY modified DESC LIMIT 10',
I wonder if instead of the most recent 10 issues we should show issues closed in
the last 7 days?
http://codereview.appspot.com/970/diff/9/102#newcode839
Line 839: def close(request):
The more I think about it, the more I want "closed" to be a checkbox in the edit
form. (a) this allows reopening issues, and (b) it makes it possible to update
the description at the same time, which is a common pattern in issue trackers.
(E.g. close it and add a comment "submitted as r99".)
http://codereview.appspot.com/970/diff/9/102#newcode1169
Line 1169: def bulk_edit(request):
This function should isist that request.method == "POST".
http://codereview.appspot.com/970/diff/9/102#newcode1170
Line 1170: query = db.GqlQuery('SELECT * FROM %s' %
It turns out that if you enter an undefined model name, you get an exception
here. I recommend calling db.class_for_kind() with the model name, catching the
errors, and reporting those. Then you can use cls.all() instead of a GQL query.
http://codereview.appspot.com/970/diff/9/102#newcode1171
Line 1171: (request.GET.get('model')))
You don't need the parentheses around (request.GET.get('model')).
http://codereview.appspot.com/970/diff/9/102#newcode1172
Line 1172: if query.count() == 0:
Calling query.count() is really expensive -- you end up doing the query twice.
I would just maintain a counter of how many entities were updated, and check
that after the loop.
http://codereview.appspot.com/970/diff/9/102#newcode1173
Line 1173: return HttpResponse('No entities found')
I'd add the model name to the error message in case they didn't realize they had
a typo.
http://codereview.appspot.com/970/diff/9/102#newcode1175
Line 1175: prop = getattr(entity, request.GET.get('property_name'))
If the attribute name is not supported, this raises an exception. Please catch
that and report. You should be able to do that outside the for-loop, getting
the property object from the mdoel class. The property has an attribute
data_type giving the data type. In many cases (but not for bool) calling that
data type with a string will do the correct conversion, e.g. for int() it will
work.
http://codereview.appspot.com/970/diff/9/102#newcode1183
Line 1183: return HttpResponse('Input Format Error')
I'd echo back the bad value string here and also indicate that it's a boolean
property.
http://codereview.appspot.com/970/diff/9/102#newcode1186
Line 1186: entity.put()
I wonder what will happen if there are many entities to change and we run out of
time. (Reading + writing one entity is relatively expensive so this would
perhaps support only a few dozen entities, or a few hundred at most.) We should
at least have some way of skipping entities that already have the correct value,
saving the put() calls for those. (But you cannot query for that, because the
query won't return objects that don't have the attribute set at all.) Also the
default value may mean it's difficult to tell whether it's been set in the
database or only on the in-memory copy.
Let's worry about this later.
http://codereview.appspot.com/970/diff/9/106
File templates/admin.html (right):
http://codereview.appspot.com/970/diff/9/106#newcode5
Line 5: <form action="{%url codereview.views.bulk_edit%}" method="get">
At least add a header "Bulk Edit"; get the example from edit.html.
http://codereview.appspot.com/970/diff/9/105
File templates/edit.html (right):
http://codereview.appspot.com/970/diff/9/105#newcode3
Line 3: <h2>Edit Issue: {{issue.subject}} {%if not issue.active %} (Inactive)
{%endif%}</h2>
I would say "closed".
By the way, I wonder if a better alternative to the batch edit command isn't ...
15 years, 10 months ago
(2008-05-14 15:12:27 UTC)
#5
By the way, I wonder if a better alternative to the batch edit command isn't
simply a form that takes some Python code and executes it. In fact, we don't
need to do anything for tat if we add a binding for google.appengine.ext.admin
to app.yaml (this is the same code that the dev_appserver includes as /_ah/admin
by default).
http://codereview.appspot.com/970/diff/9/102 File codereview/views.py (right): http://codereview.appspot.com/970/diff/9/102#newcode354 Line 354: query = db.GqlQuery('SELECT * FROM Issue WHERE active=True ...
15 years, 10 months ago
(2008-05-14 18:43:51 UTC)
#6
http://codereview.appspot.com/970/diff/9/102
File codereview/views.py (right):
http://codereview.appspot.com/970/diff/9/102#newcode354
Line 354: query = db.GqlQuery('SELECT * FROM Issue WHERE active=True '
On 2008/05/14 00:30:31, GvR wrote:
> make that
>
> WHERE active = TRUE
>
> since TRUE is a gql constant. (And I want spaces around my operators -- also
> below. ;-)
Done.
http://codereview.appspot.com/970/diff/9/102#newcode399
Line 399: 'AND owner = :1 ORDER BY modified DESC',
On 2008/05/14 00:30:31, GvR wrote:
> If you can, break before 'WHERE' -- I think the whole WHERE clause will fit on
a
> line, and that way the syntax is slightly clearer.
Done.
http://codereview.appspot.com/970/diff/9/102#newcode407
Line 407: 'AND owner = :1 ORDER BY modified DESC LIMIT 10',
I'm fine with either way here. Done.
http://codereview.appspot.com/970/diff/9/102#newcode839
Line 839: def close(request):
On 2008/05/14 00:30:31, GvR wrote:
> The more I think about it, the more I want "closed" to be a checkbox in the
edit
> form. (a) this allows reopening issues, and (b) it makes it possible to update
> the description at the same time, which is a common pattern in issue trackers.
> (E.g. close it and add a comment "submitted as r99".)
Done.
Good suggestion about changing flag active to closed. I just realised whenever 'active' is used, ...
15 years, 10 months ago
(2008-05-14 21:45:52 UTC)
#8
Good suggestion about changing flag active to closed. I just realised whenever
'active' is used, there is always a 'not' before that:)
And the blanks lines are restored.
Issue 970: Adding "close issue" feature
(Closed)
Created 15 years, 10 months ago by jiayao
Modified 14 years, 8 months ago
Reviewers: GvR
Base URL: http://rietveld.googlecode.com/svn/trunk/
Comments: 32