already existing ticket suggestions

Ajax based duplicate ticket suggestions

Used existing implementation of /+search view to implement Ajax based searching of ticket items. Used WIDGET_SEARCH widget in the summary input field, which acts as a search field for ticket items. Added code in search.js to filter only ticket items in the search result. ajaxsearch.html contains an if-else block to check whether it is ticket create(/+modify) view or /+search view. Therefore, the original code is re-indented and pushed into the else block so the diff for ajaxsearch.html looks a bit ugly.

[sksaurabhkathpalia, 2015-05-27 11:45:18]

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#...
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...
File MoinMoin/templates/ticket/submit.html (right):

https://codereview.appspot.com/236490043/diff/20001/MoinMoin/templates/ticket...
MoinMoin/templates/ticket/submit.html:23: </p>
Remove TAB before this <p> tag

[sksaurabhkathpalia, 2015-05-27 12:24:24]

https://codereview.appspot.com/236490043/diff/20001/MoinMoin/apps/frontend/vi...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/236490043/diff/20001/MoinMoin/apps/frontend/vi...
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

[vipul, 2015-05-27 12:35:51]

https://codereview.appspot.com/236490043/diff/20001/MoinMoin/apps/frontend/vi...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/236490043/diff/20001/MoinMoin/apps/frontend/vi...
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#...
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?

[Thomas.J.Waldmann, 2015-05-29 06:57:51]

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/apps/frontend/vi...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/apps/frontend/vi...
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/vi...
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....
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#...
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/ticke...
File MoinMoin/static/css/ticket.css (right):

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/css/ticke...
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...
MoinMoin/static/js/search.js:22: }));
this looks same as above

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/js/search...
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....
File MoinMoin/templates/forms.html (right):

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/forms....
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....
MoinMoin/templates/forms.html:157: {% endmacro %}
almost same as above

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket...
File MoinMoin/templates/ticket/base.html (right):

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket...
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...
File MoinMoin/templates/ticket_ajaxsearch.html (right):

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket...
MoinMoin/templates/ticket_ajaxsearch.html:2: {% if results is defined %}
wat?

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket...
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?

[vipul, 2015-05-29 12:58:31]

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/apps/frontend/vi...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/apps/frontend/vi...
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/vi...
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....
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#...
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/ticke...
File MoinMoin/static/css/ticket.css (right):

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/css/ticke...
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....
File MoinMoin/templates/forms.html (right):

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/forms....
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...
File MoinMoin/templates/ticket_ajaxsearch.html (right):

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/templates/ticket...
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...
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

[vipul, 2015-05-29 20:42:51]

Made changes as suggested in the previous CR. Please review the new patch set.

https://codereview.appspot.com/236490043/diff/120001/MoinMoin/static/css/tick...
File MoinMoin/static/css/ticket.css (right):

https://codereview.appspot.com/236490043/diff/120001/MoinMoin/static/css/tick...
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/searc...
File MoinMoin/static/js/search.js (right):

https://codereview.appspot.com/236490043/diff/120001/MoinMoin/static/js/searc...
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/ajaxs...
File MoinMoin/templates/ajaxsearch.html (right):

https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ajaxs...
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/ticke...
File MoinMoin/templates/ticket/base.html (right):

https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ticke...
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.

[Thomas.J.Waldmann, 2015-06-01 20:40:25]

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/css/ticke...
File MoinMoin/static/css/ticket.css (right):

https://codereview.appspot.com/236490043/diff/80001/MoinMoin/static/css/ticke...
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/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/236490043/diff/120001/MoinMoin/apps/frontend/v...
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/searc...
File MoinMoin/static/js/search.js (right):

https://codereview.appspot.com/236490043/diff/120001/MoinMoin/static/js/searc...
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/ajaxs...
File MoinMoin/templates/ajaxsearch.html (right):

https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ajaxs...
MoinMoin/templates/ajaxsearch.html:93: {% endif %}
did you just reindent / move all this stuff?

[vipul, 2015-06-04 11:52:44]

https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ajaxs...
File MoinMoin/templates/ajaxsearch.html (right):

https://codereview.appspot.com/236490043/diff/120001/MoinMoin/templates/ajaxs...
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

[sksaurabhkathpalia, 2015-06-17 10:02:12]

https://codereview.appspot.com/236490043/diff/140001/MoinMoin/templates/ticke...
File MoinMoin/templates/ticket/submit.html (right):

https://codereview.appspot.com/236490043/diff/140001/MoinMoin/templates/ticke...
MoinMoin/templates/ticket/submit.html:21: <div id="finalresults">
change id to "moin-ticket-finalresults"

[vipul, 2015-06-17 15:27:34]

https://codereview.appspot.com/236490043/diff/140001/MoinMoin/templates/ticke...
File MoinMoin/templates/ticket/submit.html (right):

https://codereview.appspot.com/236490043/diff/140001/MoinMoin/templates/ticke...
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" ?

[sksaurabhkathpalia, 2015-06-18 05:27:43]

On 2015/06/17 15:27:34, vipul wrote:
>
https://codereview.appspot.com/236490043/diff/140001/MoinMoin/templates/ticke...
> File MoinMoin/templates/ticket/submit.html (right):
> 
>
https://codereview.appspot.com/236490043/diff/140001/MoinMoin/templates/ticke...
> 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

[Thomas.J.Waldmann, 2015-06-23 15:52:46]

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/ajaxs...
File MoinMoin/templates/ajaxsearch.html (right):

https://codereview.appspot.com/236490043/diff/180001/MoinMoin/templates/ajaxs...
MoinMoin/templates/ajaxsearch.html:95: {% endif %}
changes like above are impossible to review...

(just look at the diff yourself)

[vipul, 2015-06-25 08:07:54]

fixed summary label in the UI