|
Description
Tomás Cohen Arazi (tcohen)
2021-05-12 18:10:49 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> 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> 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. So I'm failing QA, BUT once that's working as intended, I think it's a really nice solution for bug 28312! 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> (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! 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> 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> *** Bug 28312 has been marked as a duplicate of this bug. *** 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> 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> 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> 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> All works as expected and it's a cleaner approach overall.. No QA failures, tests all passing. Passed QA *** Bug 28284 has been marked as a duplicate of this bug. *** Pushed to master for 21.11, thanks to everybody involved! Pushed to 21.05.x for 21.05.01 Depends on Bug 27931 not in 20.11.x 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.
|