110012 - Spank show_bug.cgi hard - templatize and combine

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Gervase Markham [:gerv]]

show_bug.cgi, process_bug.cgi, show_activity.cgi, xml.cgi and bug_form.pl should
all be one templatised CGI called show_bug.cgi. 

This will bring bug activity inline, make XML output the template it was always
meant to be, stop people bookmarking process_bug.cgi, and generally make life
nicer all round. 

This will involve a _severe_ spanking. :-)

I believe this is the way to go to templatise the bug display code.

Gerv

Comment 1

[Dave Miller [:justdave]]

blocks a blocker so it's a blocker.

Comment 2

[Matthew Tuck [:CodeMachine]]

Maybe it will stop bookmarking process_bug.cgi, but is it really worth it?  I
suppose it is consistent with most of the product, but it sounds like a major
effort, and could easily result in a whopper of a CGI.

Comment 3

[Jacob Steenhagen]

Once bug 109530 is fixed, Bug.pm could be quite useful for this template as it
already puts most (all?) relevent information into a single hash.  Anything that
isn't in that has that's needed could (and probably should) be added.

Comment 4

[Gervase Markham [:gerv]]

I'm already a long way through this; if someone wants me to use Bug.pm, they'd
better explain how, and quick :-)

Gerv

Comment 5

[Jacob Steenhagen]

#!/usr/bin/perl -wT

use strict;

use lib ".";
use Bug;
require "CGI.pl";

my $bug_num = 1;
my $user_id = 1;

my $bug = new Bug($bug_num, $user_id);

print $bug->{'short_desc'} . "\n";

=================================

That's a brief summary :)

Comment 6

[Gervase Markham [:gerv]]

OK, so I bit off a bit more than I could chew here; I'm trying to scale back the
work to the minimum possible. Stay tuned.

Gerv

Comment 7

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Here we are. I've:
- templatised show_bug (actually bug_form.pl), and turned it into a procedure
rather than a "do".
- modified a few globals and things to support all this.

Templatising process_bug.cgi should wait until this is in; it's a more complex
job. Once this is in, I might have another go at combining process_bug and
show_bug - it can be done, but it's a big job.

This patch is unavoidably tied up with the one for long_list.cgi - they share
common code.

Gerv

Comment 8

[Gervase Markham [:gerv]]

CCing some people.

Gerv

Comment 9

[SineSwiper]

Was directed to this bug for a reqest to get the reset button (re)moved from bug
93908.  Should be a simple change to make.

Comment 10

[John Vandenberg]

When applying the patches for long_list and show_bug (bug 115369) have common
blocks ... tho all works out in the end.
Except ... this patch for show_bug ends up with no footer.

Comment 11

[John Vandenberg]

Sorry ... to explain better ... PutFooter or templatised equivalent is not
included, resulting in no footer in the output.
Also, in order to template/default/attachment/list.atml to use bug ids to create
valid links for 'Create a New Attachment' and 'View All', bugid needs to be
added to $vars; or list.atml

    $vars->{'bug'} = \%bug;
    $vars->{'bugid'} = $bug{'bug_id'};
    $vars->{'user'} = \%user;

Comment 12

[Gervase Markham [:gerv]]

OK, so I've added in the "footer" call to the bottom of show_bug.tmpl and
removed the Reset button. However, I think that some more reviewing can be done
here before a new patch shows up. Come on, someone :-)

Gerv

Comment 13

[Dave Miller [:justdave]]

*** Bug 122272 has been marked as a duplicate of this bug. ***

Comment 14

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

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

>Index: template/default/show/show_bug.tmpl

These should be moved to the "bug" directory per our discussion in
n.p.m.webtools (unless you disagree; I didn't see another post from
you on this subject in the thread).

news://news.mozilla.org:119/[e-mail]


>+      <td>&nbsp;</td>
>+      [% PROCESS select sel = { description => "Platform", 
>+                                name => "rep_platform" } %]
>+      <td align="right">

Abstracting out the select code to this extent makes it difficult to see
the structure of the table, which is important in templates because we
want non-programmers to be able to hack them.  It would be better to use
the select block to generate only the select itself and add in the field 
label separately.


>+          <a href="describecomponents.cgi?product=[% bug.product %]">
>+            Component</a>:

No need to be this severe in limiting yourself to 80-character long lines
at the expense of readability. :-)


>+        <select name="component">
>+          [% FOREACH x = component_ %]
>+            <option value="[% x %]"
>+              [% " selected" IF x == bug.${sel.name} %]>[% x %]</option>

${sel.name} -> component


>+          <a href="bug_status.html#bug_status">Status</a>:

