Closed Bug 123030 Opened 23 years ago Closed 21 years ago

Move query.cgi javascript to separate file.

Categories

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

Product:
Bugzilla
Component:
Bugzilla-General
Version:
2.15
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.20

People

(Reporter: kiko, Assigned: kiko)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 4 obsolete files)

kiko_v4: yeah, don't use tms for two things at the same time
(deleted), patch
myk
: review+
Details | Diff | Splinter Review
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.
Attached patch kiko_v1: remove (most) JS from atml (obsolete) (deleted) — — Splinter Review
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?
Attached file kiko_v1_b: productmenu.js (no cvs add here :P) (obsolete) (deleted) —
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) (deleted) — — 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".
Attached patch kiko_v3: myk's solution is so much cleaner :-) (obsolete) (deleted) — — Splinter Review
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: 21 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.

Attachment

General

Creator:
Created:
Updated:
Size: