Bug 10993 - Transfer limits should be checked when placing a hold/reservation in OPAC.
Summary: Transfer limits should be checked when placing a hold/reservation in OPAC.
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P4 enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 11005
Blocks: 11020
  Show dependency treegraph
 
Reported: 2013-10-04 06:56 UTC by Olli-Antti Kivilahti
Modified: 2021-01-07 18:00 UTC (History)
6 users (show)

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


Attachments
Bug 10993 [ENH] - Transfer limits should be checked when placing a hold/reservation in OPAC. (19.67 KB, patch)
2013-10-09 10:15 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
quickie test document (2.04 KB, text/plain)
2013-10-09 10:16 UTC, Olli-Antti Kivilahti
Details
Bug 10993 - Transfer limits should be checked when placing a hold/reservation in OPAC. (17.37 KB, patch)
2013-10-15 10:40 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Manual test cases (2.17 KB, text/plain)
2013-10-15 10:41 UTC, Olli-Antti Kivilahti
Details
Bug 10993 - Transfer limits should be checked when placing a hold/reservation in OPAC. (19.32 KB, patch)
2013-10-23 15:13 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 10993 - Transfer limits should be checked when placing a hold/reservation in OPAC. (164.12 KB, patch)
2013-11-26 16:31 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2013-10-04 06:56:56 UTC
If a Borrower cannot get an Item transfered to his pickup location of choice, the
Borrower needs to know that before he places a hold.
This is especially true when placing Item-level holds.
If a patron can place a hold on an item, he expects the item to be available at his pickup location of choice. Currently there is no warning or hold block for holds that violate the UseBranchTransferLimits-setting.

Solution. Create or use an existing centralized CheckBranchTransferLimits-function inside the C4::Reserves::CanItemBeReserved(), to see if an item can be reserved for the desired pickup location.
When the borrower changes his pickup location in
koha/opac-reserve.pl?biblionumber=xxx, the BranchTransferLimits need to be recalculated.


UseBranchTransferLimits should also affect placing holds via the Restful API's.
$pickuplocation is not always available in the Restful API -requests so we can use the Borrowers homebranch as the default pickup location.
Comment 1 Olli-Antti Kivilahti 2013-10-04 06:58:16 UTC
BTW: Should this be optional?
Is it against the initial spirit of UseBranchTransferLimits?
Comment 2 Olli-Antti Kivilahti 2013-10-07 08:31:59 UTC
Also when a Patron is placing a hold,
the Patron should be able to choose the pickup location even if there are no available/holdable Items in his home branch.
So instead of failing to reach the "Place hold"-dialogue and choose the pickuplocation, the Patron should be sent to the koha/opac-reserve.pl?biblionumber=xxx with a notifcation that no items are available to his pickup location, prompting the Patron to choose another pickup location.
Comment 3 Olli-Antti Kivilahti 2013-10-09 10:15:09 UTC Comment hidden (obsolete)
Comment 4 Olli-Antti Kivilahti 2013-10-09 10:16:24 UTC Comment hidden (obsolete)
Comment 5 Olli-Antti Kivilahti 2013-10-09 15:50:00 UTC
Sponsored by the Joensuu Regional Library
Comment 6 Olli-Antti Kivilahti 2013-10-15 10:40:32 UTC Comment hidden (obsolete)
Comment 7 Olli-Antti Kivilahti 2013-10-15 10:41:21 UTC
Created attachment 22001 [details]
Manual test cases
Comment 8 Paul Poulain 2013-10-22 23:51:56 UTC
Some QA comments about this patch (I haven't tested, just looking at the code)
 * you must use 4 spaces and not tabs
 * +1 for the "manual test case". Note that we use the term "test plan". It could confuse some tester that are not english natives & could search the test plan

No other code or patch comment (which does not mean passed QA yet, I haven't tested it either)
Comment 9 Olli-Antti Kivilahti 2013-10-23 15:13:40 UTC Comment hidden (obsolete)
Comment 10 Olli-Antti Kivilahti 2013-10-23 15:24:05 UTC
Fixed tabs to spaces in indentation. Didn't realize my IDE uses tabs instead of spaces until bug 11024/7376.
Not sure about templates since they are such a mess regarding indentation.
Comment 11 Olli-Antti Kivilahti 2013-11-26 16:31:44 UTC
Created attachment 23163 [details] [review]
Bug 10993 - Transfer limits should be checked when placing a hold/reservation in OPAC.

Depends on bug 11005, 11020

Prevents a user from placing holds on material whose branch transfer is denied. Displays clear messages that items are
unavailable and re-checks in the business layer in case of UI blocks failing.
Enables choosing another pickup location.

Manual test plan included.

Formatted with perltidy and Perl::Critic level 5
Comment 12 Paul Poulain 2014-10-11 20:32:53 UTC
trying to test this patch during hackfest KohaCon14.
Unfortunately, it does not apply :
fatal: sha1 information is lacking or useless (C4/Circulation.pm).

Another problem: you're mixed "real", "new" code and reindent/perlity.
This patch cannot be QAed.

I encourage you to perltidy/reindent, but in a separate patch please.
Comment 13 Olli-Antti Kivilahti 2014-10-13 08:57:21 UTC
Sorry, I cannot work on this currently. I will let this slide to the next one who encounters this issue and wants to fix it. Hopefully using some of my code :)
Comment 14 Mark Tompsett 2015-04-05 01:28:18 UTC
(In reply to Olli-Antti Kivilahti from comment #13)
> Sorry, I cannot work on this currently. I will let this slide to the next
> one who encounters this issue and wants to fix it. Hopefully using some of
> my code :)

I felt like fainting getting half way through with no sign of any obvious changes, except for white space. Do you have time to make reading and testing it more feasible?
Comment 15 Marie-Luce Laflamme 2021-01-07 17:57:27 UTC
Koha seems to correctly apply the transfer limit now, but Koha doesn't warn patron when the hold cannot be placed.

Specifications:
Koha with at least 2 branches: branch A , branch B
Systeme preference: UseBranchTransferLimits  ---> Enforce
Systeme preference BranchTransferLimitsType  ---> item type
System preference OPACAllowUserToChooseBranch ---> Don’t allow
      
 
Configurations – transfer limit
1. Go to Administration › Set Library checkin and transfer limit
2. Select a branch A (top menu) 
3. Click on the item type tab to block any transfer
4. Uncheck branch B 
5. Save
6. Select branch B (top menu) 
7. Click on the item type tab
8. Uncheck branch A to block any transfer
9. Save

Test Plan
1. Find a patron from branch B 
2. Go on the OPAC, find an item currently checkout from branch A (same item type that should be blocked)
3. Place a hold in the patron account (branch B)
4. Confirm the hold
–> notice that Koha doesn’t add the hold in the patron account, but there is no explanation, it is uncleared what just happen. The usual message «There are no items that can be placed on holds» isn’t popping up on screen to confirm that this action cannot be done.
Comment 16 Marie-Luce Laflamme 2021-01-07 18:00:37 UTC
Further notes:
If you are modifying the system preference OPACAllowUserToChooseBranch to allow the pick up branch selection, Koha will block the hold by removing the blocked branch name from the menu. Therefore, Koha prevent the patron to place an hold and send it to their branch when the transfer is not allowed.