Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy
Summary: On shelf holds allowed > "If all unavailable" ignores default hold policy
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Mirko Tietgen
QA Contact: Nick Clemens
URL:
Keywords:
: 18366 19543 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-05-05 07:44 UTC by Mirko Tietgen
Modified: 2019-10-14 19:57 UTC (History)
10 users (show)

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


Attachments
Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy (2.24 KB, patch)
2017-05-05 07:55 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy (2.33 KB, patch)
2017-05-07 13:53 UTC, Alex Buckley
Details | Diff | Splinter Review
Add tests (6.72 KB, patch)
2017-08-31 18:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy (2.68 KB, patch)
2017-10-31 10:51 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Add tests (6.77 KB, patch)
2017-11-13 15:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy (2.73 KB, patch)
2017-11-13 15:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18547: QA Followup (7.65 KB, patch)
2017-11-13 15:38 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Mirko Tietgen 2017-05-05 07:55:47 UTC
Created attachment 63157 [details] [review]
Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy

If in the circ rules matrix you set "On shelf holds allowed" to "If all unavailable",
default hold policy "From home library" is ignored.

Test plan:

- Have a test user from one branch (eg Centerville)

- Set "On shelf holds allowed" to "If all unavailable" for your patron and item
  category (or everyone and everything)
- For "Default checkout, hold and return policy", set hold policy to "From home library"
  -> make sure there is no "Default holds policy by item type" to override the setting

- Have two items for a record.
  1. An item with home branch same as test user (eg Centerville)
  -> check this item out to somebody else
  2. an item with a different home branch (eg Fairfield)
  -> available, not checked out
- Try to place a hold for your test user. Does not work.

- Apply the patch
- Try to place a hold. Should work now.
Comment 2 Alex Buckley 2017-05-07 13:53:54 UTC
Created attachment 63227 [details] [review]
Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy

If in the circ rules matrix you set "On shelf holds allowed" to "If all unavailable",
default hold policy "From home library" is ignored.

Test plan:

- Have a test user from one branch (eg Centerville)

- Set "On shelf holds allowed" to "If all unavailable" for your patron and item
  category (or everyone and everything)
- For "Default checkout, hold and return policy", set hold policy to "From home library"
  -> make sure there is no "Default holds policy by item type" to override the setting

- Have two items for a record.
  1. An item with home branch same as test user (eg Centerville)
  -> check this item out to somebody else
  2. an item with a different home branch (eg Fairfield)
  -> available, not checked out
- Try to place a hold for your test user. Does not work.

- Apply the patch
- Try to place a hold. Should work now.

Followed test plan, worked as intended
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 3 Claudio Costales 2017-05-07 19:20:50 UTC
I tested it in a local installation (16.11.07) but can not make it works...

