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: 28496
  Show dependency treegraph
 
Reported: 2021-04-30 15:04 UTC by Tomás Cohen Arazi
Modified: 2021-06-01 15:55 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
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