Sorting of Org Unit Names on "Choose a library to search" OPAC interface out of order

[Michael Peters]

When accessing the "Choose a library to search" button on the Evergreen OPAC using the Google Chrome browser, the sort order is by Org Unit ID rather than Alphabetical (as it is in Firefox, IE7, IE8, and Safari).

In my research, the only browser to experience this is Chrome. Currently, I am using Google Chrome 4.0.249.78 (36714), however the problem persists in later versions, as well.

My thought, is perhaps that it is sorting the OrgTree.js by the first value in this string - for example:

[2,2,1,"Zionsville Public Library",1,"ZPL"]] displays first, whereas in other browsers [7,2,1,"Adams Public Library",1,"APLS"] would sort first.

[Anoop Atre (anoop-atre)]

Confirmed the bug on both classic and craftsman themes in all Evergreen versions through 1.6.0.0

[Dan Scott (denials)]

The org unit names are generated in an array of arrays in alphabetical order in OrgTree.js. There is no sorting code in the OPAC, the browser just iterates through the array elements in order and displays the contents. I suspect that Chrom(e|ium)'s V8 JavaScript engine optimizes the array by changing the order of the elements using the first integer within the contained arrays (in the assumption that the elements will be accessed in numerical order).

And some Googling shows... http://code.google.com/p/v8/issues/detail?id=6 - "No one should ever use a for..in loop on an array anyway". Guess what we're using in org_utils.js?

[Mike Rylander (mrylander)]

Here are the files and counts within each where we're using for..in. I have not looked at them yet to make sure they're array accesses, but for anyone that wants to, converting to for(;;) should be fairly trivial.

miker@whirly:~/svn/head-ILS$ gack 'for \(\s*(?:var )?i in' --type=js --ignore-dir=chrome --ignore-dir=xul --ignore-dir=acq --ignore-dir=backend --ignore-dir=conify --ignore-dir=booking --ignore-dir=reports -c |grep -v 0$
Open-ILS/web/opac/common/js/opac_utils.js:1
Open-ILS/web/opac/common/js/jscalendar/calendar-setup.js:1
Open-ILS/web/opac/common/js/jscalendar/calendar.js:1
Open-ILS/web/opac/common/js/org_utils.js:4
Open-ILS/web/opac/skin/default/js/rdetail.js:1
Open-ILS/web/opac/skin/default/js/result_common.js:2
Open-ILS/web/js/dojo/MARC/Field.js:2
Open-ILS/web/js/dojo/MARC/Record.js:2
Open-ILS/web/js/dojo/openils/widget/TranslatorPopup.js:1
Open-ILS/web/js/dojo/openils/I18N.js:1
Open-ILS/web/js/dojo/fieldmapper/dojoData.js:9
Open-ILS/web/js/dojo/fieldmapper/OrgUtils.js:7
Open-ILS/web/js/dojo/fieldmapper/hash.js:2
miker@whirly:~/svn/head-ILS$

I'll take the Dojo stuff ... anyone want to look at the opac proper?

--miker

[Mike Rylander (mrylander)]

Moving on to the opac files...

[Dan Scott (denials)]

Shouldn't we be using dojo.forEach() wherever we can?

[Mike Rylander (mrylander)]

OK ... so, I've addressed all but Open-ILS/web/opac/skin/default/js/result_common.js, and the jscalendar stuff because I don't think we're using that anymore.

There is an outstanding question about orgArraySearcher in opac_utils.js and org_utils.js, and whether it needs some reimplementation love. Let's see how what we've done here fares in Chrom[e|ium] first.

--miker

[Mike Rylander (mrylander)]

Dan: probably, but I'll leave that for later. My goal here was just to get Chrome happy, for (heh) which for(;;) required less thought and care.

[Michael Peters (mrpeters)]

I hate to reopen an old bug, but this is happening again.

