ticket comments

Supports markdown syntax, and automatically posts a comment if changes are made in the metadata of ticket with a proper message. Re-build the index before running this patch as new fields have been added for whoosh indexing.

[Thomas.J.Waldmann, 2015-07-14 18:09:20]

some hints, esp. about how to build the comments tree with just 1 whoosh query

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

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/apps/frontend/vi...
MoinMoin/apps/frontend/views.py:2393: @frontend.route('/+comment',
defaults=dict(item_name=u''), methods=['GET', 'POST'])
so if you accept both get and post here, where is the difference in handling?

GET requests must never ever modify state (e.g. of wiki storage), use POST for
that. Otherwise sooner or later a crawler will find the GET URLs and make it a
bad day for the wiki owner, triggering all sorts of random state modifications.

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/constants/keys.py
File MoinMoin/constants/keys.py (right):

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/constants/keys.p...
MoinMoin/constants/keys.py:137: ELEMENT = u"element"
means?

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/items/ticket.py#...
MoinMoin/items/ticket.py:190: 
these 2 methods should be replaced by:

1 whoosh query, getting ALL revs that refer to same (ticket) itemid.

then compute the SET of all itemids you have in the result.

sort the revs list by time (newest first, using whoosh sortedby or if that does
not work python sorted()).

process the revs in the sorted list, starting from the beginning.
if you have rev's itemid in the set, use that rev for building the tree (and
remove the itemid from the set). do that until the set is empty. that way, you
will only render the latest revisions of each item.

use the tree for rendering.

maybe you won't need that ELEMENT metadata also.

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/storage/middlewa...
File MoinMoin/storage/middleware/indexing.py (right):

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/storage/middlewa...
MoinMoin/storage/middleware/indexing.py:331: # refers to anoter item using
itemid
typo

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/templates/ticket...
File MoinMoin/templates/ticket/modify.html (right):

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/templates/ticket...
MoinMoin/templates/ticket/modify.html:54: {{ render_comment(comments) }}
indentation

[vipul, 2015-07-17 12:28:56]

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

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/apps/frontend/vi...
MoinMoin/apps/frontend/views.py:2393: @frontend.route('/+comment',
defaults=dict(item_name=u''), methods=['GET', 'POST'])
On 2015/07/14 18:09:20, Thomas.J.Waldmann wrote:
> so if you accept both get and post here, where is the difference in handling?
> 
> GET requests must never ever modify state (e.g. of wiki storage), use POST for
> that. Otherwise sooner or later a crawler will find the GET URLs and make it a
> bad day for the wiki owner, triggering all sorts of random state
modifications.

Acknowledged.

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/items/ticket.py#...
MoinMoin/items/ticket.py:190: 
On 2015/07/14 18:09:20, Thomas.J.Waldmann wrote:
> these 2 methods should be replaced by:
> 
> 1 whoosh query, getting ALL revs that refer to same (ticket) itemid.
> 
> then compute the SET of all itemids you have in the result.
> 
> sort the revs list by time (newest first, using whoosh sortedby or if that
does
> not work python sorted()).
> 
> process the revs in the sorted list, starting from the beginning.
> if you have rev's itemid in the set, use that rev for building the tree (and
> remove the itemid from the set). do that until the set is empty. that way, you
> will only render the latest revisions of each item.
> 
> use the tree for rendering.
> 
> maybe you won't need that ELEMENT metadata also.

working on the suggestion

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/storage/middlewa...
File MoinMoin/storage/middleware/indexing.py (right):

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/storage/middlewa...
MoinMoin/storage/middleware/indexing.py:331: # refers to anoter item using
itemid
On 2015/07/14 18:09:20, Thomas.J.Waldmann wrote:
> typo

Acknowledged.

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/templates/ticket...
File MoinMoin/templates/ticket/modify.html (right):

https://codereview.appspot.com/256840043/diff/20001/MoinMoin/templates/ticket...
MoinMoin/templates/ticket/modify.html:54: {{ render_comment(comments) }}
On 2015/07/14 18:09:20, Thomas.J.Waldmann wrote:
> indentation

Acknowledged.

[sksaurabhkathpalia, 2015-08-01 21:31:18]

