1043737 - Review reply formatting off

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Mark Côté [:mcote]]

See bug 1041253 comment and reply to review in https://reviewboard-dev.allizom.org/r/76/.  Only the first line of the review comment was quoted properly; the rest blends in with the reply.

Comment 1

[Mark Côté [:mcote]]

Attachment: Review for review ID: bz://1043737/mcote

/r/94 - Bug 1043737 - Fix review reply quoting.

Comment 2

[Steven MacLeod [:smacleod]]

::: pylib/rbbz/rbbz/diffs.py
(Diff revision 1)
> -def render_comment_plain(comment, context):
> +def render_comment_plain(comment, context, is_reply):

As we discussed in IRC, this is quoting the original comment from the review, not the last comment in that thread.

While the database does represent the comments such that all of them are replies to the base comment, in practice they are usually a discussion forming a single thread. In this more common case it makes more sense to quote the previous comment in the thread, not the base comment.

I discussed this with m_conley as well and he seemed to agree that it would be best to quote the previous comment.

I threw together a modified render_comment_plain to give you the gist of how I think it should be implemented: https://smacleod.pastebin.mozilla.org/5715296

Note that the pasted code hasn't been executed or tested, but should give you the idea.

::: pylib/rbbz/rbbz/diffs.py
(Diff revision 1)
>          if chunk['change'] == "equal":
>              lines.extend(render_equal_chunk(chunk, parser))

You didn't update how "equal" chunks are quoted. This shouldn't matter though since we're changing how the threading works and will only have single level quoting now.

::: pylib/rbbz/rbbz/diffs.py
(Diff revision 1)
> -                lines.append("> +%s" % parser.unescape(line[5]))
> +                lines.append("%s+%s" % (prefix, parser.unescape(line[5])))

We won't need the prefix changes here anymore, but it might be worth just keeping the var and always setting it to "> "

::: pylib/rbbz/rbbz/diffs.py
(Diff revision 1)
> +    if is_reply:

We don't *need* to pass in is_reply anymore.

Although, in the pasted code I'm executing `parent_comment = comment.reply_to` even when we know that the comment isn't a reply, which might be making a DB query. Maybe we should keep the is_reply variable and only grab `reply_to` in the reply case.

Comment 3

[Mark Côté [:mcote]]

Comment on attachment 8462982 [details]
Review for review ID: bz://1043737/mcote

/r/94 - Bug 1043737 - If review reply, reply to last reply if any others; otherwise, reply to review.

Comment 4

[Mark Côté [:mcote]]

::: pylib/rbbz/rbbz/diffs.py
(Diff revision 1)
> -                lines.append("> +%s" % parser.unescape(line[5]))
> +                lines.append("%s+%s" % (prefix, parser.unescape(line[5])))

> We won't need the prefix changes here anymore, but it might be worth just keeping the var and always setting it to "> "
I thought about that, but then we'd want to change render_equal_chunk() as well, and really it looks a lot messier for no gain.  So I'm going to leave it as it originally was.

Comment 5

[Steven MacLeod [:smacleod]]

Ship It!

Comment 6

[Steven MacLeod [:smacleod]]

Ship It!

Comment 7

[Mark Côté [:mcote]]

http://hg.mozilla.org/hgcustom/version-control-tools/rev/c143ee79f159