Code review - Issue 236490043: already existing ticket suggestionshttps://codereview.appspot.com/2015-06-25T08:07:54+00:00rietveld
Message from unknown
2015-05-26T15:40:59+00:00vipulurn:md5:0b5b96733173498f2c886314437f91a9
Message from unknown
2015-05-26T21:30:47+00:00vipulurn:md5:7fbf1c82be4083438b39516bdb9eecc8
Message from sksaurabhkathpalia@gmail.com
2015-05-27T11:45:18+00:00sksaurabhkathpaliaurn:md5:67a5cc4790e5d11b977290ba5f86b17f
https://codereview.appspot.com/236490043/diff/20001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/236490043/diff/20001/MoinMoin/items/ticket.py#newcode67
MoinMoin/items/ticket.py:67: summary = Text.using(label=L_("Summary"), optional=True).with_properties \
Why have you made summary an optional field?
https://codereview.appspot.com/236490043/diff/20001/MoinMoin/templates/ticket/submit.html
File MoinMoin/templates/ticket/submit.html (right):
https://codereview.appspot.com/236490043/diff/20001/MoinMoin/templates/ticket/submit.html#newcode23
MoinMoin/templates/ticket/submit.html:23: </p>
Remove TAB before this <p> tag
Message from sksaurabhkathpalia@gmail.com
2015-05-27T12:24:24+00:00sksaurabhkathpaliaurn:md5:d086ff046aa7fc7a0d96fbf653c760f9
https://codereview.appspot.com/236490043/diff/20001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):
https://codereview.appspot.com/236490043/diff/20001/MoinMoin/apps/frontend/views.py#newcode406
MoinMoin/apps/frontend/views.py:406: html = render_template('ticket_ajaxsearch.html',
There is no file named ticket_ajaxsearch.html in the patch. This is raising template error
Message from vipul.sharma20@gmail.com
2015-05-27T12:35:51+00:00vipulurn:md5:ad55bbd377a2596f2763a0f9e5c7efe5
https://codereview.appspot.com/236490043/diff/20001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):
https://codereview.appspot.com/236490043/diff/20001/MoinMoin/apps/frontend/views.py#newcode406
MoinMoin/apps/frontend/views.py:406: html = render_template('ticket_ajaxsearch.html',
sorry, it was a new file, forgot to add it. Included it in the new patch
On 2015/05/27 12:24:24, sksaurabhkathpalia wrote:
> There is no file named ticket_ajaxsearch.html in the patch. This is raising
> template error
https://codereview.appspot.com/236490043/diff/20001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/236490043/diff/20001/MoinMoin/items/ticket.py#newcode67
MoinMoin/items/ticket.py:67: summary = Text.using(label=L_("Summary"), optional=True).with_properties \
Earlier I thought this could work but now I see the problem. I will work on this.
On 2015/05/27 11:45:18, sksaurabhkathpalia wrote:
> Why have you made summary an optional field?
Message from unknown
2015-05-28T14:45:42+00:00vipulurn:md5:47511d5387ce1a222c54e6d0e3ba858f
Message from vipul.sharma20@gmail.com
2015-05-28T14:51:33+00:00vipulurn:md5:9c25349d43b78ca085dc33cccccb2279
Message from Thomas.J.Waldmann@gmail.com
2015-05-29T06:57:51+00:00Thomas.J.Waldmannurn:md5:54d7e513cf5d1bd29ccfa5b55a859bf2
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/apps/frontend/views.py#newcode329
MoinMoin/apps/frontend/views.py:329: if request.args.get('itemtype') == ITEMTYPE_TICKET:
there's a constant for the right one, but not for the left one?
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/apps/frontend/views.py#newcode411
MoinMoin/apps/frontend/views.py:411: else:
does it need special treatment?
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/constants/forms.py
File MoinMoin/constants/forms.py (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/constants/forms.py#newcode25
MoinMoin/constants/forms.py:25: WIDGET_TICKET_SEARCH = u'ticket_search'
doesn't sound like a widget
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/items/ticket.py#newcode67
MoinMoin/items/ticket.py:67: (widget=WIDGET_TICKET_SEARCH, placeholder=L_("Ticket Summary"))
if one is editing a ticket, it is not necessary to say that it relates to a ticket. How about "One-line summary"?
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/css/ticket.css
File MoinMoin/static/css/ticket.css (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/css/ticket.css#newcode105
MoinMoin/static/css/ticket.css:105: }
do you need different style for ticket results vs. other results?
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/js/search.js
File MoinMoin/static/js/search.js (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/js/search.js#newcode22
MoinMoin/static/js/search.js:22: }));
this looks same as above
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/js/search.js#newcode48
MoinMoin/static/js/search.js:48: }
this looks very similar to above. could it be merged and just have the itemtype in general?
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/forms.html
File MoinMoin/templates/forms.html (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/forms.html#newcode62
MoinMoin/templates/forms.html:62: 'ticket_search': ticket_search,
move it some lines up, below search?
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/forms.html#newcode157
MoinMoin/templates/forms.html:157: {% endmacro %}
almost same as above
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket/base.html
File MoinMoin/templates/ticket/base.html (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket/base.html#newcode139
MoinMoin/templates/ticket/base.html:139: <script src="{{ url_for('serve.files', name='jquery', filename='jquery.min.js') }}"></script>
hmm, not sure, but didn't we load jquery in general?
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket_ajaxsearch.html
File MoinMoin/templates/ticket_ajaxsearch.html (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket_ajaxsearch.html#newcode2
MoinMoin/templates/ticket_ajaxsearch.html:2: {% if results is defined %}
wat?
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket_ajaxsearch.html#newcode18
MoinMoin/templates/ticket_ajaxsearch.html:18: {% endif %}
if above is for tickets, why do you prefer names (esp. since they should not have a name) over revid/itemid?
Message from vipul.sharma20@gmail.com
2015-05-29T12:58:31+00:00vipulurn:md5:ccc1a299ed369e825c555ce99ce7fbce
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/apps/frontend/views.py#newcode329
MoinMoin/apps/frontend/views.py:329: if request.args.get('itemtype') == ITEMTYPE_TICKET:
On 2015/05/29 06:57:50, Thomas.J.Waldmann wrote:
> there's a constant for the right one, but not for the left one?
yes there is. I will correct that
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/apps/frontend/views.py#newcode411
MoinMoin/apps/frontend/views.py:411: else:
On 2015/05/29 06:57:50, Thomas.J.Waldmann wrote:
> does it need special treatment?
the existing search results contain few things that we may not be interested in as a duplicate ticket suggestion. So I thought of creating a separate template for rendering ticket search results and then, we can define a separate css for better display.
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/constants/forms.py
File MoinMoin/constants/forms.py (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/constants/forms.py#newcode25
MoinMoin/constants/forms.py:25: WIDGET_TICKET_SEARCH = u'ticket_search'
On 2015/05/29 06:57:50, Thomas.J.Waldmann wrote:
> doesn't sound like a widget
you mean the name of the widget right ?
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/items/ticket.py#newcode67
MoinMoin/items/ticket.py:67: (widget=WIDGET_TICKET_SEARCH, placeholder=L_("Ticket Summary"))
On 2015/05/29 06:57:50, Thomas.J.Waldmann wrote:
> if one is editing a ticket, it is not necessary to say that it relates to a
> ticket. How about "One-line summary"?
Yes, this is better
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/css/ticket.css
File MoinMoin/static/css/ticket.css (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/css/ticket.css#newcode105
MoinMoin/static/css/ticket.css:105: }
On 2015/05/29 06:57:50, Thomas.J.Waldmann wrote:
> do you need different style for ticket results vs. other results?
other results are rendered as a long list in a table. I wanted to create a scrollable div(like suggestions in stackoverflow) so that if there is a long list of suggestions, it does not become messy and be confined in a definite space with a scrollbar.
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/forms.html
File MoinMoin/templates/forms.html (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/forms.html#newcode157
MoinMoin/templates/forms.html:157: {% endmacro %}
On 2015/05/29 06:57:50, Thomas.J.Waldmann wrote:
> almost same as above
I wanted to use a different id as the ticket suggestion behaviour is little different.
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket_ajaxsearch.html
File MoinMoin/templates/ticket_ajaxsearch.html (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket_ajaxsearch.html#newcode2
MoinMoin/templates/ticket_ajaxsearch.html:2: {% if results is defined %}
On 2015/05/29 06:57:51, Thomas.J.Waldmann wrote:
> wat?
oh ! really sorry for that, will correct it
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket_ajaxsearch.html#newcode18
MoinMoin/templates/ticket_ajaxsearch.html:18: {% endif %}
On 2015/05/29 06:57:51, Thomas.J.Waldmann wrote:
> if above is for tickets, why do you prefer names (esp. since they should not
> have a name) over revid/itemid?
Yes you are right. I will fix this
Message from unknown
2015-05-29T19:48:09+00:00vipulurn:md5:0b78c36b6a604387ef1b3bbc12911963
Message from vipul.sharma20@gmail.com
2015-05-29T20:42:51+00:00vipulurn:md5:5eec65beb7260871b440afaf49872fcb
Made changes as suggested in the previous CR. Please review the new patch set.
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/static/css/ticket.css
File MoinMoin/static/css/ticket.css (right):
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/static/css/ticket.css#newcode105
MoinMoin/static/css/ticket.css:105: }
added different style for ticket results: to create a scrollable div so that if there is a long list of suggestions, it does not become messy and be confined in a definite space with a scrollbar.
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/static/js/search.js
File MoinMoin/static/js/search.js (right):
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/static/js/search.js#newcode22
MoinMoin/static/js/search.js:22: function ajaxify(query, allrevs, time_sorting, filetypes, boolticket) {
merged the previously created similar function
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ajaxsearch.html
File MoinMoin/templates/ajaxsearch.html (right):
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ajaxsearch.html#newcode2
MoinMoin/templates/ajaxsearch.html:2: {% if boolticket %}
added a condition to display: itemid and summary of existing tickets if search is made from ticket create view. I've treated this differently as some of the fields of the existing search results may not be necessary as a ticket suggestion. Rest of previous code is in else part.
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ticket/base.html
File MoinMoin/templates/ticket/base.html (right):
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ticket/base.html#newcode139
MoinMoin/templates/ticket/base.html:139: <script src="{{ url_for('serve.files', name='jquery', filename='jquery.min.js') }}"></script>
JQuery is already loaded in templates/base.html but still it does not work if I remove this from here. Even templates/search.html has loaded it again. Don't know what's the problem.
Message from Thomas.J.Waldmann@gmail.com
2015-06-01T20:40:25+00:00Thomas.J.Waldmannurn:md5:7b20def8239e15f8035ab19f897aff31
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/css/ticket.css
File MoinMoin/static/css/ticket.css (right):
https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/css/ticket.css#newcode105
MoinMoin/static/css/ticket.css:105: }
so is what you do special to tickets or is this a general improvement that would apply to the other cases as well?
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/apps/frontend/views.py#newcode338
MoinMoin/apps/frontend/views.py:338: boolticket = True if request.args.get('boolticket') else False
this is a common antipattern. if takes a boolean and you recreate the same boolean again.
how about:
boolticket = bool(request.args.get('boolticket'))
and what does boolticket mean? any better name for this?
is_ticket maybe?
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/static/js/search.js
File MoinMoin/static/js/search.js (right):
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/static/js/search.js#newcode22
MoinMoin/static/js/search.js:22: function ajaxify(query, allrevs, time_sorting, filetypes, boolticket) {
is_ticket?
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ajaxsearch.html
File MoinMoin/templates/ajaxsearch.html (right):
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ajaxsearch.html#newcode93
MoinMoin/templates/ajaxsearch.html:93: {% endif %}
did you just reindent / move all this stuff?
Message from unknown
2015-06-04T11:46:02+00:00vipulurn:md5:1d5c9f8d6b08ed25eb1c83e378dde91c
Message from vipul.sharma20@gmail.com
2015-06-04T11:52:44+00:00vipulurn:md5:9820e420f7bfcbebfa77c3a4e79e2719
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ajaxsearch.html
File MoinMoin/templates/ajaxsearch.html (right):
https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ajaxsearch.html#newcode93
MoinMoin/templates/ajaxsearch.html:93: {% endif %}
On 2015/06/01 20:40:25, Thomas.J.Waldmann wrote:
> did you just reindent / move all this stuff?
Yes I've moved the previous stuff into else block and re-indented
Message from sksaurabhkathpalia@gmail.com
2015-06-17T10:02:12+00:00sksaurabhkathpaliaurn:md5:25f4f4831e1d7be48a14707285fc4345
https://codereview.appspot.com/236490043/diff/140001/MoinMoin/templates/ticket/submit.html
File MoinMoin/templates/ticket/submit.html (right):
https://codereview.appspot.com/236490043/diff/140001/MoinMoin/templates/ticket/submit.html#newcode21
MoinMoin/templates/ticket/submit.html:21: <div id="finalresults">
change id to "moin-ticket-finalresults"
Message from unknown
2015-06-17T15:19:08+00:00vipulurn:md5:bcbec60fc088d73ba2c0792a17ff9873
Message from vipul.sharma20@gmail.com
2015-06-17T15:27:34+00:00vipulurn:md5:6d27665e4ad0117bd89e73b42b895fd9
https://codereview.appspot.com/236490043/diff/140001/MoinMoin/templates/ticket/submit.html
File MoinMoin/templates/ticket/submit.html (right):
https://codereview.appspot.com/236490043/diff/140001/MoinMoin/templates/ticket/submit.html#newcode21
MoinMoin/templates/ticket/submit.html:21: <div id="finalresults">
On 2015/06/17 10:02:12, sksaurabhkathpalia wrote:
> change id to "moin-ticket-finalresults"
search.js uses "#finalresults" as selector for displaying ajax based search results so if I change it to "#moin-ticket-finalresults" I may have to write another function. Is it ok to keep "finalresults" ?
Message from sksaurabhkathpalia@gmail.com
2015-06-18T05:27:43+00:00sksaurabhkathpaliaurn:md5:3b03e153c5c182e968c98d1963b111b6
On 2015/06/17 15:27:34, vipul wrote:
> https://codereview.appspot.com/236490043/diff/140001/MoinMoin/templates/ticket/submit.html
> File MoinMoin/templates/ticket/submit.html (right):
>
> https://codereview.appspot.com/236490043/diff/140001/MoinMoin/templates/ticket/submit.html#newcode21
> MoinMoin/templates/ticket/submit.html:21: <div id="finalresults">
> On 2015/06/17 10:02:12, sksaurabhkathpalia wrote:
> > change id to "moin-ticket-finalresults"
>
> search.js uses "#finalresults" as selector for displaying ajax based search
> results so if I change it to "#moin-ticket-finalresults" I may have to write
> another function. Is it ok to keep "finalresults" ?
Ok then let it be #finalresults
Message from Thomas.J.Waldmann@gmail.com
2015-06-23T15:52:46+00:00Thomas.J.Waldmannurn:md5:34ad21b7f45e76d9721e70872ef22920
looks ok, didn't run the code though.
the long diff could not be reviewed because you maybe changed indentation or moved code around...
https://codereview.appspot.com/236490043/diff/180001/MoinMoin/templates/ajaxsearch.html
File MoinMoin/templates/ajaxsearch.html (right):
https://codereview.appspot.com/236490043/diff/180001/MoinMoin/templates/ajaxsearch.html#newcode95
MoinMoin/templates/ajaxsearch.html:95: {% endif %}
changes like above are impossible to review...
(just look at the diff yourself)
Message from unknown
2015-06-25T08:07:43+00:00vipulurn:md5:8eb4f80a4198d61dcdf061769b833857
Message from vipul.sharma20@gmail.com
2015-06-25T08:07:54+00:00vipulurn:md5:ccc8b7bbab763b46c3f006d8a01d83ea
fixed summary label in the UI