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
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.
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.
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.
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>
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>
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.01
This patch has been pushed to 16.11.x and will be in 16.11.09.
Pushed to 16.05.x, for 16.05.15 release