Closed
Bug 158499
Opened 22 years ago
Closed 22 years ago
Templatise XML bug output
Categories
(Bugzilla :: User Interface, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: gerv, Assigned: bbaetz)
References
Details
Attachments
(1 file, 3 obsolete files)
24.56 KB,
patch
|
justdave
:
review+
bbaetz
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•22 years ago
|
||
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
Reporter | ||
Comment 2•22 years ago
|
||
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
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.18
Reporter | ||
Comment 3•22 years ago
|
||
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
Assignee | ||
Comment 4•22 years ago
|
||
Comment on attachment 92139 [details] [diff] [review] Patch v.1 needswork per comments
Attachment #92139 -
Flags: review-
Assignee | ||
Comment 5•22 years ago
|
||
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
Assignee | ||
Comment 6•22 years ago
|
||
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•22 years ago
|
||
I haven't looked at Gerv's patch carefully, but bbaetz's description is how I think this should be done.
Reporter | ||
Comment 8•22 years ago
|
||
I am assigning all the bugs I am not working on in the immediate future to nobody@bugzilla.org. 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
Assignee: gerv → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 10•22 years ago
|
||
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
Attachment #92139 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #107844 -
Flags: review?(gerv)
Assignee | ||
Comment 11•22 years ago
|
||
Comment on attachment 107844 [details] [diff] [review] take 2 dave, can you please look at this?
Attachment #107844 -
Flags: review?(justdave)
Assignee | ||
Comment 12•22 years ago
|
||
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.)
Reporter | ||
Comment 13•22 years ago
|
||
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 '. This > +is kept separate for html_quote partly for comatability with previous code > +(for ') 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
Attachment #107844 -
Flags: review?(gerv) → review-
Assignee | ||
Comment 14•22 years ago
|
||
> 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- ?
Reporter | ||
Comment 15•22 years ago
|
||
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
Attachment #107844 -
Flags: review- → review+
Assignee | ||
Comment 16•22 years ago
|
||
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)
Attachment #107844 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #107844 -
Flags: review?(justdave)
Assignee | ||
Updated•22 years ago
|
Attachment #108575 -
Flags: review?(justdave)
Assignee | ||
Comment 17•22 years ago
|
||
Comment on attachment 108575 [details] [diff] [review] v3 carrying forward gerv's r+
Attachment #108575 -
Flags: review+
Comment 18•22 years ago
|
||
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
Attachment #108575 -
Flags: review?(justdave) → review-
Comment 19•22 years ago
|
||
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.
Assignee | ||
Comment 20•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Attachment #108575 -
Attachment is obsolete: true
Comment 21•22 years ago
|
||
Comment on attachment 109334 [details] [diff] [review] v4 This one works
Attachment #109334 -
Flags: review+
Assignee | ||
Comment 22•22 years ago
|
||
Comment on attachment 109334 [details] [diff] [review] v4 carrying forward gerv's review
Updated•22 years ago
|
Flags: approval+
Assignee | ||
Comment 23•22 years ago
|
||
Fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Summary: Templatise XML output → Templatise XML bug output
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•