Currently, there's a free text field on matchpoint definition, that is of no use, as the code does some default normalization no matter what you put there (se screenshot). As pointed out on 17304 and other bugs, this normalization routine is counter productive. The purpose of this bug is to introduce a way for defining the normalization rules we'd like to apply. IT will make use of bug 17302 to have a small (but useful) list of normalization routines. This could be expanded if needed, or we could even make plugins do it (that'd certaintly belong to another bug report). I don't think removing the 'feature' is a good move now, but we could consider it with broader discussion. Someone should rise it in koha-devel, meetings, etc. This will provide a way out, until we end that fine-grained discussion.
Created attachment 55695 [details] 'Normalization rule' on the UI
Created attachment 55720 [details] [review] Bug 17318: Unit tests The current C4::Matcher::_get_match_keys implementation doesn't take normalization rules params into account. This patch makes Matcher.t test the proposed behaviour for new hardcoded values, that match string normalization routines introduced by Bu 17302. Possible values are: - upper_case - lower_case - remove_spaces - legacy_default - none * 'legacy_default' is left (for now) to catch the current behaviour. So tests which did not get a 'norm' param, now are passed 'legacy_default' and they still pass. * 'none' means no normalization, of course Note: on introducing 'none', a bug got highlighted, because the subfields traversal loop was introducing an unneeded space. The tests from 17304 get adjusted to reflect this. To test: - Run: $ sudo koha-shell kohadev ; cd kohaclone $ prove t/Matcher.t => FAIL: The routine doesn't care about the norms param.
Created attachment 55721 [details] [review] Bug 17318: Make C4::Matcher::_get_match_keys handle 'norms' param The current implementation doesn't care about that parameter, and applies a default normalization rule that seems counter-productive (in general) for its aleged purpose. This patch makes it handle the following values for 'norms': - upper_case - lower_case - remove_spaces - legacy_default - none They make it call the relevant Koha::Utils::Normalize routines. 'legacy_default' is used only for backwards compatibility, but could be removed if there's consensus. To test: - Run: $ prove t/Matcher.t => FAIL: most _get_match_keys tests fail - Apply the patch - Run: $ prove t/Matcher.t => SUCCESS: Tests pass! - Sign off :-D
Created attachment 55740 [details] [review] Bug 17318: Make C4::Matcher::_get_match_keys handle 'norms' param The current implementation doesn't care about that parameter, and applies a default normalization rule that seems counter-productive (in general) for its aleged purpose. This patch makes it handle the following values for 'norms': - upper_case - lower_case - remove_spaces - legacy_default - none They make it call the relevant Koha::Utils::Normalize routines. 'legacy_default' is used only for backwards compatibility, but could be removed if there's consensus. To test: - Run: $ prove t/Matcher.t => FAIL: most _get_match_keys tests fail - Apply the patch - Run: $ prove t/Matcher.t => SUCCESS: Tests pass! - Sign off :-D
Created attachment 55741 [details] [review] Bug 17318: Allow chosing fixed normalization routines This patch changes the matching-rules.pl page, so it allows the user to choose from the different hardcoded normalization routines. It is done using TT blocks and the routine descriptions are translatable. To test: - Create some matching rule - Have free text values on the 'Normalization rule' field - Apply the patches - Create a new routine => SUCCESS: You can now only choose between fixed option - Save the rule - Open it for editing => SUCCESS: Chosen rules are preserved - Edit the original free-text including rule => SUCCESS: Rules fall back to 'None' - Sign off :-D TODO: A followup will add a warning about the current value and the fact that saving the matching rule will overwrite those, with 'none'.
Created attachment 55753 [details] [review] Bug 17318: Unit tests The current C4::Matcher::_get_match_keys implementation doesn't take normalization rules params into account. This patch makes Matcher.t test the proposed behaviour for new hardcoded values, that match string normalization routines introduced by Bu 17302. Possible values are: - upper_case - lower_case - remove_spaces - legacy_default - none * 'legacy_default' is left (for now) to catch the current behaviour. So tests which did not get a 'norm' param, now are passed 'legacy_default' and they still pass. * 'none' means no normalization, of course Note: on introducing 'none', a bug got highlighted, because the subfields traversal loop was introducing an unneeded space. The tests from 17304 get adjusted to reflect this. To test: - Run: $ sudo koha-shell kohadev ; cd kohaclone $ prove t/Matcher.t => FAIL: The routine doesn't care about the norms param. Sponsored-by: FIT
Created attachment 55754 [details] [review] Bug 17318: Make C4::Matcher::_get_match_keys handle 'norms' param The current implementation doesn't care about that parameter, and applies a default normalization rule that seems counter-productive (in general) for its aleged purpose. This patch makes it handle the following values for 'norms': - upper_case - lower_case - remove_spaces - legacy_default - none They make it call the relevant Koha::Utils::Normalize routines. 'legacy_default' is used only for backwards compatibility, but could be removed if there's consensus. To test: - Run: $ prove t/Matcher.t => FAIL: most _get_match_keys tests fail - Apply the patch - Run: $ prove t/Matcher.t => SUCCESS: Tests pass! - Sign off :-D Sponsored-by: FIT
Created attachment 55755 [details] [review] Bug 17318: Allow chosing fixed normalization routines This patch changes the matching-rules.pl page, so it allows the user to choose from the different hardcoded normalization routines. It is done using TT blocks and the routine descriptions are translatable. To test: - Create some matching rule - Have free text values on the 'Normalization rule' field - Apply the patches - Create a new routine => SUCCESS: You can now only choose between fixed option - Save the rule - Open it for editing => SUCCESS: Chosen rules are preserved - Edit the original free-text including rule => SUCCESS: Rules fall back to 'None' - Sign off :-D TODO: A followup will add a warning about the current value and the fact that saving the matching rule will overwrite those, with 'none'. Sponsored-by: FIT
Created attachment 55779 [details] [review] Bug 17318: Unit tests The current C4::Matcher::_get_match_keys implementation doesn't take normalization rules params into account. This patch makes Matcher.t test the proposed behaviour for new hardcoded values, that match string normalization routines introduced by Bu 17302. Possible values are: - upper_case - lower_case - remove_spaces - legacy_default - none * 'legacy_default' is left (for now) to catch the current behaviour. So tests which did not get a 'norm' param, now are passed 'legacy_default' and they still pass. * 'none' means no normalization, of course Note: on introducing 'none', a bug got highlighted, because the subfields traversal loop was introducing an unneeded space. The tests from 17304 get adjusted to reflect this. To test: - Run: $ sudo koha-shell kohadev ; cd kohaclone $ prove t/Matcher.t => FAIL: The routine doesn't care about the norms param. Sponsored-by: FIT Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 55780 [details] [review] Bug 17318: Make C4::Matcher::_get_match_keys handle 'norms' param The current implementation doesn't care about that parameter, and applies a default normalization rule that seems counter-productive (in general) for its aleged purpose. This patch makes it handle the following values for 'norms': - upper_case - lower_case - remove_spaces - legacy_default - none They make it call the relevant Koha::Utils::Normalize routines. 'legacy_default' is used only for backwards compatibility, but could be removed if there's consensus. To test: - Run: $ prove t/Matcher.t => FAIL: most _get_match_keys tests fail - Apply the patch - Run: $ prove t/Matcher.t => SUCCESS: Tests pass! - Sign off :-D Sponsored-by: FIT Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 55781 [details] [review] Bug 17318: Allow chosing fixed normalization routines This patch changes the matching-rules.pl page, so it allows the user to choose from the different hardcoded normalization routines. It is done using TT blocks and the routine descriptions are translatable. To test: - Create some matching rule - Have free text values on the 'Normalization rule' field - Apply the patches - Create a new routine => SUCCESS: You can now only choose between fixed option - Save the rule - Open it for editing => SUCCESS: Chosen rules are preserved - Edit the original free-text including rule => SUCCESS: Rules fall back to 'None' - Sign off :-D TODO: A followup will add a warning about the current value and the fact that saving the matching rule will overwrite those, with 'none'. Sponsored-by: FIT Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
*** Bug 15541 has been marked as a duplicate of this bug. ***
*** Bug 14238 has been marked as a duplicate of this bug. ***
Tomas, would not we want to eventually cumulate the different rules (UC+remove spaces for instance)?
(In reply to Jonathan Druart from comment #14) > Tomas, would not we want to eventually cumulate the different rules > (UC+remove spaces for instance)? Forgive me for interjecting. But, yes! That's why norms is an array and it loops through it. Feel free to correct me if I misread the code.
(In reply to Jonathan Druart from comment #14) > Tomas, would not we want to eventually cumulate the different rules > (UC+remove spaces for instance)? Yes, i just didn't want to spend time on that until this went through QA. One enhancement per bug!
Created attachment 55901 [details] [review] Bug 17318: Unit tests The current C4::Matcher::_get_match_keys implementation doesn't take normalization rules params into account. This patch makes Matcher.t test the proposed behaviour for new hardcoded values, that match string normalization routines introduced by Bu 17302. Possible values are: - upper_case - lower_case - remove_spaces - legacy_default - none * 'legacy_default' is left (for now) to catch the current behaviour. So tests which did not get a 'norm' param, now are passed 'legacy_default' and they still pass. * 'none' means no normalization, of course Note: on introducing 'none', a bug got highlighted, because the subfields traversal loop was introducing an unneeded space. The tests from 17304 get adjusted to reflect this. To test: - Run: $ sudo koha-shell kohadev ; cd kohaclone $ prove t/Matcher.t => FAIL: The routine doesn't care about the norms param. Sponsored-by: FIT Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 55902 [details] [review] Bug 17318: Make C4::Matcher::_get_match_keys handle 'norms' param The current implementation doesn't care about that parameter, and applies a default normalization rule that seems counter-productive (in general) for its aleged purpose. This patch makes it handle the following values for 'norms': - upper_case - lower_case - remove_spaces - legacy_default - none They make it call the relevant Koha::Utils::Normalize routines. 'legacy_default' is used only for backwards compatibility, but could be removed if there's consensus. To test: - Run: $ prove t/Matcher.t => FAIL: most _get_match_keys tests fail - Apply the patch - Run: $ prove t/Matcher.t => SUCCESS: Tests pass! - Sign off :-D Sponsored-by: FIT Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 55903 [details] [review] Bug 17318: Allow chosing fixed normalization routines This patch changes the matching-rules.pl page, so it allows the user to choose from the different hardcoded normalization routines. It is done using TT blocks and the routine descriptions are translatable. To test: - Create some matching rule - Have free text values on the 'Normalization rule' field - Apply the patches - Create a new routine => SUCCESS: You can now only choose between fixed option - Save the rule - Open it for editing => SUCCESS: Chosen rules are preserved - Edit the original free-text including rule => SUCCESS: Rules fall back to 'None' - Sign off :-D TODO: A followup will add a warning about the current value and the fact that saving the matching rule will overwrite those, with 'none'. Sponsored-by: FIT Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Wooo! Go team!
Pushed to master for 16.11, thanks Tomas!
Unfortunately, I've discovered that this wasn't really a complete duplicate of bug 15541... C4::Search::SimpleSearch() performs additional query normalization which interferes with getting correct matches (primarily with URLs). The QueryParser should also fallback to the regular query builder in C4::Matcher::get_matches() when there are multiple comma separated qualifiers.
Tomas, if you have ideas about how to fix the other problems I've mentioned, I'd be happy to hear them : ).
Tomas, I get the following error when I import a record using the 'Biblionumber' matching rule: Invalid normalization routine required (Biblionumber) at /home/vagrant/kohaclone/C4/Matcher.pm line 844., referer: http://pro.kohadev.vm/cgi-bin/koha/tools/stage-marc-import.pl This rule is added by the installer files as well as 'ISBN' and 'ISSN'.