Bug 18822 - Advanced editor - Rancor - searching broken under Elasticsearch
Summary: Advanced editor - Rancor - searching broken under Elasticsearch
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 17739 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-06-19 15:17 UTC by Nick Clemens
Modified: 2018-07-30 13:51 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18822 - Advanced editor - Rancor - searching broken (1.20 KB, patch)
2017-06-19 15:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18822 - Better error response (1.32 KB, patch)
2017-06-19 15:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18822 - Advanced editor - Rancor - searching broken (1.26 KB, patch)
2017-10-28 13:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18822 - Better error response (1.38 KB, patch)
2017-10-28 13:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18822: Check if we have MARC::Record and convert if not when using ES (1.43 KB, patch)
2018-05-25 09:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18822 - Better error response (1.44 KB, patch)
2018-07-08 15:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18822: Check if we have MARC::Record and convert if not when using ES (1.49 KB, patch)
2018-07-08 15:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18822 - Better error response (1.53 KB, patch)
2018-07-13 08:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18822: Check if we have MARC::Record and convert if not when using ES (1.58 KB, patch)
2018-07-13 08:55 UTC, Marcel de Rooy
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-06-19 15:17:10 UTC
It seems we expect MARC::Record objects, but get MarcXML for local searches - since we can double process the easiest method seem to be attempting conversion and using the updated record if successful
Comment 1 Nick Clemens 2017-06-19 15:23:15 UTC
Created attachment 64431 [details] [review]
Bug 18822 - Advanced editor - Rancor - searching broken

This patch attempt to convert records before processing as we sometimes
get XML instead of record objects.

To test:
1 - Perform an advanced search from the advanced editor, make sure to
select 'Local catalog' and a remote source
2 - Error 'Internal search error [Object object]'
3 - Apply patch
4 - Repeat search, success!
5 - Select and deselect various servers, search should still work
Comment 2 Nick Clemens 2017-06-19 15:23:18 UTC
Created attachment 64432 [details] [review]
Bug 18822 - Better error response

Apply this patch first to see a change in the error feedback - instead
of [Object object] you should get a text readable error
Comment 3 Lee Jamison 2017-06-22 14:23:09 UTC
I don't get the error indicated in the test plan on a fresh kohadevbox. I did the following steps and did not get an error:

(in Kohadevbox)

1. reset_all
2. Login to Intranet
3. Set EnableAdvancedCatalogingEditor to "Enable" under SysPrefs -> Labs
4. Navigate to the Cataloging module
5. Click Advanced Editor
6. Under Search, clicking "Advanced >>"
7. Select LIBRARY OF CONGRESS and Local catalog as servers
8. Type "history" in the Title field and click Search
9. Search results are listed and no error appears
Comment 4 Alex Buckley 2017-06-25 18:32:42 UTC
As with Lee I also do not get the error specified in step 2 of the test plan of comment 1.

