158499 - Templatise XML bug output

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Gervase Markham [:gerv]]

This is a collaboration between xml.cgi and Bug.pm. We need to work out how to
make XML generation a template. Preferably a template for show_bug.cgi.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This patch does the following:
- removes XML generation from Bug.pm
- makes long_list.cgi support multiple formats, and do better error-reporting
- creates an xml FILTER
- creates a new template for long_list.cgi with format=xml
- makes xml.cgi just put up the "Which bugs would you like?" form, and have it
  submit to long_list.cgi
- adds the ability to query for attachments with their data to Attachment.pm

Gerv

Comment 2

[Gervase Markham [:gerv]]

myk: I'd appreciate your views on this patch, but (if you are short of time)
particularly the Attachment changes (is querywithdata() the correct name?)

Hmm. This patch does make Bug.pm somewhat redundant...

Gerv

Comment 3

[Gervase Markham [:gerv]]

OK, this patch breaks move.pl. I need to work out a way to invoke long_list.cgi
and capture the output for emailing.

Gerv

Comment 4

[Bradley Baetz (:bbaetz)]

Comment on attachment 92139 [details] [diff] [review]
Patch v.1

needswork per comments

Comment 5

[Bradley Baetz (:bbaetz)]

Comment on attachment 92139 [details] [diff] [review]
Patch v.1

Further:

strictly speaking, your xml escape thing is wrong - what about non-ascii chars?
I suppose tahts all part of the separate encoding bug, though.

You should have xml.cgi redirect to long_list.cgi, then, and you can drop
choose-xml.html.tmpl after that.

in the template, urlbase is already required to end with a /, so don't check it
here.

If you're going to disable the attachment data, use a TT comment

Comment 6

[Bradley Baetz (:bbaetz)]

gerv and I discussed this on IRC last night, and he asked me to say describe
what I think should happen.

The short version:
-----------------

a) show_bug.cgi becomes a nine line script to handle html, xml, and long_list
(probably renamed) formats
b) xml.cgi goes away (redirected for backwards compat)
c) longlist.cgi goes away (redirected for backwards compat)
d) bug_form.pl goes away

The long version:
a) show_bug.cgi contains nine lines, after the initial connect/login/security
check stuff:

my $format = ValidateOutputFormat($::FORM{'format'}, "show", "bug/show");
my @bugs;
foreach my $id in (@bug_ids) {
  push @bugs, new Bug($id);
}
$vars->{'bugs'} = \@bugs;
print "Content-Type: $format->{'contenttype'}\n\n;
$template->process("bug/show/$format-?{'template'}", $vars)
  || ThrowTemplateError($template->error());

The rationale:
-------------

a) This splits up the frontend (the output format) from the backend (Bug.pm). We
can modify Bug.pm to lazily load attachments, so that they're only retreived if
someone asks for them.
  - We can write regression tests only on the backend
  - Its cleaner

b) We don't have duplicate code
  - bug_form.pl and Bug.pm currently have duplicate code
  - this code gets out of sync
  - We also have duplicate code for 'you doidn't enter an id' things
  - specific differences in requirements can still be handled in the template
(ie show.html.tmpl can have an error [% IF bugs.size != 1 %])

c) It fixes the move.pl problem.
  - Since the backend is shared, move.pl just invokes a particular template
    - (Ideally, move.pl would be a package, not a separate script, but thast a
different issue)

The problem with gerv's approach is that it goes the opposite way, moving stuff
further towards the front end (as seen with the move.pl issue) This sort of
stuff is even more important when we have custom fields, and we don't want to be
duplicating that code everywhere.

Comments?

Comment 7

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

I haven't looked at Gerv's patch carefully, but bbaetz's description is how I
think this should be done.

Comment 8

[Gervase Markham [:gerv]]

I am assigning all the bugs I am not working on in the immediate future to
[e-mail]. This means:

- I will be able to search for bugs assigned to me as a list of bugs I'm going
to fix (which is as it should be), and
- people won't falsely assume I might be about to fix a bug when I'm not.

Gerv

Comment 9

[Bradley Baetz (:bbaetz)]

Mine, mine, all mine!

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: take 2

Basically, this does what comment 6 said. show_bug is longer than 9 lines,
mainly because show_bug for one bug (in html mode) is still slightly different
to the multi-bug case. See comments in the patch for why.

TT version upgrade is because that version of TT will auto-convert scalars into
1-element lists, which is why the FOREACH in the xml template works.

