Closed Bug 123030 Opened 21 years ago Closed 19 years ago

Move query.cgi javascript to separate file.

Categories

(Bugzilla :: Bugzilla-General, defect, P3)

2.15
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: kiko, Assigned: kiko)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 4 obsolete files)

Spun off from bug 96983 because of something myk also needs in bug 98801. This
is a simple bug to remove the JS from the code and into a separate JS file.
Ideally the JS is actually a .cgi file that generates the full JS for products
for us, though this is not what 96983 asks for.

I would like to know what the best approach would be: make it a .cgi file and
generate the JS component arrays on the fly, or make it the responsability of
the including file (possibly a template) to generate them?
Blocks: 96983
Status: NEW → ASSIGNED
Summary: Move query.cgi JS to separate file. → Move query.cgi javascript to separate file.
Without CVS write access, how do I diff for a new file (I can't do cvs add)?
A template in the "global" sub-directory might be the ideal way to generate the
JavaScript versions of the arrays.  Also, check out the file
global/perl2js.js.tmpl in attachment 64800 [details] [diff] [review] of bug 116922.

The rest of the code (the stuff that modifies the component, version, and
milestone arrays when you select a product) can be placed into a separate static
file.
I've added this as a tentative first patch. I haven't removed the actual array
creation from the .atml file because I'm not sure it's the best idea.

I have changed the selectProduct API to pass in four form controls 
(for p c v m) allowing one to pass in false as values if they don't have the
corresponding control.

Yes, I know the comments need to be updated. Apart from that, is it ok?

Myk: I think this API suits you?
Here's the JS file, to be put in the toplevel dir. Should I use a js/ dir or
something?
Keywords: patch, review
Myk, the problem with global/components.js.tmpl is that it doesn't actually
remove inline JS from the code - just separates it perl-wise. I would have to
include the file into a <script> section -- possibly the section in <head> I
have included. This doesn't solve the requirements set by bug 96983 (removing
<!--, cleaning up HTML).

I think the proper thing to do would be to create a real .cgi that generated the
values. But I'd like to hear other opinions. But isn't that bug 72837?
Attachment #67536 - Attachment description: kiko_v1_b: productmenu.ps (no cvs add here :P) → kiko_v1_b: productmenu.js (no cvs add here :P)
I haven't done a full review, but at first glance it looks good.  My only
suggestion would be to name the parameters something like "objField", "objFld"
or "objMenu", where "obj" is "components", "versions", or "milestones", rather
than "objs", to clarify that those parameters take the actual form field itself.

>I think the proper thing to do would be to create a real .cgi that generated
>the values. But I'd like to hear other opinions. But isn't that bug 72837?

Yes, you are right, this is the better approach.  What this means is that HTML
that needs this JavaScript should point to the static localconfig.js if the user
is not logged in or the installation is not using product groups (i.e. the user
should only see products available to everyone or all products are available to
everyone); otherwise the HTML should point to the CGI and let the CGI
dynamically generate the JavaScript based on the user's permissions.

>Here's the JS file, to be put in the toplevel dir. Should I use a js/ dir or
>something?

No, I think it's fine at the top level.

Depends on: 72837
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.18
This looks fine to me - has this bitrotted in the meantime, though?
*** Bug 169759 has been marked as a duplicate of this bug. ***
What needs to be done here now that bug 72837 has landed?
Actually use that list in the query.cgi javascript. I'm a bit overburdened at
work, unfortunately, so if somebody can pick this up, it might be faster than
waiting for(ever) me.
OS: Linux → All
Hardware: PC → All
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Kiko on IRC said move to 2.20
Attached patch kiko_v2: a few years later... (obsolete) — Splinter Review
This is more or less a forward-port of the patches, with comments fixed and
some good testing to ensure this works. In this patch I manage to reduce to a
bare minimum the target milestone code -- if you don't supply the control to
selectProduct, nothing happens. 

I moved all the JS code to where it should live: in the header. I used an
anonymous block to put it where it belongs, and store the form controls in a
form_controls string. It's fun!

This code can be reused to do all sort of JS updating -- for instance, updating
component based on product in show_bug.cgi.
Attachment #67535 - Attachment is obsolete: true
Attachment #67536 - Attachment is obsolete: true
Attachment #153565 - Flags: review?(caillon)
Attachment #153565 - Flags: review?(myk)
Blocks: 251416
Note that bug 72837 is orthogonal to this bug, but not to the blocked bug 251416
-- I want to use the config.js arrays saved as cached javascript code that
accelerates this page for public products.
Comment on attachment 153565 [details] [diff] [review]
kiko_v2: a few years later...

