Bug 17710

Summary: C4::Matcher::get_matches and C4::ImportBatch::GetBestRecordMatch should use same logic
Product: Koha Reporter: David Cook <dcook>
Component: MARC Bibliographic record staging/importAssignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: fridolin.somers, katrin.fischer, kyle, mtj
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 17710 - C4::Matcher::get_matches and C4::ImportBatch::GetBestRecordMatch should use same logic
Bug 17710 - C4::Matcher::get_matches and C4::ImportBatch::GetBestRecordMatch should use same logic
Bug 17710 - C4::Matcher::get_matches and C4::ImportBatch::GetBestRecordMatch should use same logic

Description David Cook 2016-12-02 03:35:55 UTC
C4::Matcher::get_matches sorts its rows in descending order using score, while C4::ImportBatch::GetBestRecordMatch sorts its rows in descending order using score and record id. 

To get the best record match, C4::ImportBatch::GetBestRecordMatch takes the top result. 

By contrast, C4::Matcher::get_matches could easily return different rows in the top position if there are multiple rows with the same score, since its array is populated using a hash where order doesn't matter. 

It's an easy enough fix,  although I will admit to finding the solution on StackOverflow:

"Since <=> and cmp return 0 to indicate equality, and that's false, and because Perl's logical Boolean operators return the deciding value instead of just 0 or 1, sorting by multiple keys is as easy as stringing multiple comparisons together with or or ||"

http://stackoverflow.com/questions/10395383/sorting-an-array-of-hash-by-multiple-keys-perl
Comment 1 David Cook 2016-12-02 05:51:06 UTC
So:

@results = sort { $b->{'score'} cmp $a->{'score'} } @results;

Would become:

@results = sort {
    $b->{'score'} cmp $a->{'score'} or
    $b->{'record_id'} cmp $a->{'record_id'}
} @results;

--

I've already tested this and it works a treat. I'll post a patch next week I reckon.
Comment 2 David Cook 2017-01-11 04:35:31 UTC
I've written a patch, but not sure how to write the test plan for this one.

C4::ImportBatch::GetBestRecordMatch achieves its sort via SQL rather than Perl, so it's not like a common function could be used between them. 

I suppose I could refactor the sorting as a little function, but that doesn't really help with testing C4::Matcher::get_matches.
Comment 3 David Cook 2017-01-12 00:25:28 UTC
Created attachment 58859 [details] [review]
Bug 17710 - C4::Matcher::get_matches and C4::ImportBatch::GetBestRecordMatch should use same logic

C4::ImportBatch::GetBestRecordMatch uses SQL to sort by score descending
then candidate_match_id descending. With C4::Matcher::get_matches, I
implement the same sort but use Perl code to do it, since we're sorting
search results.

It's a simple change, but it's in a big block of code, so I don't have
unit tests.
Comment 4 Alex Buckley 2017-05-28 09:51:56 UTC
Created attachment 63771 [details] [review]
Bug 17710 - C4::Matcher::get_matches and C4::ImportBatch::GetBestRecordMatch should use same logic

C4::ImportBatch::GetBestRecordMatch uses SQL to sort by score descending
then candidate_match_id descending. With C4::Matcher::get_matches, I
implement the same sort but use Perl code to do it, since we're sorting
search results.

It's a simple change, but it's in a big block of code, so I don't have
unit tests.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 5 Kyle M Hall 2017-06-15 13:50:29 UTC
Created attachment 64326 [details] [review]
Bug 17710 - C4::Matcher::get_matches and C4::ImportBatch::GetBestRecordMatch should use same logic

C4::ImportBatch::GetBestRecordMatch uses SQL to sort by score descending
then candidate_match_id descending. With C4::Matcher::get_matches, I
implement the same sort but use Perl code to do it, since we're sorting
search results.

It's a simple change, but it's in a big block of code, so I don't have
unit tests.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Jonathan Druart 2017-06-15 19:07:48 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 7 Fridolin Somers 2017-06-20 08:30:32 UTC
Pushed to 17.05.x, will be in 17.05.01
Comment 8 Katrin Fischer 2017-06-21 06:28:12 UTC
This patch has been pushed to 16.11.x and will be in 16.11.09.
Comment 9 Mason James 2017-07-31 11:26:09 UTC
Pushed to 16.05.x, for 16.05.15 release