templatize describecomponents.cgi

RESOLVED FIXED in Bugzilla 2.16

Status

()

--
blocker
RESOLVED FIXED
17 years ago
6 years ago

People

(Reporter: gerv, Assigned: bbaetz)

Tracking

2.15
Bugzilla 2.16

Details

Attachments

(1 attachment, 5 obsolete attachments)

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
Severity: normal → blocker
Target Milestone: --- → Bugzilla 2.16
(Assignee)

Comment 2

17 years ago
Taking. Since I can't use templates in mozilla, I guess I better use them
somewhere, right?
Assignee: gerv → bbaetz
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

17 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

17 years ago
Created attachment 65787 [details] [diff] [review]
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.
(Assignee)

Comment 6

17 years ago
Created attachment 65809 [details] [diff] [review]
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?
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

17 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

17 years ago
Created attachment 66590 [details] [diff] [review]
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.
Attachment #65787 - Attachment is obsolete: true
Keywords: patch, review
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

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

17 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

17 years ago
Created attachment 67865 [details] [diff] [review]
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.
Attachment #65809 - Attachment is obsolete: true
Attachment #66590 - Attachment is obsolete: true
(Assignee)

Comment 15

17 years ago
Created attachment 67866 [details] [diff] [review]
diff before commiting...
Attachment #67865 - Attachment is obsolete: true
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

17 years ago
Created attachment 68762 [details] [diff] [review]
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
Attachment #67866 - Attachment is obsolete: true
Comment on attachment 68762 [details] [diff] [review]
patch

r=gerv.

Gerv
Attachment #68762 - Flags: review+
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

17 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

17 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.