Bug 14345

Summary: parsed isbn has semi-colon fails to nicely link for idreambooks
Product: Koha Reporter: Mark Tompsett <mtompset>
Component: OPACAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: trivial    
Priority: P5 - low CC: jonathan.druart, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14318
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14318    
Bug Blocks:    
Attachments: Bug 14345: broken isbn logic prevents display of idreambooks image
Bug 14345: broken isbn logic prevents display of idreambooks image
Bug 14345: broken isbn logic prevents display of idreambooks image
Bug 14345: broken isbn logic prevents display of idreambooks image

Description Mark Tompsett 2015-06-05 16:01:03 UTC
While testing Bug 14318, I discovered this semicolon issue.

<img src="https://idreambooks.com/remotereadometer/9780812993257;.jpg" alt="The Road to Character by David Brooks" title="Rating based on reviews of The Road to Character">

The isbn logic didn't remove trailing semi-colon.
Comment 1 Mark Tompsett 2015-06-05 16:28:50 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2015-06-12 19:20:29 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2015-06-12 19:23:37 UTC
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
Comment 4 Nick Clemens 2015-06-12 20:13:04 UTC
(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
Comment 5 Jonathan Druart 2015-06-24 09:54:52 UTC
Silly question but... Why do you have an isbn ending in ';'?

Shouldn't the ibns be sent to the template once sanitized?
Comment 6 Mark Tompsett 2015-06-24 12:13:00 UTC
(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.
Comment 7 Jonathan Druart 2015-06-24 13:02:06 UTC
(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>
Comment 8 Mark Tompsett 2015-06-24 18:44:37 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2015-06-24 18:56:03 UTC
(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.
Comment 10 Jonathan Druart 2015-07-02 11:26:21 UTC
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>
Comment 11 Tomás Cohen Arazi 2015-07-08 17:41:28 UTC
Patch pushed to master.

Thanks Mark!