(see #18366)
Comment 4 Claudio Costales 2017-05-10 14:43:09 UTC
(In reply to Alex Buckley from comment #2)
> Created attachment 63227 [details] [review] [review]
> Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default
> hold policy
> 
> If in the circ rules matrix you set "On shelf holds allowed" to "If all
> unavailable",
> default hold policy "From home library" is ignored.
> 
> Test plan:
> 
> - Have a test user from one branch (eg Centerville)
> 
> - Set "On shelf holds allowed" to "If all unavailable" for your patron and
> item
>   category (or everyone and everything)
> - For "Default checkout, hold and return policy", set hold policy to "From
> home library"
>   -> make sure there is no "Default holds policy by item type" to override
> the setting
> 
> - Have two items for a record.
>   1. An item with home branch same as test user (eg Centerville)
>   -> check this item out to somebody else
>   2. an item with a different home branch (eg Fairfield)
>   -> available, not checked out
> - Try to place a hold for your test user. Does not work.
> 
> - Apply the patch
> - Try to place a hold. Should work now.
> 
> Followed test plan, worked as intended
> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Alex, this is a great fix but does not work. I cannot figure the reason...
Comment 5 Mirko Tietgen 2017-05-10 15:01:31 UTC
(In reply to Claudio from comment #4)

> Alex, this is a great fix but does not work. I cannot figure the reason...

We use this patch in production and it works. Maybe you have another problem related to this feature which is not covered by the patch.
Comment 6 Claudio Costales 2017-05-10 17:58:35 UTC
(In reply to Mirko Tietgen from comment #5)
> (In reply to Claudio from comment #4)
> 
> > Alex, this is a great fix but does not work. I cannot figure the reason...
> 
> We use this patch in production and it works. Maybe you have another problem
> related to this feature which is not covered by the patch.

Thanks Mirko!

I noticed that changing ReservesControlBranch syspref to "item's home library" the fix works (in my Koha installation ReservesControlBranch was set to "patron's home library"). Is it right?
Comment 7 Jonathan Druart 2017-05-11 15:49:02 UTC
Any chances to see tests covering this change?
Comment 8 Marcel de Rooy 2017-05-19 09:49:11 UTC
(In reply to Jonathan Druart from comment #7)
> Any chances to see tests covering this change?

Changing status.
Comment 9 Katrin Fischer 2017-08-17 11:27:23 UTC
Any chance to get this moving again? I think this is one of the things that a lot of libraries would like.
Comment 10 Katrin Fischer 2017-08-17 13:39:27 UTC
*** Bug 18366 has been marked as a duplicate of this bug. ***
Comment 11 Jonathan Druart 2017-08-31 18:35:45 UTC
Created attachment 66675 [details] [review]
Add tests
Comment 12 Jonathan Druart 2017-08-31 18:36:27 UTC
(In reply to Jonathan Druart from comment #11)
> Created attachment 66675 [details] [review] [review]
> Add tests

Make sure these tests make sense!
Comment 13 Caroline Cyr La Rose 2017-09-29 18:50:53 UTC
I'm not sure of the test plan and expected result. Do we have to place the hold from the OPAC or staff interface? And for the result, is it supposed to override the default holds policy?

- I changed one of my circulation rules to If all unavailable for one of my patron categories (x all item types).

- Fixed a record so that one of the items was in the home library of the user and one was in another branch (both current location and home library)

- Check out the item that was in the same home library as the patron from the fiex patron category to another user

- Tried to place a hold for my patron in the staff interface. It says "Not on hold OR Due DD/MM/AAAA Item level hold not allowed from OPAC. There is a little yellow "!" triangle icon next to BOTH items but I can still override the circ rule. I have a pop-up saying "This item normally cannot be put on hold except for patrons from (2nd branch), Place hold on this item?"

- From the OPAC, I logged in as my user and tried to place the hold and it says "Sorry non of these items can be placed on hold." / "No available items".

- After the patch, the yellow "!" triangle icon is ONLY next to the item from the 2nd location (not patron's home library) in the staff interface. So, no problem to reserve the checked out item, but I have to override to reserve the available item.

- From the OPAC, I was able to place my hold on the bib record without problem. I went into the "Holds to pull" report and saw that the item from the 2nd location was there, waiting to be pulled from the shelf.
Comment 14 Victor Grousset/tuxayo 2017-10-31 10:51:34 UTC
Created attachment 68854 [details] [review]
Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy

If in the circ rules matrix you set "On shelf holds allowed" to "If all unavailable",
default hold policy "From home library" is ignored.

Test plan:

- Have a test user from one branch (eg Centerville)

- Set "On shelf holds allowed" to "If all unavailable" for your patron and item
  category (or everyone and everything)
- For "Default checkout, hold and return policy", set hold policy to "From home library"
  -> make sure there is no "Default holds policy by item type" to override the setting

- Have two items for a record.
  1. An item with home branch same as test user (eg Centerville)
  -> check this item out to somebody else
  2. an item with a different home branch (eg Fairfield)
  -> available, not checked out
- Try to place a hold for your test user. Does not work.

- Apply the patch
- Try to place a hold. Should work now.

Followed test plan, worked as intended
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Followed test plan in the intranet and OPAC, worked as intended.
With few assumptions made:
- when the hold works, should be on the item 1 (Centerville)
  item 2 is only there to trigger the bug
- the circ rules were identically setup on each branch and in "Standard rules for all libraries"
Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>
Comment 15 Victor Grousset/tuxayo 2017-10-31 11:00:12 UTC
@Caroline Cyr La Rose
I got results consistent with yours.
Thanks for raising the OPAC vs intranet issue, I didn't think of this when first trying the test plan. And it seems that the test plan was written for the OPAC as more details are needed for doing it in the intranet. (related the overriding the circ rule (disabling it or ignoring it))
Comment 16 Katrin Fischer 2017-11-01 11:05:48 UTC
Glad to see this one moving, thx for taking the time to test!
Comment 17 Nick Clemens 2017-11-13 13:06:16 UTC
*** Bug 19543 has been marked as a duplicate of this bug. ***
Comment 18 Nick Clemens 2017-11-13 15:38:37 UTC
Created attachment 69125 [details] [review]
Add tests

https://bugs.koha-community.org/show_bug.cgi?id=18547

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Nick Clemens 2017-11-13 15:38:40 UTC
Created attachment 69126 [details] [review]
Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy

If in the circ rules matrix you set "On shelf holds allowed" to "If all unavailable",
default hold policy "From home library" is ignored.

Test plan:

- Have a test user from one branch (eg Centerville)

- Set "On shelf holds allowed" to "If all unavailable" for your patron and item
  category (or everyone and everything)
- For "Default checkout, hold and return policy", set hold policy to "From home library"
  -> make sure there is no "Default holds policy by item type" to override the setting

- Have two items for a record.
  1. An item with home branch same as test user (eg Centerville)
  -> check this item out to somebody else
  2. an item with a different home branch (eg Fairfield)
  -> available, not checked out
- Try to place a hold for your test user. Does not work.

- Apply the patch
- Try to place a hold. Should work now.

Followed test plan, worked as intended
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Followed test plan in the intranet and OPAC, worked as intended.
With few assumptions made:
- when the hold works, should be on the item 1 (Centerville)
  item 2 is only there to trigger the bug
- the circ rules were identically setup on each branch and in "Standard rules for all libraries"
Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Nick Clemens 2017-11-13 15:38:43 UTC
Created attachment 69127 [details] [review]
Bug 18547: QA Followup

Added comments to make clearer

Tests had some confusion with names and results, though they do seem to
cover the changes here.

I believe the tests highlight that ReserveControlBranch has no effect,
and possibly it should? Beyond the scope here but left those tests
for future work to be done

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Jonathan Druart 2017-11-26 16:25:56 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 22 Marcel de Rooy 2017-11-27 10:51:49 UTC
I was looking at 4319 that also proposes changes for this sub in Reserves, and somehow came here.

The changes here seem too selective to me.

Why are you looking at $branchitemrule->{holdallowed} == 1 only? Why do you not check the 0 value (no holds allowed) ? And why do you add this test only for If all unavailable? Should it not be tested globally, over the different values of On shelf holds allowed?

Does not look good to me. Do I overlook something ?
Comment 23 Marcel de Rooy 2017-11-27 10:52:43 UTC
Removed Academy. Should (generally) not be added to Holds patches.
Comment 24 Jonathan Druart 2018-08-06 22:46:25 UTC
Comment on attachment 69126 [details] [review]
Bug 18547 - On shelf holds allowed > "If all unavailable" ignores default hold policy

Review of attachment 69126 [details] [review]:
-----------------------------------------------------------------

::: C4/Reserves.pm
@@ +1161,5 @@
>  
>          foreach my $i (@items) {
> +
> +            my $circ_control_branch = C4::Circulation::_GetCircControlBranch( $i->unblessed(), $borrower );
> +            my $branchitemrule = C4::Circulation::GetBranchItemRule( $circ_control_branch, $i->itype );

Hum, should not $i->itype be $i->effective_itemtype?