Bug 17710 - C4::Matcher::get_matches and C4::ImportBatch::GetBestRecordMatch should use same logic
Summary: C4::Matcher::get_matches and C4::ImportBatch::GetBestRecordMatch should use s...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic record staging/import (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-02 03:35 UTC by David Cook
Modified: 2018-12-03 20:03 UTC (History)
4 users (show)

See Also:
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 (1.20 KB, patch)
2017-01-12 00:25 UTC, David Cook
Details | Diff | Splinter Review
Bug 17710 - C4::Matcher::get_matches and C4::ImportBatch::GetBestRecordMatch should use same logic (1.26 KB, patch)
2017-05-28 09:51 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17710 - C4::Matcher::get_matches and C4::ImportBatch::GetBestRecordMatch should use same logic (1.31 KB, patch)
2017-06-15 13:50 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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