Bug 17318 - Make 'Normalization rule' configurable on matchpoint definition
Summary: Make 'Normalization rule' configurable on matchpoint definition
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic data support (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
: 14238 (view as bug list)
Depends on: 17302 17304
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-19 15:06 UTC by Tomás Cohen Arazi
Modified: 2018-06-04 20:10 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
'Normalization rule' on the UI (22.38 KB, image/png)
2016-09-19 15:12 UTC, Tomás Cohen Arazi
Details
Bug 17318: Unit tests (6.99 KB, patch)
2016-09-20 15:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17318: Make C4::Matcher::_get_match_keys handle 'norms' param (4.98 KB, patch)
2016-09-20 15:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17318: Make C4::Matcher::_get_match_keys handle 'norms' param (4.98 KB, patch)
2016-09-21 18:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17318: Allow chosing fixed normalization routines (9.83 KB, patch)
2016-09-21 18:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17318: Unit tests (7.01 KB, patch)
2016-09-22 17:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17318: Make C4::Matcher::_get_match_keys handle 'norms' param (5.00 KB, patch)
2016-09-22 17:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17318: Allow chosing fixed normalization routines (9.85 KB, patch)
2016-09-22 17:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17318: Unit tests (7.06 KB, patch)
2016-09-23 16:03 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17318: Make C4::Matcher::_get_match_keys handle 'norms' param (5.05 KB, patch)
2016-09-23 16:04 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17318: Allow chosing fixed normalization routines (9.90 KB, patch)
2016-09-23 16:04 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17318: Unit tests (7.13 KB, patch)
2016-09-29 07:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17318: Make C4::Matcher::_get_match_keys handle 'norms' param (5.12 KB, patch)
2016-09-29 07:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17318: Allow chosing fixed normalization routines (9.97 KB, patch)
2016-09-29 07:55 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2016-09-19 15:06:41 UTC
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.
Comment 1 Tomás Cohen Arazi 2016-09-19 15:12:45 UTC
Created attachment 55695 [details]
'Normalization rule' on the UI
Comment 2 Tomás Cohen Arazi 2016-09-20 15:32:10 UTC
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.
Comment 3 Tomás Cohen Arazi 2016-09-20 15:32:14 UTC
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
Comment 4 Tomás Cohen Arazi 2016-09-21 18:02:08 UTC
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
Comment 5 Tomás Cohen Arazi 2016-09-21 18:03:05 UTC
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'.
Comment 6 Tomás Cohen Arazi 2016-09-22 17:09:57 UTC
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
Comment 7 Tomás Cohen Arazi 2016-09-22 17:10:04 UTC
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
Comment 8 Tomás Cohen Arazi 2016-09-22 17:10:10 UTC
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
Comment 9 Mark Tompsett 2016-09-23 16:03:55 UTC
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>
Comment 10 Mark Tompsett 2016-09-23 16:04:01 UTC
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>
Comment 11 Mark Tompsett 2016-09-23 16:04:07 UTC
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>
Comment 12 Mark Tompsett 2016-09-23 16:06:57 UTC
*** Bug 15541 has been marked as a duplicate of this bug. ***
Comment 13 Tomás Cohen Arazi 2016-09-26 17:32:23 UTC
*** Bug 14238 has been marked as a duplicate of this bug. ***
Comment 14 Jonathan Druart 2016-09-28 08:37:27 UTC
Tomas, would not we want to eventually cumulate the different rules (UC+remove spaces for instance)?
Comment 15 Mark Tompsett 2016-09-28 12:34:25 UTC
(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.
Comment 16 Tomás Cohen Arazi 2016-09-28 12:58:31 UTC
(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!
Comment 17 Jonathan Druart 2016-09-29 07:55:11 UTC
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>
Comment 18 Jonathan Druart 2016-09-29 07:55:15 UTC
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>
Comment 19 Jonathan Druart 2016-09-29 07:55:18 UTC
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>
Comment 20 David Cook 2016-10-11 06:07:05 UTC
Wooo! Go team!
Comment 21 Kyle M Hall 2016-10-21 16:17:14 UTC
Pushed to master for 16.11, thanks Tomas!
Comment 22 David Cook 2016-11-29 06:22:09 UTC
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.
Comment 23 David Cook 2016-11-29 06:25:25 UTC
Tomas, if you have ideas about how to fix the other problems I've mentioned, I'd be happy to hear them : ).
Comment 24 Jonathan Druart 2017-02-27 11:22:03 UTC
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'.