On 2015/07/17 12:28:56, vipul wrote:
>
https://codereview.appspot.com/256840043/diff/20001/MoinMoin/apps/frontend/vi...
> File MoinMoin/apps/frontend/views.py (right):
> 
>
https://codereview.appspot.com/256840043/diff/20001/MoinMoin/apps/frontend/vi...
> MoinMoin/apps/frontend/views.py:2393: @frontend.route('/+comment',
> defaults=dict(item_name=u''), methods=['GET', 'POST'])
> On 2015/07/14 18:09:20, Thomas.J.Waldmann wrote:
> > so if you accept both get and post here, where is the difference in
handling?
> > 
> > GET requests must never ever modify state (e.g. of wiki storage), use POST
for
> > that. Otherwise sooner or later a crawler will find the GET URLs and make it
a
> > bad day for the wiki owner, triggering all sorts of random state
> modifications.
> 
> Acknowledged.
> 
> https://codereview.appspot.com/256840043/diff/20001/MoinMoin/items/ticket.py
> File MoinMoin/items/ticket.py (right):
> 
>
https://codereview.appspot.com/256840043/diff/20001/MoinMoin/items/ticket.py#...
> MoinMoin/items/ticket.py:190: 
> On 2015/07/14 18:09:20, Thomas.J.Waldmann wrote:
> > these 2 methods should be replaced by:
> > 
> > 1 whoosh query, getting ALL revs that refer to same (ticket) itemid.
> > 
> > then compute the SET of all itemids you have in the result.
> > 
> > sort the revs list by time (newest first, using whoosh sortedby or if that
> does
> > not work python sorted()).
> > 
> > process the revs in the sorted list, starting from the beginning.
> > if you have rev's itemid in the set, use that rev for building the tree (and
> > remove the itemid from the set). do that until the set is empty. that way,
you
> > will only render the latest revisions of each item.
> > 
> > use the tree for rendering.
> > 
> > maybe you won't need that ELEMENT metadata also.
> 
> working on the suggestion
> 
>
https://codereview.appspot.com/256840043/diff/20001/MoinMoin/storage/middlewa...
> File MoinMoin/storage/middleware/indexing.py (right):
> 
>
https://codereview.appspot.com/256840043/diff/20001/MoinMoin/storage/middlewa...
> MoinMoin/storage/middleware/indexing.py:331: # refers to anoter item using
> itemid
> On 2015/07/14 18:09:20, Thomas.J.Waldmann wrote:
> > typo
> 
> Acknowledged.
> 
>
https://codereview.appspot.com/256840043/diff/20001/MoinMoin/templates/ticket...
> File MoinMoin/templates/ticket/modify.html (right):
> 
>
https://codereview.appspot.com/256840043/diff/20001/MoinMoin/templates/ticket...
> MoinMoin/templates/ticket/modify.html:54: {{ render_comment(comments) }}
> On 2015/07/14 18:09:20, Thomas.J.Waldmann wrote:
> > indentation
> 
> Acknowledged.

You haven't updated the patch. It doesn't work.

[vipul, 2015-08-02 20:17:38]

https://codereview.appspot.com/256840043/diff/80001/MoinMoin/static/js/ticket...
File MoinMoin/static/js/tickets.js (right):

https://codereview.appspot.com/256840043/diff/80001/MoinMoin/static/js/ticket...
MoinMoin/static/js/tickets.js:46: type: "GET",
I need to do it using a POST request. Currently I am adding a textarea and as
one clicks on save, a GET request is made and few fields are sent as args. How
should I do it using POST request suggestions ?

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

https://codereview.appspot.com/256840043/diff/80001/MoinMoin/templates/ticket...
MoinMoin/templates/ticket/base.html:60: posted by {{ comment.meta['author'] }}
on {{ comment.meta['mtime'] }}
{{ comment.meta['mtime'] }} renders int object but the meta shows a datetime
object when I try using strftime() it shows the error: 'int object' has no
attribute 'strftime'. So clearly it renders int. How should I render the correct
datetime here? the meta is like {u'mtime': datetime.datetime(2015, 8, 2, 19, 29,
4)} but when I do meta['mtime'] it renders as: 1438543744 and also strftime()
displays error as mentioned before