Note: I haven't yet done a line-by-line review of this.

>Index: js/productform.js

>+ *     - product, component, varsion and milestone: form controls

Nit: varsion -> version


>+function selectProduct(product, component, version, milestone) {

Smart, since it means forms can name these fields differently.


>Index: template/en/default/search/search-advanced.html.tmpl

>+[% form_controls = "document.forms['queryform'].product, "   _
>+                   "document.forms['queryform'].component, " _
>+                   "document.forms['queryform'].version"       %]
>+
>+[% IF Param("usetargetmilestone") %]
>+  [% form_controls = form_controls _ 
>+                        ", document.forms['queryform'].target_milestone" %]
>+[% ELSE %]
>+  [% form_controls = form_controls _ ", null" %]
>+[% END %]

These are only used in two places, so it seems overcomplicated to generate and
store them in a separate TT variable just to make those two places less
cluttered.  Instead, use a temporary variable in those places to make the code
smaller, i.e.:


>+                            onchange="f = this.form; selectProduct(f.product, f.component, f.version, f.target_milestone);">

Alternately, factor out the code into JavaScript rather than TT, which is
simpler because it crosses fewer languages:

>+                            onchange="doOnSelectProduct();">

function doOnSelectProduct() {
  var form = document.forms['queryform'];
  selectProduct(form.product, form.component, form.version,
form.target_milestone);
}

Note that you don't have to check for the existence of the target_milestone
field before passing it to selectProduct(), since if it doesn't exist the
function call will just pass "undef", which returns false just like "null" in a
boolean conditional.


>-[% PROCESS search/form.html.tmpl %]
>+[% PROCESS search/form.html.tmpl form_controls=form_controls %]

Note that this is unnecessary (even if you were using form_controls), since
PROCESS provides processor template variables in the processee variable stash.
Attachment #153565 - Flags: review?(myk) → review-
(In reply to comment #14)
> Alternately, factor out the code into JavaScript rather than TT, which is
> simpler because it crosses fewer languages:
> 
> >+                            onchange="doOnSelectProduct();">
> 
> function doOnSelectProduct() {
>   var form = document.forms['queryform'];
>   selectProduct(form.product, form.component, form.version,
> form.target_milestone);
> }
> 
> Note that you don't have to check for the existence of the target_milestone
> field before passing it to selectProduct(), since if it doesn't exist the
> function call will just pass "undef", which returns false just like "null" in 

AFAICS this will raise a Javascript strict warning upon accessing an undefined
form.target_milestore, which is why I didn't do it initially. Now what?

> >-[% PROCESS search/form.html.tmpl %]
> >+[% PROCESS search/form.html.tmpl form_controls=form_controls %]
> 
> Note that this is unnecessary (even if you were using form_controls), since
> PROCESS provides processor template variables in the processee variable stash.

"Explicit is better an Implicit" :-) 

AFAICS this will raise a Javascript strict warning upon accessing an undefined
form.target_milestore, which is why I didn't do it initially. Now what?

Try "form.target_milestone || null".
Attachment #153565 - Attachment is obsolete: true
Attachment #153565 - Flags: review?(caillon)
Attachment #153830 - Flags: review?(myk)
Comment on attachment 153830 [details] [diff] [review]
kiko_v3: myk's solution is so much cleaner :-)

The patch looks good, but after applying it to my local installation the target
milestone field on query.cgi loses its values.	Reversing the patch brings the
values back.  Something is going screwy there.
Attachment #153830 - Flags: review?(myk) → review-
A variable context issue, apparently.
Attachment #153830 - Attachment is obsolete: true
Attachment #153842 - Flags: review?(myk)
Attachment #153842 - Flags: review?(myk) → review+
Flags: approval+
Thanks. Great to close this one after so long!

/cvsroot/mozilla/webtools/bugzilla/js/productform.js,v  <--  productform.js
initial revision: 1.1
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/form.html.tmpl,v 
<--  form.html.tmpl
new revision: 1.25; previous revision: 1.24
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search-advanced.html.tmpl,v
 <--  search-advanced.html.tmpl
new revision: 1.19; previous revision: 1.18
Status: ASSIGNED → RESOLVED
Closed: 19 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.