Bug 27865 - Hold pickup location dropdown on patron pages should respect hold fulfillment policies
Summary: Hold pickup location dropdown on patron pages should respect hold fulfillment...
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: Testopia
URL:
Keywords:
Depends on: 27898 28254 27205 27863
Blocks: 28261
  Show dependency treegraph
 
Reported: 2021-03-04 15:31 UTC by Nick Clemens
Modified: 2021-05-11 12:07 UTC (History)
6 users (show)

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


Attachments
Bug 27865: Pass the x-koha-override header to PUT /holds/:hold_id from the patron's page (1.88 KB, patch)
2021-03-08 17:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27865: Pass the x-koha-override header to PUT /holds/:hold_id from the patron's page (1.93 KB, patch)
2021-03-11 13:13 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 27865: Pass the x-koha-override header to PUT /holds/:hold_id from the patron's page (1.88 KB, patch)
2021-04-28 19:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27865: Pass the x-koha-override header to PUT /holds/:hold_id from the patron's page (1.95 KB, patch)
2021-04-29 10:50 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 Nick Clemens 2021-03-04 15:31:16 UTC
Same as bug 27864 but for patron account pages on staff client
Comment 1 Tomás Cohen Arazi 2021-03-08 17:36:20 UTC
This was caused by bug 27205.
Comment 2 Tomás Cohen Arazi 2021-03-08 17:41:39 UTC
Created attachment 117941 [details] [review]
Bug 27865: Pass the x-koha-override header to PUT /holds/:hold_id from the patron's page

This patch makes the AJAX call to update an existing hold, pass the
'x-koha-override' header with the value of 'any', so any pickup location
policy override that is needed, is applied.

As the override is taken into account only if AllowHoldPolicyOverride is
enabled, this in fact restores the previous behavior.

To test:
1. Have a patron with a hold
2. Have AllowHoldPolicyOverride set to 'yes'
3. Try changing the pickup location to an invalid one
=> FAIL: The options are the same as before 27205, yet the AJAX call
         fails because of invalid pickup location.
