Part of the Bugzilla templatisation effort. A template a day keeps the doctor away. 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

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

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

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.

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.

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?

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

> 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.

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 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

> 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.

> > 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

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

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 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

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 on attachment 68762 [details] [diff] [review] patch r=gerv. Gerv

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 on attachment 68762 [details] [diff] [review] patch <justdave> I would says testing counts as a review r=afranke

Checked in.