Bug 28919 - When placing a multi-hold from results in staff pickup location is empty
Summary: When placing a multi-hold from results in staff pickup location is empty
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 28273
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-29 15:16 UTC by Katrin Fischer
Modified: 2022-02-25 05:14 UTC (History)
6 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:


Attachments
Bug 28919: Select logged in library as pickup for multi holds (3.80 KB, patch)
2021-09-02 07:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28919: Select logged in library as pickup for multi holds (3.85 KB, patch)
2021-09-23 19:39 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28919: Select logged in library as pickup for multi holds (3.91 KB, patch)
2021-10-02 21:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28919: [Alternative] Select logged in library as pickup for multi holds (3.53 KB, patch)
2021-12-02 01:37 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2021-08-29 15:16:39 UTC
When placing multiple holds from the result list, the pick-up location is empty now, which makes it a more tedious process than it needs to be. 

When placing a single hold the home library of the patron is used a default, same should apply if you place multiple holds.

Could this be a side effect of bug 28273?
Comment 1 Katrin Fischer 2021-08-29 15:18:21 UTC
Hm testing again - it's actually probably the library currently logged in at? Not the homebranch... in any case: same logic should apply for single hold and multi-holds.
Comment 2 Jonathan Druart 2021-09-02 07:50:19 UTC
Created attachment 124419 [details] [review]
Bug 28919: Select logged in library as pickup for multi holds

When placing multiple holds from the result list, the pick-up location is empty now, which makes it a more tedious process than it needs to be.

Logged in library must be selected to match the single hold logic.
Comment 3 Jonathan Druart 2021-09-02 07:50:44 UTC
The very happy with the patch but it's working.
Comment 4 Owen Leonard 2021-09-23 19:39:50 UTC
Created attachment 125221 [details] [review]
Bug 28919: Select logged in library as pickup for multi holds

When placing multiple holds from the result list, the pick-up location
is empty now, which makes it a more tedious process than it needs to be.

Logged in library must be selected to match the single hold logic.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Katrin Fischer 2021-09-26 14:02:57 UTC
I am sorry, I tried "restart_all" multiple times, but the pull downs remain empty.

I am using the admin user (42), logged in at Centerville.

When placing a single hold, Centerville is selected.
Comment 6 Jonathan Druart 2021-09-29 13:42:38 UTC
(In reply to Katrin Fischer from comment #5)
> I am sorry, I tried "restart_all" multiple times, but the pull downs remain
> empty.
> 
> I am using the admin user (42), logged in at Centerville.
> 
> When placing a single hold, Centerville is selected.

The pull down from the top, the "Pickup at:"?

https://snipboard.io/pRHAMn.jpg
Comment 7 Katrin Fischer 2021-09-29 21:30:20 UTC
> 
> The pull down from the top, the "Pickup at:"?
> 
> https://snipboard.io/pRHAMn.jpg

Oh! I never noticed the one on top - will try to retry :)
Comment 8 Katrin Fischer 2021-10-02 21:42:08 UTC
Created attachment 125658 [details] [review]
Bug 28919: Select logged in library as pickup for multi holds

When placing multiple holds from the result list, the pick-up location
is empty now, which makes it a more tedious process than it needs to be.

Logged in library must be selected to match the single hold logic.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2021-10-02 21:45:22 UTC
Ok, this works as I think it should, pre-selecting the right branch. 

BUT: When you try to save the holds, it complains:
Please make sure all selected titles have a pickup location set

If you switch the pick-up location to another, they will all be set below as well. So I think we do not only need to pre-select for the one on top, but for all holds displayed.
Comment 10 Jonathan Druart 2021-10-13 12:16:08 UTC
Well, what's the point of the first/main select then? Tomas?
Comment 11 David Cook 2021-12-02 01:37:11 UTC
Created attachment 128144 [details] [review]
Bug 28919: [Alternative] Select logged in library as pickup for multi holds

Default pickup location to the logged in library for staff placed multiholds.

Holds with a blank pickup location will cause 500 errors in the Reserves
tab of the circulation screen, so we want to make sure the pickup location
does not default to blank.
Comment 12 David Cook 2021-12-02 01:41:38 UTC
Attaching an alternative patch here, although I think Jonathan's is better overall (just needs to be applies to the "Pickup location" as well). 

This is causing frequent problems for a library of mine, so I'm keen to get a fix implemented ASAP. 