4. Apply this patch
5. Repeat 3
=> SUCCESS: The API call succeeds, and the hold is updated (it is
        actually refetch, so just verify it has the value you selected
6. Sign off :-D
Comment 3 Peter Vashchuk 2021-03-11 13:13:15 UTC
Created attachment 118119 [details] [review]
Bug 27865: Pass the x-koha-override header to PUT /holds/:hold_id from the patron's page

This patch makes the AJAX call to update an existing hold, pass the
'x-koha-override' header with the value of 'any', so any pickup location
policy override that is needed, is applied.

As the override is taken into account only if AllowHoldPolicyOverride is
enabled, this in fact restores the previous behavior.

To test:
1. Have a patron with a hold
2. Have AllowHoldPolicyOverride set to 'yes'
3. Try changing the pickup location to an invalid one
=> FAIL: The options are the same as before 27205, yet the AJAX call
         fails because of invalid pickup location.
4. Apply this patch
5. Repeat 3
=> SUCCESS: The API call succeeds, and the hold is updated (it is
        actually refetch, so just verify it has the value you selected
6. Sign off :-D

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 4 Katrin Fischer 2021-04-03 23:44:30 UTC
There is a lot of bugs depending on this I feel would be in better hands with other QA team members, but I wanted to test the end result a bit:

I am not sure if this works correctly. I am testing this right after bug 27894 with the same holds.

For my holds, I only allow the pick-up location = item's home library. 
Also Fairview is set to be no pickup location on library level.

In the patron account, on details and checkouts tabs:
- Fairview doesn't show up - OK
- All other libraries show independent of the AllowHoldPolicyOverride setting - NOT OK. With Don't allow only the home library of the item should be available.
- In comparison: for the same holds, the pull down on detail page > holds only shows the home library as expected with the stricter setting.

Should we not also establish a visual warning here like 27894 does?
Comment 5 Tomás Cohen Arazi 2021-04-05 11:30:14 UTC
(In reply to Katrin Fischer from comment #4)
> There is a lot of bugs depending on this I feel would be in better hands
> with other QA team members, but I wanted to test the end result a bit:
> 
> I am not sure if this works correctly. I am testing this right after bug
> 27894 with the same holds.
> 
> For my holds, I only allow the pick-up location = item's home library. 
> Also Fairview is set to be no pickup location on library level.
> 
> In the patron account, on details and checkouts tabs:
> - Fairview doesn't show up - OK
> - All other libraries show independent of the AllowHoldPolicyOverride
> setting - NOT OK. With Don't allow only the home library of the item should
> be available.
> - In comparison: for the same holds, the pull down on detail page > holds
> only shows the home library as expected with the stricter setting.
> 
> Should we not also establish a visual warning here like 27894 does?

Yeah, adding the visual feedback here would be helpful. The idea here was to provide a backportable fix to a regression, though. Will fix it to make it show all pickup locations only if AllowHoldPolicyOverride is set.
Comment 6 Tomás Cohen Arazi 2021-04-05 15:52:18 UTC
I now recall it. This bug wasn't about fixing the dropdown, but fixing the PUT, which was failing even though the dropdown presents the libraries to override.
Comment 7 Katrin Fischer 2021-04-05 16:06:38 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> I now recall it. This bug wasn't about fixing the dropdown, but fixing the
> PUT, which was failing even though the dropdown presents the libraries to
> override.

So I should be able to change the pickup location to an invalid one or only when the override is possible?
Comment 8 Tomás Cohen Arazi 2021-04-05 16:17:21 UTC
(In reply to Katrin Fischer from comment #7)
> (In reply to Tomás Cohen Arazi from comment #6)
> > I now recall it. This bug wasn't about fixing the dropdown, but fixing the
> > PUT, which was failing even though the dropdown presents the libraries to
> > override.
> 
> So I should be able to change the pickup location to an invalid one or only
> when the override is possible?

Only when the override is possible. The PUT route should reject the request. But that's not in the scope of this bug. That is taken care of on bug 27898, and it is correctly tested in my opinion. What you might be facing here, is wrong dropdown rendering. Which could be solved the way we did in bug 27894 and friends.

As I said, the idea was to make it easy to backport this, and move the dropdown rendering into using Select2 + API + visual feedback on overrides, on a separate bug.
Comment 9 Tomás Cohen Arazi 2021-04-28 19:03:46 UTC
Created attachment 120291 [details] [review]
Bug 27865: Pass the x-koha-override header to PUT /holds/:hold_id from the patron's page

This patch makes the AJAX call to update an existing hold, pass the
'x-koha-override' header with the value of 'any', so any pickup location
policy override that is needed, is applied.

As the override is taken into account only if AllowHoldPolicyOverride is
enabled, this in fact restores the previous behavior.

To test:
1. Have a patron with a hold
2. Have AllowHoldPolicyOverride set to 'yes'
3. Try changing the pickup location to an invalid one
=> FAIL: The options are the same as before 27205, yet the AJAX call
         fails because of invalid pickup location.
4. Apply this patch
5. Repeat 3
=> SUCCESS: The API call succeeds, and the hold is updated (it is
        actually refetch, so just verify it has the value you selected
6. Sign off :-D

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 10 Kyle M Hall 2021-04-29 10:50:40 UTC
Created attachment 120308 [details] [review]
Bug 27865: Pass the x-koha-override header to PUT /holds/:hold_id from the patron's page

This patch makes the AJAX call to update an existing hold, pass the
'x-koha-override' header with the value of 'any', so any pickup location
policy override that is needed, is applied.

As the override is taken into account only if AllowHoldPolicyOverride is
enabled, this in fact restores the previous behavior.

To test:
1. Have a patron with a hold
2. Have AllowHoldPolicyOverride set to 'yes'
3. Try changing the pickup location to an invalid one
=> FAIL: The options are the same as before 27205, yet the AJAX call
         fails because of invalid pickup location.
4. Apply this patch
5. Repeat 3
=> SUCCESS: The API call succeeds, and the hold is updated (it is
        actually refetch, so just verify it has the value you selected
6. Sign off :-D

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Jonathan Druart 2021-05-04 13:24:55 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 12 Fridolin Somers 2021-05-11 12:07:01 UTC
Dependencies not in 20.11.x