Last Comment Bug 509152 - line breaks erroneously present in bug xml
: line breaks erroneously present in bug xml
Status: RESOLVED FIXED
: regression
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 3.4.1
: All All
: P1 normal (vote)
: Bugzilla 3.4
Assigned To: Thomas Ehrnhoefer
: default-qa
Mentors:
Depends on: 457657
Blocks: bz-clients
  Show dependency treegraph
 
Reported: 2009-08-07 17:58 PDT by Robert Elves
Modified: 2010-02-28 10:49 PST (History)
8 users (show)
LpSolit: approval+
LpSolit: approval3.4+
mkanat: blocking3.4.3+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
suggested patch (1.62 KB, patch)
2009-08-10 15:23 PDT, Thomas Ehrnhoefer
mkanat: review-
Details | Diff | Review
patch v2 (2.27 KB, text/plain)
2009-08-11 11:10 PDT, Thomas Ehrnhoefer
no flags Details
patch v3 (2.27 KB, patch)
2009-08-11 11:15 PDT, Thomas Ehrnhoefer
mkanat: review-
Details | Diff | Review
patch v.4 (2.35 KB, patch)
2009-09-30 10:59 PDT, Thomas Ehrnhoefer
LpSolit: review+
Details | Diff | Review

Description Robert Elves 2009-08-07 17:58:25 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: 3.4.1

