Bug 12586 - Record matching rules - Required match checks doesn't work with MARCXML
Summary: Record matching rules - Required match checks doesn't work with MARCXML
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Victor Grousset/tuxayo
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-16 14:51 UTC by Olli-Antti Kivilahti
Modified: 2018-06-14 13:06 UTC (History)
8 users (show)

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


Attachments
le patch (4.53 KB, patch)
2014-07-16 15:02 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 12586 - Record matching rules - Required match checks doesn't work with MARCXML (4.74 KB, patch)
2014-07-22 08:52 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 12586 - Record matching rules - Required match checks doesn't work with MARCXML (4.77 KB, patch)
2016-01-07 03:26 UTC, David Cook
Details | Diff | Splinter Review
Bug 12586 - Record matching rules - Required match checks doesn't work with MARCXML (follow-up) (1.67 KB, patch)
2016-01-07 03:26 UTC, David Cook
Details | Diff | Splinter Review
Bug 12586 - Record matching rules - Required match checks doesn't work with MARCXML (follow-up) (1.81 KB, patch)
2016-08-10 15:32 UTC, Andreas Hedström Mace
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2014-07-16 14:51:08 UTC
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.
Comment 1 Olli-Antti Kivilahti 2014-07-16 15:02:26 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2014-07-16 15:04:07 UTC
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.
Comment 3 Olli-Antti Kivilahti 2014-07-16 15:04:45 UTC
Setting this to needs signoff, because there is no "needs help" -status
Comment 4 Olli-Antti Kivilahti 2014-07-22 08:52:39 UTC Comment hidden (obsolete)
Comment 5 David Cook 2016-01-07 03:20:16 UTC
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.
Comment 6 David Cook 2016-01-07 03:26:14 UTC
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>
Comment 7 David Cook 2016-01-07 03:26:20 UTC Comment hidden (obsolete)
Comment 8 David Cook 2016-01-07 03:27:32 UTC
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 9 David Cook 2016-01-07 03:32:59 UTC
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
Comment 10 David Cook 2016-01-07 03:33:51 UTC
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.
Comment 11 Andreas Hedström Mace 2016-08-10 15:32:18 UTC
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>
Comment 12 Andreas Hedström Mace 2016-08-11 15:08:05 UTC
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)?
Comment 13 Katrin Fischer 2016-08-24 23:52:33 UTC
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/
Comment 14 Katrin Fischer 2016-08-28 22:12:56 UTC
This works in my tests, but is missing unit tests. 
Would it be possible to add a unit test covering the new behaviour here?
Comment 15 Fridolin SOMERS 2016-09-23 10:13:24 UTC
Wahou, dirty bug.
In my opinion, its not an enhancement : matching checks are broken.
Comment 16 David Cook 2016-09-26 05:13:56 UTC
(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?
Comment 17 Jonathan Druart 2016-09-30 09:48:27 UTC
Olli, any chances to see you provide tests for these changes?
Comment 18 Fridolin SOMERS 2016-11-08 08:09:18 UTC
(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