Closed
Bug 110013
Opened 23 years ago
Closed 23 years ago
templatize describecomponents.cgi
Categories
(Bugzilla :: Bugzilla-General, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: gerv, Assigned: bbaetz)
Details
Attachments
(1 file, 5 obsolete files)
16.34 KB,
patch
|
gerv
:
review+
bbaetz
:
review+
|
Details | Diff | Splinter Review |
Part of the Bugzilla templatisation effort. A template a day keeps the doctor away.
Gerv
Reporter | ||
Comment 1•23 years ago
|
||
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
Severity: normal → blocker
Target Milestone: --- → Bugzilla 2.16
Assignee | ||
Comment 2•23 years ago
|
||
Taking. Since I can't use templates in mozilla, I guess I better use them
somewhere, right?
Assignee: gerv → bbaetz
Reporter | ||
Comment 3•23 years ago
|
||
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
Assignee | ||
Comment 4•23 years ago
|
||
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.
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•23 years ago
|
||
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.
Assignee | ||
Comment 6•23 years ago
|
||
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?
Reporter | ||
Comment 7•23 years ago
|
||
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
Assignee | ||
Comment 8•23 years ago
|
||
> 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.
Assignee | ||
Comment 9•23 years ago
|
||
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.
Attachment #65787 -
Attachment is obsolete: true
Reporter | ||
Updated•23 years ago
|
Reporter | ||
Comment 10•23 years ago
|
||
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
Assignee | ||
Comment 11•23 years ago
|
||
> 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.
Reporter | ||
Comment 12•23 years ago
|
||
> > 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
Assignee | ||
Comment 13•23 years ago
|
||
Actually, the proddesc stuff becomes better once I remove the proddesc hack.
Lets get enter_bug in, and then I'll merge that.
Assignee | ||
Comment 14•23 years ago
|
||
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.
Attachment #65809 -
Attachment is obsolete: true
Attachment #66590 -
Attachment is obsolete: true
Assignee | ||
Comment 15•23 years ago
|
||
Attachment #67865 -
Attachment is obsolete: true
Reporter | ||
Comment 16•23 years ago
|
||
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
Attachment #67866 -
Flags: review-
Assignee | ||
Comment 17•23 years ago
|
||
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
Attachment #67866 -
Attachment is obsolete: true
Reporter | ||
Comment 18•23 years ago
|
||
Comment on attachment 68762 [details] [diff] [review]
patch
r=gerv.
Gerv
Attachment #68762 -
Flags: review+
Comment 19•23 years ago
|
||
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.
Assignee | ||
Comment 20•23 years ago
|
||
Comment on attachment 68762 [details] [diff] [review]
patch
<justdave> I would says testing counts as a review
r=afranke
Attachment #68762 -
Flags: review+
Assignee | ||
Comment 21•23 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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
•