675941 - New [diff] suffix not removed when replying to comment

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: User Interface, defect)

Description

[[e-mail]]

Prior to bug 652332, attachment numbers referenced in comments used to be suffixed by [details]. The comment reply function was clever enough to detect this and remove it before quoting. However now the [diff] suffix precedes it the reply function gets confused and completely ignores all of the suffixes. The subsequent comment display then adds a new set of suffixes. In a chain of replies they can therefore accumulate very quickly.

Comment 1

[:glob ✱]

the 'reply to' code has never stripped the suffixes, and i'm not seeing the [view] or [diff] markers being added multiple times.

however i am seeing multiple [review] markers, eg https://bugzilla.mozilla.org/show_bug.cgi?id=652332&mark=14#c14

neil: are you able to provide a sample bug for us to eyeball?

Comment 2

[[e-mail]]

(In reply to comment #1)
> are you able to provide a sample bug for us to eyeball?
Bugzilla has this thing called "search" whereby I was able to locate bug 674229 comment 17 as the worst offender.

Comment 3

[David Lawrence [:dkl]]

(In reply to comment #2)
> (In reply to comment #1)
> > are you able to provide a sample bug for us to eyeball?
> Bugzilla has this thing called "search" whereby I was able to locate bug
> 674229 comment 17 as the worst offender.

What is happening is the text stored in the database which doesn't have the suffixes has them added at render time. The reply to javascript sees only the post rendered text and is copying it into the comment field which is then stored in the database with the suffixes. Then the newly updated show_bug.cgi is then substituting the attachment text with new suffixes and this can go on and on depending on how many times the same text is replied to. Two options for fixes would be:

1. Have the replyto javascript code copy the pre-rendered original text instead in to the comment field. We would need to put the original text into a jscript data structure, or some other hidden dom element. 
2. Fix the code that does the regex text substitutions to not do it again if the text already has a previous suffixes added. It would the somehow need to replace the previous suffixes with proper links so that everything looks proper. 

I tend to think the former would be easier to implement but the latter would be the 'right' thing to do. glob, what are your thoughts?

dkl

Comment 4

[[e-mail]]

Well, you also need to consider the existing code (wherever it was) that used to remove the [details] suffix before the [diff] suffix was added.

Comment 5

[:glob ✱]

(In reply to comment #3)
> 1. Have the replyto javascript code copy the pre-rendered original text
> instead in to the comment field. We would need to put the original text into
> a jscript data structure, or some other hidden dom element. 
> 2. Fix the code that does the regex text substitutions to not do it again if
> the text already has a previous suffixes added. It would the somehow need to
> replace the previous suffixes with proper links so that everything looks
> proper. 
> 
> I tend to think the former would be easier to implement but the latter would
> be the 'right' thing to do. glob, what are your thoughts?

storing two copies of each comment to fix this problem is the right thing to do.

it would be better (and easier) to change the reply-to javascript to strip out any action buttons after attachment links (which is what neil is hinting at in comment 4).

something like:

  var text = getText(text_elem).replace(/(attachment\s+\d+)(\s+\[[^\]]+\])+/gi, '$1');

Comment 6

[:glob ✱]

(In reply to comment #5)
> storing two copies of each comment to fix this problem is the right thing to
> do.

oops, is *not* the right thing to do.  missed an important word from that sentence :)

Comment 7

[David Lawrence [:dkl]]

Attachment: Patch to fix incorrect attachment links in reply text (v1)

Thanks glob. Your one liner worked wonders. Here is a patch that integrates it into the right place to fix the broken links.

dkl

Comment 8

[:glob ✱]

Comment on attachment 551390 [details] [diff] [review]
Patch to fix incorrect attachment links in reply text (v1)

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

r=glob

Comment 9

[David Lawrence [:dkl]]

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

dkl