Bug 17897

Summary: Koha::SearchMappingManager - Simplify search mapping modifications
Product: Koha Reporter: Olli-Antti Kivilahti <olli-antti.kivilahti>
Component: SearchingAssignee: Emmi Takkinen <emmi.takkinen>
Status: Failed QA --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: david, emmi.takkinen, katrin.fischer, m.de.rooy, nick, vfaafia29
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17885    
Bug Blocks:    
Attachments: Bug 17897 - Koha::SearchMappingManager - Simplify search mapping modifications
Bug 17897 - (follow-up) Cleanup after tests
Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications
Bug 17897: (follow-up) Cleanup after tests
Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications
Bug 17897: (follow-up) Cleanup after tests
Bug 17897: Fix failing tests
Bug 17897: Fix failing tests
Bug 17897: Fix failing tests and QA issues
Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications
Bug 17897: (follow-up) Cleanup after tests
Bug 17897: Fix failing tests and QA issues
Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications
Bug 17897: (follow-up) Cleanup after tests
Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications
Bug 17897: (follow-up) Cleanup after tests
Bug 17897: Fix failing tests and QA issues
Bug 17897: Add search_marc_to_field.filter to SearhMappingManager
Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications
Bug 17897: (follow-up) Cleanup after tests
Bug 17897: Fix failing tests and QA issues
Bug 17897: Add search_marc_to_field.filter to SearhMappingManager

