Currently what this limit does is a filter on onloan:false This means that: - Any biblio with at least one checked out item will be hidden from the results, even if it has other available items - A biblio whose items are all lost, damaged, withdrawn or "notforloan" will not be hidden, but the results will show "X items, none available"
Created attachment 104356 [details] [review] Bug 25375: Fix 'available' facet in elasticsearch Add a new boolean ES field named 'available', which is true if at least one item is available, which means the item is not on loan, not "notforloan", not withdrawn, not lost and not damaged A full indexation is required Test plan: 1. Apply patch and run updatedatabase.pl 2. Run `misc/search_tools/rebuild_elasticsearch.pl -d -b` 3. Make sure you have some biblios whose items are all unavailable, some biblios whose items are all available, and some biblios with at least one item available and at least one item unavailable 4. Use the 'available' filter on both opac and intranet and make sure it works as expected.
I'm getting an error in the Elastic rebuild: root@287fd5fefa08:koha(master)$ perl misc/search_tools/rebuild_elasticsearch.pl -d -b Can't call method "items" on an undefined value at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch.pm line 710.
(In reply to Andrew Fuerste-Henry from comment #2) > I'm getting an error in the Elastic rebuild: > root@287fd5fefa08:koha(master)$ perl > misc/search_tools/rebuild_elasticsearch.pl -d -b > Can't call method "items" on an undefined value at > /kohadevbox/koha/Koha/SearchEngine/Elasticsearch.pm line 710. MARC21 doesn't store the biblionumber in the 001 Should probably call GetMarcFromKohaField( 'biblio.biblionumber' ) to determine which field to use here I don't know if calling that for all the item fields may be more performant that fetching the biblio and iterating items as opposed to checking the MARC as well
Created attachment 109647 [details] [review] Bug 25375: Get biblionumber from the correct MARC field
Created attachment 109659 [details] [review] Bug 25375: Fix 'available' facet in elasticsearch Add a new boolean ES field named 'available', which is true if at least one item is available, which means the item is not on loan, not "notforloan", not withdrawn, not lost and not damaged A full indexation is required Test plan: 1. Apply patch and run updatedatabase.pl 2. Run `misc/search_tools/rebuild_elasticsearch.pl -d -b` 3. Make sure you have some biblios whose items are all unavailable, some biblios whose items are all available, and some biblios with at least one item available and at least one item unavailable 4. Use the 'available' filter on both opac and intranet and make sure it works as expected. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 109660 [details] [review] Bug 25375: Get biblionumber from the correct MARC field Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 109951 [details] [review] Bug 25375: (QA follow-up) Count available items rather than iterating
Created attachment 109952 [details] [review] Bug 25375: Fix 'available' facet in elasticsearch Add a new boolean ES field named 'available', which is true if at least one item is available, which means the item is not on loan, not "notforloan", not withdrawn, not lost and not damaged A full indexation is required Test plan: 1. Apply patch and run updatedatabase.pl 2. Run `misc/search_tools/rebuild_elasticsearch.pl -d -b` 3. Make sure you have some biblios whose items are all unavailable, some biblios whose items are all available, and some biblios with at least one item available and at least one item unavailable 4. Use the 'available' filter on both opac and intranet and make sure it works as expected. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 109953 [details] [review] Bug 25375: Get biblionumber from the correct MARC field Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 109954 [details] [review] Bug 25375: (QA follow-up) Count available items rather than iterating
This works well, we should highlight the change in the release notes - in my testing nothing breaks, but we will need a full reindex for any ES site to make it work correctly. We will also need to highlight the new field for any sites using custom mapping files I added a followup - the performance doesn't seem to change greatly, but there is slight improvement on records with many items, and I think it is a bit simpler
Tests are missing.
Why not use "not-onloan-count" like for Zebra ?
Giving this a bump as it continues to impact libraries. Julian, will you be able to revisit this?
Created attachment 131310 [details] [review] Bug 25375: Fix 'available' facet in elasticsearch Add a new boolean ES field named 'available', which is true if at least one item is available, which means the item is not on loan, not "notforloan", not withdrawn, not lost and not damaged A full indexation is required Test plan: 1. Apply patch and run updatedatabase.pl 2. Run `misc/search_tools/rebuild_elasticsearch.pl -d -b` 3. Make sure you have some biblios whose items are all unavailable, some biblios whose items are all available, and some biblios with at least one item available and at least one item unavailable 4. Use the 'available' filter on both opac and intranet and make sure it works as expected. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 131311 [details] [review] Bug 25375: Get biblionumber from the correct MARC field Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 131312 [details] [review] Bug 25375: (QA follow-up) Count available items rather than iterating
Created attachment 131313 [details] [review] Bug 25375: Add tests for the "available" ES field
Patches rebased, and tests added. Back to Signed off
Created attachment 132327 [details] [review] Bug 25375: Fix 'available' facet in elasticsearch Add a new boolean ES field named 'available', which is true if at least one item is available, which means the item is not on loan, not "notforloan", not withdrawn, not lost and not damaged A full indexation is required Test plan: 1. Apply patch and run updatedatabase.pl 2. Run `misc/search_tools/rebuild_elasticsearch.pl -d -b` 3. Make sure you have some biblios whose items are all unavailable, some biblios whose items are all available, and some biblios with at least one item available and at least one item unavailable 4. Use the 'available' filter on both opac and intranet and make sure it works as expected. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 132328 [details] [review] Bug 25375: Get biblionumber from the correct MARC field Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 132329 [details] [review] Bug 25375: (QA follow-up) Count available items rather than iterating Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 132330 [details] [review] Bug 25375: Add tests for the "available" ES field Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
I wonder if 'not "notforloan"' should not only be for on order items that have not yet arrived at the library. As it is now we exclude all reference items as 'not available' which is not true usually. A lot of special libraries have large collections that are not for loan but can still be consulted.
Same for damaged - that something is damaged doesn't equal unavailable in a lot of libraries. It might just be so that when the item is checked out the patron is not blamed for pre-existing damage. Maybe we do need to have some configuration options here? If I could pick, I'd pick lost, withdrawn and on loan as unavailable.
(In reply to Katrin Fischer from comment #25) > Same for damaged - that something is damaged doesn't equal unavailable in a > lot of libraries. It might just be so that when the item is checked out the > patron is not blamed for pre-existing damage. > > Maybe we do need to have some configuration options here? > > If I could pick, I'd pick lost, withdrawn and on loan as unavailable. I think it would be great if we had a pref like the OpacHiddenItems systems preference so each library could set it up. Lisette
Created attachment 138800 [details] [review] Bug 25375: Match current Zebra availability limit
(In reply to Katrin Fischer from comment #25) > Maybe we do need to have some configuration options here? > If I could pick, I'd pick lost, withdrawn and on loan as unavailable. Current behavior is wrong, can we agree to making it match Zebra behavior for a small improvement, then expand/fix on another bug? (In reply to Lisette Scheer from comment #26) > I think it would be great if we had a pref like the OpacHiddenItems systems > preference so each library could set it up. > I filed bug 31304
Created attachment 138802 [details] Test records Import - checkout one of the items from 'one checked out' Search for 'bug 25375' to return all of these
Created attachment 138803 [details] [review] Bug 25375: Match current Zebra availability limit In Zebra the availability limit code states: 'available' is defined as (items.onloan is NULL) and (items.itemlost = 0) We should make ES follow the same behaviour so that changing the engine does not change this. To test: 1 - Find or create several records with two items each 2 - For those records setup items as: a - one checked out, one available b - both marked lost c - both damaged d - both withdrawn e - both notforloan 3 - Enable ES and reindex to confirm everything is set 4 - Perform an advanced search to return those records and 'limit to available items' 5 - Confirm you see records b,c,d,e and records say '2 items, none available' 6 - Apply patch 7 - Run `misc/search_tools/rebuild_elasticsearch.pl -d -b` 8 - Perform an advanced search to return those records and 'limit to available items' 9 - Confirm you see records a,b,c,d and al but a say '2 items, none available'
Created attachment 140053 [details] [review] Bug 25375: Fix 'available' facet in elasticsearch Add a new boolean ES field named 'available', which is true if at least one item is available, which means the item is not on loan, not "notforloan", not withdrawn, not lost and not damaged A full indexation is required Test plan: 1. Apply patch and run updatedatabase.pl 2. Run `misc/search_tools/rebuild_elasticsearch.pl -d -b` 3. Make sure you have some biblios whose items are all unavailable, some biblios whose items are all available, and some biblios with at least one item available and at least one item unavailable 4. Use the 'available' filter on both opac and intranet and make sure it works as expected. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 140054 [details] [review] Bug 25375: Get biblionumber from the correct MARC field Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 140055 [details] [review] Bug 25375: (QA follow-up) Count available items rather than iterating Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 140056 [details] [review] Bug 25375: Add tests for the "available" ES field Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 140057 [details] [review] Bug 25375: Match current Zebra availability limit In Zebra the availability limit code states: 'available' is defined as (items.onloan is NULL) and (items.itemlost = 0) We should make ES follow the same behaviour so that changing the engine does not change this. To test: 1 - Find or create several records with two items each 2 - For those records setup items as: a - one checked out, one available b - both marked lost c - both damaged d - both withdrawn e - both notforloan 3 - Enable ES and reindex to confirm everything is set 4 - Perform an advanced search to return those records and 'limit to available items' 5 - Confirm you see records b,c,d,e and records say '2 items, none available' 6 - Apply patch 7 - Run `misc/search_tools/rebuild_elasticsearch.pl -d -b` 8 - Perform an advanced search to return those records and 'limit to available items' 9 - Confirm you see records a,b,c,d and al but a say '2 items, none available' Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
(In reply to Nick Clemens from comment #28) > (In reply to Katrin Fischer from comment #25) > > Maybe we do need to have some configuration options here? > > If I could pick, I'd pick lost, withdrawn and on loan as unavailable. > > Current behavior is wrong, can we agree to making it match Zebra behavior > for a small improvement, then expand/fix on another bug? I agree, passing QA.
Hi all, tests are failing for me. Please review.
Created attachment 140572 [details] [review] Bug 25375: Adjust tests and test itemlost, not notforloan Previous commit correctly lists Zebra availability test, but was a bad edit. This fixes the availability test to use itemlost Additiionally, tests were written before limits were agreed. Adjusted for the current settings
Created attachment 140899 [details] [review] Bug 23735: New atomicupdate syntax Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 140900 [details] [review] Bug 23735: Spelling fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 22.11. Nice work everyone, thanks!
Created attachment 140979 [details] [review] Bug 25375: Fix QueryBuilder.t tests
Follow-up pushed to master, thanks for promptly addressing it, Julian!
Backported to 22.05.x for upcoming 22.05.07 release
applied to 21.11 for 21.11.14
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed. Nothing to document it seems, marking resolved.
This requires a full reindex, don't we need to reindex the bibliographic record if the holdability changed? Same for notforloan if modified at itemtype level. (In reply to Joonas Kylmälä from comment #35) > Created attachment 140057 [details] [review] [review] > Bug 25375: Match current Zebra availability limit Do we really need that? On the bibliographic detail page, "Available" is only displayed if the item is actually available :)