|
Description
Alex Arnaud
2022-05-13 08:12:55 UTC
Created attachment 134952 [details] [review] Bug 30745: Use query builder and search_compat in label-item-search Test plan: - apply this patch, - make some searches in labels/label-item-search.pl using the 3 form input (index, after date, before date), - check it works like before Created attachment 134953 [details] [review] Bug 30745: handle ES ranges for date of acquisition Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests For Bug 16355, the patch doesn't apply and the last comment is from 2018. I tested the patch with elasticsearch and it works for me, but I do not have a functioning zebra to test with. Do I signoff nonetheless? *** Bug 30908 has been marked as a duplicate of this bug. *** Created attachment 135784 [details] [review] Bug 30745: Use query builder and search_compat in label-item-search Test plan: - apply this patch, - make some searches in labels/label-item-search.pl using the 3 form input (index, after date, before date), - check it works like before Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 135785 [details] [review] Bug 30745: handle ES ranges for date of acquisition Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Alex, please don't forget to assign the bug :) Created attachment 135966 [details] [review] Bug 30745: Use query builder and search_compat in label-item-search Test plan: - apply this patch, - make some searches in labels/label-item-search.pl using the 3 form input (index, after date, before date), - check it works like before Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Created attachment 135967 [details] [review] Bug 30745: handle ES ranges for date of acquisition Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 This tests well, so happy to sign-off, but we are missing unit tests for the changes to Querybuilder.pm. Let us know if you need help with this. *** Bug 28987 has been marked as a duplicate of this bug. *** Patches need rebase Created attachment 157192 [details] [review] Bug 30745: Use query builder and search_compat in label-item-search Test plan: - apply this patch, - make some searches in labels/label-item-search.pl using the 3 form input (index, after date, before date), - check it works like before Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Created attachment 157194 [details] [review] Bug 30745: Use query builder and search_compat in label-item-search Test plan: - apply this patch, - make some searches in labels/label-item-search.pl using the 3 form input (index, after date, before date), - check it works like before Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Created attachment 157195 [details] [review] Bug 30745: handle ES ranges for date of acquisition Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 The patch has been rebased. Created attachment 157934 [details] [review] Bug 30745: Display pagination for the results page I have fixed the pagination issue when the search returns multiple results. Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests pagination tests - Perform a search that returns more than 20 notices - Navigate through different result pages using page numbers, the "Next" button, and the "Previous" button => On each page, check that "Results X through X of X" displays the correct information Is there a kind volunteer who could spare some time to test this rebased patch? Your feedback is much appreciated. Cheers! I was about to sign off, but then came across this Zebra only error - I get this error after the patches are applied (and a reindex): Error: Can't use an undefined value as an ARRAY reference at /kohadevbox/koha/labels/label-item-search.pl line 88 .... 1. Generated by putting a letter in the input box for these index searches (without any dates in the filters): publication date, standard number, ISBN, ISSN, call number, acquisition date 2. I could also generate this error with a number in a text field, for example: Author index, 2011 (but with 2000 no error is generated) 3. With Elasticsearch, no results are displayed and the form just refreshes (both before and after the patches, and the same with Zebra before the patches) Other notes =========== 1. I couldn't work out how or where cgi-bin/koha/labels/label-item-search.pl is used/accessed from - something to do with acquisitions? 2. I've changed the assignee. 3. I didn't test with a UNIMARC installation. Testing notes (using koha-testing-docker) ========================================= Setup ~~~~~ To test with Elasticsearch or OpenSearch - start up KTD with Elasticsearch or OpenSearch: ktd --es8 up OR ktd --os2 up (I didn't test with Open Search, just Zebra and Elasticsearch) Go to this page in the staff interface: http://127.0.0.1:8081/cgi-bin/koha/labels/label-item-search.pl Searching using Zebra (after KTD is started with --es8): - Change the SearchEngine system preference to Zebra - Reload http://127.0.0.1:8081/cgi-bin/koha/labels/label-item-search.pl - Reindex: koha-rebuild-zebra -d -f -v kohadev - Repeat all the example searches - you should get the same results Demonstrating the issue and that the patches fix it ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Notes: In the sample data, the acquisition date is recorded in 952$d - Date acquired. For many items, 09/04/2014 is used. For some items, these dates are used 05/08/2014, 05/07/2014, 10/15/2014, 12/04/2014, 1/05/2015. Many items don't have a date recorded. If there is no value in the index field, it is ignored. If an index is selected and a value is entered, the results should be filtered by any values entered in 'Added on or after date' and 'Added on or before date' fields. Before the patches: index searches work, but when used with 'Added on or after date' and 'Added on or before date' options, the search results ignore them. However, entering a date for the 'Acquisition date' index does work. A title index search: - Title index: perl => Expected result: 11 => Actual result: 11 A combined search with dates: - Title index: perl - Added on or after date: 09/04/2014 => Expected result: 10 (1 perl record has no items) => Actual result: No results A combined search with dates: - Title index: perl - Added on or before date: 09/04/2014 => Expected result: 10 (1 perl record has no items) => Actual result: No results Acquisition date index search: - Index: select 'Acquisition date' from the dropdown list - Value to enter: 09/04/2014 => Expected result: 352 results => Actual result: 352 result Acquisition date index search: - Index: select 'Acquisition date' from the dropdown list - Value to enter: 09/04/2014 - Added on or after date: 09/04/2014 OR - Added on or before date: 09/04/2014 OR Use both or none => Expected result: 352 results => Actual result: 352 results Search using 'Added on or after date' (after date) only: - Added on or after date: 09/04/2014 => Expected result: should get 408 records => Actual result: 0 results Search using 'Added on or before date' (before date) only: - Added on or after date: 09/04/2014 => Expected result: should get 354 records => Actual result: 0 results Publication date index search using 2011 (AFTER patches are applied - doesn't work as expected BEFORE the patches are applied): - Results if no before or after filter added: 8 (as expected) - Results if Added on or after date: 09/04/2014 - 8 (as expected) - Results if Added on or before date: 09/04/2014 - 7 (as expected) - Results for a date range - 7 (as expected): . Added on or after date: 09/04/2014 . Added on or before date: 09/05/2014 - Results for a date range - 8 (as expected): . Added on or after date: 09/04/2014 . Added on or before date: 10/15/2014 - Results for a date range - 1 (as expected): . Added on or after date: 09/05/2014 . Added on or before date: 10/15/2014 (One record and all its items has acquisition dates of 10/15/2014 - Blackstone's statutes on company law 2014-2015; the rest have 09/04/2014 acquisition dates) Search using just a date range (and no index) (AFTER patches are applied - doesn't work as expected BEFORE the patches are applied): - Added on or after date: 09/04/2014 => should get 408 results - Added on or before date: 09/03/2014 => should get 3 results - Added on or before date: today's date => should get 410 results (note: 408 + 3 doesn't equal 410 but it is correct, as one record has items with different acquisition dates: The C programming language has items with different acquisition dates - 1 x 09/04/2014 and 3 x 05/08/2014) - Added on or after date: 09/05/2014 => should get 56 results - Added on or after date: 12/04/2014 => should get 40 results - Added on or after date: 09/05/2014 - Added on or before date: 12/03/2014 => should get 16 records Pagination of search results ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1. Perform a search that returns more than 20 results - for example Title index, a: 2. Navigate through different result page navigation using page numbers, the "Next" button, and the "Previous" button => On each page, check that "Results X through X of X" displays the correct information - 20 results are displayed for each page - the total number of results should not change Created attachment 162855 [details] [review] Bug 30745 : Fix undefined value error while using zebra Plan test : - Apply this batch - Set Zebra as searchEngine - Put a letter in the input box for these index searches (without any dates in the filters): publication date, standard number, ISBN, ISSN, call number, acquisition date - The page's supposed to refresh itself (so that it has the same behavior as elasticSearch) Thanks for fixing the error identified in the previous testing. This no longer occurs (using Zebra and putting a letter in number index fields such as ISBN, Publication date, and so on). However, for: 1. Search results: the pagination is no longer working - it just goes back to the search form if you click on next or any of the numbers. There are some console errors on the search results page (using the web developer tools) 2. The select all and clear all links no longer work Why the pagination and links aren't working working may be because of the CSRF changes in bug 34478 and bug 36192 (but I don't know enough to know if that is the cause). Created attachment 163955 [details] [review] Bug 30745 : Fix pagination error 1. I've fixed the pagination option 2. "Select all" and "clear all" buttons seem all functional for me. Could you double check it with my new test plan? Test Plan - Activate Zebra as search engine - Cataloging -> Label Creator - Click on "New" then "Label Batch" - Click on "Add item(s)" - Choose a search option (For exemple i put "a" for "Title" so that we can obtain a result that has multiple pages) - Click on "Search" - Scroll all the pages using page number or "Previous" and "Next" button to test the pagination option - Click on "Select all" or "Clear all" to test the functionality of both buttons then click on "Add checked", the barcode of the seleccted items will be added to the "Add by barcode(s) or itemnumbers(s)" case We've bumped into this issue, so I will be looking at this more. -- Note for testing: 1. Go to /cgi-bin/koha/labels/label-edit-batch.pl?op=new 2. Click "Add item(s)" 3. In the pop-up window, click the date picker next to "Added on or before date:" and choose today's date 4. Click "Search" button The original implementation of the "ccl_query" parameter is bizarre. A search interface with 4 boxes shouldn't be that overcooked. I'm just reviewing the code right now, but some of these patches look weird. Probably because the original code is weird. Oh well. I'm going to wear my Tester hat instead of my QA hat for this one... Another idea would be to use Elasticsearch specific syntax here like "acqdate:>=XXXX-XX-XX" and "acqdate:<=XXXX-XX-XX". (See bug 28899) I am actually tempted to try that out locally and hot fix using that... -- Admittedly, that would set a precedent for creating a ES specific query instead of a Zebra specific query. But... it could be much less painful and make for a much neater change. As Katrin has noted, there are no unit tests for the change to Koha/SearchEngine/Elasticsearch/QueryBuilder.pm which is going to be an automatic "Failed QA" here anyway. What one could do is create a Koha::* class of some kind that builds the search for label-item-search.pl and unit test that. Anyway, food time... Thanks all for keeping this alive ;) Note that the paging is broken due to CSRF stuff in master as well. (In reply to David Cook from comment #28) > Note that the paging is broken due to CSRF stuff in master as well. See bug 36931 So... my workaround works and is considerably less scary:
my $search_engine = C4::Context->preference('SearchEngine');
my $is_elasticsearch = 1 if $search_engine && $search_engine eq 'Elasticsearch';
$idx = $query->param('idx');
$ccl_textbox = $query->param('ccl_textbox');
if ( $ccl_textbox && $idx ) {
$ccl_query = "$idx:$ccl_textbox";
}
$datefrom = $query->param('datefrom');
$dateto = $query->param('dateto');
if ($datefrom) {
$ccl_query .= ' AND ' if $ccl_textbox;
if ($is_elasticsearch){
$ccl_query .= "acqdate:>" . $datefrom;
}
else {
$ccl_query .= "acqdate,ge,st-date-normalized=" . $datefrom;
}
}
if ($dateto) {
$ccl_query .= ' AND ' if ( $ccl_textbox || $datefrom );
if ($is_elasticsearch){
$ccl_query .= "acqdate:<" . $dateto;
}
else {
$ccl_query .= "acqdate,le,st-date-normalized=" . $dateto;
}
}
But... we do need a solution for handling date range searching throughout Koha with Elastic! I'm going to use my workaround for now locally, but we do need to keep working on this. -- Note that this is a "Failed QA" for a few reasons. 1. "Bug 30745 : Fix pagination error" isn't how we've chosen to fix this in Koha. Since it's a stateless operation, it should be a GET and not a POST. That's reported on bug 36931 I think a number of these patches probably should be on separate bugs... 2. It doesn't add any unit tests. (And actually now that I look at t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t it looks like we already have some interesting date handling for ranges. Something to look at further there...) The more I read... the more I'm not convinced that build_query_compat and search_compat are the way to go. If you look at _create_query_string in Koha/SearchEngine/Elasticsearch/QueryBuilder.pm, there is some handling for st-year. I think we might want to look at _modify_string_by_type() as well. We'll need unit tests regardless. For the controllers and template... it is still bonkers that we're passing query strings around when we can just pass the date values. Admittedly, that's a pre-existing issue. So I might spin that out as another bug report and attach it as a dependency. In any case, I'm planning to take another look at this tomorrow. Hello. Can anyone fixed this issue? (In reply to kamilzade.toghrul from comment #33) > Hello. Can anyone fixed this issue? Unfortunately, the library I had experiencing this problem has switched back to Zebra as a workaround, so this issue is very low priority for my organisation. Created attachment 170677 [details] [review] Bug 30745: Use query builder and search_compat in label-item-search Test plan: - apply this patch, - make some searches in labels/label-item-search.pl using the 3 form input (index, after date, before date), - check it works like before Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Created attachment 170678 [details] [review] Bug 30745: handle ES ranges for date of acquisition Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Created attachment 170679 [details] [review] Bug 30745: Display pagination for the results page I have fixed the pagination issue when the search returns multiple results. Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests pagination tests - Perform a search that returns more than 20 notices - Navigate through different result pages using page numbers, the "Next" button, and the "Previous" button => On each page, check that "Results X through X of X" displays the correct information Created attachment 170680 [details] [review] Bug 30745 : Fix error 500 when item is not found while using zebra Plan test : - Apply this batch - Set Zebra as searchEngine - Go to Cataloging -> # Label creator - Click on New, Select Label batch - Click on Add item(s) - Search for for inexisting items => The page should refresh itself (so that it has the same behavior as elasticSearch) Created attachment 170681 [details] [review] Bug 30745: Use query builder and search_compat in label-item-search Test plan: - apply this patch, - make some searches in labels/label-item-search.pl using the 3 form input (index, after date, before date), - check it works like before Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Created attachment 170682 [details] [review] Bug 30745: handle ES ranges for date of acquisition Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Created attachment 170683 [details] [review] Bug 30745: Display pagination for the results page I have fixed the pagination issue when the search returns multiple results. Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests pagination tests - Perform a search that returns more than 20 notices - Navigate through different result pages using page numbers, the "Next" button, and the "Previous" button => On each page, check that "Results X through X of X" displays the correct information Created attachment 170684 [details] [review] Bug 30745: Fix error 500 when item is not found while using zebra Plan test : - Apply this batch - Set Zebra as searchEngine - Go to Cataloging -> # Label creator - Click on New, Select Label batch - Click on Add item(s) - Search for for inexisting items => The page should refresh itself (so that it has the same behavior as elasticSearch) (In reply to David Nind from comment #21) > However, for: > > 1. Search results: the pagination is no longer working - it just goes back > to the search form if you click on next or any of the numbers. There are > some console errors on the search results page (using the web developer > tools) > > 2. The select all and clear all links no longer work There are some console errors because we are not supposed to access labels/label-item-search.pl directly from the url, the test plan is not correct we should go to Cataloging -> # Label creator -> Click on New -> Select Label batch then Click on Add item(s) (In reply to Alex Arnaud from comment #2) > Created attachment 134953 [details] [review] [review] > Bug 30745: handle ES ranges for date of acquisition > > Test plan: > - Enable elasticsearch > - git to labels/label-item-search.pl > - search using after date only > => check you get items with date-of-acquisition greater than after > date > - search using before date only > => check you get items with date-of-acquisition less than before > date > - search using after and before date > => check you get items with date-of-acquisition between after and > before > - Combine this searches with a specified index > - enable zebra and repeat these tests Here's a correct test plan Test plan: - Enable elasticsearch - Go to Cataloging -> # Label creator - Click on New, Select Label batch - Click on Add item(s) - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests Created attachment 170729 [details] [review] Bug 30745: Use query builder and search_compat in label-item-search Test plan: - apply this patch, - make some searches in labels/label-item-search.pl using the 3 form input (index, after date, before date), - check it works like before Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Created attachment 170730 [details] [review] Bug 30745: handle ES ranges for date of acquisition Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Created attachment 170731 [details] [review] Bug 30745: Display pagination for the results page I have fixed the pagination issue when the search returns multiple results. Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests pagination tests - Perform a search that returns more than 20 notices - Navigate through different result pages using page numbers, the "Next" button, and the "Previous" button => On each page, check that "Results X through X of X" displays the correct information Created attachment 170732 [details] [review] Bug 30745: Fix error 500 when item is not found while using zebra Plan test : - Apply this batch - Set Zebra as searchEngine - Go to Cataloging -> # Label creator - Click on New, Select Label batch - Click on Add item(s) - Search for for inexisting items => The page should refresh itself (so that it has the same behavior as elasticSearch) Created attachment 170733 [details] [review] Bug 30745: (follow-up) adding some handling for st-date-normalized and adding unit tests Created attachment 171002 [details] [review] Bug 30745: Use query builder and search_compat in label-item-search Test plan: - apply this patch, - make some searches in labels/label-item-search.pl using the 3 form input (index, after date, before date), - check it works like before Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Created attachment 171003 [details] [review] Bug 30745: handle ES ranges for date of acquisition Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Created attachment 171004 [details] [review] Bug 30745: Display pagination for the results page I have fixed the pagination issue when the search returns multiple results. Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests pagination tests - Perform a search that returns more than 20 notices - Navigate through different result pages using page numbers, the "Next" button, and the "Previous" button => On each page, check that "Results X through X of X" displays the correct information Created attachment 171005 [details] [review] Bug 30745: Fix error 500 when item is not found while using zebra Plan test : - Apply this batch - Set Zebra as searchEngine - Go to Cataloging -> # Label creator - Click on New, Select Label batch - Click on Add item(s) - Search for for inexisting items => The page should refresh itself (so that it has the same behavior as elasticSearch) Created attachment 171006 [details] [review] Bug 30745: (follow-up) adding some handling for st-date-normalized and adding unit tests The patch says it as a conflict, but i can't seem to be able to find any conflict for the patch "adding some handling for st-date-normalized", i'm not sure if its just my installation or if the patch will need to be rewritten and pushed again (In reply to Olivier Vezina from comment #55) > The patch says it as a conflict, but i can't seem to be able to find any > conflict for the patch "adding some handling for st-date-normalized", i'm > not sure if its just my installation or if the patch will need to be > rewritten and pushed again Hi Olivier, welcome :) You tried to apply the patch, but it does no longer apply because of a conflict? Please switch the status to "Patch doesn't apply" in that case. Olivier needs to read the error output :-) He gets the dreaded SHA1. He'll rewrite the patch. Created attachment 173280 [details] [review] Bug 30745: (follow-up) Rewritting the st-date-normalized test The patch was corrupted, and was rewritten as the original one by Hammat Created attachment 173943 [details] [review] Bug 30745: Use query builder and search_compat in label-item-search Test plan: - apply this patch, - make some searches in labels/label-item-search.pl using the 3 form input (index, after date, before date), - check it works like before Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Created attachment 173944 [details] [review] Bug 30745: handle ES ranges for date of acquisition Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Created attachment 173945 [details] [review] Bug 30745: Display pagination for the results page I have fixed the pagination issue when the search returns multiple results. Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests pagination tests - Perform a search that returns more than 20 notices - Navigate through different result pages using page numbers, the "Next" button, and the "Previous" button => On each page, check that "Results X through X of X" displays the correct information Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Created attachment 173946 [details] [review] Bug 30745: Fix error 500 when item is not found while using zebra Plan test : - Apply this batch - Set Zebra as searchEngine - Go to Cataloging -> # Label creator - Click on New, Select Label batch - Click on Add item(s) - Search for for inexisting items => The page should refresh itself (so that it has the same behavior as elasticSearch) Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Created attachment 173947 [details] [review] Bug 30745: (follow-up) Rewritting the st-date-normalized test The patch was corrupted, and was rewritten as the original one by Hammat Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> At a glance, I think this is looking pretty good. I'll make a note to come back and look at this again, as this was a blocker for one of my libraries moving to Elasticsearch... I'm not super happy with the way the changes have been implemented to label-item-search.pl, but I can confirm that it appears to work, and that's the key thing. I'd say that's good enough. Since label-item-search.pl is a fairly isolated and niche script, I'm not fussed.
I'm going to just double-check the QueryBuilder stuff a bit more...
I'm also noticing the following warnings with the QA tools:
WARN Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
WARN tidiness
The file is less tidy than before (bad/messy lines before: 315, now: 322)
OK koha-tmpl/intranet-tmpl/prog/en/modules/labels/result.tt
WARN labels/label-item-search.pl
WARN tidiness
The file is less tidy than before (bad/messy lines before: 53, now: 63)
WARN t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
WARN tidiness
The file is less tidy than before (bad/messy lines before: 326, now: 336)
(In reply to David Cook from comment #65) > I'm going to just double-check the QueryBuilder stuff a bit more... Hmm... we still don't have feature parity between Zebra and Elasticsearch here in terms of the QueryBuilder. We're not supporting the "ge" and "le" with acqdate in Elasticsearch: Zebra: acqdate,ge,st-date-normalized=2015 4 results Elasticsearch: acqdate,ge,st-date-normalized=2015 0 results -- But the approach used in these patches works for both Zebra and Elasticsearch: Zebra: acqdate,st-date-normalized=2015 - Elasticsearch: acqdate,st-date-normalized="2015 -" And it looks like we don't have any hard-coded places where ",ge" or ",le" are used with acqdate. -- I tried out the "Acquisition date (yyyy-mm-dd)" in advanced search and it appears to still work for both Zebra and Elasticsearch. I think this particular scenario is a tough one due to very specific search engine syntax required... but I think this works and doesn't break anything else. I'm happy to give the thumbs up once the QA tool warnings are cleared. I might take a quick look at doing that now... Created attachment 174486 [details] [review] Bug 30745: Use query builder and search_compat in label-item-search Test plan: - apply this patch, - make some searches in labels/label-item-search.pl using the 3 form input (index, after date, before date), - check it works like before Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 174487 [details] [review] Bug 30745: handle ES ranges for date of acquisition Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=30845 Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 174488 [details] [review] Bug 30745: Display pagination for the results page I have fixed the pagination issue when the search returns multiple results. Test plan: - Enable elasticsearch - git to labels/label-item-search.pl - search using after date only => check you get items with date-of-acquisition greater than after date - search using before date only => check you get items with date-of-acquisition less than before date - search using after and before date => check you get items with date-of-acquisition between after and before - Combine this searches with a specified index - enable zebra and repeat these tests pagination tests - Perform a search that returns more than 20 notices - Navigate through different result pages using page numbers, the "Next" button, and the "Previous" button => On each page, check that "Results X through X of X" displays the correct information Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 174489 [details] [review] Bug 30745: Fix error 500 when item is not found while using zebra Plan test : - Apply this batch - Set Zebra as searchEngine - Go to Cataloging -> # Label creator - Click on New, Select Label batch - Click on Add item(s) - Search for for inexisting items => The page should refresh itself (so that it has the same behavior as elasticSearch) Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 174490 [details] [review] Bug 30745: (follow-up) Rewritting the st-date-normalized test The patch was corrupted, and was rewritten as the original one by Hammat Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 174491 [details] [review] Bug 30745: (QA follow-up) tidy Signed-off-by: David Cook <dcook@prosentient.com.au> Pushed for 24.11! Well done everyone, thank you! |