Bug 9728 - XISBN use simple search instead of SQL
Summary: XISBN use simple search instead of SQL
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Fridolin Somers
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-28 11:35 UTC by Fridolin Somers
Modified: 2014-12-07 20:02 UTC (History)
3 users (show)

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


Attachments
Proposed patch (2.80 KB, patch)
2013-02-28 11:55 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 9728: XISBN use simple search instead of SQL (2.86 KB, patch)
2013-03-21 13:56 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 9728: XISBN use simple search instead of SQL (2.91 KB, patch)
2013-06-05 01:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9728: XISBN unit test update (5.82 KB, patch)
2013-06-18 14:59 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2013-02-28 11:35:17 UTC
XISBN API uses normalized ISBN of a biblio to get a list of ISBNs, then searches via SQL in database for notices with those ISBNs.

I noticed that if input ISBN has hyphens then returned ISBNs have hyphens else they don't have. So SQL query will not find if ISBNs have hyphens in database.
Also, if biblio has several ISBN, only first one can be found with actual SQL query (isbn LIKE '$xisbn%').

So I propose to use a simple search "nb=$xisbn". This will find biblio from ISBN with or without hyphen.
Comment 1 Fridolin Somers 2013-02-28 11:55:09 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2013-03-20 01:22:33 UTC
This appears to be working, but I'm not sure what it means in the test plan, "Perform a simple search on thoses ISBNs : nb:1234567890."

The description of the problem describes issues with hyphens and multiple ISBNs. If the patch addresses those problems, how can I test those aspects of it?
Comment 3 Fridolin Somers 2013-03-21 12:52:33 UTC
> Perform a simple search on thoses ISBNs : nb:1234567890
I mean perform a search on the previously found ISBNs.
You can use the simple search field with a CCL query : "nb:1234567890" (replace 1234567890 with the ISBN) or the advanced search.
Comment 4 Fridolin Somers 2013-03-21 13:02:38 UTC
(In reply to comment #2)
> The description of the problem describes issues with hyphens and multiple
> ISBNs. If the patch addresses those problems, how can I test those aspects of it?

To test this, edit a matched record : 
- Multiple ISBN : add an new ISBN field after and before matching ISBN field and retest
- Hyphens : edit matched ISBN to remove or add hyphens and retest
Comment 5 Paul Poulain 2013-03-21 13:56:34 UTC
Patch tested with a sandbox, by jmbroust <jean-manuel.broust@univ-lyon2.fr>
Comment 6 Paul Poulain 2013-03-21 13:56:51 UTC Comment hidden (obsolete)
Comment 7 Chris Cormack 2013-06-05 01:59:54 UTC
Created attachment 18650 [details] [review]
Bug 9728: XISBN use simple search instead of SQL

XISBN API uses normalized ISBN of a biblio to get a list of ISBNs, then searches via SQL in database for notices with those ISBNs.

I noticed that if input ISBN has hyphens then returned ISBNs have hyphens else they don't have. So SQL query will not find if ISBNs have hyphens in database.
Also, if biblio has several ISBN, only first one can be found with actual SQL query (isbn LIKE '$xisbn%').

This patch replaces SQL query by a simple search "nb=$xisbn". This will find biblio from ISBN with or without hyphen.

Test plan :
- Activate FRBRizeEditions and XISBN sysprefs
- Go to a biblio witch has several editions
- Note its normalized ISBN (you may look in amazon links)
- Replace [ISBN] by biblio normalized ISBN in this URL : http://xisbn.worldcat.org/webservices/xid/isbn/[ISBN]?method=getEditions&format=xml&fl=form,year,lang,ed
- Go to this URL and see which ISBNs are returned
- Perform a simple search on thoses ISBNs : nb:1234567890
- Look at "Editions" tab
=> Check that diplayed biblios are the same you found by simple search

Signed-off-by: jmbroust <jean-manuel.broust@univ-lyon2.fr>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 8 Galen Charlton 2013-06-17 20:41:06 UTC
Pushed to master.  Thanks, Fridolyn!
Comment 9 Fridolin Somers 2013-06-18 12:07:21 UTC
Seems to have broken Unit Test.
I work on a followup to fix this.
Comment 10 Fridolin Somers 2013-06-18 14:59:13 UTC
Created attachment 19122 [details] [review]
Bug 9728: XISBN unit test update

Follow-up to update XISBN unit tests.
Since now XISBN uses search engin, I added a mock on C4::Search::SimpleSearch.pm.
A test for ThingISBN connexion already existed, I added on XISBN connexion.
Comment 11 Galen Charlton 2013-06-18 16:48:04 UTC
(In reply to comment #9)
> Seems to have broken Unit Test.
> I work on a followup to fix this.

Oops, I should have caught that.  I've pushed the follow-up to master.  Thanks, Fridolyn!
Comment 12 Tomás Cohen Arazi 2014-01-09 18:41:16 UTC
The UT updates have been pushed to 3.12.x, will be in 3.12.9.

Thanks Fridolin!