Bug 19582 - Elasticsearch: Auth-finder.pl must use search_auth_compat
Summary: Elasticsearch: Auth-finder.pl must use search_auth_compat
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Alex Arnaud
URL:
Keywords:
Depends on:
Blocks: 19604 20273
  Show dependency treegraph
 
Reported: 2017-11-06 17:16 UTC by Nick Clemens
Modified: 2018-12-03 20:04 UTC (History)
7 users (show)

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


Attachments
Bug 19582 - Use compat routines for searching authorities in auth_finder.pl (3.57 KB, patch)
2017-11-06 17:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19582 - Use compat routines for searching authorities in auth_finder.pl (3.63 KB, patch)
2017-11-07 01:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19582: Unit tests (2.46 KB, patch)
2017-11-07 01:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19582 - Use compat routines for searching authorities in auth_finder.pl (3.69 KB, patch)
2017-11-08 15:55 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19582: Unit tests (2.52 KB, patch)
2017-11-08 15:55 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19582: (follow-up) get params in correct order (1.11 KB, patch)
2018-03-12 16:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19582 - Use compat routines for searching authorities in auth_finder.pl (3.75 KB, patch)
2018-03-13 14:47 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19582: Unit tests (2.58 KB, patch)
2018-03-13 14:47 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19582: (follow-up) get params in correct order (1.17 KB, patch)
2018-03-13 14:47 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19582 - remove use of C4::AuthoritiesMarc (642 bytes, patch)
2018-03-13 15:09 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 19582 - Use compat routines for searching authorities in auth_finder.pl (3.80 KB, patch)
2018-03-13 15:13 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 19582: Unit tests (2.63 KB, patch)
2018-03-13 15:13 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 19582: (follow-up) get params in correct order (1.22 KB, patch)
2018-03-13 15:13 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 19582 - remove use of C4::AuthoritiesMarc (696 bytes, patch)
2018-03-13 15:13 UTC, Alex Arnaud
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2017-11-06 17:16:46 UTC
When Elasticsearch is enable the authorities search from cataloging is using Zebra, we need to use the compat functions to select the correct engine
Comment 1 Nick Clemens 2017-11-06 17:19:19 UTC
Created attachment 68984 [details] [review]
Bug 19582 - Use compat routines for searching authorities in auth_finder.pl

To test:
1 - Enable Zebra
2 - Perform an auth search
3 - note results
4 - Enable ES
5 - Repeat search, note (likely) diff results
6 - Open a record in cataloging and use the button to launch auth search
7 - Perform same search as above, note results match for eiher engine
selected
8 - NOTE: Disbale sorting for ES search - this will be dealt with in
another report
Comment 2 Nick Clemens 2017-11-07 01:31:38 UTC
Created attachment 68992 [details] [review]
Bug 19582 - Use compat routines for searching authorities in auth_finder.pl

To test:
1 - Enable Zebra
2 - Perform an auth search
3 - note results
4 - Enable ES
5 - Repeat search, note (likely) diff results
6 - Open a record in cataloging and use the button to launch auth search
7 - Perform same search as above, note results match for eiher engine
selected
8 - NOTE: Disable sorting for ES search - this will be dealt with in
another report
Comment 3 Nick Clemens 2017-11-07 01:31:41 UTC
Created attachment 68993 [details] [review]
Bug 19582: Unit tests
Comment 4 David Bourgault 2017-11-08 15:55:42 UTC
Created attachment 69031 [details] [review]
Bug 19582 - Use compat routines for searching authorities in auth_finder.pl

To test:
1 - Enable Zebra
2 - Perform an auth search
3 - note results
4 - Enable ES
5 - Repeat search, note (likely) diff results
6 - Open a record in cataloging and use the button to launch auth search
7 - Perform same search as above, note results match for eiher engine
selected
8 - NOTE: Disbale sorting for ES search - this will be dealt with in
another report

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 5 David Bourgault 2017-11-08 15:55:45 UTC
Created attachment 69032 [details] [review]
Bug 19582: Unit tests

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 6 David Bourgault 2017-11-08 15:57:46 UTC
Followed test plan, but it took me a long time you meant the auth search in the record editor...

Before patch the search would actually straight-up fail for me, with patch I got the same results as a normal auth search.

QA tools and test passed green.
Comment 7 Alex Arnaud 2018-02-08 14:17:14 UTC
Hello Nick,

I have nothing to say from a QA point of view about the current changes in the code but i'm wondering if we should not do the same work for autocomplete feature (it still use C4::AuthoritiesMarc::SearchAuthorities) or do it in another BZ.
Comment 8 Jonathan Druart 2018-02-08 14:35:15 UTC
(In reply to Alex Arnaud from comment #7)
> Hello Nick,
> 
> I have nothing to say from a QA point of view about the current changes in
> the code but i'm wondering if we should not do the same work for
> autocomplete feature (it still use C4::AuthoritiesMarc::SearchAuthorities)
> or do it in another BZ.

Yes of course, every occurrence of SearchAuthorities must be replaced. Please open a new bug report and put on QA stamp on this one ;)
Comment 9 Katrin Fischer 2018-02-18 20:49:42 UTC
Hi Alex, can you please take another look here? Thx!
Comment 10 Alex Arnaud 2018-02-22 09:43:17 UTC
Hello Nick,

