Bug 28338 - Validate item holdability and pickup location separately
Summary: Validate item holdability and pickup location separately
Status: RESOLVED FIXED
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: Martin Renvoize
URL:
Keywords:
: 28284 28312 (view as bug list)
Depends on: 27931
Blocks: 28748
  Show dependency treegraph
 
Reported: 2021-05-12 18:10 UTC by Tomás Cohen Arazi
Modified: 2021-10-18 16:15 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.01


Attachments
Bug 28338: Make item-level holds use locally defined pickup branches (11.55 KB, patch)
2021-05-12 18:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28338: Default to holding branch to save clicks (4.29 KB, patch)
2021-05-12 20:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28338: Default to holding branch to save clicks (6.40 KB, patch)
2021-05-12 20:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28338: Make item-level holds use locally defined pickup branches (11.61 KB, patch)
2021-05-13 11:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28338: Default to holding branch to save clicks (6.45 KB, patch)
2021-05-13 11:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28338: Make item-level holds use locally defined pickup branches (11.68 KB, patch)
2021-05-13 14:15 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28338: Default to holding branch to save clicks (6.51 KB, patch)
2021-05-13 14:15 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28338: Make item-level holds use locally defined pickup branches (11.72 KB, patch)
2021-05-13 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28338: Default to holding branch to save clicks (6.57 KB, patch)
2021-05-13 15:13 UTC, Martin Renvoize
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-05-12 18:10:49 UTC
Several bugs tried to address this question: when calling CanItemBeReserved, what pickup location parameter should be passed? And we've been going back and forth between userenv, reservescontrolbranch, etc. And there are two open bugs related to the area.

Truth is, CanItemBeReserved is being passed a branchcode, but what we really be doing, is checking if the item can be reserved (at any pickup location) and provide a way for item-level holds to have a valid pickup location selected by staff members.
Comment 1 Tomás Cohen Arazi 2021-05-12 18:40:07 UTC
Created attachment 120896 [details] [review]
Bug 28338: Make item-level holds use locally defined pickup branches

Besides the commit subject, this patch does much more:
- It makes request.pl stop passing a pickup location to
  CanItemBeReserved
- It makes the page use the API to render a dropdown for each item, with
  their valid pickup locations
- Items with no valid pickup locations have a nice message about why
  they are disabled for selection

To test:
1. Apply this patch
2. Choose a biblio for placing a hold
3. Choose a patron
=> SUCCESS: You are presented with a new layout, that includes a
dropdown for choosing each item's pickup location. If an item is not
holdable, it still isn't.
4. Try having an item whose home branch is not marked as a pickup
   location
=> SUCCESS: Notice you cannot choose that item
5. CHoose an item, but do not choose a branch, and click 'Place hold'
=> SUCCESS: It shows an alert about the need to choose a pickup location
6. Choose one of the (only possible) pickup locations for the specific
   item
7. Place the item level hold
=> SUCCESS: All goes as expected!
8. Sign off :D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-05-12 20:14:37 UTC
Created attachment 120905 [details] [review]
Bug 28338: Default to holding branch to save clicks

This patch makes request.pl pass the holding library object to the
template, if it is a valid pickup location for the item. This way, the
template can set a good default to save clicks.

To test:
1. Have "Hold pickup library match" set to "Item's home library"
2. Have a record with items in three different branches. For example:
   - item1: homebranch: MPL, holdingbranch: MPL
   - item2: homebranch: FPL, holdingbranch: FPL
   - item3: homebranch: CPL, holdingbranch: IPT
3. Have FPL marked as 'No' for pickup location
4. On the record, open the page for placing a hold for a patron
   (acevedo?)
=> SUCCESS: You are presented the regular hold placing page, with an
extra column on the items for pickup location setting
=> SUCCESS: The item2 (on FPL) cannot be selected, there's a clear message
about not having valid pickup locations
=> FAIL: The other ones don't have anything pre-selected on the
dropdowns
5. Apply this patch
6. Repeat 4 (go back to the record, etc)
=> SUCCESS: Nothing changed BUT the item with holding branch = MPL has
it set by default in the dropdown.
=> SUCCESS: IPT is not a valid pickup location for item3, so not set by
default in this case.
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Andrew Fuerste-Henry 2021-05-12 20:29:50 UTC
In testing, I have a bib with two items. One CPL and one FFL. FFL is set to be not a pickup location.

When I go to place a hold, the FFL item correctly has a blank pickup location and forces me to pick a branch before I can place an item-level hold on it.

However, my FFL item is listed second in the holdings table on request.pl. When my hold is placed, it's saving the Pickup Location selected for the first item in the holdings table instead.
Comment 4 Andrew Fuerste-Henry 2021-05-12 20:31:08 UTC
So I'm failing QA, BUT once that's working as intended, I think it's a really nice solution for bug 28312!
Comment 5 Tomás Cohen Arazi 2021-05-12 20:45:46 UTC
Created attachment 120908 [details] [review]
Bug 28338: Default to holding branch to save clicks

