Bug 28273

Summary: Multi-holds allow invalid pickup locations
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Hold requestsAssignee: Tomás Cohen Arazi <tomascohen>
Status: Pushed to master --- QA Contact: Kyle M Hall <kyle>
Severity: major    
Priority: P5 - low CC: andreas.jonsson, andrewfh, dcook, fridolin.somers, gmcharlt, jonathan.druart+koha, kyle, martin.renvoize, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27864
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00
Bug Depends on:    
Bug Blocks: 28919, 7703, 28496    
Attachments: Bug 28273: Unit tests
Bug 28273: Add Koha::Biblios->pickup_locations
Bug 28273: Add Koha::Biblios->pickup_locations
Bug 28273: Multi-hold should not offer invalid pickup locations
Bug 28273: Add form validation for pickup locations
Bug 28273: (follow-up) Remove unused variable
Bug 28273: Add Koha::Biblios->pickup_locations
Bug 28273: Multi-hold should not offer invalid pickup locations
Bug 28273: Add form validation for pickup locations
Bug 28273: (follow-up) Remove unused variable
Bug 28273: Add Koha::Biblios->pickup_locations
Bug 28273: Add Koha::Biblios->pickup_locations
Bug 28273: Multi-hold should not offer invalid pickup locations
Bug 28273: Add form validation for pickup locations
Bug 28273: (follow-up) Remove unused variable

Description Tomás Cohen Arazi 2021-04-30 15:04:06 UTC
Steps to reproduce:
1. Perform a search
2. Select several biblios, that can have holds
3. Click on 'Place hold'
4. You are presented a list of biblios and some biblio-level hold options, including pickup location selection
=> FAIL: It presents the full list of pickup locations in the system
5. Place a hold with a pickup location that is invalid for some title
=> FAIL: The hold goes through!

The solution for this is revamping this UI, probably calculating the pickup locations intersection for display, and allowing individual titles to be set a specific pickup location. (i.e. global or local setting based on intersection is null => only local, intersection is not null => use it as default, and allow override on specific titles).
Comment 1 Tomás Cohen Arazi 2021-05-03 14:08:57 UTC
Created attachment 120407 [details] [review]
Bug 28273: Unit tests

https://bugs.koha-community.org/show_bug.cgi?id=26273
Comment 2 Tomás Cohen Arazi 2021-05-03 14:09:02 UTC
Created attachment 120408 [details] [review]
Bug 28273: Add Koha::Biblios->pickup_locations

https://bugs.koha-community.org/show_bug.cgi?id=26273
Comment 3 Tomás Cohen Arazi 2021-05-13 12:14:01 UTC
Created attachment 120923 [details] [review]
Bug 28273: Add Koha::Biblios->pickup_locations

https://bugs.koha-community.org/show_bug.cgi?id=26273
Comment 4 Tomás Cohen Arazi 2021-05-13 12:14:06 UTC
Created attachment 120924 [details] [review]
Bug 28273: Multi-hold should not offer invalid pickup locations

This patch makes the multi-hold page offer only valid pickup locations
for the selected biblios. Prior to this, all system-wide pickup
locations were offered.

To test:
1. Set 'Hold pickup library match' to 'Item's home branch' so we put a
   constraint on the valid pickup locations for easier testing.
2. Choose two or more biblios from a search, which contain  in total 2
   or 3 item home branches.
3. Click 'Place hold'
4. Choose a patron
=> FAIL: The dropdown offers all system's pickup locations
5. Apply this patches
6. Reload the page
=> SUCCESS: Only valid pickup locations are offered
7. Sign off :-D
Comment 5 Tomás Cohen Arazi 2021-05-13 12:14:10 UTC
Created attachment 120925 [details] [review]
Bug 28273: Add form validation for pickup locations

This patch introduces validation on the new pickup locations column.
This way, if a selected title doesn't have a pickup location set, it
will prevent form submission and a suitable error message will pop-up.

To test:
1. Apply this patch
2. Attempt to place multi-title level holds
3. Make sure some selected titles don't have the pickup location set
4. Submit
=> SUCCESS: Form submission halts, an idiomatic error message shows.
5. Unselect all biblios and repeat 4
=> SUCCESS: You are not allowed to proceed, a message is displayed.
6. Have all selected titles set a pickup location, submit
=> SUCCESS: Holds are placed as they should.
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2021-05-13 12:49:39 UTC
Created attachment 120926 [details] [review]
Bug 28273: (follow-up) Remove unused variable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Andrew Fuerste-Henry 2021-05-13 14:20:08 UTC
Created attachment 120933 [details] [review]
Bug 28273: Add Koha::Biblios->pickup_locations

https://bugs.koha-community.org/show_bug.cgi?id=26273

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 8 Andrew Fuerste-Henry 2021-05-13 14:20:12 UTC
Created attachment 120934 [details] [review]
Bug 28273: Multi-hold should not offer invalid pickup locations

