675502 - show_bug.cgi ctype=xml should include some more comment information

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Frank Becker]

Attachment: patch V1

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7) AppleWebKit/534.48.3 (KHTML, like Gecko) Version/5.1 Safari/534.48.3

Steps to reproduce:

Mylyn want to add support for private comments.

For this reason we need to have information about the number of an comment within an bug and if the description is private or not. 


Actual results:

comments in xml output did not include the same information from the HTML output. 


Expected results:

xml output should include the comment number within the bug and not only the comment ID.
We also need to know if the description is included in the comments

Comment 1

[Frédéric Buclin]

Wouldn't it make sense to leave config.cgi in favor of the WebServices API? The Bug.comments method already gives you this information.

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> Wouldn't it make sense to leave config.cgi

I meant leaving the XML/RDF interface in favor of WebServices.

Comment 3

[Frank Becker]

(In reply to comment #1)
> Wouldn't it make sense to leave config.cgi in favor of the WebServices API?
> The Bug.comments method already gives you this information.

This bug is about show_bug.cgi and not config.cgi!

Yes I prefer the WebSwervice API but by example Eclipse did not setup the WebServices because the are not shire that he are stable.

So I thing for Mylyn we need to support both!

Comment 4

[Max Kanat-Alexander]

> Yes I prefer the WebSwervice API but by example Eclipse did not setup the
> WebServices because the are not shire that he are stable.

  The bug XML is not a stable interface, actually, but the WebServices are. However, the WebServices only go back to 3.0.

> So I thing for Mylyn we need to support both!

  Probably a good idea. :-)

Comment 5

[Max Kanat-Alexander]

Comment on attachment 549660 [details] [diff] [review]
patch V1

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

::: template/en/default/bug/show.xml.tmpl
@@ +65,5 @@
>        [% PROCESS section_flags obj => bug %]
>  
>        [% IF displayfields.long_desc %]
> +          [% sort_order = user.settings.comment_sort_order.value %]
> +          <sort_order>[% sort_order %]</sort_order>  

That should be long_desc_sort_order, I'd think.

@@ +179,5 @@
> +  [% END %]
> +        <long_desc isprivate="[% c.is_private FILTER xml %]" isdescription="[% isdesc %]" >
> +            <commentid>[% c.id FILTER xml %]</commentid>
> +      [% IF number != description %]
> +            <commentnumber>[% number %]</commentnumber>

I believe comment.number is now accurate, so you shouldn't need anything special here, nor should you need the code you currently have below <sort_order>.

Comment 6

[Frank Becker]

