When trying to Match MARCXML-records from Zebra using Required match checks, C4::Matcher::_passes_required_checks() tries to make a MARC::Record out of MARCXML while thinking it is USMARC. This leads to matching records getting not-matched, and subsequently importing the same records again and again. Very hard to debug or create a test case. I accidentally noticed this while building an automatic acquisitions record overlay mechanism. There are no big errors, simply no match is found, even though a match exists. --------- TEST PLAN --------- 1. Create a Matcher with "Required match checks" in addition to normal checks. 2. Find matches using the C4::Matcher->get_matches() and notice that you don't find matches. 3. Somebody who is actually using this Matcher could help me out here with defining a test plan.
Created attachment 29758 [details] [review] le patch
Also refactored the Matcher a bit to prevent excessive MARC::Record creation for each "Required match check" for each search results. This probably would cripple any reasonable batch Matching operations.
Setting this to needs signoff, because there is no "needs help" -status
Created attachment 29948 [details] [review] Bug 12586 - Record matching rules - Required match checks doesn't work with MARCXML When trying to Match MARCXML-records from Zebra using Required match checks, C4::Matcher::_passes_required_checks() tries to make a MARC::Record out of MARCXML while thinking it is USMARC. This leads to matching records getting not-matched, and subsequently importing the same records again and again. Very hard to debug or create a test case. I accidentally noticed this while building an automatic acquisitions record overlay mechanism. There are no big errors, simply no match is found, even though a match exists. Also refactored the Matcher a bit to prevent excessive MARC::Record creation for each "Required match check" for each search results. This probably would cripple any reasonably sized batch Matching operations. --------- TEST PLAN --------- 1. Create a Matcher with "Required match checks" in addition to normal checks. 2. Find matches using the C4::Matcher->get_matches() and notice that you don't find matches. 3. Somebody who is actually using this Matcher could help me out here with defining a test plan.
Comment on attachment 29948 [details] [review] Bug 12586 - Record matching rules - Required match checks doesn't work with MARCXML Review of attachment 29948 [details] [review]: ----------------------------------------------------------------- I ran into this problem today and this is pretty much exactly what I had in mind for a fix. I've just added a couple comments about things to change. Suggested test plan: 1) Create a bibliographic record with a control number of 123456 2) Create a matching rule with the following: a) Search index "Control-number" b) Score 100 c) Matchpoint components tag 001 d) Match check Source Tag 001 e) Match check Target Tag 001 3) Download the bibliographic from Step 1 as ISO MARC 4) Upload it via Stage Marc for Import, and use the record matching rule from Step 2 Before applying patch: 5) Note that the no records match with your rule Apply the patch: 6) Repeat Step 4 (ie upload using the matching rule from step 2) 7) Note that the record DOES match using the matching rule ::: C4/Matcher.pm @@ +689,5 @@ > + foreach my $matched ( @{$searchresults} ) { > + my $record = C4::Search::new_record_from_zebra( 'biblioserver', $matched ); > + $matches{$record}->{score} += $matchpoint->{'score'}; #Using $record HASH string representation as the key :) > + $matches{$record}->{record} = $record; > + } This block all looks good. @@ +704,1 @@ > keys %matches unless ($self->{'record_type'} eq 'auth'); There's no such record_type as 'auth'... this should be 'authority'. I know it's not related to this bug per se, but may as well change it now. @@ +711,4 @@ > my $record_number; > my $result = C4::Biblio::TransformMarcToKoha(C4::Context->dbh, $target_record, ''); > $record_number = $result->{'biblionumber'}; > + push @results, { 'record_id' => $record_number, 'score' => $matches{$hashkey}->{score}, 'target_record' => $target_record, 'target_biblio' => $result }; Why have you included "target_record" and "target_biblio" here? This seems unnecessary and is an addition to the original code. If it's being used elsewhere, I think that should be separated into a separate patch. @@ +760,4 @@ > } > > sub _passes_required_checks { > + my ($source_record, $target_record, $matchchecks) = @_; Looks good.
Created attachment 46380 [details] [review] Bug 12586 - Record matching rules - Required match checks doesn't work with MARCXML When trying to Match MARCXML-records from Zebra using Required match checks, C4::Matcher::_passes_required_checks() tries to make a MARC::Record out of MARCXML while thinking it is USMARC. This leads to matching records getting not-matched, and subsequently importing the same records again and again. Very hard to debug or create a test case. I accidentally noticed this while building an automatic acquisitions record overlay mechanism. There are no big errors, simply no match is found, even though a match exists. Also refactored the Matcher a bit to prevent excessive MARC::Record creation for each "Required match check" for each search results. This probably would cripple any reasonably sized batch Matching operations. --------- TEST PLAN --------- 1. Create a Matcher with "Required match checks" in addition to normal checks. 2. Find matches using the C4::Matcher->get_matches() and notice that you don't find matches. 3. Somebody who is actually using this Matcher could help me out here with defining a test plan. Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 46381 [details] [review] Bug 12586 - Record matching rules - Required match checks doesn't work with MARCXML (follow-up) The follow-up fixes a typo ('auth' to 'authority') and removes 'target_result' and 'target_biblio' which were introduced, seemingly unnecessarily, by the first patch in the set.
So I've signed off Olli's original fix, and I added a follow-up to fix a typo in a related bit of code, and to remove some seemingly superfluous data added by Olli's fix.
Comment on attachment 46380 [details] [review] Bug 12586 - Record matching rules - Required match checks doesn't work with MARCXML Review of attachment 46380 [details] [review]: ----------------------------------------------------------------- ::: C4/Matcher.pm @@ +703,5 @@ > + foreach my $matched ( @{$searchresults} ) { > + my $record = C4::Search::new_record_from_zebra( 'biblioserver', $matched ); > + $matches{$record}->{score} += $matchpoint->{'score'}; #Using $record HASH string representation as the key :) > + $matches{$record}->{record} = $record; > + } Actually, looking at this again, there's no point repeating the loop... it would've made more sense to have 1 foreach loop and done the record_type checks within it... Not a big deal though really
Maybe also worth mentioning that this fix ONLY fixes match checks for bibliographic records... Match checks don't work for authority records. More refactoring could be done to fix that... but that's outside the scope of this bug I think.
Created attachment 54278 [details] [review] Bug 12586 - Record matching rules - Required match checks doesn't work with MARCXML (follow-up) The follow-up fixes a typo ('auth' to 'authority') and removes 'target_result' and 'target_biblio' which were introduced, seemingly unnecessarily, by the first patch in the set. Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se>
I added a sign-off since the patch works as intended. I'm unsure why it didn't already have this status since it appears that it has previously been signed-off by David Cook (see comment #6)?
Interesting read in this context - I wasn't aware of the database side check option before: http://bywatersolutions.com/2013/07/23/match-points-in-koha/
This works in my tests, but is missing unit tests. Would it be possible to add a unit test covering the new behaviour here?
Wahou, dirty bug. In my opinion, its not an enhancement : matching checks are broken.
(In reply to Fridolin SOMERS from comment #15) > Wahou, dirty bug. > In my opinion, its not an enhancement : matching checks are broken. Do you mean that it's a bug fix rather than an enhancement?
Olli, any chances to see you provide tests for these changes?
(In reply to David Cook from comment #16) > (In reply to Fridolin SOMERS from comment #15) > > Wahou, dirty bug. > > In my opinion, its not an enhancement : matching checks are broken. > > Do you mean that it's a bug fix rather than an enhancement? Yes
I will try to work on this
Hmm, are you sure that this is actually needed? I think that this one might be invalid now. Bug 26199 uses required match checks and MARCXML and it works correctly.
Looks like it has been fixed by Bug 24348