[Thomas.J.Waldmann, 2015-08-11 15:41:18]

https://codereview.appspot.com/256840043/diff/80001/MoinMoin/static/js/ticket...
File MoinMoin/static/js/tickets.js (right):

https://codereview.appspot.com/256840043/diff/80001/MoinMoin/static/js/ticket...
MoinMoin/static/js/tickets.js:46: type: "GET",
On 2015/08/02 20:17:37, vipul wrote:
> I need to do it using a POST request. Currently I am adding a textarea and as
> one clicks on save, a GET request is made and few fields are sent as args. How
> should I do it using POST request suggestions ?

$.ajax with type POST or $.post (IIRC). post also support a "form", similar as
url args for get.

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

https://codereview.appspot.com/256840043/diff/80001/MoinMoin/templates/ticket...
MoinMoin/templates/ticket/base.html:60: posted by {{ comment.meta['author'] }}
on {{ comment.meta['mtime'] }}
not sure why you see different behaviour, but the int might be a unix timestamp
(seconds since 1970).
your can create a formatted string or a datetime object from such a timestamp.
see datetime or time lib help.

[vipul, 2015-08-11 15:52:31]

On 2015/08/11 15:41:18, Thomas.J.Waldmann wrote:
>
https://codereview.appspot.com/256840043/diff/80001/MoinMoin/static/js/ticket...
> File MoinMoin/static/js/tickets.js (right):
> 
>
https://codereview.appspot.com/256840043/diff/80001/MoinMoin/static/js/ticket...
> MoinMoin/static/js/tickets.js:46: type: "GET",
> On 2015/08/02 20:17:37, vipul wrote:
> > I need to do it using a POST request. Currently I am adding a textarea and
as
> > one clicks on save, a GET request is made and few fields are sent as args.
How
> > should I do it using POST request suggestions ?
> 
> $.ajax with type POST or $.post (IIRC). post also support a "form", similar as
> url args for get.
> 
>
https://codereview.appspot.com/256840043/diff/80001/MoinMoin/templates/ticket...
> File MoinMoin/templates/ticket/base.html (right):
> 
>
https://codereview.appspot.com/256840043/diff/80001/MoinMoin/templates/ticket...
> MoinMoin/templates/ticket/base.html:60: posted by {{ comment.meta['author'] }}
> on {{ comment.meta['mtime'] }}
> not sure why you see different behaviour, but the int might be a unix
timestamp
> (seconds since 1970).
> your can create a formatted string or a datetime object from such a timestamp.
> see datetime or time lib help.

Thanks for the info, but I've already updated the patch after that twice in
which I've fixed this issue