There is no "#bug_status" anchor in the bug_status.html file.


>+[%# *** AssignedTo Milestone QAContact URL Whiteboard Keywords *** %]

Don't forget summary.  Also, why group "assigned to" and "milestone" with
the fields below it rather than the fields above it whose presentational
structure they share?


>+        <td></td>
>+        <td></td>
>+        <td>&nbsp;</td>

<td colspan="3">?


>+[%# *** Attachments *** %]

The old attachment interface has now been removed via bug 104521, so this 
entire section can be replaced by [% INCLUDE attachment/list.atml %].


>+        <a href="showdependencytree.cgi?id=$id">Show dependency tree</a>  
  ...
>+          <a href="showdependencygraph.cgi?id=$id">Show dependency graph</a>

$id -> [% bug.bug_id %]


>+      <input type="checkbox" name="bit-[% group.bit %]" value="1" 
>+        [% " checked" IF group.ingroup %]
>+        [% " disabled='disabled'" IF NOT group.ison %]>

" checked='checked'"?


>+  [% IF bug.bug_status == "UNCONFIRMED" && user.canmodify %]

"canmodify" is ambiguous; "caneditorconfirm", although clumsier, is clearer.
However, since this is actually the only place where it is needed, you might
as well just get rid of it and check both canedit and canconfirm here.


>+  [% IF user.canedit %]
>+    [% IF bug.isopened %]
>+      [% IF bug.status != "ASSIGNED" %]
>+        <input type="radio" name="knob" value="accept" />
>+        Accept bug (
>+        [% "confirm bug, " IF user.canmodify && bug.isunconfirmed %]change

If the user has canedit privileges (checked in one of the enclosing 
conditionals), then they certainly have canmodify privileges, so canmodify
does not need to be checked here.


>+      [% IF bug.isunconfirmed && user.canmodify %]

Another unnecessary canmodify.


>+        &nbsp;&nbsp;&nbsp;&nbsp;<input type="checkbox" name="andconfirm">
>+        and confirm bug (change status to <b>NEW</b>)
>+        <br>
>+      [% END %]
>+      [% knum = knum + 1 %]
>+
>+      <input type="radio" name="knob" value="reassignbycomponent">
>+      Reassign bug to owner
>+      [% "and QA contact" IF useqacontact %]
>+      of selected component
>+      <br>
>+      [% IF bug.isunconfirmed && user.canmodify %]

Another unnecessary canmodify.


>+[%############################################################################%]
>+[%# Block for SELECT fields                                                  #%]
>+[%############################################################################%]
>+
>+[% BLOCK select %]  
>+
>+  [% DEFAULT sel.url = "bug_status.html#$sel.name" %]  
>+  <td align="right">
>+    <b>
>+      [% IF sel.url != "none" %]
>+        <a href="[% sel.url %]">[% sel.description %]</a>:
>+      [% ELSE %]
>+        [% sel.description %]:
>+      [% END %]
>+    </b>
>+  </td>
>+  <td>
>+    <select name="[% sel.name %]">
>+      [% FOREACH x = ${sel.name} %]
>+        <option value="[% x %]"
>+          [% " selected" IF x == bug.${sel.name} %]>[% x %]</option>
>+      [% END %]
>+    </select>
>+  </td>
>+  <td>&nbsp;</td>
>+[% END %]

This block is elegant and efficient, but it's also overcomplicated for
template code, which non-programmers should be able to understand.
Besides focusing the block on the select menu itself (mentioned earlier),
the block would be much clearer if it received parameters individually
rather than as part of a "sel" hash and if it was passed the list of
options directly rather than having to interpolate it from the menu name.


>Index: template/default/show/navigate.tmpl
>+[% IF bug_list.size %]

I used to do this myself, but I recommend doing "bug_list.size > 0"
instead for clarity (particularly for non-programmers).


>Index: template/default/show/choose_bug.tmpl

Our file namespace is a mess of concatenated and underscored words.  
If I was able to start over again I would standardize the lot of it; in 
the meantime we should standardize on something for the template files 
at least.  How about hyphens?  They make for both readable and typable
filenames on all manner of filesystems and are recommended by the 
Mozilla style guide.

http://mozilla.org/README-style.html


>+    <input type="submit" value="Show Me This Bug" size="6" />

Size?


>Index: show_bug.cgi

>+show_bug();

It's a bit strange for this function to be called "show_bug" but defined
in "bug_form" which is called from the show_bug.cgi script.  Nevertheless,
the real solution is to move the show_bug function into Bug.pm as Bug::display
and do away with bug_form.pl altogether.

Since show_bug is the only function in bug_form.pl, it should be easy
to do this, and as I think about it now I would grant review on the code 
even without Bug::display being integrated with the rest of the code in Bug.pm.
As you said before, doing everything originally called for in this bug 
is a huge load of work, and it makes sense to split it up into multiple pieces.

Also, show_bug.cgi should handle the case where the user loads that script 
without giving it a bug ID, and show_bug/Bug::display should return nothing 
if the bug ID it is given is not defined, since displaying a bug and 
displaying a form for entering a bug number to display are entirely separate 
activities, and separating them allows us to reuse show_bug/Bug::display in 
situations where we may not necessarily want to return a form if a bug number 
does not exist.


>Index: Attachment.pm

> sub list
> {
>-  # Displays a table of attachments for a given bug along with links for
>-  # viewing, editing, or making requests for each attachment.
>-
>+  # Creates an array of hashes containing the data for a table of attachments      # for a given bug. These data should be given to attachment/list.atml in an
>+  # "attachments" variable.
>   my ($bugid) = @_;

This function should be renamed since its purpose is changing.	I have been 
using the name "query" in similar modules (i.e. Request.pm) for the function 
that retrieves the set of objects matching given criteria (i.e. bug ID, 
status, active flag, etc.).

Since you are rewriting this comment, you might as well clean up my erroneous
overuse of technical terminology to describe what is happening here:
"Retrieves and returns an array of attachment records for a given bug." 
should do the trick.  The word "records" in that sentence refers to both 
database records and Perl "records", as complex data structures involving 
multiple levels of hashes and arrays are often called in Perl.


>-    # Format the attachment's creation/modification date into something readable.
>+    # Format the attachment's creation/modification date into something 
>+    # readable.

This hunk fails because of the fix for bug 93037 and should be removed.


>Index: bug_form.pl

>+    # Attachments
>+    if (Param('useattachmenttracker')) {
>+        use Attachment;
>+        $bug{'attachments'} = &Attachment::list($id);
>+    } else {
>+        my @attachments;
>+        SendSQL("SELECT attach_id, creation_ts, mimetype, description " .
>+                "FROM attachments WHERE bug_id = $id");
>+        while (MoreSQLData()) {
>+            my ($attachid, $date, $mimetype, $desc) = FetchSQLData();
>+            if ($date =~ /^(\d\d)(\d\d)(\d\d)(\d\d)(\d\d)(\d\d)(\d\d)$/) {
>+                $date = "$3/$4/$2 $5:$6";
>+            }

This stuff can all go away now that bug 104521 has landed.


>+            # This measns that all product groups will be skipped, but 

measns -> means


>+    $bug{'isopened'} = 
>+                ($bug{'bug_status'} =~ /^(UNCONFIRMED|NEW|ASSIGNED|REOPENED)$/);

Use the "IsOpenedState" function from globals.pl here.


>+        my $this_bug_idx = lsearch(\@bug_list, $id);
>+        if ($this_bug_idx != -1 && $this_bug_idx + 1 < @bug_list) {
>+            $vars->{'next_bug_id'} = $bug_list[$this_bug_idx + 1];

Looks like a bit of left-over code from before you moved this logic
into navigate.tmpl.


>+    # This is length in number of comments
>+    $bug{'longdesclength'} = scalar(@{$bug{'comments'}});

In the original code this value is, believe it or not, the length 
of the string containing the HTML representation of the description 
and all the comments on the bug.  Presumably this anticipates an 
editable description/comments.	In any case, you are doing it right,
just make sure you also hack process_bug.cgi to use this value.


>Index: globals.pl
>+    SendSQL("SELECT profiles.realname, profiles.login_name, 
>+                date_format(longdescs.bug_when,'%Y-%m-%d %H:%i'), 
>+                longdescs.thetext
>+            FROM longdescs, profiles
>+            WHERE profiles.userid = longdescs.who
>+                AND longdescs.bug_id = $id 
>+            ORDER BY longdescs.bug_when");

The formatting of this query seems a little drunk.

Comment 15

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Myk - thanks for the great review :-)

> These should be moved to the "bug" directory per our discussion in

That discussion's not finished yet :-) I need to comment to that message.

> Besides focusing the block on the select menu itself (mentioned earlier),
> the block would be much clearer if it received parameters individually
> rather than as part of a "sel" hash and if it was passed the list of
> options directly rather than having to interpolate it from the menu name.

I agree with you 50%. I've changed it not to use a hash, but I believe
interpolating from the menu name is the right thing to do - it should become an
idiom rather than an abnormality. I've used it in several templates.

> Our file namespace is a mess of concatenated and underscored words.  
> If I was able to start over again I would standardize the lot of it; in 
> the meantime we should standardize on something for the template files 
> at least.  How about hyphens?  They make for both readable and typable
> filenames on all manner of filesystems and are recommended by the 
> Mozilla style guide.

They are for HTML file names; source file names are different. I think we
should go with underscore-separated words, because that won't invalidate every
single filename we've got (as hyphens would) because it's been followed in
quite a few cases.


>Index: Attachment.pm

> > sub list
> > {
> 
> This function should be renamed since its purpose is changing.	I have
> been using the name "query" in similar modules (i.e. Request.pm) for the 
> function that retrieves the set of objects matching given criteria (i.e. bug 

> ID, status, active flag, etc.).

Do you want me to do this?

Other than that, all comments (where I could understand what you wanted)
addressed :-) Please have another look.

Gerv

Comment 16

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

>They are for HTML file names; source file names are different. I think we
>should go with underscore-separated words, because that won't invalidate every
>single filename we've got (as hyphens would) because it's been followed in
>quite a few cases.

There are no existing template files with underscores, and only six out of 40
CGI scripts have underscores (enter_bug.cgi, long_list.cgi, post_bug.cgi,
process_bug.cgi, show_activity.cgi, show_bug.cgi), all of which will be removed
during post-templatization script consolidation, so there is no precedent or
advantage to sticking with underscores.

Nevertheless, the files will have to be renamed anyway after consolidation, and
perhaps it makes more sense to name them in the old style pending that work, so
go ahead and leave the names as-is.


>Index: Attachment.pm
> > This function should be renamed since its purpose is changing.
>Do you want me to do this?

Yes.


>Other than that, all comments (where I could understand what you wanted)
>addressed :-) Please have another look.

Overall looks good.  I rescind my review comments telling you to put show_bug()
into Bug.pm and the template files into the bug/ sub-directory.  Those things
need to be done, but they can happen during the script consolidation process
after this patch has been checked in.  So, the only things left to do in this
patch are:

1. Modify process_bug.cgi to use the new code for displaying "the next bug on
your list".


>+    SendSQL("SELECT profiles.realname, profiles.login_name, 
>+                    date_format(longdescs.bug_when,'%Y-%m-%d %H:%i'), 
>+                    longdescs.thetext
>+            FROM longdescs, profiles
>+            WHERE profiles.userid = longdescs.who 
>+              AND longdescs.bug_id = $id 
>+            ORDER BY longdescs.bug_when");

2. Re-align this query, which is still inconsistently aligned and hard to read.
 Left-align the edges of all text following capitalized SQL keywords and then
either left-align or right-align the edges of the SQL keywords themselves, i.e.:

>+    SendSQL("SELECT   profiles.realname, profiles.login_name, 
>+                      date_format(longdescs.bug_when,'%Y-%m-%d %H:%i'), 
>+                      longdescs.thetext
>+             FROM     longdescs, profiles
>+             WHERE    profiles.userid = longdescs.who 
>+             AND      longdescs.bug_id = $id 
>+             ORDER BY longdescs.bug_when");

or:

>+    SendSQL("SELECT profiles.realname, profiles.login_name, 
>+                    date_format(longdescs.bug_when,'%Y-%m-%d %H:%i'), 
>+                    longdescs.thetext
>+               FROM longdescs, profiles
>+              WHERE profiles.userid = longdescs.who 
>+                AND longdescs.bug_id = $id 
>+           ORDER BY longdescs.bug_when");


>+our $template = Template->new(

3. Remove the global templatization code, which has already landed and is now
part of globals.pl, and use "$::template" and "$::vars" instead of "our
$template" and "our $vars" for compatibility with Perl 5.005.

Comment 17

[Gervase Markham [:gerv]]

> There are no existing template files with underscores,
<snip>

So what you are saying is that we have a completely clean slate? I didn't know
about the disappearance of the underscored files you mention. If they are going
away, I'd be happy with either underscores or hyphens.

> >Index: Attachment.pm
> > > This function should be renamed since its purpose is changing.
> >Do you want me to do this?
> 
> Yes.

Right. I'm not certain I know how to do this - where is it called?

> 1. Modify process_bug.cgi to use the new code for displaying "the next bug on
> your list".

Ah. Yes.

Gerv

Comment 18

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

>So what you are saying is that we have a completely clean slate?

Not immediately, but in the near future yes.

>Right. I'm not certain I know how to do this - where is it called?

Yours is the only invocation, so you can just rename it. :-)

Comment 19

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Here's version 3. Most things addressed, although I haven't renamed the
template files yet, as it's not clear what scheme we are going for. I've had to
hack on a fair number of things to make the required changes...

Prompt review requested - of all the big templatisation patches, this one is
furthest from being cooked, so needs special attention.

Gerv

Comment 20

[Gervase Markham [:gerv]]

Attachment: Patch v.4

Note to self: save all files before doing cvs diff.

Gerv

Comment 21

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

Comment on attachment 67791 [details] [diff] [review]
Patch v.4

r=myk;	There are still some issues that could be addressed, but nothing that
needs to hold up checkin.

Comment 22

[Gervase Markham [:gerv]]

Cool - thanks, Myk.

Dave, Myk - who else could we line up to review this?

Gerv

Comment 23

[Dave Miller [:justdave]]

Comment on attachment 67791 [details] [diff] [review]
Patch v.4

This won't apply to the tip, and if I back-date my repository, apply the patch,
then update, I get tons of conflicts.

Comment 24

[Dave Miller [:justdave]]

Just FYI, I *tried* to resolve the conflicts, but the bug_form.pl file was just
one big huge conflict.  Even telling patch to ignore whitespace.  Looks like you
pretty much rewrite the whole thing, and too many changes have been made to it
since the patch was made.

Comment 25

[Gervase Markham [:gerv]]

Attachment: Patch v.5

Here's a refreshed version. Bradley, please stop checking into bug_form.pl ;-)

This will need to be refreshed again after long_list goes in (which should
happen first.) But it can certainly be reviewed meantime.

Gerv

Comment 26

[Bradley Baetz (:bbaetz)]

Bleh. Its only conflicting because you indented...

Anyway, the attachment.cgi change is bogus, and has been correctly fixed in cvs
- your version allows any text through, which defeats the purpose of taint
checking....

Rather than using lsearch in the template, can you use contains from the bits
myk added?

That was just a quick glance - I'll do a proper review tomorrow, if I get time.

Comment 27

[Dave Miller [:justdave]]

OK, what order are we checking in?  comment 25 indicates the patch on bug 115369
needs to be checked in before this one, but bug 115369 comment 10 and bug 115369
comment 16 indicate that this patch should be checked in before that one.

Can someone make up our minds please?

Comment 28

[Gervase Markham [:gerv]]

Bug 115369 first :-) Then, I'll refresh this patch to remove the bits which are
in bug 115369. After that, someone can look at it again.

Sorry for the confusion :-)

Gerv

Comment 29

[Gervase Markham [:gerv]]

Attachment: Patch v.6

> Rather than using lsearch in the template, can you use contains from the bits

> myk added?

They do different things. I use lsearch to get the position in the array -
Myk's stuff doesn't give that.

Patch v.6, post long_list.cgi checkin, attached.

Gerv

Comment 30

[Dave Miller [:justdave]]

There's an awful lot of whitespace between comments with this patch... 
otherwise it looks pretty good.

the existing system already has a little more than there should be I think, but
this adds even more.  There's 4 blank lines after each comment before the header
for the next comment.  (there are 3 between each on b.m.o right now, I think 2
would be plenty).  Actually, if I copy and paste, the second of these blank
lines has two spaces on it. :-)

Comment 31

[Gervase Markham [:gerv]]

Dave - comments.tmpl is already checked in, as part of long_list.cgi's
templatisation, so if you have problems with its formatting, you need to file a
separate bug. ;-)

Apart from that, is that a review? Myk, does Patch v.6 still have your review?

Gerv

Comment 32

[Bradley Baetz (:bbaetz)]

Again, I haven't gone through this fully yet.

You ignore Param('usetargetmilestone') - the field always shows.

The spacing is different. If you look at landfill bug 2 before and after, after
your patch there is extra wrapping (target milestone + tara's name is wrapped,
and before it isn't) Actually, this may be because you now put a space between
the : and the field, on the page. Not sure if it matters, then - I think its
just an edge case.

Resolved bugs are missing the option to VERIFY them.

Should the navigation stuff always give you the first/last options, even if
you're not on a list?

Can you make teh action in choose_bug a paramater, so that it can be used for
xml.cgi, too?

Thats the template part done...

The comments extra line thing is now bug 127507, which needs comment - the
solution is trivial, but there are two choices.

Comment 33

[Bradley Baetz (:bbaetz)]

Comment on attachment 70414 [details] [diff] [review]
Patch v.6

needs work from my last comment, but there is still other stuff that could be
looked at

Comment 34

[Gervase Markham [:gerv]]

> Should the navigation stuff always give you the first/last options, even if
> you're not on a list?

Sure.

> Can you make teh action in choose_bug a paramater, so that it can be used for
> xml.cgi, too?

xml.cgi requires you to be able to choose multiple bugs. We could use the same
template for both, but this would require show_bug to redirect to long_list if
multiple bugs were selected. Is that the right idea?

Is it OK if I regenerate the patch after you've had time to look a bit closer?

Gerv

Comment 35

[Bradley Baetz (:bbaetz)]

Good point, wrt xml.cgi. OTOH, eventually weren't you going to merge long_list
and show_bug + the xml output, or something?

Anyway, with the help of cvs diff -w, here are my other comments:

bug_form.pl: Ug, we don't use the votes cache for this? /me sighs, and moves on.

Group boxes + associated bits aren't being shown...

+    $user{'canedit'} = UserInGroup("editbugs") 
+                       || $::userid == $bug{'assigned_to'}
+                       || $::userid == $bug{'reporter'}
+                       || $::userid == $bug{'qa_contact'}
+                       || $::userid == 0;                   
+    $user{'canconfirm'} = UserInGroup("canconfirm") || ($::userid == 0);

Test UserInGroup last, since that involves a db query which you can possibly avoid.

I haven't played arround with the various permissions testing stuff, but it
seems good. I'm ready for your next set ;)

Comment 36

[Bradley Baetz (:bbaetz)]

Oops, missed the other files.

Attachment.pm: Why is this needed? Why should the attachments be on the bugs
object from the POV of this file? just PROCESS it with attachments =
bug.attachments from show_bug. ditto for the corresponding template changes

process_bug: Don't use our $vars; that requires perl 5.6.

You seem to have lost the taint checks for showing the next page.

Oh, and I suppose making bug_form.pl a module of some sort, or part of Bug.pm,
or something, is outside the scope of this bug? ;)

Comment 37

[Gervase Markham [:gerv]]

Attachment: Patch v.7

> You ignore Param('usetargetmilestone') - the field always shows.

Is checking that it has a milestone not sufficient? Oh, well - check added.

> Resolved bugs are missing the option to VERIFY them.

Urk. 

> Should the navigation stuff always give you the first/last options, even if
> you're not on a list?

Probably :-) See the new version, and tell me what you think.

> Attachment.pm: Why is this needed? Why should the attachments be on the bugs
> object from the POV of this file? just PROCESS it with attachments =
> bug.attachments from show_bug. ditto for the corresponding template changes

I take your point about including the template correctly, but the changes to
Attachment.pm are still needed, surely? We don't want Attachment.pm to output
the attachment stuff otherwise it appears at the very top of the file :-)
Attachments.pm should return a data structure.

> Oh, and I suppose making bug_form.pl a module of some sort, or part of 
> Bug.pm, or something, is outside the scope of this bug? ;)

:-P

Ready for re-review.

Gerv

Comment 38

[Bradley Baetz (:bbaetz)]

> Is checking that it has a milestone not sufficient? Oh, well - check added.

Welcome to the bugzilla database schema:

target_milestone varchar(20) not null default "---"

;)

Comment 39

[Bradley Baetz (:bbaetz)]

Comment on attachment 71181 [details] [diff] [review]
Patch v.7


>+  [% IF Param('usestatuswhiteboard') %] 
...
>+  [% END %]
>+
>+  [% IF use_keywords %]
...
>+  [% END %]

Why are these two different?

>+  [% IF use_votes %]
...
>+  [% END %]

This is differnet because the product may disable voting, right?

>+  [% IF groups %]
...
>+      <input type="checkbox" name="bit-[% group.bit %]" value="1" 
>+        [% " 'checked'" IF group.ison %]
>+        [% " disabled='disabled'" IF NOT group.ingroup %] />

The default checked stuff isn't working, and it should be checked='checked'.
All groups default to off.

>+[%# *** Knob *** %]

>+  [% knum = 1 %]

This is for js, right?

>+    [% ELSE %]
>+      [% IF bug.resolution != "MOVED" ||
>+           (bug.resolution == "MOVED" && user.canmove) %]  
>+        <input type="radio" name="knob" value="reopen" /> Reopen bug
>+        <br />

missing an add for knum here

>+      [% END %]

>+[%############################################################################%]
>+[%# Block for SELECT fields                                                  #%]
>+[%############################################################################%]
>+
>+[% BLOCK select %]  
>+  <td>
>+    <select name="[% selname %]">
>+      [% FOREACH x = ${selname} %]
>+        <option value="[% x %]"
>+          [% " selected" IF x == bug.${selname} %]>[% x %]</option>
>+      [% END %]
>+    </select>
>+  </td>
>+  <td>&nbsp;</td>
>+[% END %]

This is used in so many places. All of them are identical, aren't they? Can you
move this into a
BLOCK initialised in the constructor in globals.pl?

(See
http://www.template-toolkit.org/docs/plain/Manual/Config.html#Template_Files_an
d_Blocks, the BLOCKS section)

A later bug can be filed to pull out the other places; maybe you want to leave
this, too.

>Index: process_bug.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v
>retrieving revision 1.116
>diff -u -r1.116 process_bug.cgi
>--- process_bug.cgi	13 Feb 2002 03:05:15 -0000	1.116
>+++ process_bug.cgi	24 Feb 2002 10:42:49 -0000

>-if (defined $::FORM{'id'}) {
>-    navigation_header();
>-    if (defined $::next_bug) {
>-        # If there is another bug, then we're going to display it,
>-        # so check that its a legal bug
>-        # We need to check that its a number first
>-        if (!(detaint_natural($::next_bug) && CanSeeBug($::next_bug))) {
>-            # This isn't OK
>-            # Rather than error out (which could validly happen if there
>-            # was a bug in the list whose group was changed in the meantime)
>-            # just remove references to it
>-            undef $::next_bug;
>-        }
>-    }
>-}

>+# Show next bug, if it exists.
>+if ($::COOKIE{"BUGLIST"} && $::FORM{'id'}) {
>+    my @bugs = split(/:/, $::COOKIE{"BUGLIST"});
>+    my $cur = lsearch(\@bugs, $::FORM{"id"});
>+    if ($cur >= 0 && $cur < $#bugs) {
>+        my $next_bug = $bugs[$cur + 1];
>+        detaint_natural($next_bug);

Err, when I said that you'd missed out my taint stuff, I didn't just mean that
you should detaint it.
Your code allows any bug to be seen + you don't even check the result...

You need to only show the bug if CanSeeBug($next_bug) returns true (if it
returns false, $next_bug
will have no value). Before passing it through canseebug, you need to check
that its a number, first,
to prevent passing arbitary sql through. Then you need to undef $::next_bug so
that the
navigation_header routine doesn't show it. You obviously shouldn't have to do
the logic exactly as I
did it, expecially since I presume that the plan is to rmeove that routine from
CGI.pl, and you're
reordering in preparation.

IF you're doing that, then I wont' coimplain that you're duplicating work which
is in that routine.

Comment 40

[Gervase Markham [:gerv]]

Attachment: Patch v.8

> >+  [% IF Param('usestatuswhiteboard') %] 
> >+  [% IF use_keywords %]
> 
> Why are these two different?

usestatuswhiteboard is a param. You work out whether you are using keywords or
not by finding out whether @::legal_keywords contains any items. It's not a
param.

> This is differnet because the product may disable voting, right?

Yep.

> >+	    [% " 'checked'" IF group.ison %]
> 
> The default checked stuff isn't working, and it should be checked='checked'.
> All groups default to off.

The inner set of quotes is the problem. Oops.

> >+  [% knum = 1 %]
> 
> This is for js, right?

Yep.

> This is used in so many places. All of them are identical, aren't they? Can 
> you move this into a BLOCK initialised in the constructor in globals.pl?

Er... this looks a little bit complex. Can we file a bug and optimise it later?


I've renamed all the templates to conform to the current thinking on style.

Gerv

Comment 41

[Bradley Baetz (:bbaetz)]

Comment on attachment 71191 [details] [diff] [review]
Patch v.8

I've jsut noticed that in bug_form.pl, you only moced the useringroup call to
the end for canconfirm, but canedit still has it at the front. Make the == 0
check the first one, then the checks for assigned to/reporter/qa_contact, and
only then make the db query.

You've change the template files tom cply with this weeks format, I guess.

The checked options should be checked='checked' for xhtml compliance, I think.

Anyway, fix those and I think that'll be it.

Comment 42

[Gervase Markham [:gerv]]

Attachment: Patch v.9

Review comments addressed.

Gerv

Comment 43

[Bradley Baetz (:bbaetz)]

Comment on attachment 71245 [details] [diff] [review]
Patch v.9

r=bbaetz

Comment 44

[Bradley Baetz (:bbaetz)]

Comment on attachment 71245 [details] [diff] [review]
Patch v.9

Actually, this fails tests. removing first-review (sorry!)

Comment 45

[Gervase Markham [:gerv]]

Attachment: Patch v.10

This one passes tests. r=bbaetz? :-)

Gerv

Comment 46

[Bradley Baetz (:bbaetz)]

Comment on attachment 72829 [details] [diff] [review]
Patch v.10

r=bbaetz (the accesskey change too...)

Comment 47

[Ville Skyttä]

Attachment: Patch v.11

(X)HTML fixes:
lowercase elements and attributes, checked="checked" and friends,
added <label>'s.  Obsoletes v.10.

Comment 48

[Gervase Markham [:gerv]]

Comment on attachment 74610 [details] [diff] [review]
Patch v.11

Having chatted to Ville, he's going to do the XHTML stuff as a later patch if
it's still thought necessary.

Gerv

Comment 49

[Andreas Franke (gone)]

It seems to me that Patch v.10 doesn't apply cleanly any more: At least
Attachment.pm and bug_form.pl seem to have rejects... :(

Gerv, can you check and provide a new one if necessary?

Comment 50

[Gervase Markham [:gerv]]

Attachment: Patch v.12

Here's an unrotted version.

Gerv

Comment 51

[Gervase Markham [:gerv]]

Comment on attachment 75743 [details] [diff] [review]
Patch v.12

r= carries (it's the same patch.)

Can I ask again: please, someone, review this! It's blocking a whole load of
other 2.16 work.

Gerv

Comment 52

[Bradley Baetz (:bbaetz)]

Comment on attachment 75743 [details] [diff] [review]
Patch v.12

[this time I'll comment before clicking submit]

>+<form method="get" action="show_bug.cgi">
>+  <p>
>+    You may find a single bug by entering its bug id here:
>+    <input name="id" size="6"/>

You're missing a space there, which is probably going to cause problems for
older browsers

However, I have been infomred that we're not meant to be using xhtml,
apparently,. because it won't
validate with an html doctype, evne though it will work.

So all the <foo /> style tags probably need to be removed, or so I've been told
recently.
See #mozwebtols for more info

Comment 53

[Gervase Markham [:gerv]]

Attachment: Patch v.13

Remove XHTML slashes, for a quiet life. When this argument is resolved, we can
add them back in - or not. In the mean time, let's get review!

Gerv

Comment 54

[Gervase Markham [:gerv]]

Comment on attachment 75752 [details] [diff] [review]
Patch v.13

r=bbaetz (carried over; confirmed on IRC.)

Gerv

Comment 55

[Andreas Franke (gone)]

Comment on attachment 75752 [details] [diff] [review]
Patch v.13

I installed this patch on my local installation, and it seems to work. I've
played around with some changes, and I haven't found any problems. I feel
comfortable to give it t(ested)=afranke, which counts as a review last I heard.

Feel free to play with it at http://bugzilla.mathweb.org:8000/show_bug.cgi?id=1
.

Comment 56

[Andreas Franke (gone)]

Gerv, have you tried this with Mozilla 0.9.9? On bugzilla.mozilla.org, there is
already some strange overlapping between the field names and the text input
fields (look at "QA Contact", "URL", "Summary", "Status Whiteboard", and
"Keywords"), but with your patch, the colons at the other fields above are not
part of the link any more, and so the same overlap occurs there. 

I guess that this is not a bug in your patch, but a problem with Mozilla. Do you
know if there's an open bug about it? Will it be fixed for 1.0? Or is there a
workaround?

Comment 57

[Gervase Markham [:gerv]]

> I guess that this is not a bug in your patch, but a problem with Mozilla. Do you
> know if there's an open bug about it? Will it be fixed for 1.0? Or is there a
> workaround?

I'm not seeing the problem with 20020315 on Linux. If it seems to occur widely,
we can juggle the template to work around it.

Checking in show_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
new revision: 1.18; previous revision: 1.17
done
Checking in Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.8; previous revision: 1.7
done
Checking in bug_form.pl;
/cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v  <--  bug_form.pl
new revision: 1.89; previous revision: 1.88
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.118; previous revision: 1.117
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/show/choose_bug.html.tmpl,v
done
Checking in template/default/show/choose_bug.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/show/choose_bug.html.tmpl,v
 <--  choose_bug.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/show/show_bug.html.tmpl,v
done
Checking in template/default/show/show_bug.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/show/show_bug.html.tmpl,v 
<--  show_bug.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/show/navigate.html.tmpl,v
done
Checking in template/default/show/navigate.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/show/navigate.html.tmpl,v 
<--  navigate.html.tmpl
initial revision: 1.1
done

Wahey! Now let's get working on those dependencies.

Gerv

Comment 58

[Andreas Franke (gone)]

> I'm not seeing the problem with 20020315 on Linux.

Indeed, it seems to be a Mozilla problem that is fixed in current nightlies.