Searching on entire record doesn't work for me. You may replace all occurences of "value_any" by "value_all" ?
Comment 11 Alex Arnaud 2018-02-22 15:28:16 UTC
Also, i think that "C4::AuthoritiesMarc;" is no longer needed
Comment 12 Alex Arnaud 2018-02-22 15:33:45 UTC
I'm not sure for value_any by value_all. It could be an issue related to my unimarc authorities mapping. Can anyone using marc21 confirm this issue?
Comment 13 Nick Clemens 2018-03-12 16:03:11 UTC
Created attachment 72717 [details] [review]
Bug 19582: (follow-up) get params in correct order

We were ordering the values out of sync with the order of the mathclist
param and so causing 'entire record' and 'all headings' to be reversed
Comment 14 Nicolas Legrand 2018-03-13 14:47:31 UTC
Created attachment 72782 [details] [review]
Bug 19582 - Use compat routines for searching authorities in auth_finder.pl

To test:
1 - Enable Zebra
2 - Perform an auth search
3 - note results
4 - Enable ES
5 - Repeat search, note (likely) diff results
6 - Open a record in cataloging and use the button to launch auth search
7 - Perform same search as above, note results match for eiher engine
selected
8 - NOTE: Disbale sorting for ES search - this will be dealt with in
another report

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>


Current status: Signed Off
Comment 15 Nicolas Legrand 2018-03-13 14:47:37 UTC
Created attachment 72783 [details] [review]
Bug 19582: Unit tests

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 16 Nicolas Legrand 2018-03-13 14:47:42 UTC
Created attachment 72784 [details] [review]
Bug 19582: (follow-up) get params in correct order

We were ordering the values out of sync with the order of the mathclist
param and so causing 'entire record' and 'all headings' to be reversed

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 17 Nicolas Legrand 2018-03-13 14:49:39 UTC
with unimarc setting and mapping from 17373

$ perl t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t 
1..2
# Subtest: build_authorities_query_compat() tests
    ok 1 - Creating new query builder object for authorities
    ok 2
    ok 3
    ok 4
    ok 5
    ok 6
    ok 7
    ok 8
    ok 9 - Exception thrown on invalid value in the marclist param
    1..9
ok 1 - build_authorities_query_compat() tests
# Subtest: build query from form subtests
    1..5
    ok 1 - Expected search is populated
    ok 2 - Only defined search is populated
    ok 3 - First index searched as expected
    ok 4 - Second index searched when populated
    ok 5 - Only defined searches are populated
ok 2 - build query from form subtests
Comment 18 Alex Arnaud 2018-03-13 15:09:50 UTC
Created attachment 72792 [details] [review]
Bug 19582 - remove use of C4::AuthoritiesMarc
Comment 19 Alex Arnaud 2018-03-13 15:13:25 UTC
Created attachment 72793 [details] [review]
Bug 19582 - Use compat routines for searching authorities in auth_finder.pl

To test:
1 - Enable Zebra
2 - Perform an auth search
3 - note results
4 - Enable ES
5 - Repeat search, note (likely) diff results
6 - Open a record in cataloging and use the button to launch auth search
7 - Perform same search as above, note results match for eiher engine
selected
8 - NOTE: Disbale sorting for ES search - this will be dealt with in
another report

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 20 Alex Arnaud 2018-03-13 15:13:34 UTC
Created attachment 72794 [details] [review]
Bug 19582: Unit tests

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 21 Alex Arnaud 2018-03-13 15:13:44 UTC
Created attachment 72795 [details] [review]
Bug 19582: (follow-up) get params in correct order

We were ordering the values out of sync with the order of the mathclist
param and so causing 'entire record' and 'all headings' to be reversed

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 22 Alex Arnaud 2018-03-13 15:13:51 UTC
Created attachment 72796 [details] [review]
Bug 19582 - remove use of C4::AuthoritiesMarc

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 23 Jonathan Druart 2018-03-27 20:33:29 UTC
Please take care of commit messages and author:
  Author:     Koha User <support@biblibre.com>
  Bug 19582 - remove use of C4::AuthoritiesMarc

  Bug 19582 - Use compat routines for searching authorities in auth_finder.pl
Comment 24 Jonathan Druart 2018-03-27 20:45:13 UTC
Nick, please care about spaces and alignment:
In your patch it looks like:
-        $query->param('value_any')     || undef,
-        $query->param('value_match')   || undef
+        $query->param('value_match')     || undef,
+        $query->param('value_any')   || undef

With correct alignement:
+        $query->param('value_match')   || undef,
         $query->param('value_any')     || undef,
-        $query->param('value_match')   || undef

Much easier to read, right?
Will be done on pushing.
Comment 25 Jonathan Druart 2018-03-27 20:55:36 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 26 Jonathan Druart 2018-03-27 20:57:40 UTC
And I messed up with the bug number when pushing the last patch...
  Bug 19583: Remove use of C4::AuthoritiesMarc
should have been
  Bug 19582: Remove use of C4::AuthoritiesMarc
Comment 27 Nick Clemens 2018-05-14 21:27:39 UTC
Small(ish) change, considered as bug

Pushed to stable for 17.11.06

Awesome work all!
Comment 28 Fridolin Somers 2018-05-21 07:17:48 UTC
Enhancement not pushed to 17.05.x

Viva ElasticSearch ;)
Comment 29 Fridolin Somers 2018-05-21 08:22:23 UTC
Oups in GIT last commit message has the wrong bug id : Bug 19583

http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=f60bff7aa0e3ca932f5572b7837d912b8681ca2a