Description
Katrin Fischer
2019-10-29 16:42:44 UTC
Created attachment 134381 [details] [review] Bug 23919: Add new system preference SearchWithISSNVariations Created attachment 134382 [details] [review] Bug 23919: Zebra search by ISSN variations Created attachment 134383 [details] [review] Bug 23919: Add tests for biblioItems search by ISBN and ISSN with variations (sysprefs SearchWithISBNVariations/SearchWithISSNVariations) Created attachment 134384 [details] [review] Bug 23919: Items search by ISBN variations and ISSN variations Created attachment 134385 [details] [review] Bug 23919: Add tests for Items search by ISBN and ISSN with variations (sysprefs SearchWithISBNVariations/SearchWithISSNVariations) Created attachment 134422 [details] [review] Bug 23919: Add new system preference SearchWithISSNVariations Created attachment 134423 [details] [review] Bug 23919: Zebra search by ISSN variations Created attachment 134424 [details] [review] Bug 23919: Add tests for biblioItems search by ISBN and ISSN with variations (sysprefs SearchWithISBNVariations/SearchWithISSNVariations) Created attachment 134425 [details] [review] Bug 23919: Items search by ISBN variations and ISSN variations Created attachment 134426 [details] [review] Bug 23919: Add tests for Items search by ISBN and ISSN with variations (sysprefs SearchWithISBNVariations/SearchWithISSNVariations) Created attachment 134440 [details] [review] Bug 23919: Add new system preference SearchWithISSNVariations Created attachment 134441 [details] [review] Bug 23919: Zebra search by ISSN variations Created attachment 134442 [details] [review] Bug 23919: Add tests for biblioItems search by ISBN and ISSN with variations (sysprefs SearchWithISBNVariations/SearchWithISSNVariations) Created attachment 134443 [details] [review] Bug 23919: Items search by ISBN variations and ISSN variations Created attachment 134444 [details] [review] Bug 23919: Add tests for Items search by ISBN and ISSN with variations (sysprefs SearchWithISBNVariations/SearchWithISSNVariations) Hello Slava, Do you have a test plan we can use when trying this patchset? Thanks! Hello Lucas, sure. 1. SearchWithISSNVariations setting is missing in the system preference 2. Apply patch 3. Check that SearchWithISSNVariations is present in the system preferences 4. Check that SearchWithISSNVariations has "search" and "don't search" parameters Here is an updated test plan I came up with - I'll add a sign-off shortly. I hope this covers all the changes and things to test for! This enhancement enables searching by ISSN without using hyphens and using a space instead of the hyphen. This works in the advanced search (staff interface and OPAC) and item search (staff interface). It is enabled using a new system preference, SearchWithISSNVariations, with two options - "don't search" (the default), and "search". This is similar to the SearchWithISBNVariations system preference. Test plan: 1. Search for an ISSN number with a hyphen, without a hyphen (no space), and with a space instead of the hyphen (example record: 1064-3923 (record #359 in test data for koha-testing-docker)): (a) standard search (staff inerface and OPAC) => result found with hyphen, not found if no hyphen or space used [note: no change after patch applied] (b) ISSN keyword field in the advanced search (staff interface and OPAC) => result found, not found if no hyphen or space used (c) item search in the staff interface (acces from Search drop down list in the navigation at the top of the page > Item search > field search - change to ISSN) => result found, not found if no hyphen or space used 2. Search for SearchWithISSNVariations in the system preferences search => no system preference found with that name. 3. Apply the patch, update the database (updatedatabase), flush_memcached, restart_all (I also reindexed - not sure whether this is necessary: koha-rebuild-zebra -f -v kohadev) (koha-testing-docker). 4. Search for the system preference SearchWith - system preference SearchWithISSNVariations found, with default setting set as "don't search". 5. Check that the text for the system preference reads well (it uses similar wording to the SearchWithISBNVariations system preference). 6. Repeat step 1 with SearchWithISSNVariations set as "don't search" => current behavior, that is, no results found if no hyphen or space used. 7. Repeat step 1 with SearchWithISSNVariations set as "search" => should now find the record with and without the hyphen and with a space instead of the hyphen for steps 1(b) and 1(c). 8. Run the tests - these should pass: . prove t/Search/buildQuery.t . prove t/db_dependent/Items.t 8. Sign off. Notes: - I think the QA Team will probably want the commit message(s?) to follow the guidelines for the descriptions (see https://wiki.koha-community.org/wiki/Commit_messages#Bug.2Ffeature_description) - Maybe an additional enhancement (as a separate bug) would be to make this work when searching on the ISSN number in the serials search. Created attachment 135288 [details] [review] Bug 23919: Add new system preference SearchWithISSNVariations Signed-off-by: David Nind <david@davidnind.com> Created attachment 135289 [details] [review] Bug 23919: Zebra search by ISSN variations Signed-off-by: David Nind <david@davidnind.com> Created attachment 135290 [details] [review] Bug 23919: Add tests for biblioItems search by ISBN and ISSN with variations (sysprefs SearchWithISBNVariations/SearchWithISSNVariations) Signed-off-by: David Nind <david@davidnind.com> Created attachment 135291 [details] [review] Bug 23919: Items search by ISBN variations and ISSN variations Signed-off-by: David Nind <david@davidnind.com> Created attachment 135292 [details] [review] Bug 23919: Add tests for Items search by ISBN and ISSN with variations (sysprefs SearchWithISBNVariations/SearchWithISSNVariations) Signed-off-by: David Nind <david@davidnind.com> (In reply to Katrin Fischer from comment #0) > For the ISBN we have the option to activate search with and without hyphens > - but this doesn't work for ISSN. We shoudl make it work there as well. Curious about this one... Since Zebra normalizes the data by stripping out hyphens, it shouldn't be relevant for Zebra? (In reply to David Cook from comment #24) > (In reply to Katrin Fischer from comment #0) > > For the ISBN we have the option to activate search with and without hyphens > > - but this doesn't work for ISSN. We shoudl make it work there as well. > > Curious about this one... > > Since Zebra normalizes the data by stripping out hyphens, it shouldn't be > relevant for Zebra? IIRC if you search without the hyphen, you will not find the case with hyphen as it's treated as a space (or it might be in our installations, but it looks like we are not the only ones) (In reply to Katrin Fischer from comment #25) > IIRC if you search without the hyphen, you will not find the case with > hyphen as it's treated as a space (or it might be in our installations, but > it looks like we are not the only ones) Ahhh right. My mistake. That makes sense. Created attachment 138311 [details] [review] Bug 23919: Add new system preference SearchWithISSNVariations Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 138312 [details] [review] Bug 23919: Zebra search by ISSN variations Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 138313 [details] [review] Bug 23919: Add tests for biblioItems search by ISBN and ISSN with variations (sysprefs SearchWithISBNVariations/SearchWithISSNVariations) Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 138314 [details] [review] Bug 23919: Items search by ISBN variations and ISSN variations Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 138315 [details] [review] Bug 23919: Add tests for Items search by ISBN and ISSN with variations (sysprefs SearchWithISBNVariations/SearchWithISSNVariations) Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> No mention of Elasticsearch in the test plan? Does this already work on Elasticsearch? It's a search expansion, not an indexing change - searching for different variations. The search engine should not matter :) Created attachment 138480 [details] [review] Bug 23919: (QA follow-up) Tidy query build code This is really opinionated, but I found this to be much cleaner to read and thought it was worth pusing as well. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 22.11. Nice work everyone, thanks! Enhancement will not be backported to 22.05.x I am sad to say, but adding this to item search was not a great idea :( Item search is SQL based, not a search engine, which changes things quite a bit. We are exploding the search now with up to 4 forms of ISBN into something like: isbn like 'isbn1' or isbn like 'isbn2' ... But: We are not truncating. As soon as there is more than one ISBN or ISSN (these are repeatable fields) or the library actually follows MARC21 standard adding ISBD punctuation, this fails. We store in biblioitems.isbn like this: isbn1 ; | isbn2 ; | isbn3. isbn1. This also seems to create quite a performance issue on big databases and it can't be turned off separately from the normal search. I've filed bug 33514. (In reply to Katrin Fischer from comment #37) > I am sad to say, but adding this to item search was not a great idea :( > > Item search is SQL based, not a search engine, which changes things quite a > bit. > > We are exploding the search now with up to 4 forms of ISBN into something > like: > > isbn like 'isbn1' or isbn like 'isbn2' ... > > But: We are not truncating. > > As soon as there is more than one ISBN or ISSN (these are repeatable fields) > or the library actually follows MARC21 standard adding ISBD punctuation, > this fails. We store in biblioitems.isbn like this: > > isbn1 ; | isbn2 ; | isbn3. > isbn1. > > This also seems to create quite a performance issue on big databases and it > can't be turned off separately from the normal search. > > I've filed bug 33514. Can you please address bug 33514? In the meanwhile should we remove this? (In reply to Jonathan Druart from comment #38) > (In reply to Katrin Fischer from comment #37) > > I am sad to say, but adding this to item search was not a great idea :( > > > > Item search is SQL based, not a search engine, which changes things quite a > > bit. > > > > We are exploding the search now with up to 4 forms of ISBN into something > > like: > > > > isbn like 'isbn1' or isbn like 'isbn2' ... > > > > But: We are not truncating. > > > > As soon as there is more than one ISBN or ISSN (these are repeatable fields) > > or the library actually follows MARC21 standard adding ISBD punctuation, > > this fails. We store in biblioitems.isbn like this: > > > > isbn1 ; | isbn2 ; | isbn3. > > isbn1. > > > > This also seems to create quite a performance issue on big databases and it > > can't be turned off separately from the normal search. > > > > I've filed bug 33514. > > Can you please address bug 33514? In the meanwhile should we remove this? What do you mean with "address"? It's all explained there as well. I was actually asking the author of 23919 (here) to fix the bug they introduced. Can you please help on the follow-up bug report? See bug 33514. |