Currently choosing a Product in query.cgi triggers JavaScript that causes the components select to be limited to only the components in the selected Products. It would be useful if, when only one Product were selected, the link to describecomponents.cgi were also changed to be the link for that specific product rather than the link to choose the components for one of the products. Steps to reproduce: 1. load https://bugzilla.mozilla.org/query.cgi?format=advanced 2. click on "Bugzilla" in the product dropdown 3. click on the hyperlink "Component" Actual results: 3. go to https://bugzilla.mozilla.org/describecomponents.cgi Expected results: 3. go to https://bugzilla.mozilla.org/describecomponents.cgi?product=Bugzilla

Javascript for for the above bug (please let me knoe if it's over the top) together with trivial fix for bug number 423078.

Comment on attachment 331428 [details] [diff] [review] Bug Fix for 344606 v1 I'm not a reviewer. Mkanat will be able to review this though.

(In reply to comment #2) > I'm not a reviewer. pyrzak, I'm the one who suggested the author of the patch to ask you to look at it. You can do informal reviews for now (as we did in the past with potential future reviewers).

First off nice first patch. A few comments: function doChangeLink() { + var cutString = /\?.*/i; + var curLink = document.getElementById("oldurl"); Since this is being called by oldurl you can just do doChangeLink(this) and then change the function call to function doChangeLink( componentLabel ) + var oldLink = new String(curLink); + var resLink = new Array(oldLink.split(cutString)); + newLink = new String(resLink[0]); + var newBase = newLink.replace(",",""); + var selCount = 0; + var selText = ""; It looks like you're getting the curLink href but i don't see any reference to it. That would make this code easier to understand. Also not sure why being so fancy with regex. The simple solution of putting describecomponents.cgi in the href to blank it out would do the trick with less confusion and complexity in the code. + for (i=0;i<document.getElementById("product").length;i++) { + if (document.getElementById("product")[i].selected) { + selCount = selCount + 1; + selText = document.getElementById("product")[i].value; + } + } You should break as soon as the number > 2 some instances of bugzilla have LOTS of products and we don't want to slow things down. Even then I'm concerned about looping through this list. + if (selCount == 1) { + curLink.href = (newBase + "?product=" + selText); + } else { + curLink.href = (newBase); + } If you make the change I suggested above then you don't need to do the else part above. and lastly i know this is dumb but: - Any one of: + Any of: should be made to bug 423078 not this one. This is for traceability later on. So... R-. Make those changes and I'll check it out again.

Comment on attachment 331428 [details] [diff] [review] Bug Fix for 344606 v1 Fix pyrzak's comments, and when pyrzak signs off on this, ask me or some other reviewer for review.

Problem with using 'this' in routine as incorrect data passed. All other comments have been addressed.

Comment on attachment 333092 [details] [diff] [review] Patch for bug 344606 V2 I like it! I think some of the 1 liners (like the if selcount) might need to be broken up into 2 lines for readability but I'll defer to mkanat for that and other little things that might bother him. The code itself is good, tested on FF2 & FF3 on mac.

Comment on attachment 333092 [details] [diff] [review] Patch for bug 344606 V2 >Index: form.html.tmpl >+function doChangeLink() { >+var e=document.getElementById("product"); >+var u=document.getElementById("oldurl"); There's no indentation! And there should be spaces around = >+var selCount = 0; >+var selText = ""; >+for (i=0;i<e.length;i++) { And spaces after the ; and around the < >+ if (selCount > 2){break} No curly-braces if this is a one-liner. But we still need a semicolon! >+if (selCount == 1) { >+ u.href = ("describecomponents.cgi?product=" + selText); >+ } else { And the indentation there needs to be fixed... pyrzak: All of this is stuff that you should be catching before me.

Comment on attachment 333092 [details] [diff] [review] Patch for bug 344606 V2 please make mkanat's changes and resubmit

Comment on attachment 333942 [details] [diff] [review] Patch for bug 344606 V3 >Index: form.html.tmpl > >+function doChangeLink() { >+ var e = document.getElementById("product"); >+ var u = document.getElementById("oldurl"); >+ var selCount = 0; >+ var selText = ""; >+ for (i=0; i < e.length; i++) { >+ if (e[i].selected) { >+ selCount = selCount + 1; >+ selText = e[i].value; >+ } >+ if (selCount > 2) break; >+ } Indent is off again. If statement after the for loop should be indented. > </script> > > >@@ -210,7 +230,7 @@ function doOnSelectProduct(selectmode) { > <table> > <tr valign="bottom"> > <th align="left"> >- <label for="component" accesskey="m"><a href="describecomponents.cgi">Co<u>m</u>ponent</a></label>: >+ <label for="component" accesskey="m"><a id="oldurl" onclick=doChangeLink() href="describecomponents.cgi">Co<u>m</u>ponent</a></label>: > </th> > </tr> > <tr valign="top"> please fix that and resubmit. Other than that it looks OK.

I hope this is now corrected to your requirements. I would like to point out however that the indentation is now different to the doOnSelectProduct() javascript routine above, the indentation of which I copied.

Comment on attachment 334515 [details] [diff] [review] Patch for bug 344606 V4 indent for second if is still off (which can be fixed on commit... however... ./runtest.pl fails b/c you have tabs in your file. Make sure your text editor replaces tabs with spaces. Or manually replace the tabs with 4 spaces. I know this seems super annoying but it's the little things that keeps the bugzilla treee happy.

Comment on attachment 334689 [details] [diff] [review] Patch for bug 344606 V5 > >+function doChangeLink() { >+ var e = document.getElementById("product"); >+ var u = document.getElementById("oldurl"); >+ var selCount = 0; >+ var selText = ""; >+ for (i=0; i < e.length; i++) { don't you need this to be e.options.length >+ if (e[i].selected) { And this to be e.options[i].selected >+ selCount = selCount + 1; >+ selText = e[i].value; Same here. Mostly I'm concerned about IE 6 getting angry, also helps with readability. >+ } >+ if (selCount > 2) break; >+ } >+ if (selCount == 1) { >+ u.href = ("describecomponents.cgi?product=" + selText); >+ } else { >+ u.href = ("describecomponents.cgi"); >+ } >+} >+ > </script> > > >@@ -210,7 +230,7 @@ function doOnSelectProduct(selectmode) { > <table> > <tr valign="bottom"> > <th align="left"> >- <label for="component" accesskey="m"><a href="describecomponents.cgi">Co<u>m</u>ponent</a></label>: >+ <label for="component" accesskey="m"><a id="oldurl" onclick=doChangeLink() href="describecomponents.cgi">Co<u>m</u>ponent</a></label>: The onclick should be in quotes and have a semicolon after it. > </th> > </tr> > <tr valign="top"> Sorry it took so long for me to get to this.

Wrote a new function called doChangeComponentLink and called it in doOnSelectProduct.

Comment on attachment 355013 [details] [diff] [review] javascript function to change the component link when a product is selected Joel is no longer doing reviews so let's try Guy Pyrzak instead because he reviewed previous patches and is our JS guru.

Comment on attachment 355013 [details] [diff] [review] javascript function to change the component link when a product is selected only nit i have is spacing is a bit funny on the if( start == -1) but that's easy to fix on commit.

Comment on attachment 355013 [details] [diff] [review] javascript function to change the component link when a product is selected >+ // The first selected option >+ var start = product.selectedIndex; >+ >+ // Return if no option is selected >+ if ( start == -1) >+ return; >+ >+ // If more than one option is selected, >+ // Set the main URL and return >+ for (var i = start+1; i < product.length; i++) { >+ if (product.options[i].selected) { >+ document.getElementById(id).href = "describecomponents.cgi"; >+ return; >+ } >+ } Instead of all this, you can just use selectedIndex directly. At least in Firefox, on a multi-select, selectedIndex is the index of the last item selected, which makes sense to me. Though actually, I think if there is more than one item selected, we probably should not be linking to a specific product for describecomponents.