Upon upgrading to 3.4.1, the text returned for description is different, resulting in false incoming notification in Mylyn (http://eclipse.org/mylyn).  This difference is due to line breaks being present in the xml for the description (possibly other fields as well).  There were no line breaks present upon original submit (at look at the raw data in the database also reveals no additional line breaks).  It appears as though the line wrapping being applied for html presentation is being applied to the xml output as well.  

Reproducible: Always

Steps to Reproduce:
1.Post a long line as a comment to a 3.2 repository
2.Upgrade the repository to 3.4.1
3.Open the task in Mylyn and see false incoming on description due to wrapping in xml


Expected Results:  
Presentation wrapping should not be applied to XML output.
Comment 1 Frédéric Buclin 2009-08-09 05:20:46 PDT
Which fields are affected?
Comment 2 Max Kanat-Alexander 2009-08-09 12:52:43 PDT
Yes, this is happening because Bugzilla::Bug uses format_comment, which wraps text. format_comment should not wrap the text, I think.
Comment 3 Thomas Ehrnhoefer 2009-08-10 15:23:10 PDT
Created attachment 393634 [details] [diff] [review]
suggested patch

@Frederic: only comments are affected

@Max: Thanks for your hint. Based on that I tried this patch and it works fine for us. Comments are not getting wrapped in the XML, but still in the WebUI. Not sure though if that is the best way to do it.
Comment 4 Max Kanat-Alexander 2009-08-10 21:38:16 PDT
Comment on attachment 393634 [details] [diff] [review]
suggested patch

No, this is incorrect. You'll want to look at the patch that originally implemented format_comment to see what you have to revert as far as where wrapped_comment goes and where you have to wrap comments.

Also, check out our development process so you can be sure that if you submit another patch, it gets reviewed:

http://wiki.mozilla.org/Bugzilla:Developers
Comment 5 Thomas Ehrnhoefer 2009-08-11 09:10:37 PDT
Thanks for your looking into this Max. 
Excuse me if I am wrong, but I am totally new to the Bugzilla sources and tried my best to understand what was and is going on in regards to the comment wrapping...

As for the patch that implemented the new format_comment.txt.tmpl, it seems to me that the formatting (adding the "duplicate of" and such comments) of comments was done in code (where format_comment is called now), and there was no wrapping done there (at least non that I could find). That's why I thought by removing the wrapping from the template I basically reverted the changes (but leaving the added comments as is)


In detail:

h3. version from 3.3.4

bc.. 
# Format language specific comments. This routine must not update
# $comment{'body'} itself, see BugMail::prepare_comments().
sub format_comment {
    my $comment = shift;
    my $body;

    if ($comment->{'type'} == CMT_DUPE_OF) {
        $body = $comment->{'body'} . "\n\n" .
                get_text('bug_duplicate_of', { dupe_of => $comment->{'extra_data'} });
    }
    elsif ($comment->{'type'} == CMT_HAS_DUPE) {
        $body = get_text('bug_has_duplicate', { dupe => $comment->{'extra_data'} });
    }
    elsif ($comment->{'type'} == CMT_POPULAR_VOTES) {
        $body = get_text('bug_confirmed_by_votes');
    }
    elsif ($comment->{'type'} == CMT_MOVED_TO) {
        $body = $comment->{'body'} . "\n\n" .
                get_text('bug_moved_to', { login => $comment->{'extra_data'} });
    }
    else {
        $body = $comment->{'body'};
    }
    return $body;
}

p. 

As you can see, there was no wrapping in the format_comment, and I could not find it whereever format_comment is called either.
Comment 6 Max Kanat-Alexander 2009-08-11 10:23:12 PDT
Hey thomas. That's right, you have to find where wrap_comment was removed and put it back.
Comment 7 Thomas Ehrnhoefer 2009-08-11 11:10:08 PDT
Created attachment 393815 [details]
patch v2

Ah, I get it. It seems bug 457657 introduced this new formatting, and was just intended for emails. However, the change also affected XML content.
Pretty sure there is a smarter way about this, but with my limited perl knowledge, this is all I can do to fix it.
With my previous patch, HTML was wrapped, XML was not (as intended), but also Email was not wrapped (that is what you meant by "find where it was removed
Comment 8 Thomas Ehrnhoefer 2009-08-11 11:12:23 PDT
sorry, hit the wrong key...., continuing comment 7:

...
With my previous patch, HTML was wrapped, XML was not (as intended), but also Email was not wrapped (that is what you meant by "find where it was removed" I guess). With the v2 patch email now gets wrapped again.
I know this is not pretty, but it fixes the regression introduced.
Comment 9 Thomas Ehrnhoefer 2009-08-11 11:15:50 PDT
Created attachment 393816 [details] [diff] [review]
patch v3

Me again. didn't submit the previous patch correctly, here it goes. Previous 2 comments still apply to this patch.
Comment 10 Max Kanat-Alexander 2009-08-11 11:33:36 PDT
I don't think Gerv is the right reviewer. Maybe one of the people who wrote or reviewed the original patch would be better.
Comment 11 Thomas Ehrnhoefer 2009-08-11 11:49:08 PDT
Comment on attachment 393816 [details] [diff] [review]
patch v3

:) I used the linked document to find the reviewer for BugMail.pm. I will try Marc, who provided the original patch.
Comment 12 Marc Schumann [:Wurblzap] 2009-08-20 15:01:08 PDT
Comment on attachment 393816 [details] [diff] [review]
patch v3

This looks really good to me at first glance. I'll take a closer look as soon as possible.
Comment 13 Robert Elves 2009-08-27 10:43:36 PDT
Hi Marc, Eclipse.org will be moving to 3.4.1 this weekend. Any chance of iterating on the patch this week?
Comment 14 Max Kanat-Alexander 2009-08-27 15:29:16 PDT
Comment on attachment 393816 [details] [diff] [review]
patch v3

I can already tell you that this will break everywhere that we were previously using comment wrapping--you'll want to check the original patch and revert the correct sections.
Comment 15 Robert Elves 2009-08-28 14:53:06 PDT
We're running with this patch here and wrapping still works in the web ui and in emails but the xml output is fixed. Of course there may be other scenarios that I'm unaware of,  Marc?
Comment 16 Max Kanat-Alexander 2009-08-28 16:54:14 PDT
Wrapping is only working in the UI because you are depending on the CSS to do it for you. It is actually going to be much wider than you're used to and will vary between browsers.
Comment 17 Frédéric Buclin 2009-09-04 15:23:11 PDT
Fixing this bug may also fix bug 514703, or vice versa.
Comment 18 otaylor 2009-09-08 17:31:25 PDT
(In reply to comment #16)
> Wrapping is only working in the UI because you are depending on the CSS to do
> it for you. It is actually going to be much wider than you're used to and will
> vary between browsers.

I don't think this is actually a problem.

Bug 457657 is the bug where the wrapping was introduced to format_comment. (attachment 379597 [details] [diff] [review] is very close to what was committed to CVS.)

The only place where wrapping was removed as in BugMail.pm - which Thomas's patch reintroduces.

Wrapping is still done for the web UI in ./template/en/default/bug/comments.html.tmpl

The handling for email will be slightly different after this patch - because any text introduced in a long-winded translation of the format_comment template will be also be wrapped. But if anything, that seems like a change for the better.

(Don't know if spontaneous reviews from random non-developers are appreciated, but the patch looks correct to me; I don't see any problems in it.)
Comment 19 Marc Schumann [:Wurblzap] 2009-09-25 15:13:03 PDT
Comment on attachment 393816 [details] [diff] [review]
patch v3

I like patch v3. I like the idea of handling comments generally unwrapped, and do the wrapping only when it needs to happen. And as you say, show_bug.cgi and bugmail are already covered.

Unfortunately, the patch doesn't apply to HEAD any more, so I can't test; please upload an unrotted patch. I'll see whether I come accross other places in need of wrapping.
Comment 20 Thomas Ehrnhoefer 2009-09-30 10:59:36 PDT
Created attachment 403812 [details] [diff] [review]
patch v.4

Hi Marc

Thanks for looking at this. Here is an updated version that works against HEAD again.
Comment 21 Frédéric Buclin 2009-10-19 04:20:59 PDT
As we got no comment from Marc for almost a month, I'm going to review this patch.
Comment 22 Frédéric Buclin 2009-10-19 05:06:56 PDT
Comment on attachment 403812 [details] [diff] [review]
patch v.4

>Index: webtools/bugzilla/Bugzilla/BugMail.pm

>+        $comment->{body} = ($comment->{'already_wrapped'} ? $comment->{body} : wrap_comment($comment->{body}));

Nit: parentheses are useless and can be removed on checkin (I know they were present in the original code; that's not a big deal).

The patch is correct and is working fine, for bugmails, XML files and bugs displayed in the web browser (including format for printing, aka long format). r=LpSolit

And yes, the wrapping happens independently of CSS; comments.html.tmpl handles this correctly.
Comment 23 Frédéric Buclin 2009-10-19 05:22:37 PDT
tip:

Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.129; previous revision: 1.128
done
Checking in template/en/default/bug/format_comment.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/format_comment.txt.tmpl,v  <--  format_comment.txt.tmpl
new revision: 1.3; previous revision: 1.2
done

3.4.2:

Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.124.2.5; previous revision: 1.124.2.4
done
Checking in template/en/default/bug/format_comment.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/format_comment.txt.tmpl,v  <--  format_comment.txt.tmpl
new revision: 1.1.2.3; previous revision: 1.1.2.2
done
Comment 24 Thomas Ehrnhoefer 2009-10-19 08:20:20 PDT
Thanks Frederic for reviewing this and applying the fix!
Comment 25 Marc Schumann [:Wurblzap] 2009-10-19 09:03:38 PDT
Thanks for picking this up, Frédéric.
Comment 26 Frédéric Buclin 2009-10-19 09:07:07 PDT
No problem. Why resetting the version field??
Comment 27 Marc Schumann [:Wurblzap] 2009-10-20 00:06:12 PDT
(In reply to comment #26)
> No problem. Why resetting the version field??

I don't know how this happened. It was not intentional.

Note You need to log in before you can comment on or make changes to this bug.