See the Evergreen Indiana OPAC (http://evergreen.lib.in.us) in Chrome or IE (maybe others?) and the org tree is improperly sorted. The org tree properly sorts in Firefox 9.

[Ben Shum (bshum)]

Hmm, never noticed the issue in IE, but it definitely seems to be affecting our 2.0 systems as well. Tested with IE8 and Chrome 16.0.912.77.

Marking confirmed to re-open bug ticket.

[Mike Rylander (mrylander)]

Since the fix is likely to be the same, and is really a bitesize bug, here's a rerun of the magic spell that identified likely culprits before:

miker@foolery:~/git/head-ILS (master)$ gack 'for \(\s*(?:var )?i in' --type=js --ignore-dir=chrome --ignore-dir=xul --ignore-dir=acq --ignore-dir=backend --ignore-dir=conify --ignore-dir=booking --ignore-dir=reports --ignore-dir=jscalendar -c| grep -v 0$
Open-ILS/web/opac/common/js/org_utils.js:2
Open-ILS/web/opac/common/js/opac_utils.js:1
Open-ILS/web/opac/skin/default/js/result_common.js:1
Open-ILS/web/js/dojo/openils/widget/FacetSidebar.js:1
Open-ILS/web/js/dojo/openils/AuthorityControlSet.js:1
Open-ILS/web/js/dojo/MARC/Field.js:1
Open-ILS/web/js/dojo/MARC/Record.js:1
Open-ILS/web/js/dojo/fieldmapper/dojoData.js:1
Open-ILS/web/js/dojo/fieldmapper/hash.js:1
Open-ILS/web/js/dojo/fieldmapper/OrgUtils.js:4

[Robert J Jackson (rjackson-deactivatedaccount)]

We have libraries that are reporting this issue from the general public OPAC use. With almost 100 libraries to chose from, having a non-sorted list makes it difficult to select the desired site.

[swenyu duan (dsy88)]

I'm a GSOC student and I'm fixing it now.

[Michael Peters (mrpeters)]

Thanks, Swenyu! We look forward to it.

[Remington Steed (rjs7)]

I developed a fix for this bug as a learning experience, so take it or leave it. I would have submitted it sooner, but I was wrestling with github. Swenyu is still welcome to submit his own fix also.

Here's my branch with the fix:

https://github.com/hektech/Evergreen/tree/lp520632

[Michael Peters (mrpeters)]

Remington -- I've tested your commit https://github.com/hektech/Evergreen/commit/995a87f70238e71f3a8dc918051972a660b56994 but it doesn't work. The search order is still incorrect.

http://evergreen.lib.in.us/opac/en-US/skin/default/xml/index.xml if you want to have a look for yourself.

[Remington Steed (rjs7)]

Michael, the change has two commits, and it looks like you only applied the last one. Here's the other. It's the next to last commit in the branch.

https://github.com/hektech/Evergreen/commit/d42c75c79eb7974e2c1b124c2838f4f6c02d6bd2

[Michael Peters (mrpeters)]

Remington,

No, i have that code too. The tree is still broken.

http://git.evergreen.lib.in.us/git/?p=Evergreen.INCustom/Evergreen/.git;a=commitdiff;h=fafbe81cf41066f57cddbcd3c561db5e3d52d3f8 is what I have installed, looks the same as what you have in your branch.

[Remington Steed (rjs7)]

Michael, sorry if I was unclear. My changes involve two files: org_utils.js and opac_utils.js. Your git repo shows the org_utils.js changes, but it's missing the opac_utils.js changes.

[Michael Peters (mrpeters)]

Sorry Remington, I suck. Totally blind, I guess.

That did the trick!

[Michael Peters (mrpeters)]

Signed off, and branchified this

user/mrpeters-isl/orgarraysearcher_fix_signoff

Thank you very much, Remington! Very glad to exterminate this one.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commitdiff;h=b372e3de4d2742fb6e635fae08f4ec3c234d5f00

[Michael Peters (mrpeters)]

Remington -- for future reference, a push to the evergreen-ils.org Git server is preferable to Github, as it makes it easier for folks to test, signoff, and commit your submission.

Some great information on getting started with that is available at http://evergreen-ils.org/dokuwiki/doku.php?id=dev:git

As for commit messages, at a minimum, the commit message should consist of a subject line (i.e., the first line of the commit message), then a blank line, then an optional description of the patch, followed by one or more signoffs. The subject line should be brief, ideally no more than 60-70 characters, and should include a bug number from LaunchPad if relevant. Here is an example of a minimum commit message:

LP#24544: fix the quuxifier
Signed-off-by: Jane Hacker <email address hidden>

If you need any help getting started with submitting code this way in the future, please feel free to reach out to me (or any of the other devs) via email, or the Evergreen IRC chat. (#evergreen on Freenode.net).

Thank you, again, for your quick work in solving this bug!

[Mike Rylander (mrylander)]

Merged to master. Thanks, Remington!

[Michael Peters (mrpeters)]

Folks, we just discovered that this breaks the OPAC login. When this code is in place, you can't click login or cancel. This is resolved after reverting the versions.

[Michael Peters (mrpeters)]

Disregard me. It was a bad merge from the Github branch to my 2.1.1 branch.

[Jason Stephenson (jstephenson)]

Given the discussion and the age of the fix, looks like fix released to me.