Bug 25375 - Elasticsearch: Limit on available items does not work
Summary: Elasticsearch: Limit on available items does not work
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-05 10:21 UTC by Julian Maurice
Modified: 2024-03-27 10:51 UTC (History)
14 users (show)

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


Attachments
Bug 25375: Fix 'available' facet in elasticsearch (3.84 KB, patch)
2020-05-05 10:24 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25375: Get biblionumber from the correct MARC field (1.31 KB, patch)
2020-09-04 08:26 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25375: Fix 'available' facet in elasticsearch (3.92 KB, patch)
2020-09-04 13:23 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25375: Get biblionumber from the correct MARC field (1.38 KB, patch)
2020-09-04 13:23 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25375: (QA follow-up) Count available items rather than iterating (1.79 KB, patch)
2020-09-11 15:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25375: Fix 'available' facet in elasticsearch (3.98 KB, patch)
2020-09-11 15:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25375: Get biblionumber from the correct MARC field (1.43 KB, patch)
2020-09-11 15:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25375: (QA follow-up) Count available items rather than iterating (1.79 KB, patch)
2020-09-11 15:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25375: Fix 'available' facet in elasticsearch (3.93 KB, patch)
2022-03-03 10:22 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25375: Get biblionumber from the correct MARC field (1.49 KB, patch)
2022-03-03 10:22 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25375: (QA follow-up) Count available items rather than iterating (1.80 KB, patch)
2022-03-03 10:22 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25375: Add tests for the "available" ES field (5.43 KB, patch)
2022-03-03 10:22 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25375: Fix 'available' facet in elasticsearch (3.99 KB, patch)
2022-03-28 13:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25375: Get biblionumber from the correct MARC field (1.54 KB, patch)
2022-03-28 13:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25375: (QA follow-up) Count available items rather than iterating (1.86 KB, patch)
2022-03-28 13:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25375: Add tests for the "available" ES field (5.48 KB, patch)
2022-03-28 13:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25375: Match current Zebra availability limit (927 bytes, patch)
2022-08-08 12:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Test records (10.66 KB, text/xml)
2022-08-08 13:19 UTC, Nick Clemens
Details
Bug 25375: Match current Zebra availability limit (1.79 KB, patch)
2022-08-08 13:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25375: Fix 'available' facet in elasticsearch (4.13 KB, patch)
2022-09-01 18:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25375: Get biblionumber from the correct MARC field (1.69 KB, patch)
2022-09-01 18:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25375: (QA follow-up) Count available items rather than iterating (2.00 KB, patch)
2022-09-01 18:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25375: Add tests for the "available" ES field (5.62 KB, patch)
2022-09-01 18:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25375: Match current Zebra availability limit (1.94 KB, patch)
2022-09-01 18:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 25375: Adjust tests and test itemlost, not notforloan (3.04 KB, patch)
2022-09-13 13:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23735: New atomicupdate syntax (1.78 KB, patch)
2022-09-23 12:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23735: Spelling fixes (1.76 KB, patch)
2022-09-23 12:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25375: Fix QueryBuilder.t tests (1.02 KB, patch)
2022-09-26 12:49 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2020-05-05 10:21:18 UTC
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"
Comment 1 Julian Maurice 2020-05-05 10:24:24 UTC
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.
Comment 2 Andrew Fuerste-Henry 2020-06-03 12:08:46 UTC
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.
Comment 3 Nick Clemens 2020-07-02 13:28:09 UTC
(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
Comment 4 Julian Maurice 2020-09-04 08:26:35 UTC
Created attachment 109647 [details] [review]
Bug 25375: Get biblionumber from the correct MARC field
Comment 5 Andrew Fuerste-Henry 2020-09-04 13:23:26 UTC
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>
Comment 6 Andrew Fuerste-Henry 2020-09-04 13:23:29 UTC
Created attachment 109660 [details] [review]
Bug 25375: Get biblionumber from the correct MARC field

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 7 Nick Clemens 2020-09-11 15:23:57 UTC
Created attachment 109951 [details] [review]
Bug 25375: (QA follow-up) Count available items rather than iterating
Comment 8 Nick Clemens 2020-09-11 15:24:58 UTC
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>
Comment 9 Nick Clemens 2020-09-11 15:25:03 UTC
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>
Comment 10 Nick Clemens 2020-09-11 15:25:07 UTC
Created attachment 109954 [details] [review]
Bug 25375: (QA follow-up) Count available items rather than iterating
Comment 11 Nick Clemens 2020-09-11 15:27:28 UTC
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
Comment 12 Jonathan Druart 2020-09-18 09:21:43 UTC
Tests are missing.
Comment 13 Fridolin Somers 2020-09-23 12:06:11 UTC
Why not use "not-onloan-count" like for Zebra ?
Comment 14 Andrew Fuerste-Henry 2021-02-10 17:01:09 UTC
Giving this a bump as it continues to impact libraries. Julian, will you be able to revisit this?
Comment 15 Julian Maurice 2022-03-03 10:22:29 UTC
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>
Comment 16 Julian Maurice 2022-03-03 10:22:33 UTC
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>
Comment 17 Julian Maurice 2022-03-03 10:22:38 UTC
Created attachment 131312 [details] [review]
Bug 25375: (QA follow-up) Count available items rather than iterating
Comment 18 Julian Maurice 2022-03-03 10:22:42 UTC
Created attachment 131313 [details] [review]
Bug 25375: Add tests for the "available" ES field
Comment 19 Julian Maurice 2022-03-03 10:25:19 UTC
Patches rebased, and tests added.
Back to Signed off
Comment 20 Nick Clemens 2022-03-28 13:41:55 UTC
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>
Comment 21 Nick Clemens 2022-03-28 13:41:58 UTC
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>
Comment 22 Nick Clemens 2022-03-28 13:42:02 UTC
Created attachment 132329 [details] [review]
Bug 25375: (QA follow-up) Count available items rather than iterating

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Nick Clemens 2022-03-28 13:42:06 UTC
Created attachment 132330 [details] [review]
Bug 25375: Add tests for the "available" ES field

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Katrin Fischer 2022-04-01 21:20:01 UTC
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.
Comment 25 Katrin Fischer 2022-04-01 21:22:46 UTC
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.
Comment 26 Lisette Scheer 2022-06-01 15:37:57 UTC
(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
Comment 27 Nick Clemens 2022-08-08 12:17:33 UTC
Created attachment 138800 [details] [review]
Bug 25375: Match current Zebra availability limit
Comment 28 Nick Clemens 2022-08-08 12:19:29 UTC
(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
Comment 29 Nick Clemens 2022-08-08 13:19:00 UTC
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
Comment 30 Nick Clemens 2022-08-08 13:19:52 UTC
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'
Comment 31 Joonas Kylmälä 2022-09-01 18:00:26 UTC
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>
Comment 32 Joonas Kylmälä 2022-09-01 18:00:31 UTC
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>
Comment 33 Joonas Kylmälä 2022-09-01 18:00:37 UTC
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>
Comment 34 Joonas Kylmälä 2022-09-01 18:00:42 UTC
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>
Comment 35 Joonas Kylmälä 2022-09-01 18:00:47 UTC
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>
Comment 36 Joonas Kylmälä 2022-09-01 18:01:43 UTC
(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.
Comment 37 Tomás Cohen Arazi 2022-09-12 19:24:12 UTC
Hi all, tests are failing for me. Please review.
Comment 38 Nick Clemens 2022-09-13 13:08:34 UTC
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
Comment 39 Tomás Cohen Arazi 2022-09-23 12:04:13 UTC
Created attachment 140899 [details] [review]
Bug 23735: New atomicupdate syntax

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2022-09-23 12:06:29 UTC
Created attachment 140900 [details] [review]
Bug 23735: Spelling fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2022-09-23 12:11:47 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 42 Julian Maurice 2022-09-26 12:49:02 UTC
Created attachment 140979 [details] [review]
Bug 25375: Fix QueryBuilder.t tests
Comment 43 Tomás Cohen Arazi 2022-09-26 12:54:22 UTC
Follow-up pushed to master, thanks for promptly addressing it, Julian!
Comment 44 Lucas Gass 2022-11-01 22:00:07 UTC
Backported to 22.05.x for upcoming 22.05.07 release
Comment 45 Arthur Suzuki 2022-11-14 17:21:42 UTC
applied to 21.11 for 21.11.14
Comment 46 Victor Grousset/tuxayo 2022-11-26 03:58:05 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.