Description Olli-Antti Kivilahti 2017-01-13 10:50:59 UTC
Currently there is a lot of DBIC-magic everywhere where search mappings (search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed.

Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes code-reusability and thus, simpler test case writing.

Includes tests for Bug 17885
Comment 1 Olli-Antti Kivilahti 2017-01-13 10:53:03 UTC
Created attachment 58942 [details] [review]
Bug 17897 - Koha::SearchMappingManager - Simplify search mapping modifications

Currently there is a lot of DBIC-magic everywhere where search mappings
(search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed.

Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes
code-reusability and thus, simpler test case writing.

Includes tests for Buug 17885
Comment 2 Olli-Antti Kivilahti 2017-01-13 10:54:08 UTC
Test plan is to run the elasticsearch-dependent tests in t/db_dependent/Koha*
Comment 3 Lari Taskula 2017-08-01 12:28:56 UTC
Created attachment 65392 [details] [review]
Bug 17897 - (follow-up) Cleanup after tests

Adds rollback to test data in t/db_dependent/Koha/SearchEngine/ElasticSearch.t

To test:
1. prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t
Comment 4 Nick Clemens 2018-03-12 14:47:33 UTC
fatal: sha1 information is lacking or useless (Koha/SearchEngine/Elasticsearch.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Comment 5 Emmi Takkinen 2020-04-07 07:36:08 UTC
Created attachment 102475 [details] [review]
Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications

Rebased patch. Added 'search' to 'get_search_mappings' and adjusted tests to work with changed mappings.
Comment 6 Emmi Takkinen 2020-04-07 07:37:03 UTC
Created attachment 102476 [details] [review]
Bug 17897: (follow-up) Cleanup after tests
Comment 7 Victoria Faafia 2021-01-18 23:10:17 UTC
git bz apply 17897
Bug 17897 - Koha::SearchMappingManager - Simplify search mapping modifications

102475 - Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications
102476 - Bug 17897: (follow-up) Cleanup after tests

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications
Using index info to reconstruct a base tree...
M	Koha/SearchEngine/Elasticsearch.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/SearchEngine/Elasticsearch.pm
CONFLICT (content): Merge conflict in Koha/SearchEngine/Elasticsearch.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-17897-KohaSearchMappingManager---Simplify-sear-JiTslW.patch

patch does not apply
Comment 8 Emmi Takkinen 2021-01-19 13:54:59 UTC
Created attachment 115320 [details] [review]
Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications

Currently there is a lot of DBIC-magic everywhere where search mappings
(search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed.

Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes
code-reusability and thus, simpler test case writing.

Includes tests for Buug 17885

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 9 Emmi Takkinen 2021-01-19 13:55:48 UTC
Created attachment 115321 [details] [review]
Bug 17897: (follow-up) Cleanup after tests

Adds rollback to test data in t/db_dependent/Koha/SearchEngine/ElasticSearch.t

To test:
1. prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 10 Emmi Takkinen 2021-01-19 13:56:20 UTC
Created attachment 115322 [details] [review]
Bug 17897: Fix failing tests

Tests failed when txn_begin and rollback were used inside
of subtests.

To test prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t
Comment 11 David Nind 2022-07-08 13:06:01 UTC
Patch still applies, but tests fail:

 prove -vvv t/db_dependent/Koha/SearchEngine/ElasticSearch.t
t/db_dependent/Koha/SearchEngine/ElasticSearch.t .. 
# Subtest: Reset Elasticsearch mappings
    ok 1 - Scenario: Reset Elasticsearch mappings to an empty database
    ok 2 - Given empty search mapping tables
    ok 3 - When reset_elasticsearch_mappings() has been ran
    ok 4 - Then search mapping tables have been populated
    ok 5 - Scenario: Reset Elasticsearch mappings when custom mappings already exist
    1..5
ok 1 - Reset Elasticsearch mappings
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'type' at row 1 at /kohadevbox/koha/Koha/Objects.pm line 114
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 11 just after 1.
Dubious, test returned 11 (wstat 2816, 0xb00)
All 1 subtests passed 

Test Summary Report
-------------------
t/db_dependent/Koha/SearchEngine/ElasticSearch.t (Wstat: 2816 Tests: 1 Failed: 0)
  Non-zero exit status: 11
  Parse errors: No plan found in TAP output
Files=1, Tests=1,  4 wallclock secs ( 0.03 usr  0.00 sys +  3.53 cusr  0.19 csys =  3.75 CPU)
Result: FAIL
Comment 12 Emmi Takkinen 2022-11-11 08:09:09 UTC
Created attachment 143744 [details] [review]
Bug 17897: Fix failing tests

Tests failed when txn_begin and rollback were used inside
of subtests.

To test prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t
Comment 13 Emmi Takkinen 2022-11-11 08:10:30 UTC
Fixed tests, patches should apply cleanly.
Comment 14 David Nind 2022-11-11 10:45:22 UTC
Tests are still failing for me:

prove -vvv t/db_dependent/Koha/SearchEngine/ElasticSearch.t
t/db_dependent/Koha/SearchEngine/ElasticSearch.t .. 
# Subtest: Reset Elasticsearch mappings
    ok 1 - Scenario: Reset Elasticsearch mappings to an empty database
    ok 2 - Given empty search mapping tables
    ok 3 - When reset_elasticsearch_mappings() has been ran
    ok 4 - Then search mapping tables have been populated
    ok 5 - Scenario: Reset Elasticsearch mappings when custom mappings already exist
    1..5
ok 1 - Reset Elasticsearch mappings
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'type' at row 1 at /kohadevbox/koha/Koha/Objects.pm line 114
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 11 just after 1.
Dubious, test returned 11 (wstat 2816, 0xb00)
All 1 subtests passed 

Test Summary Report
-------------------
t/db_dependent/Koha/SearchEngine/ElasticSearch.t (Wstat: 2816 Tests: 1 Failed: 0)
  Non-zero exit status: 11
  Parse errors: No plan found in TAP output
Files=1, Tests=1,  4 wallclock secs ( 0.02 usr  0.00 sys +  3.65 cusr  0.15 csys =  3.82 CPU)
Result: FAIL
Comment 15 Emmi Takkinen 2023-10-17 09:57:18 UTC
I just realized tests also lack "tests", "plan tests" etc. and subtests aren't declared inside sub. Also, should test file be named something else? It's now ElasticSearch.t but we also have Elasticsearch.t.
Comment 16 Emmi Takkinen 2023-10-17 10:27:07 UTC
(In reply to David Nind from comment #14)
> Tests are still failing for me:
> 
> prove -vvv t/db_dependent/Koha/SearchEngine/ElasticSearch.t
> t/db_dependent/Koha/SearchEngine/ElasticSearch.t .. 
> # Subtest: Reset Elasticsearch mappings
>     ok 1 - Scenario: Reset Elasticsearch mappings to an empty database
>     ok 2 - Given empty search mapping tables
>     ok 3 - When reset_elasticsearch_mappings() has been ran
>     ok 4 - Then search mapping tables have been populated
>     ok 5 - Scenario: Reset Elasticsearch mappings when custom mappings
> already exist
>     1..5
> ok 1 - Reset Elasticsearch mappings
> DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st
> execute failed: Data truncated for column 'type' at row 1 at
> /kohadevbox/koha/Koha/Objects.pm line 114
> # Tests were run but no plan was declared and done_testing() was not seen.
> # Looks like your test exited with 11 just after 1.
> Dubious, test returned 11 (wstat 2816, 0xb00)
> All 1 subtests passed 
> 
> Test Summary Report
> -------------------
> t/db_dependent/Koha/SearchEngine/ElasticSearch.t (Wstat: 2816 Tests: 1
> Failed: 0)
>   Non-zero exit status: 11
>   Parse errors: No plan found in TAP output
> Files=1, Tests=1,  4 wallclock secs ( 0.02 usr  0.00 sys +  3.65 cusr  0.15
> csys =  3.82 CPU)
> Result: FAIL

Tables search_field column type only accepts following values: '','string','date','number','boolean','sum','isbn','stdno','year','callnumber'. Tests use value "keyword".
Comment 17 Emmi Takkinen 2023-10-17 10:52:54 UTC
Created attachment 157224 [details] [review]
Bug 17897: Fix failing tests and QA issues

Tests failed because
1) txn_begin and rollback were used inside
of subtests.
2) column search_field.type was filled with
unallowed value.

This patch also fixes QA issue with test file
exec rights and incorrect license statement.

To test prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t
Comment 18 Emmi Takkinen 2023-10-17 10:53:51 UTC
Fixed tests and QA issues qa-tool found. Didn't change test files name yet.
Comment 19 David Nind 2023-10-17 19:33:37 UTC
Created attachment 157282 [details] [review]
Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications

Currently there is a lot of DBIC-magic everywhere where search mappings
(search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed.

Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes
code-reusability and thus, simpler test case writing.

Includes tests for Buug 17885

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: David Nind <david@davidnind.com>
Comment 20 David Nind 2023-10-17 19:33:40 UTC
Created attachment 157283 [details] [review]
Bug 17897: (follow-up) Cleanup after tests

Adds rollback to test data in t/db_dependent/Koha/SearchEngine/ElasticSearch.t

To test:
1. prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: David Nind <david@davidnind.com>
Comment 21 David Nind 2023-10-17 19:33:43 UTC
Created attachment 157284 [details] [review]
Bug 17897: Fix failing tests and QA issues

Tests failed because
1) txn_begin and rollback were used inside
of subtests.
2) column search_field.type was filled with
unallowed value.

This patch also fixes QA issue with test file
exec rights and incorrect license statement.

To test prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 22 Katrin Fischer 2023-11-04 23:41:59 UTC
Please rebase!
Comment 23 Emmi Takkinen 2023-11-06 06:07:49 UTC
Created attachment 158487 [details] [review]
Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications

Currently there is a lot of DBIC-magic everywhere where search mappings
(search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed.

Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes
code-reusability and thus, simpler test case writing.

Includes tests for Buug 17885

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 24 Emmi Takkinen 2023-11-06 06:08:18 UTC
Created attachment 158488 [details] [review]
Bug 17897: (follow-up) Cleanup after tests

Adds rollback to test data in t/db_dependent/Koha/SearchEngine/ElasticSearch.t

To test:
1. prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 25 Emmi Takkinen 2023-11-06 06:13:05 UTC
Created attachment 158489 [details] [review]
Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications

Currently there is a lot of DBIC-magic everywhere where search mappings
(search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed.

Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes
code-reusability and thus, simpler test case writing.

Includes tests for Buug 17885

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: David Nind <david@davidnind.com>
Comment 26 Emmi Takkinen 2023-11-06 06:13:34 UTC
Created attachment 158490 [details] [review]
Bug 17897: (follow-up) Cleanup after tests

Adds rollback to test data in t/db_dependent/Koha/SearchEngine/ElasticSearch.t

To test:
1. prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: David Nind <david@davidnind.com>
Comment 27 Emmi Takkinen 2023-11-06 06:13:55 UTC
Created attachment 158491 [details] [review]
Bug 17897: Fix failing tests and QA issues

Tests failed because
1) txn_begin and rollback were used inside
of subtests.
2) column search_field.type was filled with
unallowed value.

This patch also fixes QA issue with test file
exec rights and incorrect license statement.

To test prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 28 Emmi Takkinen 2023-11-06 06:15:46 UTC
Made some mess and accidentally rebased patches without Davids sign off first. Also these are missing "filter" param, will add it in a moment.
Comment 29 Emmi Takkinen 2023-11-06 06:36:21 UTC
Created attachment 158492 [details] [review]
Bug 17897: Add search_marc_to_field.filter to SearhMappingManager

Methods in SearchMappingManager didn't take into account
search_marc_to_field.filter indtroduced in bug 27153.

To test, prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t

Sponsored-by: Koha-Suomi Oy
Comment 30 David Nind 2023-11-06 20:33:35 UTC
Created attachment 158561 [details] [review]
Bug 17897: Koha::SearchMappingManager - Simplify search mapping modifications

Currently there is a lot of DBIC-magic everywhere where search mappings
(search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed.

Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes
code-reusability and thus, simpler test case writing.

Includes tests for Buug 17885

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: David Nind <david@davidnind.com>
Comment 31 David Nind 2023-11-06 20:33:38 UTC
Created attachment 158562 [details] [review]
Bug 17897: (follow-up) Cleanup after tests

Adds rollback to test data in t/db_dependent/Koha/SearchEngine/ElasticSearch.t

To test:
1. prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t

Rebased-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: David Nind <david@davidnind.com>
Comment 32 David Nind 2023-11-06 20:33:41 UTC
Created attachment 158563 [details] [review]
Bug 17897: Fix failing tests and QA issues

Tests failed because
1) txn_begin and rollback were used inside
of subtests.
2) column search_field.type was filled with
unallowed value.

This patch also fixes QA issue with test file
exec rights and incorrect license statement.

To test prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 33 David Nind 2023-11-06 20:33:44 UTC
Created attachment 158564 [details] [review]
Bug 17897: Add search_marc_to_field.filter to SearhMappingManager

Methods in SearchMappingManager didn't take into account
search_marc_to_field.filter indtroduced in bug 27153.

To test, prove t/db_dependent/Koha/SearchEngine/ElasticSearch.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Comment 34 Marcel de Rooy 2024-04-18 11:57:49 UTC
 WARN   Koha/SearchMappingManager.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 0, now: 11)

 WARN   t/db_dependent/Koha/SearchEngine/ElasticSearch.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 0, now: 76)

