Bug 10500 - Improve isbn matching when importing records
Summary: Improve isbn matching when importing records
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic record staging/import (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 10 votes (vote)
Assignee: Kyle M Hall
QA Contact: Jonathan Druart
URL:
Whiteboard:
Keywords: rel_3_16_candidate
: 7382 (view as bug list)
Depends on:
Blocks: 11368 12243
  Show dependency treegraph
 
Reported: 2013-06-20 14:18 UTC by Kyle M Hall
Modified: 2014-12-07 20:07 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
WIP - Improve ISBN Matching (11.87 KB, patch)
2013-06-20 14:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10500 - Improve isbn matching when importing records (10.14 KB, patch)
2013-06-20 15:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10500 - Improve isbn matching when importing records (10.27 KB, patch)
2013-08-30 11:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10500 - Improve isbn matching when importing records (10.33 KB, patch)
2013-08-30 14:53 UTC, Melissa Lefebvre
Details | Diff | Splinter Review
Bug 10500: Followup for UNIMARC (793 bytes, patch)
2013-09-13 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10500 - Improve isbn matching when importing records (10.33 KB, patch)
2013-09-19 18:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10500: Followup for UNIMARC (795 bytes, patch)
2013-09-19 18:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10500 - Improve isbn matching when importing records (10.32 KB, patch)
2013-11-08 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10500: Followup for UNIMARC (795 bytes, patch)
2013-11-08 17:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10500 - Improve isbn matching when importing records (10.38 KB, patch)
2013-11-13 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10500: Followup for UNIMARC (856 bytes, patch)
2013-11-13 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
22 MARC records from B&T (13.47 KB, text/plain)
2013-11-14 00:55 UTC, Peggy Thrasher
Details
23 MARC records from OCLC (42.99 KB, text/plain)
2013-11-14 00:59 UTC, Peggy Thrasher
Details
Bug 10500 - Improve isbn matching when importing records (10.17 KB, patch)
2014-04-16 15:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10500: Followup for UNIMARC (858 bytes, patch)
2014-04-16 15:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10500 [QA Followup] - Add checks for empty isbn (1.05 KB, patch)
2014-04-16 15:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 10500 - Improve isbn matching when importing records (10.25 KB, patch)
2014-04-22 10:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 10500: Followup for UNIMARC (933 bytes, patch)
2014-04-22 10:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 10500 [QA Followup] - Add checks for empty isbn (1.12 KB, patch)
2014-04-22 10:29 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-06-20 14:18:58 UTC
The MARC standards do not require that isbn fields contain only the isbn itself, and may contain other data, e.g. "0394502884 (Random House) :". Do to this, it's possible for two records to fail to match based on isbn because though each my have the same valid isbn, the other data in the field can cause them to not match.
Comment 1 Kyle M Hall 2013-06-20 14:26:58 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-06-20 15:56:19 UTC Comment hidden (obsolete)
Comment 3 Koha Team Lyon 3 2013-06-25 08:02:52 UTC
tested on a biblibre sandbox:
pplying: Bug 10500 - Improve isbn matching when importing records
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Koha.pm
Auto-merging C4/Matcher.pm
CONFLICT (content): Merge conflict in C4/Matcher.pm
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging t/Koha.t
CONFLICT (content): Merge conflict in t/Koha.t
Failed to merge in the changes.
Comment 4 Kyle M Hall 2013-07-02 16:18:39 UTC
I'm afraid I was able to apply this patch cleanly to current master. I'm not sure why you are unable to apply it.
Comment 5 Kyle M Hall 2013-08-30 11:56:44 UTC Comment hidden (obsolete)
Comment 6 Tom McMurdo 2013-08-30 14:35:47 UTC
This has been tested and appears to work. I sign off on it.
Tom McMurdo
Collections & Digital Initiatives Librarian
Vermont Department of Libraries
thomas.mcmurdo@state.vt.us
Comment 7 Kyle M Hall 2013-08-30 14:36:51 UTC
*** Bug 7382 has been marked as a duplicate of this bug. ***
Comment 8 Melissa Lefebvre 2013-08-30 14:53:28 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2013-09-13 09:00:26 UTC
It does not work for me.

1/ In UNIMARC the isbn index is "ISBN". I will provide a followup.

2/ Even with this patch, I got "0 records with at least one match in catalog per matching rule "ISBN"".

Some debugging: If I add the line:
  warn "$source_key $target_key" unless $source_key eq $target_key;
to C4/Matcher.pm:770 (in the foreach @{ $matchchecks } loop).
I got, in logs:
  "0394502884 RANDOMHOUSE 0394502884 UNRANDOMHOUSE"

And C4::Matcher::get_matches always returns an empty array.

Marked as Failed QA (but maybe I am wrong somewhere...)
Comment 10 Jonathan Druart 2013-09-13 09:00:46 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2013-09-16 13:57:02 UTC
> 1/ In UNIMARC the isbn index is "ISBN". I will provide a followup.

Thanks!
 
> 2/ Even with this patch, I got "0 records with at least one match in catalog
> per matching rule "ISBN"".

It that with the new syspref on, off, or both?

> Some debugging: If I add the line:
>   warn "$source_key $target_key" unless $source_key eq $target_key;
> to C4/Matcher.pm:770 (in the foreach @{ $matchchecks } loop).
> I got, in logs:
>   "0394502884 RANDOMHOUSE 0394502884 UNRANDOMHOUSE"
> 
> And C4::Matcher::get_matches always returns an empty array.
> 
> Marked as Failed QA (but maybe I am wrong somewhere...)

You use use Data::Dumper on @source_keys on line 639? That will tell use if the isbn's are being transformed correctly. Does the unit test pass for you?
Comment 12 Jonathan Druart 2013-09-17 07:44:37 UTC
(In reply to Kyle M Hall from comment #11)
> > 1/ In UNIMARC the isbn index is "ISBN". I will provide a followup.
> 
> Thanks!
>  
> > 2/ Even with this patch, I got "0 records with at least one match in catalog
> > per matching rule "ISBN"".
> 
> It that with the new syspref on, off, or both?

both.

> > Some debugging: If I add the line:
> >   warn "$source_key $target_key" unless $source_key eq $target_key;
> > to C4/Matcher.pm:770 (in the foreach @{ $matchchecks } loop).
> > I got, in logs:
> >   "0394502884 RANDOMHOUSE 0394502884 UNRANDOMHOUSE"
> > 
> > And C4::Matcher::get_matches always returns an empty array.
> > 
> > Marked as Failed QA (but maybe I am wrong somewhere...)
> 
> You use use Data::Dumper on @source_keys on line 639? That will tell use if
> the isbn's are being transformed correctly.

Yes, I get: ['0-394-50288-4', '0-394-50288-4', '978-0-394-50288-5', '0394502884', '9780394502885']

> Does the unit test pass for you?

Yes.
Comment 13 Kyle M Hall 2013-09-19 18:38:06 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-09-19 18:38:17 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2013-09-19 18:49:09 UTC
Jonathan, I'm unable to reproduce your problem. The output you posted looks good and correct. Are you rebuilding your zebra index after step 1?
Comment 16 Jonathan Druart 2013-09-20 08:30:06 UTC
(In reply to Kyle M Hall from comment #15)
> Jonathan, I'm unable to reproduce your problem.

Ok, I think I found the problem. I suspect you don't have a "Required match checks" defined for your isbn matching rule. I have a check defined as the matchpoint component (010$a, 0, 0) for source and target.
I don't know what is this match check and I don't know if your patch must impact them.
Comment 17 Tom McMurdo 2013-10-29 18:21:59 UTC
Just following up: where are we in QA with this ISBN matching fix? I don't mean to seem impatient, but I know that our consortium is anxious to see this added. Let me know what comes next.
Thanks.
Comment 18 Kyle M Hall 2013-11-08 17:10:46 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2013-11-08 17:10:55 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2013-11-08 17:18:48 UTC
(In reply to Jonathan Druart from comment #16)
> (In reply to Kyle M Hall from comment #15)
> > Jonathan, I'm unable to reproduce your problem.
> 
> Ok, I think I found the problem. I suspect you don't have a "Required match
> checks" defined for your isbn matching rule. I have a check defined as the
> matchpoint component (010$a, 0, 0) for source and target.
> I don't know what is this match check and I don't know if your patch must
> impact them.

Thanks for the additional data. I don't think this is really a problem with this patch. When you add a match check, it's looking for exact values between two marc fields.

I think it makes sense for the match to fail in this case, as you're causing the matching to be much for strict, while this patch is allowing for less strict matching.

Does that make sense?
Comment 21 Jonathan Druart 2013-11-13 15:34:04 UTC
(In reply to Kyle M Hall from comment #20)
> Thanks for the additional data. I don't think this is really a problem with
> this patch. When you add a match check, it's looking for exact values
> between two marc fields.

The code looks for exact values but does it what it should do?
We can imagine a matching rule on the title and a match check on the isbn. The match check on the isbn could use the variations of isbn too.
Comment 22 Jonathan Druart 2013-11-13 15:35:29 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2013-11-13 15:35:33 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2013-11-13 15:37:02 UTC
I don't think it is blocker and I don't know if it should be required. I let the RM to decide.

Marked as Passed QA.
Comment 25 mathieu saby 2013-11-13 18:05:19 UTC
From the title of the bug, I thought it was about the ISBN13/ISBN10 matching, but it is not the case.
Do you plan to do something too for that ?

Mathieu
Comment 26 Peggy Thrasher 2013-11-14 00:55:11 UTC
Created attachment 22918 [details]
22 MARC records from B&T
Comment 27 Peggy Thrasher 2013-11-14 00:59:31 UTC
Created attachment 22919 [details]
23 MARC records from OCLC

I'm really  looking forward to this fix!  We order from B&T and import their brief MARC records.  When the books arrive, we download complete records from OCLC and overlay the brief ones.  I am supplying 2 files from today at my library, OriginalMARCRecords is 22 MARC records from B&T.  NewMARCREcords is 23 matching MARC records from OCLC (there is an extra record in this file because one of the titles is in here twice).  Of the 22 records, only 11 overlayed.  This was my first time using a sandbox, so I'd repeat my test if I were you...  Hopefully this will point out some scenarios that you didn't envision.
Peggy
Comment 28 Peggy Thrasher 2013-11-14 01:43:36 UTC
My apologies.  When I use it correctly it works wonderfully.  23 matches.  My catalogers can't wait!!
Comment 29 Nicole C. Engard 2014-03-25 01:09:45 UTC
Peggy found an error - well we found an error when Peggy reported a problem so I'm marking this as Failed QA until it's fixed.

That patch for the ISBN normalizing was throwing the error if the record contained NULL for the isbn field. 

Kyle can explain/fix.
Comment 30 Kyle M Hall 2014-04-16 15:01:29 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2014-04-16 15:01:38 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2014-04-16 15:05:12 UTC Comment hidden (obsolete)
Comment 33 Martin Renvoize 2014-04-22 10:26:25 UTC
Created attachment 27387 [details] [review]
[PASSED QA] Bug 10500 - Improve isbn matching when importing records

Test Plan:
1) Catalog a record with the ISBN "0394502884 (Random House)"
2) Export the record, edit it so the ISBN is now "0394502884 (UnRandomHouse)"
3) Using the record import tool, import this record with matching on ISBN.
4) You should not find a match
5) Apply this patch
6) Run updatedatabase.pl
7) Enable the new system preference AggressiveMatchOnISBN
8) Repeat step 3
9) The tool should now find a match

Signed-off-by: Tom McMurdo <thomas.mcmurdo@state.vp.us>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2014-04-22 10:26:33 UTC
Created attachment 27388 [details] [review]
[PASSED QA] Bug 10500: Followup for UNIMARC

In UNIMARC, the isbn index is ISBN.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2014-04-22 10:29:38 UTC
Created attachment 27389 [details] [review]
[PASSED QA] Bug 10500 [QA Followup] - Add checks for empty isbn

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2014-04-22 10:30:31 UTC
I'm happy that the followup fixes the problem.  We're still passing qa script tests and the code looks good to me.

Re-passing QA.
Comment 37 Galen Charlton 2014-05-05 19:35:23 UTC
Pushed to master.  Thanks, Kyle!

Please note the follow-up.  The original patch did not work correctly with QueryParser, and unfortunately, there's a deeper bug in QP that is getting in the way.  Consequently, the follow-up ensures that AggressiveMatchOnISBN doesn't make any change in behavior when UseQueryParser is on.