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).
Created attachment 120407 [details] [review] Bug 28273: Unit tests https://bugs.koha-community.org/show_bug.cgi?id=26273
Created attachment 120408 [details] [review] Bug 28273: Add Koha::Biblios->pickup_locations https://bugs.koha-community.org/show_bug.cgi?id=26273
Created attachment 120923 [details] [review] Bug 28273: Add Koha::Biblios->pickup_locations https://bugs.koha-community.org/show_bug.cgi?id=26273
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
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>
Created attachment 120926 [details] [review] Bug 28273: (follow-up) Remove unused variable Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Koha::Biblios::pickup_locations needs unit tests, but otherwise I'd consider it PQA.
(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!
Pushed to master for 21.05, thanks to everybody involved!
Does not apply on 20.11.x. Looks like it depends on Bug 27894
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...
Oh and it looks possible to nix the pickup location manually anyway... I'll have to test this on master...