+@THROWS die when parameters are not properly given. Should throw a Koha::Exception::BadParameter,
+            but pushing all those Exceptions to the community version would take ages.
Hmm. Just adding that exception does not take ages. One die ?

diff --git a/t/db_dependent/Koha/SearchEngine/ElasticSearch.t b/t/db_dependent/Koha/SearchEngine/ElasticSearch.t
new file mode 100644
OK Fixed later

+# Copyright 2015 Catalyst IT
Fix?

+use Modern::Perl qw(2014);
Not what we do elsewhere?

+    $rv = Koha::SearchMappingManager::flush();
No transactions, no rollbacks ?
OK Added later.

General remark / question
Given this comment: "Currently there is a lot of DBIC-magic everywhere where search mappings (search_fields, search_marc_to_field, search_marc_map -tables) are CRUD:ed. Encapsulate this hard-to-read DBIC-magic into a Manager class which promotes code-reusability and thus, simpler test case writing."
 Koha/SearchEngine/Elasticsearch.pm            |  27 +---
 Koha/SearchMappingManager.pm                  | 151 ++++++++++++++++++
 .../Koha/SearchEngine/ElasticSearch.t         | 127 +++++++++++++++
 3 files changed, 280 insertions(+), 25 deletions(-)
I am not seeing large number of replaces to start using this new class?
So could you explain the benefit of adding those 250 lines please?