Issue 102140045: Added +tickets view having list of all tickets (Closed)
|
Created:
9 years, 11 months ago by sksaurabhkathpalia Modified:
9 years, 10 months ago Reviewers:
thomas.j.waldmann, dimazest Visibility:
Public. |
DescriptionAdded +tickets view having list of all tickets
Patch Set 1 #
Total comments: 9
Patch Set 2 : Now only results are sent to tickets.html #
Total comments: 16
Patch Set 3 : #Patch Set 4 : Removed one small error #
Total comments: 12
Patch Set 5 : Made some more changes #
Total comments: 6
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Removed a small indentation error #
Total comments: 16
Patch Set 9 : Changed heading to Ticket List and also made summary, priority, severity, difficulty, effort necess… #Patch Set 10 : Added search feature in +tickets view #Patch Set 11 : Made some small changes #
Total comments: 13
Patch Set 12 : #Patch Set 13 : Added css for Ticket List table #Patch Set 14 : #Patch Set 15 : #
Total comments: 5
Patch Set 16 : #Patch Set 17 : Added js for filter and also button for showing either all or only open tickets #
Total comments: 1
Patch Set 18 : Did some minor changes #
Total comments: 15
Patch Set 19 : Made changes according to the comments #
Total comments: 17
Patch Set 20 : Used Bootstrap table, Added buttons for viewing All, Open, Close Tickets and also made some other c… #
Total comments: 8
Patch Set 21 : Added some comments in tickets.js and also removed code duplication in tickets.html #Patch Set 22 : Removed some unnecessary comments #
Total comments: 4
Patch Set 23 : Used class instead of id #Patch Set 24 : #Patch Set 25 : #
Total comments: 2
Patch Set 26 : Used class instead of id for buttons #
Total comments: 4
Patch Set 27 : Done translation in titles #Patch Set 28 : Removed comma #MessagesTotal messages: 59
https://codereview.appspot.com/102140045/diff/1/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/1/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:2201: @frontend.route('/+tickets', methods=['GET', 'POST']) why do you accept POST? https://codereview.appspot.com/102140045/diff/1/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:2204: query = And([Term(ITEMTYPE, u'ticket')]) And looks strange here https://codereview.appspot.com/102140045/diff/1/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:2212: for result in results: looks like anti pattern xs = [] for y in ys: xs.append(f(y)) can be rewritten to xs = [f(y) for y in ys] in your case it can also be a list of tuples https://codereview.appspot.com/102140045/diff/1/MoinMoin/templates/tickets.html File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/1/MoinMoin/templates/tickets.ht... MoinMoin/templates/tickets.html:23: {% for i in range(results|length) %} if you have a list of tuples than you can write for a, b in abs and you don't need to use range as you already have all the items you need.
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/1/MoinMoin/templates/tickets.html File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/1/MoinMoin/templates/tickets.ht... MoinMoin/templates/tickets.html:23: {% for i in range(results|length) %} On 2014/06/04 13:16:57, dimazest wrote: > if you have a list of tuples than you can write > > for a, b in abs > > and you don't need to use range as you already have all the items you need. Yeah I tried that only but it was causing ReaderClosed Error and I could not find the source of the error. I am still searching for the problem with that
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/1/MoinMoin/templates/tickets.html File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/1/MoinMoin/templates/tickets.ht... MoinMoin/templates/tickets.html:23: {% for i in range(results|length) %} could, please, you give more context, a traceback, then i could help you On 2014/06/04 14:26:38, sksaurabhkathpalia wrote: > On 2014/06/04 13:16:57, dimazest wrote: > > if you have a list of tuples than you can write > > > > for a, b in abs > > > > and you don't need to use range as you already have all the items you need. > > Yeah I tried that only but it was causing ReaderClosed Error and I could not > find the source of the error. > I am still searching for the problem with that
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/1/MoinMoin/templates/tickets.html File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/1/MoinMoin/templates/tickets.ht... MoinMoin/templates/tickets.html:23: {% for i in range(results|length) %} On 2014/06/04 14:36:11, dimazest wrote: > could, please, you give more context, a traceback, then i could help you > > > On 2014/06/04 14:26:38, sksaurabhkathpalia wrote: > > On 2014/06/04 13:16:57, dimazest wrote: > > > if you have a list of tuples than you can write > > > > > > for a, b in abs > > > > > > and you don't need to use range as you already have all the items you need. > > > > Yeah I tried that only but it was causing ReaderClosed Error and I could not > > find the source of the error. > > I am still searching for the problem with that > This is the error which is raised http://rn0.me/show/57NUsc3Z1UIdSbT84W6L/ when I simply pass results to tickets and then use it like this for result in results: {{result['summary']}}
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/1/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/1/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:2203: with flaskg.storage.indexer.ix[LATEST_REVS].searcher() as searcher: here is the search context starts, check my other comment https://codereview.appspot.com/102140045/diff/1/MoinMoin/templates/tickets.html File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/1/MoinMoin/templates/tickets.ht... MoinMoin/templates/tickets.html:23: {% for i in range(results|length) %} is your result a list? it has to be a list, and built inside of the search context On 2014/06/04 14:40:43, sksaurabhkathpalia wrote: > On 2014/06/04 14:36:11, dimazest wrote: > > could, please, you give more context, a traceback, then i could help you > > > > > > On 2014/06/04 14:26:38, sksaurabhkathpalia wrote: > > > On 2014/06/04 13:16:57, dimazest wrote: > > > > if you have a list of tuples than you can write > > > > > > > > for a, b in abs > > > > > > > > and you don't need to use range as you already have all the items you > need. > > > > > > Yeah I tried that only but it was causing ReaderClosed Error and I could not > > > find the source of the error. > > > I am still searching for the problem with that > > > This is the error which is raised http://rn0.me/show/57NUsc3Z1UIdSbT84W6L/ > when I simply pass results to tickets and then use it like this > for result in results: > {{result['summary']}} > https://codereview.appspot.com/102140045/diff/20001/MoinMoin/apps/frontend/vi... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/20001/MoinMoin/apps/frontend/vi... MoinMoin/apps/frontend/views.py:2206: return render_template('tickets.html', right, that's a better way to fix it! https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:14: <table border="1"> maybe this should be defined in a .css file https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:16: <th>No.</th> should this be translated? https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:27: {% if result['summary']!='' %} if result['summary'] https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:28: ] <a href="{{ url_for_item(result['itemid'], field='itemid', namespace=result['namespace'])}}" title="{{ _("ITEMID: %(itemid)s", itemid = result['itemid'])}}"> {{result['summary']}}</a> is it possible to get rid of the code duplication https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:34: {% if result['closed'] %} maybe there is a nicer way to do this https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:42: {% if result['priority'] is not defined or result['priority'] == 1 %} why not to use else for the undefined way also maybe the dict. trick would work here https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:55: {% if (result['tags']|length) > 0 %} if result['tags']
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:34: {% if result['closed'] %} On 2014/06/04 15:23:36, dimazest wrote: > maybe there is a nicer way to do this Can you give any idea of doing this in a better way
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:34: {% if result['closed'] %} {% 'Closed' if result['closed'] else 'Open'%} On 2014/06/04 15:32:12, sksaurabhkathpalia wrote: > On 2014/06/04 15:23:36, dimazest wrote: > > maybe there is a nicer way to do this > > Can you give any idea of doing this in a better way
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:34: {% if result['closed'] %} On 2014/06/04 15:37:29, dimazest wrote: > {% 'Closed' if result['closed'] else 'Open'%} > > On 2014/06/04 15:32:12, sksaurabhkathpalia wrote: > > On 2014/06/04 15:23:36, dimazest wrote: > > > maybe there is a nicer way to do this > > > > Can you give any idea of doing this in a better way > This works in python but not in jinja2
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:34: {% if result['closed'] %} http://jinja.pocoo.org/docs/templates/#if-expression On 2014/06/04 15:56:07, sksaurabhkathpalia wrote: > On 2014/06/04 15:37:29, dimazest wrote: > > {% 'Closed' if result['closed'] else 'Open'%} > > > > On 2014/06/04 15:32:12, sksaurabhkathpalia wrote: > > > On 2014/06/04 15:23:36, dimazest wrote: > > > > maybe there is a nicer way to do this > > > > > > Can you give any idea of doing this in a better way > > > > This works in python but not in jinja2
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:34: {% if result['closed'] %} On 2014/06/04 15:57:31, dimazest wrote: > http://jinja.pocoo.org/docs/templates/#if-expression > > On 2014/06/04 15:56:07, sksaurabhkathpalia wrote: > > On 2014/06/04 15:37:29, dimazest wrote: > > > {% 'Closed' if result['closed'] else 'Open'%} > > > > > > On 2014/06/04 15:32:12, sksaurabhkathpalia wrote: > > > > On 2014/06/04 15:23:36, dimazest wrote: > > > > > maybe there is a nicer way to do this > > > > > > > > Can you give any idea of doing this in a better way > > > > > > > This works in python but not in jinja2 > For this it has to be done in this way {% set status = _('Closed') if result['closed'] else _('Open') %} {{ status }} Is this Ok?
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:34: {% if result['closed'] %} not really from the docs: {{ '[%s]' % page.title if page.title }} what about {{ 'Closed' if result['closed'] else 'Open'}} On 2014/06/04 16:02:03, sksaurabhkathpalia wrote: > On 2014/06/04 15:57:31, dimazest wrote: > > http://jinja.pocoo.org/docs/templates/#if-expression > > > > On 2014/06/04 15:56:07, sksaurabhkathpalia wrote: > > > On 2014/06/04 15:37:29, dimazest wrote: > > > > {% 'Closed' if result['closed'] else 'Open'%} > > > > > > > > On 2014/06/04 15:32:12, sksaurabhkathpalia wrote: > > > > > On 2014/06/04 15:23:36, dimazest wrote: > > > > > > maybe there is a nicer way to do this > > > > > > > > > > Can you give any idea of doing this in a better way > > > > > > > > > > This works in python but not in jinja2 > > > > For this it has to be done in this way > {% set status = _('Closed') if result['closed'] else _('Open') %} > {{ status }} > Is this Ok?
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:34: {% if result['closed'] %} On 2014/06/04 16:04:21, dimazest wrote: > not really > > from the docs: > > {{ '[%s]' % page.title if page.title }} > > what about > > {{ 'Closed' if result['closed'] else 'Open'}} > > On 2014/06/04 16:02:03, sksaurabhkathpalia wrote: > > On 2014/06/04 15:57:31, dimazest wrote: > > > http://jinja.pocoo.org/docs/templates/#if-expression > > > > > > On 2014/06/04 15:56:07, sksaurabhkathpalia wrote: > > > > On 2014/06/04 15:37:29, dimazest wrote: > > > > > {% 'Closed' if result['closed'] else 'Open'%} > > > > > > > > > > On 2014/06/04 15:32:12, sksaurabhkathpalia wrote: > > > > > > On 2014/06/04 15:23:36, dimazest wrote: > > > > > > > maybe there is a nicer way to do this > > > > > > > > > > > > Can you give any idea of doing this in a better way > > > > > > > > > > > > > This works in python but not in jinja2 > > > > > > > For this it has to be done in this way > > {% set status = _('Closed') if result['closed'] else _('Open') %} > > {{ status }} > > Is this Ok? > Oh my bad I didn't see that statement. Making changes now Thanks
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:34: {% if result['closed'] %} cool! On 2014/06/04 16:07:26, sksaurabhkathpalia wrote: > On 2014/06/04 16:04:21, dimazest wrote: > > not really > > > > from the docs: > > > > {{ '[%s]' % page.title if page.title }} > > > > what about > > > > {{ 'Closed' if result['closed'] else 'Open'}} > > > > On 2014/06/04 16:02:03, sksaurabhkathpalia wrote: > > > On 2014/06/04 15:57:31, dimazest wrote: > > > > http://jinja.pocoo.org/docs/templates/#if-expression > > > > > > > > On 2014/06/04 15:56:07, sksaurabhkathpalia wrote: > > > > > On 2014/06/04 15:37:29, dimazest wrote: > > > > > > {% 'Closed' if result['closed'] else 'Open'%} > > > > > > > > > > > > On 2014/06/04 15:32:12, sksaurabhkathpalia wrote: > > > > > > > On 2014/06/04 15:23:36, dimazest wrote: > > > > > > > > maybe there is a nicer way to do this > > > > > > > > > > > > > > Can you give any idea of doing this in a better way > > > > > > > > > > > > > > > > This works in python but not in jinja2 > > > > > > > > > > For this it has to be done in this way > > > {% set status = _('Closed') if result['closed'] else _('Open') %} > > > {{ status }} > > > Is this Ok? > > > Oh my bad I didn't see that statement. Making changes now > Thanks
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/60001/MoinMoin/apps/frontend/vi... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/60001/MoinMoin/apps/frontend/vi... MoinMoin/apps/frontend/views.py:2206: return render_template('tickets.html', i think it can be one line https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:16: <th>{{_("No.")}}</th> are you sure that string constants need to be translated? maybe not
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:16: <th>{{_("No.")}}</th> On 2014/06/04 16:29:48, dimazest wrote: > are you sure that string constants need to be translated? maybe not Yeah I think they do need to get translated. I also saw in other files also there also constant strings are translated
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/60001/MoinMoin/apps/frontend/vi... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/60001/MoinMoin/apps/frontend/vi... MoinMoin/apps/frontend/views.py:2206: return render_template('tickets.html', On 2014/06/04 16:29:48, dimazest wrote: > i think it can be one line If you see the methods above this, they also have similar syntax
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:16: <th>{{_("No.")}}</th> what about this: <th>No.</th> On 2014/06/04 16:36:02, sksaurabhkathpalia wrote: > On 2014/06/04 16:29:48, dimazest wrote: > > are you sure that string constants need to be translated? maybe not > > Yeah I think they do need to get translated. > I also saw in other files also there also constant strings are translated https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:47: {{_("Trivial")}} why not tu say that it is not set https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:51: {% if result['tags'] %} maybe you dont need if here, try to just iterate here https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:53: {{ _("%(tag)s ", tag = tag)}} {{ _(tag)}}
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:16: <th>{{_("No.")}}</th> On 2014/06/05 06:43:05, dimazest wrote: > what about this: > > <th>No.</th> > > > On 2014/06/04 16:36:02, sksaurabhkathpalia wrote: > > On 2014/06/04 16:29:48, dimazest wrote: > > > are you sure that string constants need to be translated? maybe not > > > > Yeah I think they do need to get translated. > > I also saw in other files also there also constant strings are translated > But then there won't be any translation. I am not able to understand that why translation is not required here. Can you please explain?
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:53: {{ _("%(tag)s ", tag = tag)}} On 2014/06/05 06:43:05, dimazest wrote: > {{ _(tag)}} Here I want to add space between adjacent tags thats why I have written like this
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:53: {{ _("%(tag)s ", tag = tag)}} On 2014/06/05 06:52:28, sksaurabhkathpalia wrote: > On 2014/06/05 06:43:05, dimazest wrote: > > {{ _(tag)}} > Here I want to add space between adjacent tags thats why I have written like > this Oh my bad that also generates the same result
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/60001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:10: <h1>List of Tickets</h1> Look, here _() is not used
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/80001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/80001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:12: {% if results is defined %} if results https://codereview.appspot.com/102140045/diff/80001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:27: <a href="{{ url_for_item(result['itemid'], field='itemid', namespace=result['namespace'])}}" title="{{ _("ITEMID: %(itemid)s", itemid = result['itemid'])}}"> itemid=result['itemid'] https://codereview.appspot.com/102140045/diff/80001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:51: {{ _(tag) }} does it make sense to translate tags?
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/80001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/80001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:12: {% if results is defined %} On 2014/06/05 09:25:41, dimazest wrote: > if results Done. https://codereview.appspot.com/102140045/diff/80001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:27: <a href="{{ url_for_item(result['itemid'], field='itemid', namespace=result['namespace'])}}" title="{{ _("ITEMID: %(itemid)s", itemid = result['itemid'])}}"> On 2014/06/05 09:25:41, dimazest wrote: > itemid=result['itemid'] Done.
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/80001/MoinMoin/templates/ticket... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/80001/MoinMoin/templates/ticket... MoinMoin/templates/tickets.html:51: {{ _(tag) }} On 2014/06/05 09:25:41, dimazest wrote: > does it make sense to translate tags? Done.
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:4: <title>{{_("List of Tickets")}}</title> just "Tickets"? plural is indicating that it is more than 1. https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:10: <h1>{{_("List of Tickets")}}</h1> see above https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:16: <th>{{_("No.")}}</th> ID, not Number. https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:19: <th>{{_("Priority")}}</th> ok for the beginning. but in the end, we might also need difficulty, severity and work amount. especially if one looks for an easy job, a pressing issue or little work/much work, they are useful as sort criteria. https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:25: <td>{{result.pos + 1}}</td> how is that useful? i first thought that you put the (shortened) ticket itemid here, but just a running number (that has nothing to do with the ticket) is pretty useless. https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:28: {{ result['summary'] if result['summary'] else _("No summary given") }} you already made an issue textual representation for that widget a few days ago (short issue id + summary). why don't you just use the same code here? no summary given -> that should not be possible, needs to get checked by validation. https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:44: {{_("Blocker")}} NOPE! NEVER EVER! You obviously don't know yet, but it has a reason that all these (4?) metadata values have just numerical values from 1 .. 5. It is just that in every language and for everybody, 1 always means lowest, 5 always means highest (if you know that scale is from 1 .. 5). Doing a bad translation of that scale into words only creates troubles, as you see on bitbucket issue tracker. They completely fucked it up and now they can't fix it due to "compatibility". priority is a project management decision, made by: priority = f(difficulty, severity, work amount, other factors) So, if you'ld translate 1 into "Trivial", many people would think this is an easy to fix issue. No, that is NOT the case, it just has low priority, so it should not be worked on as long as there are higher prio issues. Also, if you translate 5 into "Blocker", many people would think that it blocks a release or development in general. But 5 just means high prio, should be fixed asap. This could be a difficulty 1, severity any, work amount 1 issue and managment just decided "get it of the table asap". Or it could be a severity 5 issue, like a critical bug. Nothing of that blocks anything. So, always just display the number for these 4 metadata items. https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:46: {{_("Not set")}} Validator likely should make sure it is set. It could default to 3, so it can be lowered or raised later. https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:51: {{ tag }} see how tags are represented at other places, is there a comma in between?
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:25: <td>{{result.pos + 1}}</td> On 2014/06/06 21:29:24, Thomas.J.Waldmann wrote: > how is that useful? > > i first thought that you put the (shortened) ticket itemid here, but just a > running number (that has nothing to do with the ticket) is pretty useless. > Okay will have the first 7 digits of ITEMID here as it it is there at many other places.
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:51: {{ tag }} On 2014/06/06 21:29:24, Thomas.J.Waldmann wrote: > see how tags are represented at other places, is there a comma in between? Generally comma is not used. For example in github comma is not used
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:4: <title>{{_("List of Tickets")}}</title> On 2014/06/06 21:29:24, Thomas.J.Waldmann wrote: > just "Tickets"? > > plural is indicating that it is more than 1. Done. https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:10: <h1>{{_("List of Tickets")}}</h1> On 2014/06/06 21:29:24, Thomas.J.Waldmann wrote: > see above Done. https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:16: <th>{{_("No.")}}</th> On 2014/06/06 21:29:24, Thomas.J.Waldmann wrote: > ID, not Number. Done. https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:44: {{_("Blocker")}} On 2014/06/06 21:29:24, Thomas.J.Waldmann wrote: > NOPE! NEVER EVER! > > You obviously don't know yet, but it has a reason that all these (4?) metadata > values have just numerical values from 1 .. 5. It is just that in every language > and for everybody, 1 always means lowest, 5 always means highest (if you know > that scale is from 1 .. 5). > > Doing a bad translation of that scale into words only creates troubles, as you > see on bitbucket issue tracker. They completely fucked it up and now they can't > fix it due to "compatibility". > > priority is a project management decision, made by: > > priority = f(difficulty, severity, work amount, other factors) > > So, if you'ld translate 1 into "Trivial", many people would think this is an > easy to fix issue. No, that is NOT the case, it just has low priority, so it > should not be worked on as long as there are higher prio issues. > > Also, if you translate 5 into "Blocker", many people would think that it blocks > a release or development in general. But 5 just means high prio, should be fixed > asap. This could be a difficulty 1, severity any, work amount 1 issue and > managment just decided "get it of the table asap". Or it could be a severity 5 > issue, like a critical bug. Nothing of that blocks anything. > > So, always just display the number for these 4 metadata items. Done. https://codereview.appspot.com/102140045/diff/140001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:46: {{_("Not set")}} On 2014/06/06 21:29:24, Thomas.J.Waldmann wrote: > Validator likely should make sure it is set. > > It could default to 3, so it can be lowered or raised later. Done.
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/180001/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/180001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2210: ok, looks good so far. https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:5: <title>{{_("Ticket List")}}</title> "Tickets" https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:11: <h1>{{_("Ticket List")}}</h1> didn't we discuss that already? "Tickets" https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:13: <form action="{{ url_for('frontend.ticketlist') }}" method="get" id='moin-ticketsearch-form'> maybe this could also be just "tickets" if the url is +tickets now. https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:34: <td>{{result['itemid'][:7]}}</td> we should have a standard "shorten itemid" template filter function that is used everywhere where a shortended itemid or revid is needed. otherwise this will get out of sync and one place will use 7 digits, one 6, one 8 ... https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:38: </a> same here, didn't you already create code that assembles this text from shortended itemid + text for that option list? why code it here again and not reuse it? https://codereview.appspot.com/102140045/diff/180001/MoinMoin/themes/basic/st... File MoinMoin/themes/basic/static/css/basic.css (right): https://codereview.appspot.com/102140045/diff/180001/MoinMoin/themes/basic/st... MoinMoin/themes/basic/static/css/basic.css:6384: } is it necessary to have such a lot of one style and not to use some already existing style from bootstrap?
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:38: </a> On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > same here, didn't you already create code that assembles this text from > shortended itemid + text for that option list? > > why code it here again and not reuse it? So you want to merge the above 2 columns and use result['itemid'] | shorten_id + result['summary'][:50] right?
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:5: <title>{{_("Ticket List")}}</title> On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > "Tickets" Done. https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:11: <h1>{{_("Ticket List")}}</h1> On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > didn't we discuss that already? > > "Tickets" Done. https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:13: <form action="{{ url_for('frontend.ticketlist') }}" method="get" id='moin-ticketsearch-form'> On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > maybe this could also be just "tickets" if the url is +tickets now. Done. https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:34: <td>{{result['itemid'][:7]}}</td> On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > we should have a standard "shorten itemid" template filter function that is used > everywhere where a shortended itemid or revid is needed. > > otherwise this will get out of sync and one place will use 7 digits, one 6, one > 8 ... Done. https://codereview.appspot.com/102140045/diff/180001/MoinMoin/themes/basic/st... File MoinMoin/themes/basic/static/css/basic.css (right): https://codereview.appspot.com/102140045/diff/180001/MoinMoin/themes/basic/st... MoinMoin/themes/basic/static/css/basic.css:6384: } On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > is it necessary to have such a lot of one style and not to use some already > existing style from bootstrap? Now removed unnecessary style statements
Sign in to reply to this message.
On 2014/06/10 16:45:19, sksaurabhkathpalia wrote: > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > File MoinMoin/templates/tickets.html (right): > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > MoinMoin/templates/tickets.html:5: <title>{{_("Ticket List")}}</title> > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > "Tickets" > > Done. > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > MoinMoin/templates/tickets.html:11: <h1>{{_("Ticket List")}}</h1> > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > didn't we discuss that already? > > > > "Tickets" > > Done. > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > MoinMoin/templates/tickets.html:13: <form action="{{ > url_for('frontend.ticketlist') }}" method="get" id='moin-ticketsearch-form'> > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > maybe this could also be just "tickets" if the url is +tickets now. > > Done. > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > MoinMoin/templates/tickets.html:34: <td>{{result['itemid'][:7]}}</td> > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > we should have a standard "shorten itemid" template filter function that is > used > > everywhere where a shortended itemid or revid is needed. > > > > otherwise this will get out of sync and one place will use 7 digits, one 6, > one > > 8 ... > > Done. > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/themes/basic/st... > File MoinMoin/themes/basic/static/css/basic.css (right): > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/themes/basic/st... > MoinMoin/themes/basic/static/css/basic.css:6384: } > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > is it necessary to have such a lot of one style and not to use some already > > existing style from bootstrap? > > Now removed unnecessary style statements Here is the screenshot of +tickets view http://picpaste.com/pics/tickertlist2-TdOHrFLL.1402418872.png
Sign in to reply to this message.
On 2014/06/10 16:45:19, sksaurabhkathpalia wrote: > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > File MoinMoin/templates/tickets.html (right): > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > MoinMoin/templates/tickets.html:5: <title>{{_("Ticket List")}}</title> > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > "Tickets" > > Done. > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > MoinMoin/templates/tickets.html:11: <h1>{{_("Ticket List")}}</h1> > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > didn't we discuss that already? > > > > "Tickets" > > Done. > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > MoinMoin/templates/tickets.html:13: <form action="{{ > url_for('frontend.ticketlist') }}" method="get" id='moin-ticketsearch-form'> > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > maybe this could also be just "tickets" if the url is +tickets now. > > Done. > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > MoinMoin/templates/tickets.html:34: <td>{{result['itemid'][:7]}}</td> > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > we should have a standard "shorten itemid" template filter function that is > used > > everywhere where a shortended itemid or revid is needed. > > > > otherwise this will get out of sync and one place will use 7 digits, one 6, > one > > 8 ... > > Done. > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/themes/basic/st... > File MoinMoin/themes/basic/static/css/basic.css (right): > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/themes/basic/st... > MoinMoin/themes/basic/static/css/basic.css:6384: } > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > is it necessary to have such a lot of one style and not to use some already > > existing style from bootstrap? > > Now removed unnecessary style statements Here is the screenshot of +tickets view http://picpaste.com/pics/tickertlist2-TdOHrFLL.1402418872.png
Sign in to reply to this message.
On 2014/06/10 16:45:19, sksaurabhkathpalia wrote: > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > File MoinMoin/templates/tickets.html (right): > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > MoinMoin/templates/tickets.html:5: <title>{{_("Ticket List")}}</title> > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > "Tickets" > > Done. > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > MoinMoin/templates/tickets.html:11: <h1>{{_("Ticket List")}}</h1> > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > didn't we discuss that already? > > > > "Tickets" > > Done. > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > MoinMoin/templates/tickets.html:13: <form action="{{ > url_for('frontend.ticketlist') }}" method="get" id='moin-ticketsearch-form'> > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > maybe this could also be just "tickets" if the url is +tickets now. > > Done. > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/templates/ticke... > MoinMoin/templates/tickets.html:34: <td>{{result['itemid'][:7]}}</td> > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > we should have a standard "shorten itemid" template filter function that is > used > > everywhere where a shortended itemid or revid is needed. > > > > otherwise this will get out of sync and one place will use 7 digits, one 6, > one > > 8 ... > > Done. > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/themes/basic/st... > File MoinMoin/themes/basic/static/css/basic.css (right): > > https://codereview.appspot.com/102140045/diff/180001/MoinMoin/themes/basic/st... > MoinMoin/themes/basic/static/css/basic.css:6384: } > On 2014/06/09 16:52:16, Thomas.J.Waldmann wrote: > > is it necessary to have such a lot of one style and not to use some already > > existing style from bootstrap? > > Now removed unnecessary style statements Here is the screenshot of +tickets view http://picpaste.com/pics/tickertlist2-TdOHrFLL.1402418872.png
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/260001/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/260001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2204: query = request.args.get('q') maybe as a general remark for later: be careful what you do in the html. if you use primarily GET links (not POST forms), a crawler will likely crawl them all. if that triggers expensive stuff on the server (doing lots of different searches), this is rather unwanted and should be replaced by POST stuff. https://codereview.appspot.com/102140045/diff/260001/MoinMoin/themes/basic/st... File MoinMoin/themes/basic/static/css/basic.css (right): https://codereview.appspot.com/102140045/diff/260001/MoinMoin/themes/basic/st... MoinMoin/themes/basic/static/css/basic.css:6354: background-image: url('/static/logos/search.png'); that's rather an icon, not a logo, right? https://codereview.appspot.com/102140045/diff/260001/MoinMoin/themes/basic/st... MoinMoin/themes/basic/static/css/basic.css:6397: background: #d0dafd; so how shall that fit together if someone uses a different theme? there could be dark and light themes. It is expected that the theme would set the general background colour somehow, but if you override it here, it will only fit for some cases and look really bad maybe in others.
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/260001/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/260001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2204: query = request.args.get('q') On 2014/06/12 11:50:40, Thomas.J.Waldmann wrote: > maybe as a general remark for later: > > be careful what you do in the html. if you use primarily GET links (not POST > forms), a crawler will likely crawl them all. > > if that triggers expensive stuff on the server (doing lots of different > searches), this is rather unwanted and should be replaced by POST stuff. Ok will make it post https://codereview.appspot.com/102140045/diff/260001/MoinMoin/themes/basic/st... File MoinMoin/themes/basic/static/css/basic.css (right): https://codereview.appspot.com/102140045/diff/260001/MoinMoin/themes/basic/st... MoinMoin/themes/basic/static/css/basic.css:6354: background-image: url('/static/logos/search.png'); On 2014/06/12 11:50:40, Thomas.J.Waldmann wrote: > that's rather an icon, not a logo, right? Yeah its an icon should I create a folder by name logo and place it there?
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/300001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/300001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:43: {{ result['summary'][:50] }} ThomasWaldmann I am still not able to get why we should use "Text Representation of tickets" here as this also doing the same thing and also it is putting itemid and summary in different columns which seems better than having both in one column which is generated by get_itemid_short_summary. Can you please explain why should we use this function here?
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2206: """ according to pep8 one line comments should be written in one line and be sentences (have a dot in the end): def tickets(): """Show a list of ticket items.""" https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2210: is_open = request.form['is_open'] would it be possible to convert it to boolean? https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2214: is_open = u'' should it be False? https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2220: if is_open != u'False': then this code will be simpler. https://codereview.appspot.com/102140045/diff/320001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/320001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:14: Filter: <input type="hidden" name="is_open" value="{{ 'True' if is_open!='True' else 'False'}}"> should it be: 'True' if is_open == 'True' else 'False' https://codereview.appspot.com/102140045/diff/320001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:15: <input type="submit" value="{{ 'All' if is_open=='True' else 'Open' }}" id="ticket-query-button" title="Show {{ 'All' if is_open=='True' else 'Open' }} tickets"> is_open == 'True' (missing spaces) would be nice to have is_open as boolean
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2206: """ On 2014/06/14 09:39:08, dimazest wrote: > according to pep8 one line comments should be written in one line and be > sentences (have a dot in the end): > > def tickets(): > """Show a list of ticket items.""" I have run ./m tests no pep-8 error is there. Also you can see at other places in the same file the comments are written in the same way. You can see an example here https://bitbucket.org/thomaswaldmann/moin-2.0/src/1663ebfcb4c4a474f02673b1dbe... https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2214: is_open = u'' On 2014/06/14 09:39:08, dimazest wrote: > should it be False? yeah it should be true here. will do that https://codereview.appspot.com/102140045/diff/320001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/320001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:14: Filter: <input type="hidden" name="is_open" value="{{ 'True' if is_open!='True' else 'False'}}"> On 2014/06/14 09:39:08, dimazest wrote: > should it be: > > 'True' if is_open == 'True' else 'False' No the current is fine if only open tickets are being viewed then the button should give value 'False' to is_open as then only on clicking that it would show all tickets https://codereview.appspot.com/102140045/diff/320001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:15: <input type="submit" value="{{ 'All' if is_open=='True' else 'Open' }}" id="ticket-query-button" title="Show {{ 'All' if is_open=='True' else 'Open' }} tickets"> On 2014/06/14 09:39:08, dimazest wrote: > is_open == 'True' (missing spaces) > > would be nice to have is_open as boolean Yeah I tried to have it boolean only but jinja2 doesn't allows that. I saw from here http://stackoverflow.com/questions/8433450/why-doesnt-my-condition-logic-work...
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2214: is_open = u'' On 2014/06/14 10:51:57, sksaurabhkathpalia wrote: > On 2014/06/14 09:39:08, dimazest wrote: > > should it be False? > > yeah it should be true here. will do that I mean by default it should be true. As first it should only show open tickets.
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2206: """ http://legacy.python.org/dev/peps/pep-0008/#documentation-strings On 2014/06/14 10:51:57, sksaurabhkathpalia wrote: > On 2014/06/14 09:39:08, dimazest wrote: > > according to pep8 one line comments should be written in one line and be > > sentences (have a dot in the end): > > > > def tickets(): > > """Show a list of ticket items.""" > > I have run ./m tests no pep-8 error is there. Also you can see at other places > in the same file the comments are written in the same way. You can see an > example here > https://bitbucket.org/thomaswaldmann/moin-2.0/src/1663ebfcb4c4a474f02673b1dbe... > https://codereview.appspot.com/102140045/diff/320001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/320001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:15: <input type="submit" value="{{ 'All' if is_open=='True' else 'Open' }}" id="ticket-query-button" title="Show {{ 'All' if is_open=='True' else 'Open' }} tickets"> but then you would just check:: if is_open without any comparison On 2014/06/14 10:51:57, sksaurabhkathpalia wrote: > On 2014/06/14 09:39:08, dimazest wrote: > > is_open == 'True' (missing spaces) > > > > would be nice to have is_open as boolean > > Yeah I tried to have it boolean only but jinja2 doesn't allows that. I saw from > here > http://stackoverflow.com/questions/8433450/why-doesnt-my-condition-logic-work...
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/320001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2214: is_open = u'' I mean it should be boolean, not string On 2014/06/14 10:59:11, sksaurabhkathpalia wrote: > On 2014/06/14 10:51:57, sksaurabhkathpalia wrote: > > On 2014/06/14 09:39:08, dimazest wrote: > > > should it be False? > > > > yeah it should be true here. will do that > > I mean by default it should be true. As first it should only show open tickets.
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/320001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/320001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:15: <input type="submit" value="{{ 'All' if is_open=='True' else 'Open' }}" id="ticket-query-button" title="Show {{ 'All' if is_open=='True' else 'Open' }} tickets"> On 2014/06/14 11:30:53, dimazest wrote: > but then you would just check:: > > if is_open > > without any comparison > > On 2014/06/14 10:51:57, sksaurabhkathpalia wrote: > > On 2014/06/14 09:39:08, dimazest wrote: > > > is_open == 'True' (missing spaces) > > > > > > would be nice to have is_open as boolean > > > > Yeah I tried to have it boolean only but jinja2 doesn't allows that. I saw > from > > here > > > http://stackoverflow.com/questions/8433450/why-doesnt-my-condition-logic-work... > Yeah I tried keeping is_open boolean and I did "if is_open" but that doesn't work I mean whether is_open is bool(True) or bool(False) but then the if statement is true in both the cases. Thats why I used string
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/340001/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/340001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2210: is_open = request.form['is_open'] maybe you want to directly transform it into a boolean here. https://codereview.appspot.com/102140045/diff/340001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2214: is_open = u'True' and use a boolean here also. https://codereview.appspot.com/102140045/diff/340001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2215: idx_name = ALL_REVS if history else LATEST_REVS history? do you use pycharm? it underlines undefined names in red. https://codereview.appspot.com/102140045/diff/340001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2221: q = And([Term(CLOSED, False), q]) terms = [Term(ITEMTYPE, ITEMTYPE_TICKET), ] if query: terms.append(...) if is_open: terms.append(...) q = And(terms) this saves some nesting and reduces code duplication, esp. if the amount of terms will grow in future. https://codereview.appspot.com/102140045/diff/340001/MoinMoin/static/js/ticke... File MoinMoin/static/js/tickets.js (right): https://codereview.appspot.com/102140045/diff/340001/MoinMoin/static/js/ticke... MoinMoin/static/js/tickets.js:13: //len = total number of elements in table hmm? isn't it the number of columns? https://codereview.appspot.com/102140045/diff/340001/MoinMoin/static/js/ticke... MoinMoin/static/js/tickets.js:24: } I'ld appreciate consistent use of whitespace. https://codereview.appspot.com/102140045/diff/340001/MoinMoin/static/js/ticke... MoinMoin/static/js/tickets.js:35: } why is this a separate function? do you call it more than from one place? https://codereview.appspot.com/102140045/diff/340001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/340001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:14: Filter: <input type="hidden" name="is_open" value="{{ 'True' if is_open != 'True' else 'False'}}"> don't forget to make it a real boolean here also https://codereview.appspot.com/102140045/diff/340001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:34: <th id="ticket-priority">{{_("Priority")}}</th> E D S P + title as we talked in the meeting. Check how to add a hint for translators later, otherwise they won't be able to translate single letters. https://codereview.appspot.com/102140045/diff/340001/MoinMoin/themes/basic/st... File MoinMoin/themes/basic/static/custom-less/basic.less (right): https://codereview.appspot.com/102140045/diff/340001/MoinMoin/themes/basic/st... MoinMoin/themes/basic/static/custom-less/basic.less:328: background-image: url('/static/logos/search.png'); not a logo. also: reuse stuff from bootstrap? https://codereview.appspot.com/102140045/diff/340001/MoinMoin/themes/basic/st... MoinMoin/themes/basic/static/custom-less/basic.less:352: color: #039; hardcoded colours - think of bootstrap themes...
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/340001/MoinMoin/static/js/ticke... File MoinMoin/static/js/tickets.js (right): https://codereview.appspot.com/102140045/diff/340001/MoinMoin/static/js/ticke... MoinMoin/static/js/tickets.js:35: } On 2014/06/14 19:44:40, Thomas.J.Waldmann wrote: > why is this a separate function? do you call it more than from one place? Yeah it is called when document is ready
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/340001/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/340001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2210: is_open = request.form['is_open'] On 2014/06/14 19:44:40, Thomas.J.Waldmann wrote: > maybe you want to directly transform it into a boolean here. May be now we can get rid of this and have a variable status which would be of string type which can take values "all", "closed", "open" as Now we would be having 3 buttons. What do you say about this idea? https://codereview.appspot.com/102140045/diff/340001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2214: is_open = u'True' On 2014/06/14 19:44:40, Thomas.J.Waldmann wrote: > and use a boolean here also. I tried using that only first but I saw that boolean expressions in jinja2 doesn't work as expected so I used strings. I saw a stackoverflow link also regarding the same http://stackoverflow.com/questions/8433450/why-doesnt-my-condition-logic-work...
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/340001/MoinMoin/static/js/ticke... File MoinMoin/static/js/tickets.js (right): https://codereview.appspot.com/102140045/diff/340001/MoinMoin/static/js/ticke... MoinMoin/static/js/tickets.js:13: //len = total number of elements in table On 2014/06/14 19:44:40, Thomas.J.Waldmann wrote: > hmm? isn't it the number of columns? No its the number of total elements in the table. I mean all number of "td" tag pair
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/340001/MoinMoin/themes/basic/st... File MoinMoin/themes/basic/static/custom-less/basic.less (right): https://codereview.appspot.com/102140045/diff/340001/MoinMoin/themes/basic/st... MoinMoin/themes/basic/static/custom-less/basic.less:352: color: #039; On 2014/06/14 19:44:40, Thomas.J.Waldmann wrote: > hardcoded colours - think of bootstrap themes... I didn't get you Can you please explain it?
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/340001/MoinMoin/themes/basic/st... File MoinMoin/themes/basic/static/custom-less/basic.less (right): https://codereview.appspot.com/102140045/diff/340001/MoinMoin/themes/basic/st... MoinMoin/themes/basic/static/custom-less/basic.less:328: background-image: url('/static/logos/search.png'); On 2014/06/14 19:44:40, Thomas.J.Waldmann wrote: > not a logo. > > also: reuse stuff from bootstrap? yeah now I have used bootstrap class form-control for this
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/360001/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/360001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2208: # receive values only by POST i'm not sure the comments are needed. https://codereview.appspot.com/102140045/diff/360001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2224: q = And(terms) maybe we can get rid of the variable "q" and move And(terms) inside of the line 2227 https://codereview.appspot.com/102140045/diff/360001/MoinMoin/static/js/ticke... File MoinMoin/static/js/tickets.js (right): https://codereview.appspot.com/102140045/diff/360001/MoinMoin/static/js/ticke... MoinMoin/static/js/tickets.js:4: return function () { on the other side, it would be nice to add a concise description of what the function is doing https://codereview.appspot.com/102140045/diff/360001/MoinMoin/static/js/ticke... MoinMoin/static/js/tickets.js:13: //len = total number of elements in table i'm not sure this comments is needed. https://codereview.appspot.com/102140045/diff/360001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/360001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:25: <form action="{{ url_for('frontend.tickets') }}" method="post" id='moin-ticketsearch-form'> are the form IDs the same? looks like a lot of duplicated code, maybe it's possible to reduce it. https://codereview.appspot.com/102140045/diff/360001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:34: </form><br> it worth to move <br> to the next line
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/360001/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/360001/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2224: q = And(terms) On 2014/06/15 19:04:23, dimazest wrote: > maybe we can get rid of the variable "q" > > and move And(terms) inside of the line 2227 Actually Thomas asked me do do it in this way. You can see hid comment here https://codereview.appspot.com/102140045/patch/340001/350001 https://codereview.appspot.com/102140045/diff/360001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/360001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:25: <form action="{{ url_for('frontend.tickets') }}" method="post" id='moin-ticketsearch-form'> On 2014/06/15 19:04:23, dimazest wrote: > are the form IDs the same? > > looks like a lot of duplicated code, maybe it's possible to reduce it. I have kept the same id as they are doing similar work and also same css is required.
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/400001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/400001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:15: {% for i in range(status_values|length) %} for status_value in status_values https://codereview.appspot.com/102140045/diff/400001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:16: <form action="{{ url_for('frontend.tickets') }}" method="post" id='moin-ticketsearch-form'> should the forms have different ids https://codereview.appspot.com/102140045/diff/400001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:19: <input type="submit" value="{{ status_values[i].capitalize() }}" id="ticket-query-button" title="{{ _('Show %(status)s tickets', status = status_values[i]) }}" class="{{ 'active' if status == status_values[i] }}"> status=status_value without spaces around =
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/400001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/400001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:16: <form action="{{ url_for('frontend.tickets') }}" method="post" id='moin-ticketsearch-form'> On 2014/06/16 07:59:16, dimazest wrote: > should the forms have different ids Yeah I should define class here
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/460001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/460001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:19: <input type="submit" value="{{ status_value.capitalize() }}" id="ticket-query-button" title="{{ _('Show %(status)s tickets', status=status_value) }}" class="{{ 'active' if status == status_value }}"> should the id of the <input> be unique?
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/460001/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/460001/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:19: <input type="submit" value="{{ status_value.capitalize() }}" id="ticket-query-button" title="{{ _('Show %(status)s tickets', status=status_value) }}" class="{{ 'active' if status == status_value }}"> On 2014/06/16 12:06:43, dimazest wrote: > should the id of the <input> be unique? Done.
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/410009/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/410009/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2215: terms = [Term(ITEMTYPE, ITEMTYPE_TICKET), ] comma is not needed here https://codereview.appspot.com/102140045/diff/410009/MoinMoin/templates/ticke... File MoinMoin/templates/tickets.html (right): https://codereview.appspot.com/102140045/diff/410009/MoinMoin/templates/ticke... MoinMoin/templates/tickets.html:58: <td title="Filter by effort: {{ result['effort'] }}"> "Filter by effort:" and others have to be translated
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/410009/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/410009/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2218: if status == u'open': are there any other statuses possible?
Sign in to reply to this message.
https://codereview.appspot.com/102140045/diff/410009/MoinMoin/apps/frontend/v... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/102140045/diff/410009/MoinMoin/apps/frontend/v... MoinMoin/apps/frontend/views.py:2218: if status == u'open': On 2014/06/17 07:40:49, dimazest wrote: > are there any other statuses possible? Yeah status can take values "open", "closed", "all"
Sign in to reply to this message.
|