I have tested the patches and they work the same as the current default behaviour on an up to date branch
Comment 5 Nick Clemens 2017-06-26 09:55:12 UTC
Ah, looks like I didn't realize I had Elasticsearch set when encountering this issue, that seems to be the trigger
Comment 6 Katrin Fischer 2017-10-02 23:46:00 UTC
Wanted to test, but can't get Elasticsearch working in kohadevbox :(
Comment 7 Katrin Fischer 2017-10-07 14:22:34 UTC
Still trying to get Elasticsearch to work, when reindexing from koha-shell I get:
[NoNodes] ** No nodes are available: [http://localhost:9200], called from sub Search::Elasticsearch::Role::Client::Direct::__ANON__ at /usr/share/perl5/Catmandu/Store/ElasticSearch.pm line 27.kohadev-koha@kohadevbox:/home/vagrant/kohaclone$
Comment 8 Katrin Fischer 2017-10-28 13:05:29 UTC
Created attachment 68803 [details] [review]
Bug 18822 - Advanced editor - Rancor - searching broken

This patch attempt to convert records before processing as we sometimes
get XML instead of record objects.

To test:
1 - Perform an advanced search from the advanced editor, make sure to
select 'Local catalog' and a remote source
2 - Error 'Internal search error [Object object]'
3 - Apply patch
4 - Repeat search, success!
5 - Select and deselect various servers, search should still work

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2017-10-28 13:05:32 UTC
Created attachment 68804 [details] [review]
Bug 18822 - Better error response

Apply this patch first to see a change in the error feedback - instead
of [Object object] you should get a text readable error

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Marcel de Rooy 2017-11-27 12:01:01 UTC
+        my $fixed_rec;
+        eval { $fixed_rec = MARC::Record::new_from_xml( $set->{hits}->[$j], 'UTF-8' ) };
+        $set->{hits}->[$j] = $fixed_rec unless $@;

No way to know what to expect ? Is it from internal or external ? Do we have properties of these external ones ?
And can we check ref($set->{hits}->[$j]) to see if it is already MARC::Record ?
Comment 11 Marcel de Rooy 2018-01-15 18:32:37 UTC
Comment10 did not receive an answer.
Changing the status sometimes helps :)
Comment 12 Nick Clemens 2018-01-16 13:38:58 UTC
(In reply to Marcel de Rooy from comment #10)
> +        my $fixed_rec;
> +        eval { $fixed_rec = MARC::Record::new_from_xml( $set->{hits}->[$j],
> 'UTF-8' ) };
> +        $set->{hits}->[$j] = $fixed_rec unless $@;
> 
> No way to know what to expect ? Is it from internal or external ? Do we have
> properties of these external ones ?
> And can we check ref($set->{hits}->[$j]) to see if it is already
> MARC::Record ?

We have a slightly larger issue here of sometimes returning MARC::Records and sometimes returning marcxml.

I suppose we could test the type, or assume in this case always marcxml, this was the simplest band-aid, we will need to address the larger problem on another report.
Comment 13 Nick Clemens 2018-03-12 16:26:21 UTC
(In reply to Marcel de Rooy from comment #10)
> +        my $fixed_rec;
> +        eval { $fixed_rec = MARC::Record::new_from_xml( $set->{hits}->[$j],
> 'UTF-8' ) };
> +        $set->{hits}->[$j] = $fixed_rec unless $@;
> 
> No way to know what to expect ? Is it from internal or external ? Do we have
> properties of these external ones ?
> And can we check ref($set->{hits}->[$j]) to see if it is already
> MARC::Record ?

Aha, so...
We are searching the local database via z39.50 (not ES)
We pass the results to new_record_from_zebra
That sub assumes that if ES is enabled it will only be getting ES results (not true)

I see two solutions:
1 - Add a param to new_record_from_zebra to tell it to convert records or not
2 - Move the test for ES outside the sub, always convert the records in the sub and just don't call it if using ES

Opinion Marcel? or other suggestion
Comment 14 Nick Clemens 2018-05-25 09:38:14 UTC
Created attachment 75561 [details] [review]
Bug 18822: Check if we have MARC::Record and convert if not when using ES

The new_record_from_zebra subroutine assumes that when using ES we
always get MARC::Record objects when using ES, but sometimes we get them as xml via Z39 or
internally. This adds a test to new_from_zebra to confirm we have a
record object and to convert it if not

To test:
1 - Perform an advanced search from the advanced editor, make sure to
select 'Local catalog' and a remote source
2 - Error 'Internal search error [Object object]'
3 - Apply patch
4 - Repeat search, success!
5 - Select and deselect various servers, search should still work
Comment 15 Nick Clemens 2018-05-25 09:40:29 UTC
*** Bug 17739 has been marked as a duplicate of this bug. ***
Comment 16 Katrin Fischer 2018-07-08 15:39:22 UTC
This will only explode when there is a record found locally.
Comment 17 Katrin Fischer 2018-07-08 15:43:43 UTC
Created attachment 76764 [details] [review]
Bug 18822 - Better error response

Apply this patch first to see a change in the error feedback - instead
of [Object object] you should get a text readable error

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2018-07-08 15:43:47 UTC
Created attachment 76765 [details] [review]
Bug 18822: Check if we have MARC::Record and convert if not when using ES

The new_record_from_zebra subroutine assumes that when using ES we
always get MARC::Record objects when using ES, but sometimes we get them as xml via Z39 or
internally. This adds a test to new_from_zebra to confirm we have a
record object and to convert it if not

To test:
1 - Perform an advanced search from the advanced editor, make sure to
select 'Local catalog' and a remote source
2 - Error 'Internal search error [Object object]'
3 - Apply patch
4 - Repeat search, success!
5 - Select and deselect various servers, search should still work

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Marcel de Rooy 2018-07-13 08:55:43 UTC
Created attachment 76937 [details] [review]
Bug 18822 - Better error response

Apply this patch first to see a change in the error feedback - instead
of [Object object] you should get a text readable error

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2018-07-13 08:55:48 UTC
Created attachment 76938 [details] [review]
Bug 18822: Check if we have MARC::Record and convert if not when using ES

The new_record_from_zebra subroutine assumes that when using ES we
always get MARC::Record objects when using ES, but sometimes we get them as xml via Z39 or
internally. This adds a test to new_from_zebra to confirm we have a
record object and to convert it if not

To test:
1 - Perform an advanced search from the advanced editor, make sure to
select 'Local catalog' and a remote source
2 - Error 'Internal search error [Object object]'
3 - Apply patch
4 - Repeat search, success!
5 - Select and deselect various servers, search should still work

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2018-07-13 08:57:01 UTC
Second patch: no eval around new_from_xml as in the Zebra counterpart.
No blocker.
Comment 22 Nick Clemens 2018-07-13 13:47:18 UTC
Awesome work all!

Pushed to master for 18.11
Comment 23 Martin Renvoize 2018-07-15 19:29:03 UTC
Pushed to 18.05.x for 18.05.02
Comment 24 Fridolin SOMERS 2018-07-30 13:51:47 UTC
Pushed to 17.11.x for 17.11.09