Bug 28273 - Multi-holds allow invalid pickup locations
Summary: Multi-holds allow invalid pickup locations
Status: Pushed to master
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: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks: 28919 7703 28496
  Show dependency treegraph
 
Reported: 2021-04-30 15:04 UTC by Tomás Cohen Arazi
Modified: 2022-03-29 14:26 UTC (History)
10 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:
21.05.00


Attachments
Bug 28273: Unit tests (3.91 KB, patch)
2021-05-03 14:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28273: Add Koha::Biblios->pickup_locations (1.50 KB, patch)
2021-05-03 14:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28273: Add Koha::Biblios->pickup_locations (1.50 KB, patch)
2021-05-13 12:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28273: Multi-hold should not offer invalid pickup locations (8.25 KB, patch)
2021-05-13 12:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28273: Add form validation for pickup locations (2.90 KB, patch)
2021-05-13 12:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28273: (follow-up) Remove unused variable (989 bytes, patch)
2021-05-13 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28273: Add Koha::Biblios->pickup_locations (1.56 KB, patch)
2021-05-13 14:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28273: Multi-hold should not offer invalid pickup locations (8.33 KB, patch)
2021-05-13 14:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28273: Add form validation for pickup locations (2.97 KB, patch)
2021-05-13 14:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28273: (follow-up) Remove unused variable (1.03 KB, patch)
2021-05-13 14:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28273: Add Koha::Biblios->pickup_locations (1.63 KB, patch)
2021-05-14 12:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28273: Add Koha::Biblios->pickup_locations (1.63 KB, patch)
2021-05-14 12:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28273: Multi-hold should not offer invalid pickup locations (8.38 KB, patch)
2021-05-14 12:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28273: Add form validation for pickup locations (3.03 KB, patch)
2021-05-14 12:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28273: (follow-up) Remove unused variable (1.10 KB, patch)
2021-05-14 12:15 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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...