Bug 7376

Summary: Transfer limits should be checked at check-in
Product: Koha Reporter: laurenthdl <henridamien.laurent>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: Failed QA --- QA Contact:
Severity: critical    
Priority: P5 - low CC: abl, gitbot, gmcharlt, jonathan.druart, julian.maurice, katrin.fischer, koha.aixmarseille, koha, m.de.rooy, martin.renvoize, mtompset, olli-antti.kivilahti, paul.poulain, stephane.delaye, veron
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9809
Whiteboard:
Change sponsored?: Sponsored Patch complexity: ---
Bot Control: --- When did the bot last check this: 2013-10-03 00:00:00
Who signed the patch off: Text to go in the release notes:
Bug Depends on: 11024    
Bug Blocks:    
Attachments: Bug 7376 Transfer limits should be checked at check-in
Bug 7376 Transfer limits should be checked at check-in
When failing UseBranchTransferLimits-check, don't display the "Thanks for returning this book"-message.
Bug 7376 - Transfer limits should be checked at check-in
Manual test cases
Bug 7376 - Transfer limits should be checked at check-in
Bug 7376 - Transfer limits should be checked at check-in
Bug 7376 - Transfer limits should be checked at check-in
Bug 7376 - Transfer limits should be checked at check-in
Bug 7376 Transfer limits should be checked at check-in
Bug 7376 Transfer limits should be checked at check-in
Bug 7376: Transfer limits should be checked at check-in

Description laurenthdl 2011-12-22 13:58:44 UTC
Test : 
When CirculationBranch is set to PatronLibrary,
Patron from branch B comes to branch A to checkout a book.
The librarian should be warned that the book should not be checkout.
When checking in, the librarian should be warned that the book should not be checked in before the return is done.
And the page should allow the librarian to override the warning. And accept the book.
Comment 1 laurenthdl 2011-12-26 15:52:20 UTC
sponsored by Lyon3 University.
Comment 2 Paul Poulain 2011-12-29 14:48:27 UTC
Upgrading severity, it's not an ENH, it's a bug, and a major one if you've activated branchtransfer !

Test case:
* UseBranchTransferLimits must be set
* define your branch transfer limit. Refuse transfers from libraryA to libraryB
* checkout a book owned by libraryB, from libraryB, with a librarian located at libraryB
* move the librarian to libraryA ("Set Library" link top/right)
* check-in the book => it's possible whatever your setup

After the patch, the behaviour respect the branch transfer limit parameter: you can check-in if you accept transfers, you can't if you refuse them.

