[Bug 17710] New: C4::Matcher::get_matches and C4::ImportBatch:: GetBestRecordMatch should use same logic
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 Bug ID: 17710 Summary: C4::Matcher::get_matches and C4::ImportBatch::GetBestRecordMatch should use same logic Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: MARC Bibliographic record staging/import Assignee: gmcharlt@gmail.com Reporter: dcook@prosentient.com.au QA Contact: testopia@bugs.koha-community.org 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-mult... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 --- Comment #1 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- Created attachment 58859 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=58859&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #58859|0 |1 is obsolete| | --- Comment #4 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 63771 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63771&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63771|0 |1 is obsolete| | CC| |kyle@bywatersolutions.com Status|Signed Off |Passed QA Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 64326 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64326&action=edit 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 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 17.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Patch complexity|--- |Trivial patch Status|Pushed to Master |Pushed to Stable --- Comment #7 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Pushed to 17.05.x, will be in 17.05.01 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #8 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- This patch has been pushed to 16.11.x and will be in 16.11.09. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17710 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com --- Comment #9 from Mason James <mtj@kohaaloha.com> --- Pushed to 16.05.x, for 16.05.15 release -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org