Bug 12266 - The z39.50 authority search to create new auth record works randomly
Summary: The z39.50 authority search to create new auth record works randomly
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P3 normal (vote)
Assignee: Galen Charlton
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-15 13:45 UTC by Zeno Tajoli
Modified: 2015-06-04 23:23 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12266 - The z39.50 authority search to create new auth record works randomly (2.05 KB, patch)
2014-06-02 16:19 UTC, Marc Véron
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Zeno Tajoli 2014-05-15 13:45:49 UTC
Hi to all,

I'm checking the actual master version.
I'm using FF 29.0.1 on Win7.

The new feature "New from Z39.50" in staff page for authority (../cgi-bin/koha/authorities/authorities-home.pl) doesn't work well.

Sometime I click the button and the search box in authorty z39.50 appers (and it works).

Sometime I click the button and nothing relevant appens.
I only see that address bar in the browser became
http://<mysite>:8080/cgi-bin/koha/authorities/authorities-home.pl#

I think that there is same problem on js site, with firebug I see an error on jquery:

SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data
The link in firebug send me to this function:

function showMergingInProgress() {
var alreadySelected = $.cookie('auth_to_merge');
if (alreadySelected !== null) {
alreadySelected = JSON.parse(alreadySelected);
$('#merge_in_progress').html(_("Merging with authority: ") + "<a href='detail.pl?authid=" + alreadySelected.authid + "'><span class='authorizedheading'>" + alreadySelected.summary + "</span> (" + alreadySelected.authid + ")</a> <a href='#' id='cancel_merge'>" + _("Cancel merge") + "</a>");
$('#cancel_merge').click(function(event) {
event.preventDefault();
$.cookie('auth_to_merge', '', { 'path': '/', 'expires': -1 });
$('#merge_in_progress').empty();
});
} else {
$('#merge_in_progress').empty();
}
} 

I see also this warning:
"Empty string passed to getElementById()"
with a link to Jquery line 2 (too long to be insert here).

But sometimes all is OK and I don't see those waring and error.
Comment 1 Marc Véron 2014-06-02 15:41:51 UTC
Hi to all,

With Koha version 3.17.00.007 (Current Master), I get no Z39.50 search at all. With 3.14.01.000 it works as expected.

Tested with Firefox 29.01 on Win 7. All popups allowed.
Same behaviour with Chrome 35.0.1916.114 m

To reproduce:

Step 1)
------
Go to .../cgi-bin/koha/authorities/authorities-home.pl

Result:
Error in console: 
Time: 02.06.2014 16:03:24
Error: SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data
Source: .../cgi-bin/koha/authorities/authorities-home.pl
Line: 126
This line reads:
alreadySelected = JSON.parse(alreadySelected);

Step 2)
------
Click "New from Z39.50"

Result:
Popup window should open but nothing happens.
URL changes to 
.../cgi-bin/koha/authorities/authorities-home.pl#
(same as in Zeno's remarks)

3 identical warnings in console:
Time: 02.06.2014 16:07:24
Warning: Empty string passed to getElementById().
Source: .../intranet-tmpl/lib/jquery/jquery.js
Line: 2

--

I change importance to P3 (Seriously broken, but not high impact)

Oh, found: 
There are two lines: 
if (alreadySelected !== null)
but this works only when the variable alreadySelected is defined. 

I add a test for defined - the Popup works. (However it's puzzling that the same code worked with 3.14)

Patch follows.
Comment 2 Marc Véron 2014-06-02 16:19:04 UTC
Created attachment 28620 [details] [review]
Bug 12266 - The z39.50 authority search to create new auth record works randomly

Test scenario:

Make sure that Z39.50 servers for authorities are defined (e.g. form Library of Congress)
Go to Authorities
Click on "Z39.50 search"
Without patch it can happen that the popup window for Z39.50 does not show up
Apply patch
Reload page
Click again on "Z39.50 search"
Pop up window should now display as expected.
Comment 3 Ian Palko 2014-06-05 15:47:30 UTC
I manually edited the authorities_js.inc on my test system according to Marc's patch. It restored the past functionality of the z39.50 add new authority setup, which we use often (we're currently cleaing up our authorities database and standardizing it).

Pushed to our production system after testing and also seems to fix the problem.
Comment 4 Marc Véron 2014-06-05 16:37:29 UTC
Hi Ian,

Is it possible to sign off?

Thanks

Marc
Comment 5 giuseppe.angilella 2014-06-09 04:18:32 UTC
Same problem here (after a package update).

Fixed by manually editing authorities_js.inc according to Marc Véron's patch.

15 minutes to spot the problem, (quench panic), and find your solution: Many thanks!
Comment 6 Zeno Tajoli 2014-06-11 05:59:15 UTC
Hi Marc,

in fact bug 12295 is fixed working on the same lines of your patch.
Clear you need to rebase, it is possible that the bug is over.
Comment 7 Marc Véron 2014-06-11 14:16:49 UTC
Hi Zeno,

After the latest pull (3.17.00.007, including Bug 12295), the z39.50 search window pops up as expected.

However I have a question:

Original code:
    var alreadySelected = $.cookie('auth_to_merge');
    if (alreadySelected !== null) {
	(...)
	}
	
if-statement in Patch for Bug 12295:
    if (alreadySelected !== undefined)
	
if-statement in Patch for Bug 12266:
    if ((typeof alreadySelected !== 'undefined') && (alreadySelected !== null))
	
Question:
For some reason there was a test for null in the original code. 
Is it ever possible that $.cookie('auth_to_merge') results to null? If yes, I suppose that it should be tested as well.
Comment 8 Bernardo Gonzalez Kriegel 2014-06-11 14:38:23 UTC
(In reply to Marc Véron from comment #7)
> 	
> Question:
> For some reason there was a test for null in the original code. 
> Is it ever possible that $.cookie('auth_to_merge') results to null? If yes,
> I suppose that it should be tested as well.

On Bug 12295: "... newer versions of jquery-cookie changed the return of
$.cookie('foo') from null to undefined when the cookie is not present."

Seems that checking for null is not required.
Comment 9 Marc Véron 2014-06-11 14:49:54 UTC
Thanks, Bernardo.
I change the status to RESOLVED FIXED
Comment 10 Ian Palko 2014-06-11 20:48:31 UTC
(In reply to Marc Véron from comment #4)
> Hi Ian,
> 
> Is it possible to sign off?
> 
> Thanks
> 
> Marc

Marc,

I'm not yet very experienced in git, just a amateur hacker, so don't even know how to go about doing a sign off.

Perhaps this summer I need to take a bit of free time and learn.

Thanks for the work on the bug.
Comment 11 Marc Véron 2014-06-12 08:37:22 UTC
Ian,

In the meantime the Bug is fixed, no sign-off necessary.

BTW regarding Sign-off and Git:
http://wiki.koha-community.org/wiki/Version_Control_Using_Git
http://wiki.koha-community.org/wiki/Git_bz_configuration
...and on the IRC channel you always find great support
Comment 12 Jonathan Druart 2014-06-12 09:35:43 UTC
You can also test and sign off patches using sandboxes http://wiki.koha-community.org/wiki/Sandboxes