(Note: IndependantBranches must be OFF, otherwise it's not possible to do the checkin whatever the branch transfer limits)
Comment 3 Paul Poulain 2011-12-29 14:56:56 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2011-12-29 15:29:28 UTC
(BibLibre MT8601)
Comment 5 Owen Leonard 2012-02-07 16:52:03 UTC
I don't think this is working, but maybe I misunderstand the conditions required for testing. I have:

CircControl set to "The library the patron is from" (PatronLibrary)
UseBranchTransferLimits set to "Enforce" (1) (and BranchTransferLimitsType set to "item type")

In library transfer limits I have deselected ALL checkboxes under Library A and Library B. I checked out a book from Library B to a patron from Library B at Library B. Then I switched to Library A and checked the book in. I got no warning about a blocked transfer. It simply checked in the item and initiated an automatic transfer.
Comment 6 Owen Leonard 2012-02-07 16:54:54 UTC
By the way, I tested this patch on top of the patches for Bug 6842 since otherwise I couldn't change library transfer limits.
Comment 7 Paul Poulain 2012-02-13 15:32:42 UTC
I just made a test again on my testing database, and I get, as expected, the result:

Cannot Check In
This item must be checked in at its home library. NOT CHECKED IN


Here is my setup:
* IndependantBranches = OFF (I also tested with IndependantBranches=ON, same behaviour, as expected)
* UseBranchTransferLimits & BranchTransferLimitsType = Enforce branch transfer limits based on Itemtype
* in admin/branch_transfer_limit.pl = everything is unselected, in all branches
* memcache is OFF, that's important, otherwise, when testing you can see bad results just because you get the cached version of the data, not the uptodate)
* HomeOrHoldingBranchReturn 	On checkin route the returned item to the library the item was checked out from (should not matter, but just in case)
* item-level_itypes is set to biblio record (that may be important)

Could you add
warn "==> branchtransferlimitvalue";
just BEFORE the line
    if (($hbr ne $branch && C4::Context->preference("IndependantBranches")) or (!IsBranchTransferAllowed($hbr, $branch, $branchtransferlimitvalue ))) {

It will show which flag is used for checking the branchtransfer
Comment 8 delaye 2012-03-12 16:26:59 UTC
After apply the patch i test the steps

Test case:
* UseBranchTransferLimits must be set
* define your branch transfer limit. Refuse transfers from libraryA to libraryB
* checkout a book owned by libraryB, from libraryB, with a librarian located at libraryB
* move the librarian to libraryA ("Set Library" link top/right)
* check-in the book => it's possible whatever your setup


==> ok

BUt

I can save a tranfert B ->A  and save a use with the item B in branch A... it'sl normal.. I don't understand
Comment 9 delaye 2012-03-13 10:51:05 UTC
Disregard my previous comment, I forgot to prohibit transfers from B to A. sorry

so it's ok But...

But the rules (limit) of tranfers is not interpreted with the requests for holds to the OPAC and intranet
and more specialy with the system preference OPACAllowUserToChooseBranch(=ON)
If OPACAllowUserToChooseBranch activated the transfers limits are not read. The system preference displays all sites (= select * from branches) instead of displaying only those sites that allow transfers.

Example:
A network of four sites: A, B, C and D
The adherents of A, B and C can reserve copies of A, B and C. Items with branches A, B and C can transit in sub-network A, B and C. The items with branch D can not be reserved and site D does not accept the items with branches A, B and C.

The holds rules are the following :
Branch A -> All sites
Branch B -> All sites
Branch C -> All sites
Branch D -> No holds

Rules of transfers
branch A -> A, B, C
branch B -> A, B, C
branch C -> A, B, C
branch D -> D

OPACAllowUserToChooseBranch is activated

From  OPAC a borrower located A want to book a title. The title have 3 items in use with branches A, B, and D.
Koha display all the branches A, B, C and D for pickup instead of proposing the branches A, B and C. So when returning an item (A, B or C) there is a risk that the pickuplocation will be D if the borrower have been chose D. While D don't want manage holds within the network.

Paul I have an example of sandbox1 http://catalogue.test1.biblibre.com/cgi-bin/koha/opac-reserve.pl?biblionumber=2220
(user: password and mozart: mozart)

it's important for few customers. See also ticket 7534
Comment 10 Koha Team Lyon 3 2012-03-20 14:04:01 UTC
I follow Test Plan : Ok for check in / Check Out. I think we should have some Transfert Limit for Holds. For example, at Lyon 3, tranferts are authorised when check-in to transfer the document to his home Library but we don't want to document to be transfered for chek-out when placed in hold. For example : 1. a borrower from libraryB place hold on a document from libraryA. 2. the document is check-in in his homelibribrary (A) 3. The document is automaticly set in transfer to library B :we don't want to transfer this document, we want that borrowers come in library A to check out the document hold.


Sign OFF for transfer Limit check-in.
Comment 11 Marcel de Rooy 2012-03-23 14:54:33 UTC
Some QA comments:

Your call to GetBiblioFromItemNumber could have a barcode too. Just mentioning. Furthermore, it seems that you only need itemtype from biblioitems (twice used in routine). Getting a hashref for all fields from biblio, biblioitems and items is relative expensive for that purpose ;) No blocker.

You are checking branch transfer limits without consulting UseBranchTransferLimits ? It could just be off? Suppose that you had some limits, but disabled the option later. In that case the check could be incorrect. Note that it is hypothetical. Also you would not need to call IsBranchTransferAllowed if $hbr eq $branch. (Although it should not hurt.)

Just a note on variable name: name $branchtransferlimitvalue is somewhat confusing. No blocker.

In conclusion: patch slightly fails QA now. If you still want to push it however, please send a followup for remark 2. The other points could be resolved as well, but are of minor importance.
Note also the earlier comments of several testers. Parking this on Failed QA for further attention.
Comment 12 Nicole C. Engard 2013-08-08 17:03:04 UTC
It's been a while on this, but can we get an updated patch?
Comment 13 Olli-Antti Kivilahti 2013-10-02 10:36:51 UTC
Yeah, I am curious about this bug as well. Tho we would like to extend this to cover placing holds as well. Well I already wrote the code for that as a proof of concept for our librarians. I there some way I can contribute to this bug?
Comment 14 Paul Poulain 2013-10-02 10:41:29 UTC
Jonathan, if you can have a look to this patch
Comment 15 Jonathan Druart 2013-10-02 13:03:26 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2013-10-02 13:03:41 UTC
Rebased patch.
Comment 17 I'm just a bot 2013-10-03 07:00:20 UTC
Patch applied cleanly, go forth and signoff
Comment 18 Olli-Antti Kivilahti 2013-10-04 06:39:07 UTC
I think we should make a single function
&CheckBranchTransferLimits($item, $biblio, $destinationBranch, $sourceBranch)
# either $item or $biblio should be given, preferably both due to
# C4::Context->preference("item-level_itypes")

Otherwise we get crazy code duplication since the check for BranchTransferLimits is really messy in the calling method.
Also this check is performed during circ/branchtransfers.pl and it definetely should be checked when placing reservations/holds as well.
Also some Restful API's we have, place holds and they should be UseBranchTransferLimits-capable.

Placing holds is beyond the scope of this bug, but now that a patch has been applied I'd prefer to have a central way of checking the UseBranchTransferLimit instead of replicating this code everywhere.
Comment 19 Olli-Antti Kivilahti 2013-10-04 08:35:59 UTC Comment hidden (obsolete)
Comment 20 Olli-Antti Kivilahti 2013-10-04 08:56:19 UTC
Test case parameters:
-----------------------------
* UseBranchTransferLimits must be set

* define your branch transfer limit.
  IPT -> CPL -> FFL -> IPT

Test case 1, transfer is not allowed:
-----------------------------

* move the librarian to IPT ("Set Library" link top/right)

* check-out an item, with homebranch IPT

* move the librarian to CPL ("Set Library" link top/right)

* FAIL check-in the item => Book check-in fails, but the template is incorrect
(Attachment: When failing UseBranchTransferLimits-check, don't display the "Thanks for returning this book"-message.)
  OK (check-in fails): Item's checkout status still "Checked out to Olli-Antti2 Kivilahti2, Due back on 11/01/2013"


Test case 2, transfer is allowed:
---------------------------------

* move the librarian to IPT ("Set Library" link top/right)

* check-out an item, with homebranch IPT

* move the librarian to FFL ("Set Library" link top/right)

* OK check-in succeeds and printing a transfer slip is proposed.
  Item's checkout status:  undef


Test conclusion:
--------------------------------
Covered:
Item checkout status changes (Checked out, undef)
Transfer limits (Allow transfer?, true, false)
Template in circ/returns.pl

Test fails the template check, otherwise OK.


Note:
---------------------------------
Should test for SIP2 as well, but probably it works ok, since AddReturn returns false when BranchTransferLimits check fails.
Comment 21 Jonathan Druart 2013-10-04 09:48:53 UTC
(In reply to Olli-Antti Kivilahti from comment #19)
> Created attachment 21805 [details]
> When failing UseBranchTransferLimits-check, don't display the "Thanks for
> returning this book"-message.

I don't get this message. Are you sure you applied this patch on the master branch?
Comment 22 Olli-Antti Kivilahti 2013-10-07 07:00:39 UTC
The strange message I got was due to the checkin message attached to my itemtype.
Not sure if the checkin message should be disabled if checkin fails? Propably it should.

circ/returns.pl::>>
# Check if we should display a checkin message, based on the the item
# type of the checked in item
    my $itemtype = C4::ItemType->get( $biblio->{'itemtype'} );
    if ( $itemtype->{'checkinmsg'} ) {
        ...print checking message...;
    }

Still:
The item which failed to checkin is still added to the list of checked in material at the end of the page.


laurendthl has requested the ability for a librarian to override the transfer limit. This is probably pretty useful in ILL-cases.
And they are the bug sponsor as well :)

SYNOPSIS:
-OPTIONAL: Disable checkin message
-OPTIONAL: Do not add the failed checkin to the "Checked-in items"-table
-Make it possible for a librarian to override the branchtransferlimit (no special permissions requested in the RFP)
--Log this override event to catch misbehaving librarians.
Comment 23 Koha Team AMU 2013-10-11 09:29:04 UTC
I follow the Test Plan, it's Ok. 

But the librarian can't override, it should be better.
Comment 24 Olli-Antti Kivilahti 2013-10-15 09:24:51 UTC
Agreed to fix this bug for Joubu, in exchange for QA stuff.
Comment 25 Olli-Antti Kivilahti 2013-10-15 10:57:42 UTC
sponsored by Joensuu Regional Library
Comment 26 Olli-Antti Kivilahti 2013-10-16 11:55:08 UTC Comment hidden (obsolete)
Comment 27 Olli-Antti Kivilahti 2013-10-16 11:56:05 UTC
Created attachment 22017 [details]
Manual test cases
Comment 28 Paul Poulain 2013-10-22 23:47:33 UTC
mmm... this patch has been set to "failed QA" by Aix-Marseille university, and a new patch has been attached. Shouldn't the status be reset to "need signoff" ? Otherwise, explanation welcomed !
Comment 29 Olli-Antti Kivilahti 2013-10-23 15:14:01 UTC Comment hidden (obsolete)
Comment 30 Olli-Antti Kivilahti 2013-10-23 15:21:20 UTC
"mmm... this patch has been set to "failed QA" by Aix-Marseille university, and a new patch has been attached. Shouldn't the status be reset to "need signoff" ? Otherwise, explanation welcomed !" -PP

It should. I noticed this as well but I decided to leave it as is, since someone had already signed this bug off. I knew it wasn't exactly a nice thing and was kinda hoping to skip the sign-off part.
Obviously my behaviour was not very nice. Apologies for the misconduct.

Changed tabs to spaces.
Comment 31 Olli-Antti Kivilahti 2013-10-24 12:37:42 UTC Comment hidden (obsolete)
Comment 32 Paul Poulain 2013-10-25 10:10:02 UTC
(In reply to Olli-Antti Kivilahti from comment #30)
> "mmm... this patch has been set to "failed QA" by Aix-Marseille university,
> and a new patch has been attached. Shouldn't the status be reset to "need
> signoff" ? Otherwise, explanation welcomed !" -PP
> 
> It should. I noticed this as well but I decided to leave it as is, since
> someone had already signed this bug off. I knew it wasn't exactly a nice
> thing and was kinda hoping to skip the sign-off part.
> Obviously my behaviour was not very nice. Apologies for the misconduct.
I didn't spot any misconduct, but as long as a patch is "failed QA" no one takes care of it, except the patch author. So if you submit a new patch, then you need to change the status.

When a failed QA is caused by a small/minor QA problem, like an indent one, then, it's OK to switch back to "signed off" if the patch has already been signed off. Otherwise, back to "need signoff". But not let "failed QA" status as it is.
Comment 33 Olli-Antti Kivilahti 2013-11-04 11:41:48 UTC Comment hidden (obsolete)
Comment 34 Olli-Antti Kivilahti 2013-11-26 17:04:05 UTC
Created attachment 23168 [details] [review]
Bug 7376 - Transfer limits should be checked at check-in

Extended UseBranchTransferLimits-check to the check-in case.
Made it possible to override the check-in with circulation-permission if BranchTransfer is denied.
Made unit tests for the C4/Circulation.pm and C4/SIP/t/08checkin.t

*Also added override-functionality to Circulation::AddReturn() to override BranchTransferLimits. (+unit tests)
*Renamed CanBookBeReturned to CanItemBeReturned since that function was referenced from AddReturn only.
--This is because function name misguidingly references to a book/Biblio/Title-level, even if the functionality is strictly
--Item dependent.
*Removed duplicating code by making CanItemBeTransferred() call IsBranchTransferAllowed().
*Made a unit test library out of CanItemBeTransferred.t's environment initialization parts,
  so these need not be duplicated whenever UseBranchTransferLimits-related functionality is tested.
*Documented SIP-testing hardships to C4/SIP/README
Comment 35 Christopher Brannon 2014-01-13 17:16:04 UTC
I cannot sign off on this patch.  The following error comes up when I try to apply it to a sandbox:

The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 7376:
<h1>Something went wrong !</h1>Applying: Bug 7376 - Transfer limits should be checked at check-in
fatal: sha1 information is lacking or useless (C4/Circulation.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 7376 - Transfer limits should be checked at check-in When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 7376 - Transfer limits should be checked at check-in

23168 - Bug 7376 - Transfer limits should be checked at check-in

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-7376---Transfer-limits-should-be-checked-at-ch-u0UdEh.patch .

Christopher
Comment 36 Jonathan Druart 2015-08-21 10:42:05 UTC
Be care, AddReserve prototype changed by bug 9809 (contrainst param removed).
Comment 37 Marc Véron 2015-12-24 13:21:28 UTC
Still a critical bug? - It was reported 2011-12-22.
Comment 38 Marc Véron 2015-12-24 13:47:29 UTC
Does not apply. Is this bug still valid?
Comment 39 Katrin Fischer 2018-01-22 10:01:16 UTC
From my tests on master this one is still valid. Reading to the other bugs related to branch/library transfer limits it seems that those configuration options have no effect atm. Is this correct?
Comment 40 Jonathan Druart 2018-06-29 12:20:47 UTC
Created attachment 76559 [details] [review]
Bug 7376 Transfer limits should be checked at check-in

Test case:
* UseBranchTransferLimits must be set
* define your branch transfer limit. Refuse transfers from libraryA to libraryB
* checkout a book owned by libraryB, from libraryB, with a librarian located at
libraryB
* move the librarian to libraryA ("Set Library" link top/right)
* check-in the book => it's possible whatever your setup

After the patch, the behaviour respect the branch transfer limit parameter: you
can check-in if you accept transfers, you can't if you refuse them.

(Note: IndependantBranches must be OFF, otherwise it's not possible to do the
checkin whatever the branch transfer limits)
Comment 41 Jonathan Druart 2018-06-29 12:21:15 UTC
Original patch rebased.
Comment 42 Martin Renvoize 2018-07-11 19:12:53 UTC
Compile time warnings when running the QA script on this I'm afraid.. Looks like $borrower is returned in the patch but it's not part of the method signature anymore.
Comment 43 Martin Renvoize 2018-07-11 19:14:49 UTC
(In reply to Martin Renvoize from comment #42)
> Compile time warnings when running the QA script on this I'm afraid.. Looks
> like $borrower is returned in the patch but it's not part of the method
> signature anymore.

Needs more digging.. borrower is indeed defined later in the method but it's not defined before it's called here.. failing qa until that's fixed :(
Comment 44 Jonathan Druart 2018-07-11 20:06:20 UTC
Created attachment 76866 [details] [review]
Bug 7376 Transfer limits should be checked at check-in

Test case:
* UseBranchTransferLimits must be set
* define your branch transfer limit. Refuse transfers from libraryA to libraryB
* checkout a book owned by libraryB, from libraryB, with a librarian located at
libraryB
* move the librarian to libraryA ("Set Library" link top/right)
* check-in the book => it's possible whatever your setup

After the patch, the behaviour respect the branch transfer limit parameter: you
can check-in if you accept transfers, you can't if you refuse them.

(Note: IndependantBranches must be OFF, otherwise it's not possible to do the
checkin whatever the branch transfer limits)
Comment 45 Martin Renvoize 2018-07-12 07:10:32 UTC
Created attachment 76876 [details] [review]
Bug 7376: Transfer limits should be checked at check-in

Test case:
* UseBranchTransferLimits must be set
* define your branch transfer limit. Refuse transfers from libraryA to libraryB
* checkout a book owned by libraryB, from libraryB, with a librarian located at
libraryB
* move the librarian to libraryA ("Set Library" link top/right)
* check-in the book => it's possible whatever your setup

After the patch, the behaviour respect the branch transfer limit parameter: you
can check-in if you accept transfers, you can't if you refuse them.

(Note: IndependantBranches must be OFF, otherwise it's not possible to do the
checkin whatever the branch transfer limits)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 46 Martin Renvoize 2018-07-12 07:11:44 UTC
All seems to work as expected and can't see any regressions, Signing Off.

I also amended the commit message to add the missing ':' to silence the QA scripts ;)
Comment 47 Marcel de Rooy 2018-07-13 06:30:11 UTC
Please add unit tests
Comment 48 Jonathan Druart 2018-07-13 10:55:37 UTC
(In reply to Marcel de Rooy from comment #47)
> Please add unit tests

Switching to NSO, I'd like a confirmation that the code is doing what we are expecting it to do. Then I will try to provide tests.
Comment 49 Julian Maurice 2018-08-09 09:09:59 UTC
(In reply to Jonathan Druart from comment #48)
> (In reply to Marcel de Rooy from comment #47)
> > Please add unit tests
> 
> Switching to NSO, I'd like a confirmation that the code is doing what we are
> expecting it to do. Then I will try to provide tests.
Hasn't Martin already confirmed it by signing off the patch ? Are you asking for a 2nd signoff ?
Comment 50 Jonathan Druart 2018-08-09 14:21:28 UTC
(In reply to Julian Maurice from comment #49)
> Are you asking for a 2nd signoff ?

Yes.
Comment 51 M. Tompsett 2018-09-10 14:35:14 UTC
(In reply to Jonathan Druart from comment #48)
> (In reply to Marcel de Rooy from comment #47)
> > Please add unit tests
> 
> Switching to NSO, I'd like a confirmation that the code is doing what we are
> expecting it to do. Then I will try to provide tests.

Wouldn't the tests show us what we are expecting it to do? The person who knows what it is supposed to do should be writing the tests, or at least confirming that any tests written represent what should be written.
Comment 52 Koha Team Lyon 3 2018-09-14 23:10:42 UTC
Sorry, but for me it doesn't work...
It set IndependentBranches to "no"
UseBranchTransferLimits is set to "enforce  library transfer limits based on itemtype"

In cgi-bin/koha/admin/branch_transfer_limits.pl
In the table of library A and in the tab of itemtypeA : I uncheck "allow transfer" in the line concerning "libraryB".

I checkout a book (itemtypeA) from libraryB in LibraryB and then change of library to checkin in libraryA. When I checkin this book in libraryA, I just have the message "This item needs to be transferred to Budaka".

If I have well understood, I should have a message that tells me that this book couldn't be checkin in libraryA...