Bug 29349 - Item-level holds should assume the same pickup location as bib-level holds
Summary: Item-level holds should assume the same pickup location as bib-level holds
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords: regression
Depends on: 28338
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-28 13:00 UTC by Andrew Fuerste-Henry
Modified: 2021-12-22 22:14 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Up until Koha 20.11 the pickup location when placing item-level holds was the currently logged-in library. From Koha 21.05 the holding branch was used as the default. This restores the previous behaviour so that the logged-in library (if a valid pickup location) is selected as the default pickup location for item-level holds. When it is not, an empty dropdown is used as a fallback.
Version(s) released in:
22.05.00,21.11.01,21.05.08


Attachments
Bug 29349: Do not assume holding branch is a valid pickup location (3.45 KB, patch)
2021-12-03 19:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29349: Do not assume holding branch is a valid pickup location (2.79 KB, patch)
2021-12-06 22:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29349: (follow-up) Fix width of item level dropdowns (1.49 KB, patch)
2021-12-07 19:22 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 29349: Do not assume holding branch is a valid pickup location (2.85 KB, patch)
2021-12-08 23:11 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 29349: (follow-up) Fix width of item level dropdowns (1.54 KB, patch)
2021-12-09 13:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29349: Do not assume holding branch is a valid pickup location (2.91 KB, patch)
2021-12-10 16:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29349: (follow-up) Fix width of item level dropdowns (1.61 KB, patch)
2021-12-10 16:03 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2021-10-28 13:00:34 UTC
When placing a hold via the intranet, a bib-level hold defaults to setting the pickup location to your logged-in library (and gives a message if that differs from the patron homebranch).
In 21.05 and later, however, an item-level hold will default to pickup up at the item's holding branch. This inconsistency is a change in behavior and the source of a lot of confusion.
Item-level holds should default to the same pickup location that a bib-level hold would have, unless that item is for some reason forbidden from pickup at the branch it would default to.
Comment 1 Christopher Brannon 2021-10-28 21:38:31 UTC
This is a drastic setback.  This needs to be fixed ASAP!
Comment 2 Lucas Gass 2021-11-19 21:53:20 UTC
Does this need to be a configuration option? I'd imagine some libraries would like those item level dropdowns to default to the items hold location.
Comment 3 Lucas Gass 2021-11-19 21:53:46 UTC
I meant items home location, not items hold location
Comment 4 Christopher Brannon 2021-11-19 23:14:07 UTC
From a consortium point of view, it is always going to be about where the patron is going to pick up.  I know that there are already settings that define where items should default to pickup, and this should really adhere to those existing settings.  I'm not sure why a pickup location should change if the hold is bib-level or item-level.  That seems a bit nit-picky and tedious to manage.
Comment 5 Stacey Nix 2021-12-03 17:49:01 UTC
We are also a consortium and we need to be able to have item level hold set to send them to the pickup location chosen by the patron.  It is currently setting it at the pickup location as the home library and that does not work for us at all.  Please fix this and set it back the way it was!
Comment 6 Todd Goatley 2021-12-03 18:57:51 UTC
We have received requests from libraries asking that this please be fixed as soon as possible.
Comment 7 Tomás Cohen Arazi 2021-12-03 19:52:40 UTC
Created attachment 128233 [details] [review]
Bug 29349: Do not assume holding branch is a valid pickup location

The original code for pickup locations when placing item-level holds
picked the currently logged-in library.

We made things more robust, as the logged-in library might not be a
valid pickup location for the patron and item. But it was wrongly chosen
to use the holding branch as the default.

A more robust approach is needed, and this precedence is picked this
time (it could be configuration-driven in the future):

    - Logged-in library
    - Holding branch
    - The first valid pickup location the above are not valid

To test:
1. Pick a biblio with various valid pickup locations, some not including
   the logged-in library.