This patch makes request.pl pass the holding library object to the
template, if it is a valid pickup location for the item. This way, the
template can set a good default to save clicks.

To test:
1. Have "Hold pickup library match" set to "Item's home library"
2. Have a record with items in three different branches. For example:
   - item1: homebranch: MPL, holdingbranch: MPL
   - item2: homebranch: FPL, holdingbranch: FPL
   - item3: homebranch: CPL, holdingbranch: IPT
3. Have FPL marked as 'No' for pickup location
4. On the record, open the page for placing a hold for a patron
   (acevedo?)
=> SUCCESS: You are presented the regular hold placing page, with an
extra column on the items for pickup location setting
=> SUCCESS: The item2 (on FPL) cannot be selected, there's a clear message
about not having valid pickup locations
=> FAIL: The other ones don't have anything pre-selected on the
dropdowns
5. Apply this patch
6. Repeat 4 (go back to the record, etc)
=> SUCCESS: Nothing changed BUT the item with holding branch = MPL has
it set by default in the dropdown.
=> SUCCESS: IPT is not a valid pickup location for item3, so not set by
default in this case.
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2021-05-12 20:49:06 UTC
(In reply to Andrew Fuerste-Henry from comment #4)
> So I'm failing QA, BUT once that's working as intended, I think it's a
> really nice solution for bug 28312!

Thank you!
Comment 7 Owen Leonard 2021-05-13 11:35:42 UTC
Created attachment 120920 [details] [review]
Bug 28338: Make item-level holds use locally defined pickup branches

Besides the commit subject, this patch does much more:
- It makes request.pl stop passing a pickup location to
  CanItemBeReserved
- It makes the page use the API to render a dropdown for each item, with
  their valid pickup locations
- Items with no valid pickup locations have a nice message about why
  they are disabled for selection

To test:
1. Apply this patch
2. Choose a biblio for placing a hold
3. Choose a patron
=> SUCCESS: You are presented with a new layout, that includes a
dropdown for choosing each item's pickup location. If an item is not
holdable, it still isn't.
4. Try having an item whose home branch is not marked as a pickup
   location
=> SUCCESS: Notice you cannot choose that item
5. CHoose an item, but do not choose a branch, and click 'Place hold'
=> SUCCESS: It shows an alert about the need to choose a pickup location
6. Choose one of the (only possible) pickup locations for the specific
   item
7. Place the item level hold
=> SUCCESS: All goes as expected!
8. Sign off :D

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Owen Leonard 2021-05-13 11:35:47 UTC
Created attachment 120921 [details] [review]
Bug 28338: Default to holding branch to save clicks

This patch makes request.pl pass the holding library object to the
template, if it is a valid pickup location for the item. This way, the
template can set a good default to save clicks.

To test:
1. Have "Hold pickup library match" set to "Item's home library"
2. Have a record with items in three different branches. For example:
   - item1: homebranch: MPL, holdingbranch: MPL
   - item2: homebranch: FPL, holdingbranch: FPL
   - item3: homebranch: CPL, holdingbranch: IPT
3. Have FPL marked as 'No' for pickup location
4. On the record, open the page for placing a hold for a patron
   (acevedo?)
=> SUCCESS: You are presented the regular hold placing page, with an
extra column on the items for pickup location setting
=> SUCCESS: The item2 (on FPL) cannot be selected, there's a clear message
about not having valid pickup locations
=> FAIL: The other ones don't have anything pre-selected on the
dropdowns
5. Apply this patch
6. Repeat 4 (go back to the record, etc)
=> SUCCESS: Nothing changed BUT the item with holding branch = MPL has
it set by default in the dropdown.
=> SUCCESS: IPT is not a valid pickup location for item3, so not set by
default in this case.
7. Sign off :-D

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Andrew Fuerste-Henry 2021-05-13 14:13:07 UTC
*** Bug 28312 has been marked as a duplicate of this bug. ***
Comment 10 Andrew Fuerste-Henry 2021-05-13 14:15:00 UTC
Created attachment 120931 [details] [review]
Bug 28338: Make item-level holds use locally defined pickup branches

Besides the commit subject, this patch does much more:
- It makes request.pl stop passing a pickup location to
  CanItemBeReserved
- It makes the page use the API to render a dropdown for each item, with
  their valid pickup locations
- Items with no valid pickup locations have a nice message about why
  they are disabled for selection

To test:
1. Apply this patch
2. Choose a biblio for placing a hold
3. Choose a patron
=> SUCCESS: You are presented with a new layout, that includes a
dropdown for choosing each item's pickup location. If an item is not
holdable, it still isn't.
4. Try having an item whose home branch is not marked as a pickup
   location
=> SUCCESS: Notice you cannot choose that item
5. CHoose an item, but do not choose a branch, and click 'Place hold'
=> SUCCESS: It shows an alert about the need to choose a pickup location
6. Choose one of the (only possible) pickup locations for the specific
   item
7. Place the item level hold
=> SUCCESS: All goes as expected!
8. Sign off :D

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

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 11 Andrew Fuerste-Henry 2021-05-13 14:15:05 UTC
Created attachment 120932 [details] [review]
Bug 28338: Default to holding branch to save clicks

This patch makes request.pl pass the holding library object to the
template, if it is a valid pickup location for the item. This way, the
template can set a good default to save clicks.

To test:
1. Have "Hold pickup library match" set to "Item's home library"
2. Have a record with items in three different branches. For example:
   - item1: homebranch: MPL, holdingbranch: MPL
   - item2: homebranch: FPL, holdingbranch: FPL
   - item3: homebranch: CPL, holdingbranch: IPT
3. Have FPL marked as 'No' for pickup location
4. On the record, open the page for placing a hold for a patron
   (acevedo?)
=> SUCCESS: You are presented the regular hold placing page, with an
extra column on the items for pickup location setting
=> SUCCESS: The item2 (on FPL) cannot be selected, there's a clear message
about not having valid pickup locations
=> FAIL: The other ones don't have anything pre-selected on the
dropdowns
5. Apply this patch
6. Repeat 4 (go back to the record, etc)
=> SUCCESS: Nothing changed BUT the item with holding branch = MPL has
it set by default in the dropdown.
=> SUCCESS: IPT is not a valid pickup location for item3, so not set by
default in this case.
7. Sign off :-D

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

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 12 Martin Renvoize 2021-05-13 15:13:49 UTC
Created attachment 120938 [details] [review]
Bug 28338: Make item-level holds use locally defined pickup branches

Besides the commit subject, this patch does much more:
- It makes request.pl stop passing a pickup location to
  CanItemBeReserved
- It makes the page use the API to render a dropdown for each item, with
  their valid pickup locations
- Items with no valid pickup locations have a nice message about why
  they are disabled for selection

To test:
1. Apply this patch
2. Choose a biblio for placing a hold
3. Choose a patron
=> SUCCESS: You are presented with a new layout, that includes a
dropdown for choosing each item's pickup location. If an item is not
holdable, it still isn't.
4. Try having an item whose home branch is not marked as a pickup
   location
=> SUCCESS: Notice you cannot choose that item
5. CHoose an item, but do not choose a branch, and click 'Place hold'
=> SUCCESS: It shows an alert about the need to choose a pickup location
6. Choose one of the (only possible) pickup locations for the specific
   item
7. Place the item level hold
=> SUCCESS: All goes as expected!
8. Sign off :D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2021-05-13 15:13:53 UTC
Created attachment 120939 [details] [review]
Bug 28338: Default to holding branch to save clicks

This patch makes request.pl pass the holding library object to the
template, if it is a valid pickup location for the item. This way, the
template can set a good default to save clicks.

To test:
1. Have "Hold pickup library match" set to "Item's home library"
2. Have a record with items in three different branches. For example:
   - item1: homebranch: MPL, holdingbranch: MPL
   - item2: homebranch: FPL, holdingbranch: FPL
   - item3: homebranch: CPL, holdingbranch: IPT
3. Have FPL marked as 'No' for pickup location
4. On the record, open the page for placing a hold for a patron
   (acevedo?)
=> SUCCESS: You are presented the regular hold placing page, with an
extra column on the items for pickup location setting
=> SUCCESS: The item2 (on FPL) cannot be selected, there's a clear message
about not having valid pickup locations
=> FAIL: The other ones don't have anything pre-selected on the
dropdowns
5. Apply this patch
6. Repeat 4 (go back to the record, etc)
=> SUCCESS: Nothing changed BUT the item with holding branch = MPL has
it set by default in the dropdown.
=> SUCCESS: IPT is not a valid pickup location for item3, so not set by
default in this case.
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2021-05-13 15:14:31 UTC
All works as expected and it's a cleaner approach overall.. No QA failures, tests all passing.

Passed QA
Comment 15 Nick Clemens 2021-05-13 17:49:45 UTC
*** Bug 28284 has been marked as a duplicate of this bug. ***
Comment 16 Jonathan Druart 2021-06-14 15:34:13 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 17 Kyle M Hall 2021-06-18 13:53:37 UTC
Pushed to 21.05.x for 21.05.01
Comment 18 Fridolin Somers 2021-06-21 12:49:09 UTC
Depends on Bug 27931 not in 20.11.x
Comment 19 David Cook 2021-09-21 01:46:42 UTC
Just a little comment on this one. 

On "koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt", the function "check(table)" should really be "check(form)" because it's the form element #hold-request-form passed in rather than the table element.

It would make more sense to do "check(form)" and then do something like "let table = form.find('#requestspecific');"

And then when searching through the table, you could do input[name="checkitem"] rather than input[type="radio"], so that it's more specific. (I say this as someone who works on bug 15565 though, so that's just me being demanding :p)

Hopefully I'll make the above changes via bug 15565 some day down the line, but figured I'd note them here anyway.