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.

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.

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

*** Bug 7382 has been marked as a duplicate of this bug. ***

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...)

> 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?

(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.

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?

(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.

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.

(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?

(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.

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.

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

Created attachment 22918 [details]
22 MARC records from B&T

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

My apologies.  When I use it correctly it works wonderfully.  23 matches.  My catalogers can't wait!!

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.

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>

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>

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>

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.

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.