110013 - templatize describecomponents.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Gervase Markham [:gerv]]

Part of the Bugzilla templatisation effort. A template a day keeps the doctor away.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Drat. I did this, but I appear to have lost it :-( But it didn't take very long
- a good first template for someone. This is user-facing, so a blocker.

Gerv

Comment 2

[Bradley Baetz (:bbaetz)]

Taking. Since I can't use templates in mozilla, I guess I better use them
somewhere, right?

Comment 3

[Gervase Markham [:gerv]]

Sure. If you need any help, let me know. Don't reinvent choose_product.tmpl -
I've already written it, and it's slated to go into global. I'll try and post a
message saying exactly which patch it's hidden in (I suspect enter_bug) this
evening.

Gerv

Comment 4

[Bradley Baetz (:bbaetz)]

Its enter_bug. I remember patch having Issues with it when I went to review
that. Ill do this work in the same tree that has that patch.

Comment 5

[Bradley Baetz (:bbaetz)]

Attachment: v0.5

OK, here we go. The product selection stuff is gerv's one from teh enter_bug
templatisation

Changes from the original:

- rather than calling quietly_check_login() always, the code calls
confirm_login iff Param('usebuggroups') and the user is either listing all
components, or trying to access a component which is protected (Should the
first one of these be quietly_check_login()?)

- Uses a table, rather than a popup, for choosing products (but see below)

Issues:

- If a component is closed for entry, then the description will be missing,
because of the $::proddesc hack we do. $::proddesc should be constant, and
there should be a new list of @::enterable_products, instead. This will be
better off after the enter_bug templates have gone in, and since this patch
depends on that one anyway...

- the choose product stuff has a few issues (url shouldn't be hard coded, for
example.) I've commented more on this in my reviwe of that patch.

- How can I collapse \s+ -> \s when not in quotes ? PRE/POST_CHOMP only does it
arround the template tags. IS this possible? If not, I'll collapse my tables in
a bit.

Comments? This depends on enter_bug templatisation, so this patch will change.

Comment 6

[Bradley Baetz (:bbaetz)]

Attachment: fix up proddesc

OK, I fixed up the proddesc stuff. This is against a trunk tree.

I'll merge it into this patch once etner_bug is checked in - it will just
conflict with everything otherwise. Comments on the approach, though?

Comment 7

[Gervase Markham [:gerv]]

I don't like the "enterable_products" thing very much. 

In an ideal world, what we would be doing is changing GetVersionTable completely
to return a single object hash with different entries for the different items
etc. etc. This would have a "products" entry, which would be a list of hashes,
each hash having a name, description, and an is_disabled flag.

<sigh>

I don't know how expensive calling Param("foo") is - but you might want to just
do that once, either at the top of the template or passing in the result as a
parameter.

We tend not to use studlyCaps for variable names. "BLOCK describe_comp"?

"Bugzilla component description" -> "Components for $product"

Probably "describe_components.tmpl"; the fact that we messed up the CGI name
doesn't mean we should do the same with the template ;-) All the other templates
seem to be _ separated.

Hope that's good to be going on with.

Gerv

Comment 8

[Bradley Baetz (:bbaetz)]

> I don't like the "enterable_products" thing very much. 

Well, we're not in an ideal world ;) It seems yuck to me, too. I think this is
going to have to be rethought when mod_perl support arrives in 2.18, since the
current trick of just unlink "data/versioncache" everytime we change somethign
won't work for long lived proecesses. For now, though, I think its ok, and it
does have precedent.

> I don't know how expensive calling Param("foo") is

Its cached in a global hash, so I think the answer is "not very" I worried about
this too, and checked.

[naming]

OK.

Comment 9

[Bradley Baetz (:bbaetz)]

Attachment: new patch

OK, fixed the stuff gerv wanted, except for what commented on earlier. The
choosproducts template is from gerv's latest patch, with the filter typo fixed.

Comment 10

[Gervase Markham [:gerv]]

Comment on attachment 66590 [details] [diff] [review]
new patch

>+    # Reference to a subset of %::proddesc, which the user is allowed to see
>+    my $products = \{};

Why is this a reference rather than a plain hash?

>+# Make sure the user is authorized to access this product.
>+if (Param("usebuggroups") && GroupExists($product)) {
>+    confirm_login();

In some circumstances, you call confirm_login twice. Please avoid this.

Other than that, looks good to me. Didn't test it, though.

Gerv

Comment 11

[Bradley Baetz (:bbaetz)]

> Why is this a reference rather than a plain hash?

Because TT wants references, and it was simpler to use references here rather
than copying the proddesc hash, and taking the reference later

> In some circumstances, you call confirm_login twice

No, I don't. Its called once if you don't have a product selected, but that step
outputs a template and exits the script.

The other time only comes after that step.

Comment 12

[Gervase Markham [:gerv]]

> > Why is this a reference rather than a plain hash?
> 
> Because TT wants references, and it was simpler to use references here rather
> than copying the proddesc hash, and taking the reference later

This is, however, inconsistent with the way we do it everywhere else, where we
use real data and then take references when we put things into $vars. You can
modify the proddesc hash directly - I don't understand why you don't like doing
that.

> No, I don't. Its called once if you don't have a product selected, but that 
> step outputs a template and exits the script.

Not if the prodsize is 1. But never mind - it's an edge case.

r=gerv if you insist on using the references, but I'd much prefer you change
them to fit in with the style that's been established.

Gerv

Comment 13

[Bradley Baetz (:bbaetz)]

Actually, the proddesc stuff becomes better once I remove the proddesc hack.
Lets get enter_bug in, and then I'll merge that.

Comment 14

[Bradley Baetz (:bbaetz)]

Attachment: patch

OK, merged the proddesc stuff in, and made the changes gerv wanted. Also merged
to use the global tempalte stuff rather than rolling my own.

Comment 15

[Bradley Baetz (:bbaetz)]

Attachment: diff before commiting...

Comment 16

[Gervase Markham [:gerv]]

Comment on attachment 67866 [details] [diff] [review]
diff before commiting...

>+if (lsearch(\@::enterable_products, $product) < 0) {

"== -1", please. This makes it more obvious it's an error condition.

>+use Template;
>+

You don't need this, do you? My patches seem to be getting along without it
now.

>+        $::vars->{'target'} = Param('urlbase') . "describecomponents.cgi";

I don't think you need urlbase here.

>+          "Please specify the component whose products you want described.";

Er... "product whose components"? ;-)

>+# Make sure the user specified a valid product name.  Note that
>+# if the user specifies a valid product name but is not authorized
>+# to access that product, they will receive a different error message
>+# which could enable people guessing product names to determine
>+# whether or not certain products exist in Bugzilla, even if they
>+# cannot get any other information about that product.

That's fine. We should put usability above a tiny information leak.

>+$::template->process("info/describe_components.tmpl", $::vars)

Latest word on this is "describe-components.html.tmpl". 

>+      <th align="left">Component</th>
>+      <th align="left">Default owner</th>
>+      [% IF Param("useqacontact") %]
>+        <th align="left">Default qa contact</th>
>+      [% END %]

"QA Contact" and "Owner". I'm pretty certain our style, as well as aesthetics,
means we should capitalise here.

Other than the above nits, looks pretty good. Shouldn't take long to get r=.

Gerv

Comment 17

[Bradley Baetz (:bbaetz)]

Attachment: patch

One note - the "usefulness vs information leak" problem was there before - I
just moved the comment as part of removing the proddesc hack.

Apart from that, all gerv's changes have been made

Comment 18

[Gervase Markham [:gerv]]

Comment on attachment 68762 [details] [diff] [review]
patch

r=gerv.

Gerv

Comment 19

[Andreas Franke (gone)]

Comment on attachment 68762 [details] [diff] [review]
patch

I have applied this patch on my local installation, and describecomponents.cgi
seems to work fine. I noticed that there is now the standard choose-product
page with links for each product, instead of the select box before, but that's
fine.

tested=afranke.

Comment 20

[Bradley Baetz (:bbaetz)]

Comment on attachment 68762 [details] [diff] [review]
patch

<justdave> I would says testing counts as a review

r=afranke

Comment 21

[Bradley Baetz (:bbaetz)]

Checked in.