|
Description
Nick Clemens (kidclamp)
2021-03-04 15:25:41 UTC
(In reply to Nick Clemens from comment #0) > When adjusting pickup locations for existing holds the dropdown populates > only with allowed libraries. > > The dropdown while placing holds should use the same logic. > > This should be fixed after addressing bug 27863 I think this description is exactly what 27863 does, and this bug is (as the title says) about placing holds on request.pl. Going forward with that, but wanted to note this. Created attachment 118232 [details] [review] Bug 27864: Visual feedback on overridden pickup locations when placing hold This patch makes the form for placing a hold, use the API to retrieve the valid pickup locations for a biblio. To test: 1. Try placing a hold on a biblio with several items. => SUCCESS: You get a list of valid pickup locations => FAIL: There's no sign they are overrides to rules 2. Apply this patch 3. Repeat 1 => SUCCESS: Same behavior as before, but there's a sign +tooltip for overridden ones. 4. Sign off :-D Note: styling follow-up coming Created attachment 118236 [details] [review] Bug 27864: Correct dropdown size This patch makes the dropdown width inherit the existing style. Patch no longer applies 8-(.. 118232 - Bug 27864: Visual feedback on overridden pickup locations when placing hold 118236 - Bug 27864: Correct dropdown size Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 27864: Visual feedback on overridden pickup locations when placing hold error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt). error: could not build fake ancestor Patch failed at 0001 Bug 27864: Visual feedback on overridden pickup locations when placing hold Testing notes: - To setup for testing follow the test plan as for bug 27863: . In Circulation and fines rules > Default checkout, hold and return policy, change 'Hold pickup library match' to 'item's home library' . Set AllowHoldPolicyOverride system preference to Allow Created attachment 119722 [details] [review] Bug 27864: Visual feedback on overridden pickup locations when placing hold This patch makes the form for placing a hold, use the API to retrieve the valid pickup locations for a biblio. To test: 1. Try placing a hold on a biblio with several items. => SUCCESS: You get a list of valid pickup locations => FAIL: There's no sign they are overrides to rules 2. Apply this patch 3. Repeat 1 => SUCCESS: Same behavior as before, but there's a sign +tooltip for overridden ones. 4. Sign off :-D Note: styling follow-up coming Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119723 [details] [review] Bug 27864: Correct dropdown size This patch makes the dropdown width inherit the existing style. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Looks great, Signing off Nick, could you QA this one? The biblio_id and patron_id fields are not populated and the GET request is failing Created attachment 120012 [details] [review] Bug 27864: Visual feedback on overridden pickup locations when placing hold This patch makes the form for placing a hold, use the API to retrieve the valid pickup locations for a biblio. To test: 1. Try placing a hold on a biblio with several items. => SUCCESS: You get a list of valid pickup locations => FAIL: There's no sign they are overrides to rules 2. Apply this patch 3. Repeat 1 => SUCCESS: Same behavior as before, but there's a sign +tooltip for overridden ones. 4. Sign off :-D Note: styling follow-up coming Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 120013 [details] [review] Bug 27864: Correct dropdown size This patch makes the dropdown width inherit the existing style. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Re-submitted due to some rebase problem. Created attachment 120014 [details] [review] Bug 27864: Visual feedback on overridden pickup locations when placing hold This patch makes the form for placing a hold, use the API to retrieve the valid pickup locations for a biblio. To test: 1. Try placing a hold on a biblio with several items. => SUCCESS: You get a list of valid pickup locations => FAIL: There's no sign they are overrides to rules 2. Apply this patch 3. Repeat 1 => SUCCESS: Same behavior as before, but there's a sign +tooltip for overridden ones. 4. Sign off :-D Note: styling follow-up coming Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 120015 [details] [review] Bug 27864: Correct dropdown size This patch makes the dropdown width inherit the existing style. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
FAIL forbidden patterns
forbidden pattern: Don't use select tag's size attribute where it is 1 (bug 28066) (line 445)
forbidden pattern: Don't use select tag's size attribute where it is 1 (bug 28066) (line 448)
Do you people forget to run the QA script?
(In reply to Jonathan Druart from comment #15) > FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt > FAIL forbidden patterns > forbidden pattern: Don't use select tag's size attribute > where it is 1 (bug 28066) (line 445) > forbidden pattern: Don't use select tag's size attribute > where it is 1 (bug 28066) (line 448) > > > Do you people forget to run the QA script? I don't get that. I'm pulling the latest qa tools... Created attachment 120170 [details] [review] Bug 27864: (QA follow-up) Remove size=1 on selects (28066 follow-up) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Please provide a full test plan, I don't see the visual feedback.
> => FAIL: There's no sign they are overrides to rules
I don't understand what this means.
(In reply to Jonathan Druart from comment #18) > Please provide a full test plan, I don't see the visual feedback. > > > => FAIL: There's no sign they are overrides to rules > > I don't understand what this means. Without the patch, you have a dropdown with the pickup_location=1 branches. With the patch, you have the same [1], but there's a visual sign indicating that choosing some of the pickup locations implies an override to rules. This is affected by circulation rules, so you need to make sure the item homebranch, the patron branch, and the rules make some of the pickup locations invalid (and thus require AllowHoldsPolicyOverride to be picked). [1] The same, but using Select2 and API routes. Created attachment 120243 [details] [review] Bug 27864: Visual feedback on overridden pickup locations when placing hold This patch makes the form for placing a hold, use the API to retrieve the valid pickup locations for a biblio. To test: 1 - In Circulation and fines rules > Default checkout, hold and return policy, change 'Hold pickup library match' to 'item's home library' 2 - Set AllowHoldPolicyOverride system preference to Allow 3 - Try placing a hold on a biblio with several items. Ensure that there is one item with homebranch of the patrons branch and one at another branch 4 - Note the pickup location dropdown lists all branches, even those not matching the policy (Item's home library) 5 - FAIL: There's no sign they are overrides to rules 6 - Apply the patch 7 - Repeat 3 8 - SUCCESS: Same behavior as before all libraires listed, but there's a sign + tooltip for overridden ones. 9 - Sign off :-D Note: styling follow-up coming Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 120244 [details] [review] Bug 27864: Correct dropdown size This patch makes the dropdown width inherit the existing style. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 120245 [details] [review] Bug 27864: (QA follow-up) Remove size=1 on selects (28066 follow-up) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Thank you, Nick! Thanks, Nick. There is a tiny ergonomic issue with the visual feedback: https://snipboard.io/5Wk9IQ.jpg Did you try to fix it? (In reply to Jonathan Druart from comment #24) > Thanks, Nick. > > There is a tiny ergonomic issue with the visual feedback: > https://snipboard.io/5Wk9IQ.jpg > Did you try to fix it? Hi, how did you get that? I've tried this patches now, on Firefox, Chrome and Safari and couldn't reproduce the visual glitch. (In reply to Tomás Cohen Arazi from comment #25) > (In reply to Jonathan Druart from comment #24) > > Thanks, Nick. > > > > There is a tiny ergonomic issue with the visual feedback: > > https://snipboard.io/5Wk9IQ.jpg > > Did you try to fix it? > > Hi, how did you get that? I've tried this patches now, on Firefox, Chrome > and Safari and couldn't reproduce the visual glitch. Can be reproduced on FF with zoom at 133% Something else (not directly related to this?): 1 - In Circulation and fines rules > Default checkout, hold and return policy, change 'Hold pickup library match' to 'item's home library' 2 - Set AllowHoldPolicyOverride system preference to "*NOT ALLOWED*" 3 - /cgi-bin/koha/reserve/request.pl?biblionumber=233 4 - Search "edna" => https://snipboard.io/F8oLIN.jpg Only Fairview and Midway are available but Centerville is preselected (and not in the dropdown list). Save anyway => Centerville is saved (shouldn't be rejected?) I confirm it's directly related to this patch set. Created attachment 120341 [details] [review] Bug 27864: (QA follow-up) Hide logged-in option if invalid pickup location Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 120342 [details] [review] Bug 27864: (QA follow-up) Select2 style on multi-holds Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 21.05, thanks to everybody involved! Enhancement not pushed to 20.11.x |