[Thomas.J.Waldmann, 2015-08-11 16:23:09]

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/apps/frontend/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:23: import markdown
did you look at our own parsers first (see converter/*)?

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:2403: item.modify({},
data=markdown.markdown(data), element=u'comment',
contenttype_guessed=u'text/x.moin.wiki;charset=utf-8', \
why is it markdown AND wiki here?

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/items/ticket.py...
MoinMoin/items/ticket.py:241: 
you do not read your diffs to spot accidental changes?

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/templates/ticke...
File MoinMoin/templates/ticket/base.html (right):

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/templates/ticke...
MoinMoin/templates/ticket/base.html:43: <div class="comment-content">
indentation?

[vipul, 2015-08-12 06:59:05]

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/apps/frontend/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:23: import markdown
On 2015/08/11 16:23:08, Thomas.J.Waldmann wrote:
> did you look at our own parsers first (see converter/*)?

ok I will try how it works

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:2403: item.modify({},
data=markdown.markdown(data), element=u'comment',
contenttype_guessed=u'text/x.moin.wiki;charset=utf-8', \
On 2015/08/11 16:23:08, Thomas.J.Waldmann wrote:
> why is it markdown AND wiki here?

I used u'text/x-markdown;charset=utf-8' earlier but it didn't render markdown
formatted content. What should I use here ?

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/items/ticket.py...
MoinMoin/items/ticket.py:241: 
On 2015/08/11 16:23:08, Thomas.J.Waldmann wrote:
> you do not read your diffs to spot accidental changes?

ah, how I missed that! I will correct it

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/templates/ticke...
File MoinMoin/templates/ticket/base.html (right):

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/templates/ticke...
MoinMoin/templates/ticket/base.html:43: <div class="comment-content">
On 2015/08/11 16:23:08, Thomas.J.Waldmann wrote:
> indentation?

yes, indentation needs to be fixed. I will correct this also

[Thomas.J.Waldmann, 2015-08-18 16:07:19]

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/apps/frontend/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/256840043/diff/120001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:2403: item.modify({},
data=markdown.markdown(data), element=u'comment',
contenttype_guessed=u'text/x.moin.wiki;charset=utf-8', \
if you just create a normal new wiki page, you can choose from some markup types
and you see in their metadata what is put there (and what works / what not).

if you use same and it does not work, it might be just because you do not invoke
rendering maybe?

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/items/ticket.py...
MoinMoin/items/ticket.py:191: comments[rev] = []
does that even work?
using a whole rev as dict key seems rather brave...
likely you could also just use an itemid as key here.
you could also name it childs (you already use "parent"), right?

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/items/ticket.py...
MoinMoin/items/ticket.py:194: roots.append(rev)
how many roots will you get?
doesn't every comment have a reply_to entry (and if it only point to the ticket
item itself as every "toplevel" comment is a reply to the ticket text)?

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/items/ticket.py...
MoinMoin/items/ticket.py:196: parent = lookup[rev.meta['reply_to']]
maybe reduce duplication by putting
reply_to = rev.meta['reply_to']
before the if/else block.

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/items/ticket.py...
MoinMoin/items/ticket.py:200: comments[parent] = [rev]
btw, look up what defaultdict does and how you can make this code simpler by
using it with a default of [].

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/items/ticket.py...
MoinMoin/items/ticket.py:205: if comments[root]:
the negative case, is it just an empty list?
in that case you can maybe simplify this, there is no harm in iterating over an
empty list.

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/templates/ticke...
File MoinMoin/templates/ticket/base.html (right):

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/templates/ticke...
MoinMoin/templates/ticket/base.html:42: {% macro print_comment(comment) %}
maybe use render_comment rather.
so in case someone greps for stray debug-prints, he does not find your stuff.

[vipul, 2015-08-19 06:18:33]

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/items/ticket.py...
MoinMoin/items/ticket.py:191: comments[rev] = []
On 2015/08/18 16:07:19, Thomas.J.Waldmann wrote:
> does that even work?
> using a whole rev as dict key seems rather brave...
> likely you could also just use an itemid as key here.
> you could also name it childs (you already use "parent"), right?

yes this works. We store dictionary "values" as revs and while building comment
tree we use those values i.e. the revs as "keys" to find its child comments
therefore the keys and values must be of same type hence, they need to be revs

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/items/ticket.py...
MoinMoin/items/ticket.py:194: roots.append(rev)
On 2015/08/18 16:07:19, Thomas.J.Waldmann wrote:
> how many roots will you get?
> doesn't every comment have a reply_to entry (and if it only point to the
ticket
> item itself as every "toplevel" comment is a reply to the ticket text)?

"roots" contains the first level comments which are the comments which are NOT
the replies of any other comment. There can be more than one 1st level comments
which may contain a series of comment/reply thread for that comment. Therefore,
even if we assign the "reply_to" field of 1st level comment as the "itemid" of
the ticket we will still have multiple roots i.e. multiple 1st level comments.
Therefore, keeping "reply_to" field for 1st level comments as itemid or empty
will lead to same result.

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/items/ticket.py...
MoinMoin/items/ticket.py:196: parent = lookup[rev.meta['reply_to']]
On 2015/08/18 16:07:19, Thomas.J.Waldmann wrote:
> maybe reduce duplication by putting
> reply_to = rev.meta['reply_to']
> before the if/else block.

ok will update this

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/templates/ticke...
File MoinMoin/templates/ticket/base.html (right):

https://codereview.appspot.com/256840043/diff/260001/MoinMoin/templates/ticke...
MoinMoin/templates/ticket/base.html:42: {% macro print_comment(comment) %}
On 2015/08/18 16:07:19, Thomas.J.Waldmann wrote:
> maybe use render_comment rather.
> so in case someone greps for stray debug-prints, he does not find your stuff.

yes you are right. I will update this