show_bug only accepts separate cgi params for multiple bugs - the old spliting
is handled in xml.cgi. Theres no reason an alias can't contain one of these
characters, after all.

I dropped choose-xml.html.tmpl, since it doesn't really make much sense.

I also fixed a few regressions from my Bug.pm patch:

- cclist_accessible always reported |undef| ('.' instead of ',')
- the js for changing the assignee was wrong (assigned_to_email vs
assigned_to.email, from a previous iteraton)
- blocking bugs weren't shown - this was another case of xml vs show_bug having
different field names. I changed Bug.pm (and thus the xml output), because it
was easier than also changing process_bug.
- There was an extra </a> for the target milestone milestoneurl link

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 107844 [details] [diff] [review]
take 2

dave, can you please look at this?

Comment 12

[Bradley Baetz (:bbaetz)]

Those extra fixes have gone in, so you'll get a cvs conflict for bugzilla.dtd
where I changed the blocking thing already in cvs, but obviouisly didn't do the
reordering.

The version in the patch is correct, but it really won't affect the review at
all, since its non code anyway - IOW just ignore it. (Theres also a Bug.pm
whitespace conflict which is trivial to resolve.)

Comment 13

[Gervase Markham [:gerv]]

Comment on attachment 107844 [details] [diff] [review]
take 2

> +foreach my $key (qw(error groups
> +                    longdescs milestoneurl attachments
> +                    isopened isunconfirmed
> +                    flag_types num_attachment_flag_types
> +                    show_attachment_flags use_keywords any_flags_requesteeble

Is that really right? "any_flags_requesteeble"? Ick...

> Index: checksetup.pl
> ===================================================================
>          name => 'Template', 
> -        version => '2.07' 
> +        version => '2.08' 

Are you sure earlier versions don't do scalar -> list promotion? I seem to
remember reading about it in the docs a while ago...


> Index: move.pl
> ===================================================================
> +$template->process("bug/show.xml.tmpl", { bugs => \@bugs }, \$msg)
> +  || ThrowTemplateError($template->error());

You are sure this will append to $msg and not nuke it?

> Index: show_bug.cgi
> ===================================================================
> -    $template->process("$format->{'template'}", $vars) ||
> +# 'Single' HTML bugs are treated slightly specially in a few places
> +my $single = !$cgi->param('format')
> +  && (!$cgi->param('ctype') || $cgi->param('ctype') eq 'html');

Thid should probably be $singlehtml if it truly is only single HTML bugs.

> +if ($single) {
> +    my $id = $cgi->param('id');
> +    # Its a bit silly to do the validation twice - that functionality should
> +    # probably move into Bug.pm at some point
> +    ValidateBugID($id);
> +    push @bugs, new Bug($id, $userid);
> +} else {
> +    foreach my $id ($cgi->param('id')) {
> +        my $bug = new Bug($id, $userid);
> +        push @bugs, $bug;
> +    }
> +}

Why do we not validate each bug ID in the multiple case? To put it another way
- why are these two branches different?

> +print $cgi->redirect("show_bug.cgi?ctype=xml$ids");

Hmm. This looks handy to know about.

> Index: Bugzilla/Util.pm
> ===================================================================
> -        $time = "$year-$month-$day $hour:$min " . &::Param('timezone');
> +        $time = "$year-$month-$day $hour:$min";
> +        $time .= " " . &::Param('timezone') if &::Param('timezone');

This is a different fix, right?

> +
> +=item C<xml_quote($val)>
> +
> +This is similar to C<html_quote>, except that ' is escaped to &apos;. This
> +is kept separate for html_quote partly for comatability with previous code
> +(for &apos;) and partly for futire handling of non-ASCII characters.

"for -> from", "compatibility" and "future". 

> +<?xml version="1.0" standalone="yes"?>
> +<!DOCTYPE bugzilla SYSTEM "[% Param('urlbase') %]bugzilla.dtd">

Is urlbase guaranteed to have a trailing slash?

> +      [% FOREACH a = bug.attachments %]
> +        <attachment>
> +          <attachid>[% a.attachid %]</attachid>
> +          <date>[% a.date FILTER time FILTER xml %]</date>
> +          <desc>[% a.description FILTER xml %]</desc>
> +        </attachment>
> +      [% END %]

No attachment bodies?

Gerv

Comment 14

[Bradley Baetz (:bbaetz)]

> Is that really right? "any_flags_requesteeble"? Ick...

Yeah, its right.

> Are you sure earlier versions don't do scalar -> list promotion? I seem to
> remember reading about it in the docs a while ago...

Yes - this change was made in v2.07a. (bmo probably wants v2.08b mind you, for
speed reasons)

> > +$template->process("bug/show.xml.tmpl", { bugs => \@bugs }, \$msg)
> > +  || ThrowTemplateError($template->error());
> You are sure this will append to $msg and not nuke it?

Yes, the 3rd arg to ->process may be 'a reference to a scalar (e.g. a text
string) to which output/error is appended'

> Thid should probably be $singlehtml if it truly is only single HTML bugs.

Well, its really $editable, but that didn't really flow. If we had a xul
interface, then this branch would cover that (unless that used xmlrpc or something).

> Why do we not validate each bug ID in the multiple case? To put it another way
> - why are these two branches different?

For xml.cgi, we put the type of error in the xml, so that the requestor can know
teh type of error. long_list just leaves out the bug if its not
visible/valid/etc (although this patch doen't convert long_list to a format for
show_bug - thats another patch)

> This is a different fix, right?

Sort of. The trailing space doesn't matter in html, but it does in the xml.

> Is urlbase guaranteed to have a trailing slash?

Yes, and doeditparams checks that.

> "for -> from", "compatibility" and "future".

'for' is correct, I'll fix the others.

> No attachment bodies?

Nope, current code dosn't have it (mainly for space issues, but encoding is a
problem too)