This patch makes the multi-hold page offer only valid pickup locations
for the selected biblios. Prior to this, all system-wide pickup
locations were offered.

To test:
1. Set 'Hold pickup library match' to 'Item's home branch' so we put a
   constraint on the valid pickup locations for easier testing.
2. Choose two or more biblios from a search, which contain  in total 2
   or 3 item home branches.
3. Click 'Place hold'
4. Choose a patron
=> FAIL: The dropdown offers all system's pickup locations
5. Apply this patches
6. Reload the page
=> SUCCESS: Only valid pickup locations are offered
7. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 9 Andrew Fuerste-Henry 2021-05-13 14:20:16 UTC
Created attachment 120935 [details] [review]
Bug 28273: Add form validation for pickup locations

This patch introduces validation on the new pickup locations column.
This way, if a selected title doesn't have a pickup location set, it
will prevent form submission and a suitable error message will pop-up.

To test:
1. Apply this patch
2. Attempt to place multi-title level holds
3. Make sure some selected titles don't have the pickup location set
4. Submit
=> SUCCESS: Form submission halts, an idiomatic error message shows.
5. Unselect all biblios and repeat 4
=> SUCCESS: You are not allowed to proceed, a message is displayed.
6. Have all selected titles set a pickup location, submit
=> SUCCESS: Holds are placed as they should.
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 10 Andrew Fuerste-Henry 2021-05-13 14:20:20 UTC
Created attachment 120936 [details] [review]
Bug 28273: (follow-up) Remove unused variable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 11 Kyle M Hall 2021-05-14 12:10:35 UTC
Created attachment 120957 [details] [review]
Bug 28273: Add Koha::Biblios->pickup_locations

https://bugs.koha-community.org/show_bug.cgi?id=26273

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Kyle M Hall 2021-05-14 12:15:22 UTC
Created attachment 120958 [details] [review]
Bug 28273: Add Koha::Biblios->pickup_locations

https://bugs.koha-community.org/show_bug.cgi?id=26273

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2021-05-14 12:15:42 UTC
Created attachment 120959 [details] [review]
Bug 28273: Multi-hold should not offer invalid pickup locations

This patch makes the multi-hold page offer only valid pickup locations
for the selected biblios. Prior to this, all system-wide pickup
locations were offered.

To test:
1. Set 'Hold pickup library match' to 'Item's home branch' so we put a
   constraint on the valid pickup locations for easier testing.
2. Choose two or more biblios from a search, which contain  in total 2
   or 3 item home branches.
3. Click 'Place hold'
4. Choose a patron
=> FAIL: The dropdown offers all system's pickup locations
5. Apply this patches
6. Reload the page
=> SUCCESS: Only valid pickup locations are offered
7. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2021-05-14 12:15:46 UTC
Created attachment 120960 [details] [review]
Bug 28273: Add form validation for pickup locations

This patch introduces validation on the new pickup locations column.
This way, if a selected title doesn't have a pickup location set, it
will prevent form submission and a suitable error message will pop-up.

To test:
1. Apply this patch
2. Attempt to place multi-title level holds
3. Make sure some selected titles don't have the pickup location set
4. Submit
=> SUCCESS: Form submission halts, an idiomatic error message shows.
5. Unselect all biblios and repeat 4
=> SUCCESS: You are not allowed to proceed, a message is displayed.
6. Have all selected titles set a pickup location, submit
=> SUCCESS: Holds are placed as they should.
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2021-05-14 12:15:49 UTC
Created attachment 120961 [details] [review]
Bug 28273: (follow-up) Remove unused variable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2021-05-14 12:16:16 UTC
Koha::Biblios::pickup_locations needs unit tests, but otherwise I'd consider it PQA.
Comment 17 Tomás Cohen Arazi 2021-05-14 12:35:45 UTC
(In reply to Kyle M Hall from comment #16)
> Koha::Biblios::pickup_locations needs unit tests, but otherwise I'd consider
> it PQA.

The tests were there, but it seems I wrongly obsoleted them at some point!
Please run them!
Comment 18 Jonathan Druart 2021-05-20 06:52:00 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 19 Fridolin Somers 2021-05-20 13:15:53 UTC
Does not apply on 20.11.x. Looks like it depends on Bug 27894
Comment 20 David Cook 2021-12-01 06:06:04 UTC
I'm looking into null branchcodes being used for reserves (and breaking the Reserves display in Circ), and I think that it's being caused by multiholds, since their default value for "Pickup location" is blank. 

I'm thinking that the solution is probably to remove the empty options...

I am working on an instance with customized hold functionality, so it is possible that the customizations are preventing some error handling that is in master, but exploring currently...
Comment 21 David Cook 2021-12-01 06:08:56 UTC
Oh and it looks possible to nix the pickup location manually anyway... I'll have to test this on master...