Bug 14345

Took me a while here, the missing step is "Have an ISBN ending in ; (with no space before the punctuation)"

It fixes a problem caused by questionable punctuation in record, but it doesn't hurt anything that I see

(In reply to Nick Clemens from comment #3)
> Took me a while here, the missing step is "Have an ISBN ending in ; (with no
> space before the punctuation)"
> 
> It fixes a problem caused by questionable punctuation in record, but it
> doesn't hurt anything that I see

Ah, forgive me, this actually crops up when you have more than one isbn or an ISBN with no punctuation.  It's pulling the isbn from the document so any added punctuation (semicolon between multiples or period if single) is breaking the link.  Seems to be affecting both SSL and non-SSL as well

Silly question but... Why do you have an isbn ending in ';'?

Shouldn't the ibns be sent to the template once sanitized?

(In reply to Jonathan Druart from comment #5)
> Silly question but... Why do you have an isbn ending in ';'?
> Shouldn't the ibns be sent to the template once sanitized?

Because it is parsed from idreambooks in javascript, not from the perl.

(In reply to M. Tompsett from comment #6)
> (In reply to Jonathan Druart from comment #5)
> > Silly question but... Why do you have an isbn ending in ';'?
> > Shouldn't the ibns be sent to the template once sanitized?
> 
> Because it is parsed from idreambooks in javascript, not from the perl.

And the first part for the question? :)

Just looking at the code, the isbns are retrieved from:

 228                         [% IF ( MARCISBNS ) %]
 229                             <span class="results_summary isbn"><span class="label">ISBN:</span>
                                 [% FOREACH MARCISBN IN MARCISBNS %]
                                     [% MARCISBN %][% IF ( loop.last ) %].[% ELSE %]; [% END %]
                                 [% END %]
                                 </span>
 230                         [% ELSE %]
 231                             [% IF ( normalized_isbn ) %]
 232                                 <span class="results_summary isbn"><span class="label">ISBN: </span>[% normalized_isbn %]</span>
 233                             [% END %]
 234                         [% END %]

(I added some indentation for the readability).

Does the ';' you want to remove is the one generated by [% IF ( loop.last ) %].[% ELSE %]; [% END %]?

Because looking at the JS:
    var isbn = $(".isbn:last").text().split(" ")[1];
It seems that several .isbn is expected, but only one is created.

Maybe the template should be more something like:
<span class="results_summary isbns"><span class="label">ISBN:</span>
    [% FOREACH MARCISBN IN MARCISBNS %]
        <span class="isbn"[% MARCISBN %]</span>[% IF ( loop.last ) %].[% ELSE %]; [% END %]
    [% END %]
</span>

(In reply to Jonathan Druart from comment #7)
> Does the ';' you want to remove is the one generated by [% IF ( loop.last )
> %].[% ELSE %]; [% END %]?

Yes, if:
1) there are multiple 020$a's on the biblio record, and the first one is just a number. OR
No, if:
2) someone intentionally types multiple ISBN's into 020$a on a biblio record with the first one as a number followed immediately by semi-colon as a delimiter.

We have no ISBN validation in the MARC Editor area.


> Because looking at the JS:
>     var isbn = $(".isbn:last").text().split(" ")[1];
> It seems that several .isbn is expected, but only one is created.

Actually, I was surprised that alert($(".isbn:last").text()) actually generated that entire ISBN line. It's expecting spaces. The [1] is the first ISBN in the list.


> Maybe the template should be more something like:
> <span class="results_summary isbns"><span class="label">ISBN:</span>
>     [% FOREACH MARCISBN IN MARCISBNS %]
>         <span class="isbn"[% MARCISBN %]</span>[% IF ( loop.last ) %].[%
> ELSE %]; [% END %]
>     [% END %]
> </span>

In order to make the semantics clearer, perhaps, but if you changed that just change .isbn to .isbns in the JS code section after your suggested change, you'd get the equivalent problem.

If we make the template change, we may have other things to test as well. This is the quickest fix that is easily testable.

Created attachment 40760 [details] [review]
Bug 14345: broken isbn logic prevents display of idreambooks image

TEST PLAN
---------
1) Ensure you have a book that would pull up an idreambooks image
   (e.g. The road to character / David Brooks.)
   -- I imported from the Library of Congress via z39.50
2) Ensure your system preferences for idreambook stuff is set.
3) Ensure you have multiple 020$a's for your book, and
   the first one must just be an ISBN number!
   -- I removed the '(hardcover: acidfree paper)' from the first.
   -- I took the second 020 field and changed 020$z to 020$a
4) Search for that book in OPAC and go to details.
   -- There should be a readometer image, but it will be broken.
   -- Notice the ISBN line has "#############; ..."
      This could also be attained by just putting two semi-colon
      separated ISBN's on the first 020$a.
5) Apply this patch
6) Refresh page
   -- readometer image appears. YAY!
7) run koha qa test tools

NOTE: Only changed test plan, so leaving sign-off on.

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>

Patch pushed to master.

Thanks Mark!