So, apart from the two typos (which I can fix before checkin), why the review- ?

Comment 15

[Gervase Markham [:gerv]]

Comment on attachment 107844 [details] [diff] [review]
take 2

> kept separate for html_quote

I meant this "for"; and I'm pretty sure it should be a "from" :-)

> Well, its really $editable, but that didn't really flow.

Why not? A comment saying "Currently, only single HTML bugs are editable" would
do the trick. 

Gerv

Comment 16

[Bradley Baetz (:bbaetz)]

Attachment: v3

Actually, I am going to keep this using $single. Think 'edit multiple' doing
this rather than rerunning the buglist query, or something along those lines.
Its clear what its doing; I don't think the name change will make it clearer.

This fixes the spelling issues, and updates the patch to apply cleanly (mainly
by removing the already-merged changes)

Comment 17

[Bradley Baetz (:bbaetz)]

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

carrying forward gerv's r+

Comment 18

[Dave Miller [:justdave]]

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

the XML it produces doesn't validate.

Warning: Ignorable whitespace in externally declared element bugzilla in
document declared standalone
in unnamed entity at line 9 char 5 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Start tag for undeclared element reporter_accessible
in unnamed entity at line 9 char 231 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Start tag for undeclared element cclist_accessible
in unnamed entity at line 9 char 273 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element bug in document
declared standalone
in unnamed entity at line 13 char 9 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 14 char 11 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 15 char 11 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 16 char 11 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 17 char 9 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element bug in document
declared standalone
in unnamed entity at line 18 char 9 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 19 char 11 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 20 char 11 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 21 char 11 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 22 char 9 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element bug in document
declared standalone
in unnamed entity at line 23 char 9 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 24 char 11 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 25 char 11 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 26 char 11 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 29 char 9 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element bug in document
declared standalone
in unnamed entity at line 30 char 9 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 31 char 11 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 32 char 11 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 33 char 11 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element long_desc in
document declared standalone
in unnamed entity at line 34 char 9 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element bug in document
declared standalone
in unnamed entity at line 36 char 5 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml
Warning: Ignorable whitespace in externally declared element bugzilla in
document declared standalone
in unnamed entity at line 38 char 1 of
http://landfill.bugzilla.org/bzdave/show_bug.cgi?id=55&ctype=xml

Comment 19

[Dave Miller [:justdave]]

The ignorable whitespace warnings I think we can ignore :)  because they're all
in tags that are used for nothing other than grouping other tags anyway.

But the undeclared tags we probably need to fix.

Comment 20

[Bradley Baetz (:bbaetz)]

Attachment: v4

Yeah, I have no clue about the ignorable whitespace stuff - they're _meant_ to
be ignorable; its not a bug. The w3c validator doesn't give those errors.

Added those two items to the dtd, and added line breaks for each field.

Comment 21

[Dave Miller [:justdave]]

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

This one works

Comment 22

[Bradley Baetz (:bbaetz)]

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

carrying forward gerv's review

Comment 23

[Bradley Baetz (:bbaetz)]

Fixed.