Personally, I think a lot of holds logic and error-handling needs work, but I don't mind just putting out fires for now.
Comment 13 Tomás Cohen Arazi 2021-12-02 10:39:41 UTC
(In reply to Jonathan Druart from comment #10)
> Well, what's the point of the first/main select then? Tomas?

This form had a problem, orginally: it allowed you to place holds with invalid pickup locations, and didn't even require AllowHoldPolicyOverride. The idea was to make it so you weren't offered bad pickup locations.

Asking different libraries, it turned out this wasn't a bad workflow:
- You select several biblios and click place hold
- A form is presented (similar to the one before
- There's a dropdown that lets you choose the union of the valid pickup locations from all selected titles
- Only those for which the chosen one is a valid one are set
- The rest require manually setting them
- If you manually set one, changing the top-level 'Pickup at' one should not change it
- It needs to honour the UI decision we made about overriden pickup locations, having them show a notice about them being an override to rules

So that's it. Reading the bug description, my guess is that:
- The 'Pickup at' dropdown might need some better explanation in the UI (a hint, a visible text)
- I read a comment from dcook somewhere else, and it felt like he found a bug that made the form send empty pickup locations, that actually hit the DB, that'd be a bug, not a usability discussion.
Comment 14 Tomás Cohen Arazi 2021-12-02 10:45:02 UTC
(In reply to Tomás Cohen Arazi from comment #13)
> (In reply to Jonathan Druart from comment #10)
> > Well, what's the point of the first/main select then? Tomas?
> 
> This form had a problem, orginally: it allowed you to place holds with
> invalid pickup locations, and didn't even require AllowHoldPolicyOverride.
> The idea was to make it so you weren't offered bad pickup locations.
> 
> Asking different libraries, it turned out this wasn't a bad workflow:
> - You select several biblios and click place hold
> - A form is presented (similar to the one before
> - There's a dropdown that lets you choose the union of the valid pickup
> locations from all selected titles
> - Only those for which the chosen one is a valid one are set
> - The rest require manually setting them
> - If you manually set one, changing the top-level 'Pickup at' one should not
> change it
> - It needs to honour the UI decision we made about overriden pickup
> locations, having them show a notice about them being an override to rules
> 
> So that's it. Reading the bug description, my guess is that:
> - The 'Pickup at' dropdown might need some better explanation in the UI (a
> hint, a visible text)
> - I read a comment from dcook somewhere else, and it felt like he found a
> bug that made the form send empty pickup locations, that actually hit the
> DB, that'd be a bug, not a usability discussion.

And making a default selection is good too.
Comment 15 David Cook 2021-12-03 00:45:18 UTC
(In reply to Tomás Cohen Arazi from comment #13)
> - I read a comment from dcook somewhere else, and it felt like he found a
> bug that made the form send empty pickup locations, that actually hit the
> DB, that'd be a bug, not a usability discussion.

Yeah I was thinking the same thing. We could throw an exception client-side or server-side (or both). 

(In reply to Tomás Cohen Arazi from comment #14)
> And making a default selection is good too.

I think this would be a good first step, since it requires only a small change and brings multiholds in line with require holds I think.
Comment 16 Andrew Fuerste-Henry 2021-12-03 20:44:43 UTC
I like Jonathan's patch here, it does what libraries have indicated they want. Though I agree that on initial page load all dropdowns (the overall dropdown at the top and the individual dropdowns on each title) should default to the logged in branch if that's a valid pickup location.

But if the logged-in branch is not a valid pickup location for one of the titles, then that title's dropdown should be blank to start and require one to select a valid pickup location before the form can be submitted.
Comment 17 Katrin Fischer 2021-12-05 23:04:03 UTC
(In reply to Andrew Fuerste-Henry from comment #16)
> I like Jonathan's patch here, it does what libraries have indicated they
> want. Though I agree that on initial page load all dropdowns (the overall
> dropdown at the top and the individual dropdowns on each title) should
> default to the logged in branch if that's a valid pickup location.
> 
> But if the logged-in branch is not a valid pickup location for one of the
> titles, then that title's dropdown should be blank to start and require one
> to select a valid pickup location before the form can be submitted.

+1
Comment 18 Lucas Gass 2021-12-21 21:18:48 UTC
(In reply to Andrew Fuerste-Henry from comment #16)
> I like Jonathan's patch here, it does what libraries have indicated they
> want. Though I agree that on initial page load all dropdowns (the overall
> dropdown at the top and the individual dropdowns on each title) should
> default to the logged in branch if that's a valid pickup location.
> 
> But if the logged-in branch is not a valid pickup location for one of the
> titles, then that title's dropdown should be blank to start and require one
> to select a valid pickup location before the form can be submitted.

+1

I think this would be behavior that is consistent with that Tomas did in Bug 29349
Comment 19 Jonathan Druart 2022-01-03 10:45:04 UTC
Tomas, I think you are the best placed to finish the work here.
Comment 20 David Cook 2022-02-25 05:14:34 UTC
I'm having libraries still experiencing this issue. At the moment, the build_holds_queue.pl is now sending them emails with error messages every 15 minutes...