236533 - Text "Additional Comments From ..." can be improved

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement)

Description

[Ulrich Windl]

User-Agent:       Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.4.1) Gecko/20031011
Build Identifier: 

For a change in the bug comments, I receive a mail message saying
"------- Additional Comments From {EMailAdress} {Date} -------"

The same message contains an URL to the bug number.

Can't the text "Additional Comments" be replaced with "Comment
#{CommentNumber}", just as the URL given might refer directly to the comment
being made?

This makes only little difference if you are visiting the bug shortly after
receiving the message, and when no other comments were added. Otherwise it's
easier to follow the comments.

Reproducible: Always
Steps to Reproduce:
1. Make somebody make a comment on your report

Actual Results:  
...
Additional Comments From ...
...

Expected Results:  
...
Comment #1234 From ...
...

Comment 1

[Felix Miata]

I'd like that too.

Comment 2

[Dave Miller [:justdave]]

this sounds very much related to (but not quite a duplicate of) bug 182939.

Comment 3

[Todd Stansell]

Attachment: fix based on 2.18rc2

We implemented this change a long time ago and recently upgraded to 2.18rc2. 
So, this is our change.  Of course, someone will need to review and check in
this change, but I figured I'd submit it for anyone else who wanted it.

Todd

Comment 4

[Todd Stansell]

Attachment: fix against tip

Updated fix for tip.

Todd

Comment 5

[Todd Stansell]

Attachment: v3

This simply fixes a capitalization error and cleans up the sql statement
slightly.

Todd

Comment 6

[Christian Reis]

Comment on attachment 162027 [details] [diff] [review]
v3

Nice. However, this looks somehow wrong. :-)

See, $count is being used in a while loop, and if we iterate more than once,
we'll end up with multiple comments saying they are the same number. I think
the right fix here is to ... hmmm. If we have out-of-order comments, we get
into trouble, don't we? Since the comment doesn't have an ID (nor a primary
key, matter of much ado) how can we know if a certain comment is #1, #2 or #10?

If we're not taking into account out-of-order comments, you still want to get
the number of the first one and increment it as the loop goes around.

Comment 7

[Todd Stansell]

How is this wrong?  Comment numbers are determined by the chronological order in
which they were added to the database, and we're querying for bugs, ordered by
the bug_when column.  $count gets set to the number of comments that exist prior
to the first one we're displaying.  Then, in the while loop, $count gets
incremented as each comment is processed.  I realize there's no strict ordering
of comments and if somehow a bug_when column got changed, the comment numbers
would all get changed.  But, that's not supposed to be possible.  Right?  This
uses the same logic that the web page uses in determining comment numbers.

Todd

Comment 8

[Todd Stansell]

Attachment: v4

After chatting with kiko on IRC, this patch trims down the change to almost
nothing. Part of the code that was in there before wasn't actually being used
so it led more to confusion than anything else.

Todd

Comment 9

[Christian Reis]

Comment on attachment 162046 [details] [diff] [review]
v4

Should be okay, though out-of-order comments will break here too.

Comment 10

[Vlad Dascalu]

Targeting bug to 2.20 since the 2.20 feature freeze was canceled.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 162046 [details] [diff] [review]
v4

>+            $result .= "\n\n------- Additional comment #$count from $who".Param('emailsuffix')."  ".

This text is a header, not a sentence, so "comment" should be capitalized,
although "from" is correctly lowercase.  But this can be taken care of with the
fix for bug 182939.

Comment 12

[Shane H. W. Travis]

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.286; previous revision: 1.285
done

Comment 13

[Frédéric Buclin]

*** Bug 113096 has been marked as a duplicate of this bug. ***