653634 - Change the comment reply header to include the name of the person who has commented

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[(no longer active)]

This was originally suggested by Asa for Bugzilla Tweaks: <https://bitbucket.org/ehsan/bugzilla-tweaks/issue/7/add-name-of-commenter-in-the-reply-to>

We should change the header to something like:

In reply to <username> from comment #

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to add commentors name to the reply header (v1)

We should definitely upstream this if possible but we can go ahead and add to BMO if desire is enough. Please review.

dkl

Comment 2

[:glob ✱]

Comment on attachment 548607 [details] [diff] [review]
Patch to add commentors name to the reply header (v1)

Review of attachment 548607 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

you do end up with some oddness when people put away messages in their name, but there's no way to avoid that:

(In reply to Marcia Knous [:marcia] afk 4/7 - 4/22 from comment #6)

Comment 3

[David Lawrence [:dkl]]

(In reply to comment #2)
> you do end up with some oddness when people put away messages in their name,
> but there's no way to avoid that:
> 
> (In reply to Marcia Knous [:marcia] afk 4/7 - 4/22 from comment #6)

I considered different scenarios and thought using real name would be best but it can get confusing as you mentioned. The other thing I thought about was using comment.author.login FILTER email so it would like this:

Logged in users:
(In reply to [e-mail] from comment #6)

Logged out users:
(In reply to dkl from comment #6)

Do you think that is better or stick with real names?

Another thing I just thought of is some users such as the default qa contacts for BMO components lack real names. The way I have it coded now if a real name is empty, then it would look like the old way of doing reply text. Should I instead fall back to the email filtered login name?

dkl

Comment 4

[:glob ✱]

(In reply to comment #3)
> Do you think that is better or stick with real names?

i think real names is preferable; how about

  user.name || user.nick

Comment 5

[David Lawrence [:dkl]]

Moving upsteam to try and get accepted. I can update the whiteboard to note if this is checked in to BMO or not. Also I will attach a new patch for review that uses user.nick.

dkl

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch to add commentors name to the reply header (v2)

New patch that falls back to comment.author.nick if comment.author.name is empty.

dkl

Comment 7

[:glob ✱]

Comment on attachment 548807 [details] [diff] [review]
Patch to add commentors name to the reply header (v2)

Review of attachment 548807 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

Comment 8

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bmo/4.0
modified template/en/default/bug/comments.html.tmpl
modified template/en/default
/bug/edit.html.tmpl                                                               Committed revision 7816.

dkl

Comment 9

[Max Kanat-Alexander]

Comment on attachment 548807 [details] [diff] [review]
Patch to add commentors name to the reply header (v2)

Hmmm. Instead of this, would it be worthwhile to add a reply_to column in longdescs and actually track this so we could render the text when the comment is rendered? I do sometimes reply to multiple comments in one comment though.

Comment 10

[Ed Morley [:emorley]]

The implementation that has just been added, results in people's email addresses being shown in full as part of replies, even when not logged in. This is going to result in me receiving spam, due to people quoting my comments - with no way of me preventing it.

Could I propose one of the following:
1) Only showing display name, unless display name is blank.
2) Showing display name + email as now; except obfuscating the email address (like is done already for the CC field and/or other instances of people's email addresses) if not logged in.

Thanks! :-)

Comment 11

[David Lawrence [:dkl]]

(In reply to Ed Morley [:edmorley] from comment #10)
> The implementation that has just been added, results in people's email
> addresses being shown in full as part of replies, even when not logged in.
> This is going to result in me receiving spam, due to people quoting my
> comments - with no way of me preventing it.

The way I implemented it, I only show the realname and if the realname is blank I only show the first part of the user's login. Which is only the portion before the '@' character. I do this whether the user is logged in or not so the text being pasted into the new comment should not contain any email addresses in the reply to header. Do you have a specific example of what you are reporting so I can see if there is a bug in my implementation?

Thanks
dkl

Comment 12

[Ed Morley [:emorley]]

Ah yes, see it worked for your comment above :-)

The example I was thinking of was https://bugzilla.mozilla.org/show_bug.cgi?id=676189#c13 - hope that helps!

Comment 13

[:glob ✱]

(In reply to Ed Morley [:edmorley] from comment #12)
> The example I was thinking of was
> https://bugzilla.mozilla.org/show_bug.cgi?id=676189#c13 - hope that helps!

khuey has his email address as part of his "real name".

Comment 14

[Ed Morley [:emorley]]

Ah sorry, missed that - my bad!

Comment 15

[Frédéric Buclin]

Comment on attachment 548807 [details] [diff] [review]
Patch to add commentors name to the reply header (v2)

r-, see bug 677951.

Comment 16

[David Lawrence [:dkl]]

Attachment: Patch to add commentors name to the reply header (v3)

New patch that fixes escaping issue in addReplyLink(). 

dkl

Comment 18

[Dave Garrett]

This is slightly off Bugzilla etiquette, but thank you very much for implementing this. It really improves the readability of email notifications, in particular. :)

Comment 19

[Frédéric Buclin]

Comment on attachment 552151 [details] [diff] [review]
Patch to add commentors name to the reply header (v3)

>=== modified file 'template/en/default/bug/comments.html.tmpl'

>-              addReplyLink([% count %], [% comment.id %]);
>+              addReplyLink([% count %], [% comment.id %], '[% comment.author.name || comment.author.nick FILTER js %]');

As the reviewer of bug 225731 and being involved in the discussion of the implementation of the "quoted_reply" user pref, I want to change the meaning of quoted_reply=off. The "Reply" link should always be displayed, independently of the user pref and independently of JS being turned on or off. If either JS or quoted_reply is turned off, then the "Reply" link is simply a link pointing to the "New Comment" textarea. This lets you kill the addReplyLink() JS function and its ugly document.write() entirely, and avoids some of the XSS problems discussed elsewhere. To make customizations easier, please add a class="bz_reply_link" to this anchor (in case some installations really want to hide this link, always).



>=== modified file 'template/en/default/bug/edit.html.tmpl'

>+        var escaped_name = name.replace("\'", "\\\'", "g");

If you need to do this again here, then the JS filter is wrong. | FILTER js | already escapes single quotes, see the js filter in Template.pm. My proposal above will make things easier.

Comment 20

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #19)
> As the reviewer of bug 225731 and being involved in the discussion of the
> implementation of the "quoted_reply" user pref, I want to change the meaning
> of quoted_reply=off. The "Reply" link should always be displayed,
> independently of the user pref and independently of JS being turned on or
> off. If either JS or quoted_reply is turned off, then the "Reply" link is
> simply a link pointing to the "New Comment" textarea. This lets you kill the
> addReplyLink() JS function and its ugly document.write() entirely, and
> avoids some of the XSS problems discussed elsewhere. To make customizations
> easier, please add a class="bz_reply_link" to this anchor (in case some
> installations really want to hide this link, always).

Thanks. Will look into doing something like what you are suggesting.

> >=== modified file 'template/en/default/bug/edit.html.tmpl'
> 
> >+        var escaped_name = name.replace("\'", "\\\'", "g");
> 
> If you need to do this again here, then the JS filter is wrong. | FILTER js
> | already escapes single quotes, see the js filter in Template.pm. My
> proposal above will make things easier.

I realize that. But unfortunately in this case, document.write was stripping out the escape chars which was converting back to just a single quote. By escaping the escape chars themselves, I was able to get the behavior I was looking for at the time.

dkl

Comment 21

[David Lawrence [:dkl]]

Attachment: Patch to add commentors name to the reply header (v4)

New patch with your suggested changes. Removed the need for the addReplyLink function and as a result also no longer use document.write. This removed the need to re-escape the escape chars as before. Also I added a <noscript> which shows a [reply] link even when javascript is disabled.

dkl

Comment 22

[Frédéric Buclin]

Comment on attachment 556995 [details] [diff] [review]
Patch to add commentors name to the reply header (v4)

>=== modified file 'template/en/default/bug/comments.html.tmpl'

>+              [% IF user.settings.quote_replies.value != 'off' %]
>+                YAHOO.util.Event.onDOMReady(function () {
>+                  var replyLink = document.createElement('a');
>+                  replyLink.appendChild(document.createTextNode('[reply]'));
>+                  replyLink.setAttribute('href', '#add_comment');
>+                  YAHOO.util.Dom.addClass(replyLink, 'bz_reply_link');
>+                  YAHOO.util.Event.addListener(replyLink, 'click', function(e) {
>+                    replyToComment('[% count FILTER js %]', '[% comment.id FILTER js %]', 
>+                                   '[% comment.author.name || comment.author.nick FILTER js %]');
>+                  });
>+                  var actions = YAHOO.util.Dom.getElementsByClassName('bz_comment_actions')[[% count FILTER js %]];
>+                  actions.insertBefore(replyLink, actions.firstChild);
>+                });
>+              [% END %]

I don't understand why you need all this complexity. First of all, users with the user pref turned off will see no link at all, despite it should behave the same way as for users with JS turned off, i.e. simply a link pointing to the new comment textarea. Then, I think all this code can be replaced by a single <a href="..." onclick="...">[reply]</a>

Comment 23

[David Lawrence [:dkl]]

Attachment: Patch to add commentors name to the reply header (v6)

Ugh. Yeah it was overly complex. I had one way in my head and didn't think about just adding it as onclick to the <a> tag. Anyway here is a much simpler patch.

dkl

Comment 24

[Frédéric Buclin]

Comment on attachment 557024 [details] [diff] [review]
Patch to add commentors name to the reply header (v6)

>=== modified file 'template/en/default/bug/comments.html.tmpl'

>+                onclick="replyToComment('[% count FILTER js %]', '[% comment.id FILTER js %]',  

count and comment.id are both already used as is in this template, unfiltered, including in others JS methods. As they are both integers, they are safe and you can remove FILTER js for them (and filterexceptions.pl already has them listed anyway).
Also, please remove trailing whitespaces on this line.


>=== modified file 'template/en/default/bug/edit.html.tmpl'

>+  function replyToComment(id, real_id, name) {
>+      var prefix = "(In reply to ";
>+      if (name) {
>+          prefix = prefix + name + " from";
>+      }
>+      prefix = prefix + " comment #" + id + ")\n";

This function is only called from bug/comments.html.tmpl and the name is always defined. So |if (name)| is always true and this check is useless. This will let you defined prefix in a single line.


r=LpSolit with both comments fixed. Please attach an updated patch.

Comment 25

[Frédéric Buclin]

Holding approval till the updated patch is attached (just to make sure).

Comment 26

[David Lawrence [:dkl]]

Attachment: Patch to add commentors name to the reply header (v7)

Thanks. New patch with the changes you mentioned.

dkl

Comment 27

[Frédéric Buclin]

Comment on attachment 557089 [details] [diff] [review]
Patch to add commentors name to the reply header (v7)

>=== modified file 'template/en/default/bug/comments.html.tmpl'

>+    [% IF user.is_insider %]
>+      if (document.getElementById('isprivate_' + real_id).checked) {
>+          document.getElementById('newcommentprivacy').checked = 'checked';
>+          updateCommentTagControl(document.getElementById('newcommentprivacy'), 'comment'); 
>+      }
>+    [% END %]

Incorrect indentation for this block compared to surrounding code.


>+  if (typeof Node == 'undefined') {
>+      /* MSIE doesn't define Node, so provide a compatibility object */
>+      window.Node = {
>+          TEXT_NODE: 3,
>+          ENTITY_REFERENCE_NODE: 5
>+      };
>+  }
>+
>+  /* Concatenates all text from element's childNodes. This is used
>+   * instead of innerHTML because we want the actual text (and
>+   * innerText is non-standard).
>+   */
>+  function getText(element) {
>+      var child, text = "";
>+      for (var i=0; i < element.childNodes.length; i++) {
>+          child = element.childNodes[i];
>+          var type = child.nodeType;
>+          if (type == Node.TEXT_NODE || type == Node.ENTITY_REFERENCE_NODE) {
>+              text += child.nodeValue;
>+          } else {
>+              /* recurse into nodes of other types */
>+              text += getText(child);
>+          }
>+      }
>+      return text;
>+  } 

As this code is pure JS code (no TT code in it), it should go into js/comments.js, not here.


r=LpSolit with these two comments fixed.

Comment 28

[Frédéric Buclin]

Do not forget the fixes on checkin mentioned in my previous comment.

Comment 29

[David Lawrence [:dkl]]

Thanks. Also added back the return false that was missing and causing the focus of the textarea to not work properly. Will attach the final patch to this bug as well.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified js/comments.js
modified template/en/default/bug/comments.html.tmpl
modified template/en/default/bug/edit.html.tmpl
Committed revision 7948.

dkl

Comment 30

[David Lawrence [:dkl]]

Attachment: Patch to add commentors name to the reply header (v8)

Moving over previous r+.