Bug 27864

Summary: Visual feedback on overridden pickup locations when placing biblio-level hold
Product: Koha Reporter: Nick Clemens <nick>
Component: Hold requestsAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: enhancement    
Priority: P5 - low CC: david, fridolin.somers, gmcharlt, jonathan.druart, lucas, martin.renvoize, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28273
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This patch changes the pickup location dropdown, when placing biblio-level holds, so it renders a sign, alerting that circulation rules would be overridden if the pickup location is chosen. It relies on new API routes.
Version(s) released in:
21.05.00
Bug Depends on: 27863, 27894, 27932    
Bug Blocks: 28261    
Attachments: Bug 27864: Visual feedback on overridden pickup locations when placing hold
Bug 27864: Correct dropdown size
Bug 27864: Visual feedback on overridden pickup locations when placing hold
Bug 27864: Correct dropdown size
Bug 27864: Visual feedback on overridden pickup locations when placing hold
Bug 27864: Correct dropdown size
Bug 27864: Visual feedback on overridden pickup locations when placing hold
Bug 27864: Correct dropdown size
Bug 27864: (QA follow-up) Remove size=1 on selects (28066 follow-up)
Bug 27864: Visual feedback on overridden pickup locations when placing hold
Bug 27864: Correct dropdown size
Bug 27864: (QA follow-up) Remove size=1 on selects (28066 follow-up)
Bug 27864: (QA follow-up) Hide logged-in option if invalid pickup location
Bug 27864: (QA follow-up) Select2 style on multi-holds

Description Nick Clemens 2021-03-04 15:25:41 UTC
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
Comment 1 Tomás Cohen Arazi 2021-03-08 12:21:07 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.
Comment 2 Tomás Cohen Arazi 2021-03-15 11:53:23 UTC
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
Comment 3 Tomás Cohen Arazi 2021-03-15 14:36:46 UTC
Created attachment 118236 [details] [review]
Bug 27864: Correct dropdown size

This patch makes the dropdown width inherit the existing style.
Comment 4 David Nind 2021-04-10 20:40:34 UTC
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
Comment 5 Martin Renvoize 2021-04-16 12:17:01 UTC
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>
Comment 6 Martin Renvoize 2021-04-16 12:17:05 UTC
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>
Comment 7 Martin Renvoize 2021-04-16 12:17:21 UTC
Looks great, Signing off
Comment 8 Jonathan Druart 2021-04-22 08:04:41 UTC
Nick, could you QA this one?
Comment 9 Nick Clemens 2021-04-22 15:15:56 UTC
The biblio_id and patron_id fields are not populated and the GET request is failing
Comment 10 Tomás Cohen Arazi 2021-04-22 15:58:21 UTC
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>
Comment 11 Tomás Cohen Arazi 2021-04-22 15:58:26 UTC
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>
Comment 12 Tomás Cohen Arazi 2021-04-22 15:58:53 UTC
Re-submitted due to some rebase problem.
Comment 13 Nick Clemens 2021-04-22 16:04:00 UTC
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>
Comment 14 Nick Clemens 2021-04-22 16:04:04 UTC
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>
Comment 15 Jonathan Druart 2021-04-26 10:25:20 UTC
 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?
Comment 16 Tomás Cohen Arazi 2021-04-26 11:15:57 UTC
(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...
Comment 17 Tomás Cohen Arazi 2021-04-26 11:22:40 UTC
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>
Comment 18 Jonathan Druart 2021-04-27 14:14:11 UTC
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.
Comment 19 Tomás Cohen Arazi 2021-04-27 14:50:21 UTC
(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.
Comment 20 Nick Clemens 2021-04-27 17:46:34 UTC
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>
Comment 21 Nick Clemens 2021-04-27 17:46:39 UTC
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>
Comment 22 Nick Clemens 2021-04-27 17:46:43 UTC
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>
Comment 23 Tomás Cohen Arazi 2021-04-27 18:07:50 UTC
Thank you, Nick!
Comment 24 Jonathan Druart 2021-04-28 08:31:29 UTC
Thanks, Nick.

There is a tiny ergonomic issue with the visual feedback: https://snipboard.io/5Wk9IQ.jpg
Did you try to fix it?
Comment 25 Tomás Cohen Arazi 2021-04-28 11:06:27 UTC
(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.
Comment 26 Jonathan Druart 2021-04-29 13:08:44 UTC
(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%
Comment 27 Jonathan Druart 2021-04-29 13:09:02 UTC
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?)
Comment 28 Jonathan Druart 2021-04-29 13:11:09 UTC
I confirm it's directly related to this patch set.
Comment 29 Tomás Cohen Arazi 2021-04-30 14:16:26 UTC
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>
Comment 30 Tomás Cohen Arazi 2021-04-30 14:31:30 UTC
Created attachment 120342 [details] [review]
Bug 27864: (QA follow-up) Select2 style on multi-holds

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Jonathan Druart 2021-04-30 15:17:46 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 32 Fridolin Somers 2021-05-11 12:02:23 UTC
Enhancement not pushed to 20.11.x