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.
Created attachment 39913 [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.) 2) Ensure your system preferences for idreambook stuff is set. 3) Search for that book in OPAC and go to details. -- There should be a readometer image, but it will be broken. 4) Apply this patch 5) Refresh page -- readometer image appears. YAY! 6) run koha qa test tools
Created attachment 40105 [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.) 2) Ensure your system preferences for idreambook stuff is set. 3) Search for that book in OPAC and go to details. -- There should be a readometer image, but it will be broken. 4) Apply this patch 5) Refresh page -- readometer image appears. YAY! 6) run koha qa test tools Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
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>
Created attachment 40598 [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>
(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!