(In reply to Max Kanat-Alexander from comment #5)
> Comment on attachment 549660 [details] [diff] [review] [diff] [details] [review]
> patch V1
> 
> Review of attachment 549660 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: template/en/default/bug/show.xml.tmpl
> @@ +65,5 @@
> >        [% PROCESS section_flags obj => bug %]
> >  
> >        [% IF displayfields.long_desc %]
> > +          [% sort_order = user.settings.comment_sort_order.value %]
> > +          <sort_order>[% sort_order %]</sort_order>  
> 
> That should be long_desc_sort_order, I'd think.

OK I can change this.
> 
> @@ +179,5 @@
> > +  [% END %]
> > +        <long_desc isprivate="[% c.is_private FILTER xml %]" isdescription="[% isdesc %]" >
> > +            <commentid>[% c.id FILTER xml %]</commentid>
> > +      [% IF number != description %]
> > +            <commentnumber>[% number %]</commentnumber>
> 
> I believe comment.number is now accurate, so you shouldn't need anything
> special here, nor should you need the code you currently have below
> <sort_order>.

Yes I need the code to make sure that we know how many private comments are in an bug and what is the local number of an comment within an bug. Otherwise we can not create the correct reply to comment number.

Comment 7

[Frank Becker]

Attachment: patch V2

(In reply to Max Kanat-Alexander from comment #5)
> Comment on attachment 549660 [details] [diff] [review]
> patch V1
> 
> Review of attachment 549660 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: template/en/default/bug/show.xml.tmpl
> @@ +65,5 @@
> >        [% PROCESS section_flags obj => bug %]
> >  
> >        [% IF displayfields.long_desc %]
> > +          [% sort_order = user.settings.comment_sort_order.value %]
> > +          <sort_order>[% sort_order %]</sort_order>  
> 
> That should be long_desc_sort_order, I'd think.
changed to comment_sort_order
> 
> @@ +179,5 @@
> > +  [% END %]
> > +        <long_desc isprivate="[% c.is_private FILTER xml %]" isdescription="[% isdesc %]" >
> > +            <commentid>[% c.id FILTER xml %]</commentid>
> > +      [% IF number != description %]
> > +            <commentnumber>[% number %]</commentnumber>
> 
> I believe comment.number is now accurate, so you shouldn't need anything
> special here, nor should you need the code you currently have below
> <sort_order>.
Sorry can not find comment.number so we need this code.

Comment 8

[Max Kanat-Alexander]

Ah, sorry, it's not "number" it's "count."

Comment 9

[Frank Becker]

Attachment: patch V3

Now <comment_count> is used!

Comment 10

[Max Kanat-Alexander]

Comment on attachment 615147 [details] [diff] [review]
patch V3

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

For the next review, if you'd like it in time, probably best to ask LpSolit or dkl.

::: template/en/default/bug/show.xml.tmpl
@@ +145,5 @@
> +[% BLOCK a_comment %]
> +  [% RETURN IF c.is_private && !user.is_insider %]
> +        <long_desc isprivate="[% c.is_private FILTER xml %]" >
> +            <commentid>[% c.id FILTER xml %]</commentid>
> +      <comment_count>[% c.count FILTER xml %]</comment_count>

Indentation seems funny here. <long_desc> should be aligned with the [% in RETURN and then the other members should be indented after that.

@@ +147,5 @@
> +        <long_desc isprivate="[% c.is_private FILTER xml %]" >
> +            <commentid>[% c.id FILTER xml %]</commentid>
> +      <comment_count>[% c.count FILTER xml %]</comment_count>
> +      [% IF c.is_about_attachment %]
> +            <attachid>[% c.extra_data FILTER xml %]</attachid>

Indentation in templates is two spaces.

Comment 11

[Frank Becker]

Attachment: patch V4

In reply to Max Kanat-Alexander from comment #10)
> Comment on attachment 615147 [details] [diff] [review]
> patch V3
> 
> Review of attachment 615147 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For the next review, if you'd like it in time, probably best to ask LpSolit
> or dkl.
> 
> ::: template/en/default/bug/show.xml.tmpl
> @@ +145,5 @@
> > +[% BLOCK a_comment %]
> > +  [% RETURN IF c.is_private && !user.is_insider %]
> > +        <long_desc isprivate="[% c.is_private FILTER xml %]" >
> > +            <commentid>[% c.id FILTER xml %]</commentid>
> > +      <comment_count>[% c.count FILTER xml %]</comment_count>
> 
> Indentation seems funny here. <long_desc> should be aligned with the [% in
> RETURN and then the other members should be indented after that.

Yes but now the generated output has the funny indentation.

Here is an example:

          <comment_sort_order>oldest_to_newest</comment_sort_order>  
          <long_desc isprivate="0" >
    <commentid>1</commentid>
    <comment_count>0</comment_count>
    <who name="Mylyn Test">tests</who>
    <bug_when>2012-04-15 15:39:16 +0200</bug_when>
    <thetext>comment 1</thetext>
  </long_desc><long_desc isprivate="0" >
    <commentid>3</commentid>
    <comment_count>2</comment_count>
    <who name="Mylyn Test">tests</who>
    <bug_when>2012-04-15 15:39:45 +0200</bug_when>
    <thetext>comment 3</thetext>
  </long_desc>

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 615442 [details] [diff] [review]
patch V4

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

Looks good and works as expected. r=dkl

Comment 13

[Frédéric Buclin]

Comment on attachment 615442 [details] [diff] [review]
patch V4

>+          <comment_sort_order>[% sort_order %]</comment_sort_order>  

sort_order must be filtered.


>+    <comment_count>[% c.count FILTER xml %]</comment_count>

You must update the DTD accordingly.

Comment 14

[David Lawrence [:dkl]]

Comment on attachment 615442 [details] [diff] [review]
patch V4

Actually something I forgot that LpSolit brought up, is that the DTD is now out of sync and will need to be updated to reflect the changes.

template/en/default/pages/bugzilla.dtd.tmpl

dkl

Comment 15

[Frédéric Buclin]

Comment on attachment 615442 [details] [diff] [review]
patch V4

(no reason to remove my review)

Comment 16

[Frank Becker]

Attachment: patch V5

(In reply to David Lawrence [:dkl] from comment #14)
> Comment on attachment 615442 [details] [diff] [review]
> patch V4
> 
> Actually something I forgot that LpSolit brought up, is that the DTD is now
> out of sync and will need to be updated to reflect the changes.
> 
> template/en/default/pages/bugzilla.dtd.tmpl

This is now included but I did not know where the dtd is used so I did no test if the dtd is correct.

Comment 17

[David Lawrence [:dkl]]

Comment on attachment 616260 [details] [diff] [review]
patch V5

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

Looks fine. Basically how I test is I use a program called 'xmllint' that uses the DTD to check the validity of a XML document. to include the DTD in the the XML data output, just pass dtd=1 to show_bug.cgi. Otherwise it includes it in the output as a link to the DTD back on the Bugzilla server. Either way is fine. r=dkl

Comment 18

[Frédéric Buclin]

Comment on attachment 616260 [details] [diff] [review]
patch V5

>=== modified file 'template/en/default/pages/bugzilla.dtd.tmpl'

>+                comment_sort_order,
>                 long_desc*, 

It must be comment_sort_order*, because it's displayed only if comments are. Please fix that on checkin.

Comment 19

[David Lawrence [:dkl]]

Thanks for the patch Frank.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified template/en/default/bug/show.xml.tmpl
modified template/en/default/pages/bugzilla.dtd.tmpl
Committed revision 8210

dkl

Comment 20

[Max Kanat-Alexander]

(In reply to Frank Becker from comment #11)
> Yes but now the generated output has the funny indentation.

  That's fine; we don't care about the indentation of the generated output for any of our templates unless it's signficant (for example, in a plain-text email).