Bug 8550 - Z39.50 searches for ISBN/ISSN problematic
Summary: Z39.50 searches for ISBN/ISSN problematic
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low normal (vote)
Assignee: Jared Camins-Esakov
QA Contact: Mason James
URL:
Keywords:
: 8254 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-08-01 17:31 UTC by Jared Camins-Esakov
Modified: 2014-01-06 22:54 UTC (History)
8 users (show)

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


Attachments
Bug 8550: Z39.50 searches for ISBN/ISSN problematic (1.30 KB, patch)
2012-08-01 17:36 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8550: Z39.50 searches for ISBN/ISSN problematic - Signed Off (1.35 KB, patch)
2012-08-02 16:46 UTC, Joseph Alway
Details | Diff | Splinter Review
Bug 8550: restore ISSN field to Z39.50 search (2.88 KB, patch)
2012-09-02 12:03 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8550: restore ISSN field to Z39.50 search (2.89 KB, patch)
2012-09-03 00:28 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jared Camins-Esakov 2012-08-01 17:31:26 UTC
Copy cataloging via Z39.50 is often done by searching on ISBN or ISSN. Unfortunately, the code for doing this is... problematic, and may cause problems with some Z39.50 servers.
Comment 1 Jared Camins-Esakov 2012-08-01 17:36:31 UTC Comment hidden (obsolete)
Comment 2 Joseph Alway 2012-08-02 00:38:51 UTC
(In reply to comment #1)
> Created attachment 11273 [details] [review]
> Bug 8550: Z39.50 searches for ISBN/ISSN problematic

I finally got koha running off my git install and applied the patch. It works! :-)
Comment 3 Joseph Alway 2012-08-02 16:46:05 UTC
Created attachment 11304 [details] [review]
Bug 8550: Z39.50 searches for ISBN/ISSN problematic - Signed Off
Comment 4 Mason James 2012-08-10 06:11:49 UTC
(In reply to comment #3)
> Created attachment 11304 [details] [review]
> Bug 8550: Z39.50 searches for ISBN/ISSN problematic - Signed Off
> Signed-off-by: Joseph Alway <jbalway@gmail.com>

patch looks good, passing QA

mason@xen1:~/git/head$ koha-qa.pl 
        * bc4a87d Bug 8550: Z39.50 searches for ISBN/ISSN problematic
                cataloguing/z3950_search.pl

- perlcritic-progressive tests... OK
- perl -c syntax tests... OK
- xt/tt_valid.t tests... OK
- xt/author/valid-template.t tests... OK
Comment 5 Chris Cormack 2012-08-10 22:11:42 UTC
Pushed to 3.8.x, will be in 3.8.4

Thank you very much for your sign off Joseph
Comment 6 Paul Poulain 2012-08-31 16:33:41 UTC
Patch pushed
(Joseph Alway, if I'm not mistaking, it's your 1st signoff = congratulations & welcomed !)
Comment 7 Paul Poulain 2012-08-31 16:40:18 UTC
mmm... maybe i've be a little to fast in pushing this patch: Jared, in z3950_search.pl, I see:
<li><label for="isbn">ISBN/ISSN: </label> <input type="text" id="isbn" name="isbn" value="[% isbn %]" /></li>

There is no field called issn, so $query->param('issn') is always empty and that's some dead code (which was partially already here before, I agree)

Am I right or do I have missed something ?

(however, patch already pushed, but please answer -and provide a fix if needed !-)
Comment 8 Jared Camins-Esakov 2012-09-02 12:03:26 UTC Comment hidden (obsolete)
Comment 9 Jared Camins-Esakov 2012-09-02 12:04:35 UTC
Paul,

What you were missing was the second patch for this bug, which I failed to attach. I have now done so.
Comment 10 Chris Cormack 2012-09-03 00:27:32 UTC
Simple patch, provides the last of the clean up needed to undo a regression.
Comment 11 Chris Cormack 2012-09-03 00:28:00 UTC
Created attachment 11940 [details] [review]
Bug 8550: restore ISSN field to Z39.50 search

Sometime in 2009 or so, the ISSN field was removed from the Z39.50 search
page, and the ISBN field replaced with a combined ISBN/ISSN field. Since
this breaks ISBN search for most Z39.50 targets (due to the
differing treatment of ISBN vs. ISSN), this is a bug not a feature.

A future enhancement would be to use Business::ISBN to search for both
ISBN10 and ISBN13.

To test:
1) Apply patch.
2) Do a search for an ISBN using the ISBN field.
3) Do a search for an ISSN using the ISSN field.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 12 Mason James 2012-09-03 11:43:03 UTC
(In reply to comment #11)
> Created attachment 11940 [details] [review]
> Bug 8550: restore ISSN field to Z39.50 search
> 
> Sometime in 2009 or so, the ISSN field was removed from the Z39.50 search
> page, and the ISBN field replaced with a combined ISBN/ISSN field. Since
> this breaks ISBN search for most Z39.50 targets (due to the
> differing treatment of ISBN vs. ISSN), this is a bug not a feature.
> 
> A future enhancement would be to use Business::ISBN to search for both
> ISBN10 and ISBN13.
> 
> To test:
> 1) Apply patch.
> 2) Do a search for an ISBN using the ISBN field.
> 3) Do a search for an ISSN using the ISSN field.
> 
> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

looks good, passing QA...

$ koha-qa.pl

testing 1 commit(s) (applied to commit ec365ff)
 * 301dd72 Bug 8550: restore ISSN field to Z39.50 search
      koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/z3950_search.tt

- perlcritic-progressive tests...                 OK
- perl -c syntax tests...                         OK
- xt/tt_valid.t tests...                          OK
- xt/author/valid-template.t tests...             OK
- t/00-valid-xml.t tests...                       OK
Comment 13 Joseph Alway 2012-09-04 15:59:30 UTC
All of the same problems exist in the file /usr/share/koha/intranet/cgi-bin/acqui/z3950_search.pl as noted in bug 8680.
Comment 14 Paul Poulain 2012-09-05 13:54:50 UTC
Follow-up pushed to master (thx jared)
Comment 15 Chris Cormack 2012-09-06 07:46:41 UTC
Pushed to 3.8.x, will be in 3.8.5
Comment 16 Nicole C. Engard 2012-10-09 15:39:42 UTC
*** Bug 8254 has been marked as a duplicate of this bug. ***
Comment 17 Jared Camins-Esakov 2012-12-03 13:17:36 UTC
This is working in Master.
Comment 18 David Cook 2014-01-06 22:54:03 UTC
Comment on attachment 11304 [details] [review]
Bug 8550: Z39.50 searches for ISBN/ISSN problematic - Signed Off

The first patch was obsoleted, but is needed for folks doing a git bz apply for versions older than 3.8.4 (which hopefully they shouldn't need to do, but still...)