2. Pick a patron for placing the hold
=> FAIL: Notice that (when valid pickup location) the holding branch is
always chosen
3. Apply this patch
4. Repeat 2
=> SUCCESS: If valid pickup location, the logged-in branch is picked as
default for item-type level. When it is not, the holding branch is
picked, and if not, some of the valid pickup locations is selected (i.e.
there's no empty dropdowns)
5. Sign off :-D
Comment 8 Lucas Gass 2021-12-03 20:14:56 UTC
Thanks Tomas. 

I am wondering if we should change how this falls back if the pickup location is not valid. I think it should be:

    - Logged-in library
    - Holding branch
    - Empty/blank

Instead of defaulting to the first choice alphabetically for the 3rd fallback we should set no value. This seems like it would be a rare occurrence that the logged in library and the holding branch are not valid pickup locations but if it does happen I think it can cause confusion. By leaving it empty it forces librarians to make a decision and I think in this case that is good.
Comment 9 Phil Ringnalda 2021-12-03 21:29:57 UTC
Alphabetically-first is also used for bib-level holds in the case where either the logged-in library isn't a valid pickup location at all, or is not a valid pickup location for any of the items.

Should we also change that to blank, guessing without having yet had feedback from people using local hold groups that alphabetically-first is a bad idea, or should we take this patch to restore the previous behavior for people who do not use local hold groups while retaining consistent behavior between bib-level and item-level for people who do use them, and then find out from feedback whether alphabetically-first for people who do use them is worse than blank?
Comment 10 Donna 2021-12-06 18:47:47 UTC
From a workflow point of view, logged in makes sense as the first choice.  I feel the second option should be blank.  That indicates to staff that there is something unusual about this hold, and they need to verify the pickup location.  If we make alpha the second choice then we will be back at square one with staff placing holds for pickups at locations that may never fill.  The idea behind this change originally was to prevent unfillable holds, so we need to keep that in mind.

I think the main issue was that users did not fully understand why the change was made. We just need to make sure that it is clearly explained why the change is being made.
Comment 11 Lucas Gass 2021-12-06 19:09:09 UTC
(In reply to Donna from comment #10)
> From a workflow point of view, logged in makes sense as the first choice.  I
> feel the second option should be blank.  That indicates to staff that there
> is something unusual about this hold, and they need to verify the pickup
> location.  If we make alpha the second choice then we will be back at square
> one with staff placing holds for pickups at locations that may never fill. 
> The idea behind this change originally was to prevent unfillable holds, so
> we need to keep that in mind.
> 
> I think the main issue was that users did not fully understand why the
> change was made. We just need to make sure that it is clearly explained why
> the change is being made.

+1
Comment 12 Katrin Fischer 2021-12-06 19:43:43 UTC
(In reply to Lucas Gass from comment #11)
> (In reply to Donna from comment #10)
> > From a workflow point of view, logged in makes sense as the first choice.  I
> > feel the second option should be blank.  That indicates to staff that there
> > is something unusual about this hold, and they need to verify the pickup
> > location.  If we make alpha the second choice then we will be back at square
> > one with staff placing holds for pickups at locations that may never fill. 
> > The idea behind this change originally was to prevent unfillable holds, so
> > we need to keep that in mind.
> > 
> > I think the main issue was that users did not fully understand why the
> > change was made. We just need to make sure that it is clearly explained why
> > the change is being made.
> 
> +1

+1 :)
Comment 13 Tomás Cohen Arazi 2021-12-06 22:20:05 UTC
Created attachment 128300 [details] [review]
Bug 29349: Do not assume holding branch is a valid pickup location

The original code for pickup locations when placing item-level holds
picked the currently logged-in library.

We made things more robust, as the logged-in library might not be a
valid pickup location for the patron and item. But it was wrongly chosen
to use the holding branch as the default.

A more robust approach is needed, and this precedence is picked this
time (it could be configuration-driven in the future):

    - Logged-in library
    - Empty

To test:
1. Pick a biblio with various valid pickup locations, some not including
   the logged-in library.
2. Pick a patron for placing the hold
=> FAIL: Notice that (when valid pickup location) the holding branch is
always chosen
3. Apply this patch
4. Repeat 2
=> SUCCESS: If valid pickup location, the logged-in branch is picked as
default for item-type level. When it is not, an empty dropdown is used
as a fallback.
5. Sign off :-D
Comment 14 Tomás Cohen Arazi 2021-12-06 22:21:13 UTC
(In reply to Donna from comment #10)
> From a workflow point of view, logged in makes sense as the first choice.  I
> feel the second option should be blank.  That indicates to staff that there
> is something unusual about this hold, and they need to verify the pickup
> location.  If we make alpha the second choice then we will be back at square
> one with staff placing holds for pickups at locations that may never fill. 
> The idea behind this change originally was to prevent unfillable holds, so
> we need to keep that in mind.
> 
> I think the main issue was that users did not fully understand why the
> change was made. We just need to make sure that it is clearly explained why
> the change is being made.

Agreed, patch adjusted.

If we need more options for special use cases, we will need to consider separate bug, and more complex configurations.
Comment 15 Lucas Gass 2021-12-07 19:22:03 UTC
Created attachment 128315 [details] [review]
Bug 29349: (follow-up) Fix width of item level dropdowns
Comment 16 Christopher Brannon 2021-12-07 20:59:14 UTC
(In reply to Donna from comment #10)
> From a workflow point of view, logged in makes sense as the first choice.  I
> feel the second option should be blank.  That indicates to staff that there
> is something unusual about this hold, and they need to verify the pickup
> location.  If we make alpha the second choice then we will be back at square
> one with staff placing holds for pickups at locations that may never fill. 
> The idea behind this change originally was to prevent unfillable holds, so
> we need to keep that in mind.
> 
> I think the main issue was that users did not fully understand why the
> change was made. We just need to make sure that it is clearly explained why
> the change is being made.

+1
Comment 17 Lucas Gass 2021-12-08 23:11:46 UTC
Created attachment 128370 [details] [review]
Bug 29349: Do not assume holding branch is a valid pickup location

The original code for pickup locations when placing item-level holds
picked the currently logged-in library.

We made things more robust, as the logged-in library might not be a
valid pickup location for the patron and item. But it was wrongly chosen
to use the holding branch as the default.

A more robust approach is needed, and this precedence is picked this
time (it could be configuration-driven in the future):

    - Logged-in library
    - Empty

To test:
1. Pick a biblio with various valid pickup locations, some not including
   the logged-in library.
2. Pick a patron for placing the hold
=> FAIL: Notice that (when valid pickup location) the holding branch is
always chosen
3. Apply this patch
4. Repeat 2
=> SUCCESS: If valid pickup location, the logged-in branch is picked as
default for item-type level. When it is not, an empty dropdown is used
as a fallback.
5. Sign off :-D

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 18 Tomás Cohen Arazi 2021-12-09 13:16:21 UTC
Created attachment 128372 [details] [review]
Bug 29349: (follow-up) Fix width of item level dropdowns

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2021-12-09 13:17:10 UTC
Signed on the follow-up. Thanks, Lucas!
Comment 20 Martin Renvoize 2021-12-10 16:03:39 UTC
Created attachment 128406 [details] [review]
Bug 29349: Do not assume holding branch is a valid pickup location

The original code for pickup locations when placing item-level holds
picked the currently logged-in library.

We made things more robust, as the logged-in library might not be a
valid pickup location for the patron and item. But it was wrongly chosen
to use the holding branch as the default.

A more robust approach is needed, and this precedence is picked this
time (it could be configuration-driven in the future):

    - Logged-in library
    - Empty

To test:
1. Pick a biblio with various valid pickup locations, some not including
   the logged-in library.
2. Pick a patron for placing the hold
=> FAIL: Notice that (when valid pickup location) the holding branch is
always chosen
3. Apply this patch
4. Repeat 2
=> SUCCESS: If valid pickup location, the logged-in branch is picked as
default for item-type level. When it is not, an empty dropdown is used
as a fallback.
5. Sign off :-D

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2021-12-10 16:03:44 UTC
Created attachment 128407 [details] [review]
Bug 29349: (follow-up) Fix width of item level dropdowns

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2021-12-10 16:04:56 UTC
This all appears to make sense and passes the QA scripts.. I believe it resolves the regression and doesn't cause any new ones.

Passing QA
Comment 23 Fridolin Somers 2021-12-15 21:50:54 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 24 Kyle M Hall 2021-12-17 14:53:04 UTC
Pushed to 21.11.x for 21.11.01
Comment 25 Andrew Fuerste-Henry 2021-12-17 21:13:00 UTC
Pushed to 21.05.x for 21.05.08
Comment 26 Victor Grousset/tuxayo 2021-12-22 22:14:22 UTC
Missing dependencies for 20.11.x, it shouldn't be affected, no backport.