Bug 7376

Summary: Transfer limits should be checked at check-in
Product: Koha Reporter: laurenthdl <henridamien.laurent>
Component: CirculationAssignee: Arthur Suzuki <arthur.suzuki>
Status: Failed QA --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: abl, arthur.suzuki, black23, fridolin.somers, gitbot, gmcharlt, jean-manuel.broust, jonathan.druart, josef.moravec, julian.maurice, katrin.fischer, klas.blomberg, koha.aixmarseille, koha, kyle, lari.taskula, lisette.scheer, m.de.rooy, martin.renvoize, mathsabypro, michaela.sieber, mtompset, Niamh.WalkerHeadon, olli-antti.kivilahti, paul.poulain, pierre.genty, severine.queune, stephane.delaye, veron, victor
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9809
Change sponsored?: Unsponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This patch introduces new controls to prevent a check-in at a library where transfer rules would forbid the item transfer back to this item's home/issuing library. For items set to 'float' transfer rules are not set as the check-in would not trigger a transfer.
Version(s) released in:
Bug Depends on: 11024    
Bug Blocks: 22160    
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
Bug 7376 Transfer limits should be checked at check-in
Bug 7376 Transfer limits should be checked at check-in
(ANOTHER IMPLEMENTATION) Bug 7376: Refactoring subroutines
(ANOTHER IMPLEMENTATION) Bug 7376: Fix wrong indentation of CanBookBeReturned
(ANOTHER IMPLEMENTATION) Bug 7376: Transfer limits should be checked at check-in
Bug 7376: Refactoring subroutines
(ANOTHER IMPLEMENTATION) Bug 7376: Refactoring subroutines
(ANOTHER IMPLEMENTATION) Bug 7376: Fix wrong indentation of CanBookBeReturned
(ANOTHER IMPLEMENTATION) Bug 7376: Transfer limits should be checked at check-in
Bug 7376: Refactoring subroutines
Bug 7376: Fix wrong indentation of CanBookBeReturned
Bug 7376: Transfer limits should be checked at check-in
Bug 7376: Transfer limits should be checked at check-in
Bug 7376: Unit test
Bug 7376: (follow-up) Extend test coverage and document
Bug 7376: (follow-up) Extend test coverage and document
Bug 7376: (follow-up) Make extended tests mock the actual desired behaviour
Bug 7376: (follow-up) Fix C4/Circulation.pm
Bug 7376: Clarify tests further
Bug 7376: Allow passing 'transferbranch' to CanBookBeReturned
Bug 7376: Transfer limits should be checked at check-in
Bug 7376: Unit test
Bug 7376: (follow-up) Extend test coverage and document
Bug 7376: (follow-up) Make extended tests mock the actual desired behaviour
Bug 7376: (follow-up) Fix C4/Circulation.pm
Bug 7376: Clarify tests further
Bug 7376: Allow passing 'transferbranch' to CanBookBeReturned
Bug 7376: Transfer limits should be checked at check-in
Bug 7376: Unit test
Bug 7376: (follow-up) Extend test coverage and document
Bug 7376: (follow-up) Make extended tests mock the actual desired behaviour
Bug 7376: (follow-up) Fix C4/Circulation.pm
Bug 7376: Clarify tests further
Bug 7376: Allow passing 'transferbranch' to CanBookBeReturned

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 University 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 Mark 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 University 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...
Comment 53 Klas Blomberg 2018-10-03 16:34:23 UTC
What about the override in the "final" version? Will we be able to accept ckeck-ins of items from a library where we have disallowed transfers?
Comment 54 Katrin Fischer 2018-10-04 20:42:54 UTC
(In reply to Klas Blomberg from comment #53)
> What about the override in the "final" version? Will we be able to accept
> ckeck-ins of items from a library where we have disallowed transfers?

Maybe this could be a separate bug, with a system preference like we have for other override actions?
Comment 55 Kyle M Hall 2019-02-13 20:36:52 UTC
Moving to new bug that encompasses multiple old bugs.

*** This bug has been marked as a duplicate of bug 22330 ***
Comment 56 Victor Grousset/tuxayo 2019-03-01 11:38:30 UTC
Hi, are you sure that bug 22330 encompasses this one? Because it(22330) seems to concert only holds.
Comment 57 Lari Taskula 2019-08-26 11:09:40 UTC
*** Bug 23505 has been marked as a duplicate of this bug. ***
Comment 58 Koha Team University Lyon 3 2019-09-12 15:25:25 UTC
Created attachment 92740 [details] [review]
Bug 7376 Transfer limits should be checked at check-in

to answer to Victor (comment 56), the response is no, seems that bug 22330 don't encompass this one.
When you return an unauthorized item (according to the branch transfer limits table), you get only a message "Please return this item to such branch", but the return is done as well.

So the need to add a checking on branch transfert status is still on the agenda.
The patch proposed is essentially the initial one from Martin Renvoize with a little variant to tie the checking with the "anywhere" position of ReturnToBranch syspref (because a priori, the need is much likely to appear in this case).

Test Plan :
* ReturnToBranch must be set to "anywhere"
* UseBranchTransferLimits must be set
1) define your branch transfer limit. Refuse transfers from libraryA to libraryOwner (= refuse return at libraryA of a book located at libraryOwner).
2) checkout a book owned by libraryOwner, at libraryOwner, with a librarian located at libraryOwner
3) move the librarian to libraryA ("Set Library" link top/right)
4) check-in the book => "you get the message Please return this item to libraryOwner" 
Note that you get the same message whether the transfert from libraryA to libraryOwner is authorized or not.
5) Install the patch
6) Redo steps 2 to 4
The message you get now at step 4 is "Cannot check in ... This item must be checked in at following library: libraryOwner"

Olivier Crouzet
Comment 59 Koha Team University Lyon 3 2019-09-13 08:55:37 UTC
Created attachment 92774 [details]
Bug 7376 Transfer limits should be checked at check-in

Oups ! sorry, the previous patch was for version 17.11. Here's the one for master

Olivier Crouzet
Comment 60 Martin Renvoize 2019-09-19 13:30:19 UTC
This looks good and I intend to test as soon as I get a moment.. however, it will need some unit tests to get through the QA process (and that's a good thing to prevent future regressions anyway).

Thanks for working on this.
Comment 61 Martin Renvoize 2019-09-19 13:32:18 UTC
Also.. the commit author is rather generic.. it would be great to have that fixed up to be less anonymous and also have a 'Sponsored-by: ' line added to give credit to the sponsors.
Comment 62 Koha Team University Lyon 3 2019-09-19 14:14:28 UTC
Hello,
We have only one bugzilla account for Lyon 3 University, so the patch author is our team, but we can change our habits if needed.
We sponsored this enhancement in 2011 but it have never worked or at least not for long.
But I don't know to add our University in the sponsor: Lyon3 University.
Sonia BOUIS
Comment 63 Lari Taskula 2019-09-27 09:51:38 UTC
We are also trying to solve this issue. About a month ago I created some patches for the National Library of Finland to test within the Finnish Koha fork. I did not yet rebase those patches for master, but they can be found in this PR

https://github.com/KohaSuomi/Koha/pull/104

Sorry for causing duplicate work. I should had mentioned this as I started the task.
Comment 64 Lari Taskula 2019-09-27 11:31:36 UTC
Created attachment 93190 [details] [review]
(ANOTHER IMPLEMENTATION) Bug 7376: Refactoring subroutines

CanBookBeReturned:
- Take Koha::Item as parameter instead of HASHref of an item

To test:
1. cd into your Koha root directory
2. grep -rn 'CanBookBeReturned('
3. Make sure all returned occurences are using Koha::Item
Comment 65 Lari Taskula 2019-09-27 11:31:57 UTC
Created attachment 93191 [details] [review]
(ANOTHER IMPLEMENTATION) Bug 7376: Fix wrong indentation of CanBookBeReturned

To test:
1. git diff -w  --word-diff-regex=[^[:space:]] HEAD~1
2. Observe no differences (above cmd ignores space changes)
3. Therefore patch only includes whitespace changes
Comment 66 Lari Taskula 2019-09-27 11:32:43 UTC
Created attachment 93192 [details] [review]
(ANOTHER IMPLEMENTATION) 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 67 Lari Taskula 2019-09-27 11:51:10 UTC
Created attachment 93193 [details] [review]
Bug 7376: Refactoring subroutines

CanBookBeReturned:
- Take Koha::Item as parameter instead of HASHref of an item

To test:
1. cd into your Koha root directory
2. grep -rn 'CanBookBeReturned('
3. Make sure all returned occurences are using Koha::Item

Sponsored-by: National Library of Finland
Comment 68 Lari Taskula 2019-09-27 11:51:57 UTC
Created attachment 93194 [details] [review]
(ANOTHER IMPLEMENTATION) Bug 7376: Refactoring subroutines

CanBookBeReturned:
- Take Koha::Item as parameter instead of HASHref of an item

To test:
1. cd into your Koha root directory
2. grep -rn 'CanBookBeReturned('
3. Make sure all returned occurences are using Koha::Item

Sponsored-by: National Library of Finland
Comment 69 Lari Taskula 2019-09-27 11:52:18 UTC
Created attachment 93195 [details] [review]
(ANOTHER IMPLEMENTATION) Bug 7376: Fix wrong indentation of CanBookBeReturned

To test:
1. git diff -w  --word-diff-regex=[^[:space:]] HEAD~1
2. Observe no differences (above cmd ignores space changes)
3. Therefore patch only includes whitespace changes

Sponsored-by: National Library of Finland
Comment 70 Lari Taskula 2019-09-27 11:52:43 UTC
Created attachment 93196 [details] [review]
(ANOTHER IMPLEMENTATION) 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)

Sponsored-by: National Library of Finland
Comment 71 Lari Taskula 2019-09-27 12:07:18 UTC
(In reply to Lari Taskula from comment #63)
> We are also trying to solve this issue. About a month ago I created some
> patches for the National Library of Finland to test within the Finnish Koha
> fork. I did not yet rebase those patches for master, but they can be found
> in this PR
> 
> https://github.com/KohaSuomi/Koha/pull/104
> 
> Sorry for causing duplicate work. I should had mentioned this as I started
> the task.

Here are my patches from GitHub, rebased on current master branch.

I didn't test team Lyon's patch, but it seems the main difference is that this version displays a more specific error message
"Transfer is forbidden between [% Branches.GetName( holdingbranch ) %] (item's current library) and [% LoginBranchname | html %]."

The location for logic is also different. Mine checks transfer limits in CanBookBeReturned() while the original patch does it in AddReturn(). Not sure if it currently makes a difference, but it could help covering some edge cases. AddReturn of course internally uses CanBookBeReturned so I suppose this is more appropriate place to add the logic into.

But as the original patch, this is also missing unit tests.
Comment 72 Séverine Queune 2019-10-04 07:05:22 UTC
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
Warning: commit message did not conform to UTF-8.
You may want to amend it after fixing the message, or set the config
variable i18n.commitencoding to the encoding your project uses.
Applying: Bug 7376: Refactoring subroutines
Using index info to reconstruct a base tree...
M       C4/Circulation.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/Circulation.pm
CONFLICT (content): Merge conflict in C4/Circulation.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 7376: Refactoring subroutines
The copy of the patch that failed is found in: .git/rebase-apply/patch
Comment 73 Koha Team University Lyon 3 2019-10-04 10:14:55 UTC
Hello Séverine
Seems that you can't test this patch on sandbox because both my patch and Lari Taskula ones are automatically loaded all the same.

Olivier Crouzet
Comment 74 Axelle Aix-Marseille Université 2019-10-30 07:53:17 UTC
The status was changed from "Patch does'nt apply" to "Needs signoff" today. So I tried on a sandbox but it still not works : 


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
Applying: Bug 7376: Refactoring subroutines
Using index info to reconstruct a base tree...
M       C4/Circulation.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/Circulation.pm
CONFLICT (content): Merge conflict in C4/Circulation.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 7376: Refactoring subroutines
The copy of the patch that failed is found in: .git/rebase-apply/patch
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

92774 - Bug 7376 Transfer limits should be checked at check-in
93194 - (ANOTHER IMPLEMENTATION) Bug 7376: Refactoring subroutines
93195 - (ANOTHER IMPLEMENTATION) Bug 7376: Fix wrong indentation of CanBookBeReturned
93196 - (ANOTHER IMPLEMENTATION) Bug 7376: Transfer limits should be checked at check-in

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/ANOTHER-IMPLEMENTATION-Bug-7376-Refactoring-subrou-21wcKc.patch .
Comment 75 Martin Renvoize 2019-10-30 08:18:43 UTC
Comment 74 is clear.. these patches, as there is a set of alternatives, cannot be tested on the sandboxes as they stand. They do apply however if you know how to pick individual sets.. hence my setting it to needs signoff.. right now with a 'Patch doesn't apply' status it won't be picked up with others.
Comment 76 Axelle Aix-Marseille Université 2019-10-30 09:02:01 UTC
(In reply to Martin Renvoize from comment #75)
> Comment 74 is clear.. these patches, as there is a set of alternatives,
> cannot be tested on the sandboxes as they stand. They do apply however if
> you know how to pick individual sets.. hence my setting it to needs
> signoff.. right now with a 'Patch doesn't apply' status it won't be picked
> up with others.

Ok. So, sorry for my note. I thought that because status had changed, it meant that patch was corrected and could be tested in a sandbox. I didn't know that for one bug, you could have several patches to test.

May I replace the status to "Needs signoff" ?
Comment 77 Arthur Suzuki 2019-12-03 01:20:10 UTC
Comment on attachment 92774 [details]
Bug 7376 Transfer limits should be checked at check-in

Hi Olivier,
Hope if it's ok if I obsolete your patch since Lari has provided a newer version with some correction?
That way it makes it easier to test by anyone using sandboxes :)
Arthur
Comment 78 Arthur Suzuki 2019-12-03 02:02:42 UTC
last patch still doesn't apply anyway...
Comment 79 Martin Renvoize 2020-04-15 15:32:01 UTC
Too late for 20.05 I feel.. updating target to 20.11
Comment 80 Arthur Suzuki 2020-07-20 06:20:24 UTC
Created attachment 107077 [details] [review]
Bug 7376: Refactoring subroutines

CanBookBeReturned:
- Take Koha::Item as parameter instead of HASHref of an item

To test:
1. cd into your Koha root directory
2. grep -rn 'CanBookBeReturned('
3. Make sure all returned occurences are using Koha::Item

Sponsored-by: National Library of Finland

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 81 Arthur Suzuki 2020-07-20 06:20:30 UTC
Created attachment 107078 [details] [review]
Bug 7376: Fix wrong indentation of CanBookBeReturned

To test:
1. git diff -w  --word-diff-regex=[^[:space:]] HEAD~1
2. Observe no differences (above cmd ignores space changes)
3. Therefore patch only includes whitespace changes

Sponsored-by: National Library of Finland

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 82 Arthur Suzuki 2020-07-20 06:20:35 UTC
Created attachment 107079 [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)

Sponsored-by: National Library of Finland

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 83 Katrin Fischer 2020-07-29 23:21:30 UTC
I am sorry, but in initial testing, this makes the existing circulation related unit tests fail:

Test Summary Report
-------------------
t/db_dependent/Circulation.t                          (Wstat: 256 Tests: 49 Failed: 1)
  Failed test:  22
  Non-zero exit status: 1

not ok 22 - CanBookBeIssued & AllowReturnToBranch


At this point we also really need unit tests for the fixed behaviour in order to move on with these patches.
Comment 84 Arthur Suzuki 2021-01-19 22:39:00 UTC
Created attachment 115369 [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)

Sponsored-by: National Library of Finland

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 85 Arthur Suzuki 2021-01-19 22:39:32 UTC
Created attachment 115370 [details] [review]
Bug 7376: Unit test
Comment 86 Arthur Suzuki 2021-01-19 22:40:28 UTC
I did a rebase of the patches + added unit tests
Comment 87 Martin Renvoize 2021-02-08 11:46:09 UTC
Created attachment 116493 [details] [review]
Bug 7376: (follow-up) Extend test coverage and document

I'm not sure about the requirements for this bug. This patch extends the
tests to cover the new combinations introduced and highlights exactly
what changes in the logic.. which appears to be fairly little.
Comment 88 Martin Renvoize 2021-02-08 12:08:52 UTC
Assuming the test plan written by Paul P in comment 2 still stands.. then I think this patch is all good...

If, however, we are trying to prevent a return at a library where the item cannot be transferred back to holdingbranch/homebranch after the return takes place.. which is what I think people in IRC claimed was the point.. then this patchset is wrong.

Basically.. I think I need some clarification as to exactly what the use case is here to know if we're fixing the problem.
Comment 89 Martin Renvoize 2021-02-08 12:46:38 UTC
Created attachment 116494 [details] [review]
Bug 7376: (follow-up) Extend test coverage and document

I'm not sure about the requirements for this bug. This patch extends the
tests to cover the new combinations introduced and highlights exactly
what changes in the logic.. which appears to be fairly little.
Comment 90 Katrin Fischer 2021-02-08 14:15:37 UTC
> 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)

I believe actually that is what we said on IRC just phrased a little differently.

The book with homebranch B was checked out at A.
Then the book was returned at A.
The transfer from A back to B is not permitted = so it should block.

If the patch actually does that, I cannot tell!
Comment 91 Martin Renvoize 2021-02-08 14:21:36 UTC
The patch does the oposite
Comment 92 Arthur Suzuki 2021-02-17 02:41:16 UTC
Created attachment 116927 [details] [review]
Bug 7376: (follow-up) Make extended tests mock the actual desired behaviour
Comment 93 Arthur Suzuki 2021-02-17 02:41:34 UTC
Created attachment 116928 [details] [review]
Bug 7376: (follow-up) Fix C4/Circulation.pm

This follow-up patch actually fixes the issue.
This patch forbids a patron to return a book to a library
from where the book may not return to its home or holding library.
Comment 94 Martin Renvoize 2021-02-17 14:44:59 UTC
Created attachment 116937 [details] [review]
Bug 7376: Clarify tests further

We want to prevent returns where the item cannot be transfered 'back to
where it came from (be that Homebranch or Holdingbranch as defined
respectively in the circulation rules).

This patch clarifies the tests further by testing this transfer
functionality distinctly from the 'AllowReturnToBranch' preference
functionality.
Comment 95 Martin Renvoize 2021-02-17 14:45:04 UTC
Created attachment 116938 [details] [review]
Bug 7376: Allow passing 'transferbranch' to CanBookBeReturned

This patch adds the optional 'tranferbranch' parameter to the
CanBookBeReturned routine. This allows for passing the correct
'ReturnToX' transfer to be tested at return time for transfer limit
restrictions.
Comment 96 Katrin Fischer 2021-03-06 13:45:16 UTC
Testing this:

- Activate branch transfers
- Library Centerville DISALLOWS transfer to Fairview
- Library Fairview ALLOWS transfer to Centerville

Without previous checkout! Just doing a checkin.

Test A:
Checkin of Fairview item in Centerville
Transfer is allowed, correct.

Test B:
Checkin of Centerville item in Fairview.
Transfer is allowed - that wasn't the result I expected.

This is a Centerville item. Centerville hasn't permitted it's items to go to Fairview - so should the transfer not be blocked?


Setting up the branch transfers, it says:

Check the boxes for the libraries you allow _your_ items to be transferred to.

1) Should we adapt these to include that it will also influence where you items can be checked in at? (not blocker)

2) I read this as: items from this library (homebranch = Centerville in example above) are not to be transported the unchecked locations (Fairview in my example).

What am I missing? Maybe it's a matter of the GUI not being really clear about directions.
Comment 97 Katrin Fischer 2021-03-06 13:47:12 UTC
Hm, looking at the text from the release notes:

This patch introduces new controls to prevent a check-in at a library where transfer rules would forbid the item transfer back to this item's homebranch/holdingbranch library.

At first glance "homebranch" looks right to me, but "holdingbranch" does not. Can you explain?
Comment 98 Martin Renvoize 2021-03-06 15:58:37 UTC
It depends on your preferences/circulation rules. The rules are return to home or return to holding.  I can't actually remember how they are set.. but they do correspond with the transfer reason enums 'ReturnToHome' and 'ReturnToHolding'.
Comment 99 Katrin Fischer 2021-03-06 17:01:18 UTC
(In reply to Martin Renvoize from comment #98)
> It depends on your preferences/circulation rules. The rules are return to
> home or return to holding.  I can't actually remember how they are set.. but
> they do correspond with the transfer reason enums 'ReturnToHome' and
> 'ReturnToHolding'.

Ok, maybe we should add that. My items had both homebranch and holdingbranch set to the same location, so this should not affected the test case.
Comment 100 Martin Renvoize 2021-03-08 08:37:27 UTC
Hmm, are your items set to 'Float' ?.. that's the only case I can think of that isn't handled.
Comment 101 Katrin Fischer 2021-03-08 09:01:45 UTC
(In reply to Martin Renvoize from comment #100)
> Hmm, are your items set to 'Float' ?.. that's the only case I can think of
> that isn't handled.

No, they are not. If they were floating, they'd not trigger a transfer home.
Comment 102 pierre.genty 2021-05-12 14:46:31 UTC
Just tried, patch doesn't apply : 
git diff 

++<<<<<<< HEAD
 +use Test::More tests => 54;
++=======
+ use Test::More tests => 53;
++>>>>>>> Bug 7376: Unit test
Comment 103 Victor Grousset/tuxayo 2021-05-12 19:43:55 UTC
Created attachment 120897 [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)

Sponsored-by: National Library of Finland

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 104 Victor Grousset/tuxayo 2021-05-12 19:44:03 UTC
Created attachment 120898 [details] [review]
Bug 7376: Unit test
Comment 105 Victor Grousset/tuxayo 2021-05-12 19:44:12 UTC
Created attachment 120899 [details] [review]
Bug 7376: (follow-up) Extend test coverage and document

I'm not sure about the requirements for this bug. This patch extends the
tests to cover the new combinations introduced and highlights exactly
what changes in the logic.. which appears to be fairly little.
Comment 106 Victor Grousset/tuxayo 2021-05-12 19:44:20 UTC
Created attachment 120900 [details] [review]
Bug 7376: (follow-up) Make extended tests mock the actual desired behaviour
Comment 107 Victor Grousset/tuxayo 2021-05-12 19:44:27 UTC
Created attachment 120901 [details] [review]
Bug 7376: (follow-up) Fix C4/Circulation.pm

This follow-up patch actually fixes the issue.
This patch forbids a patron to return a book to a library
from where the book may not return to its home or holding library.
Comment 108 Victor Grousset/tuxayo 2021-05-12 19:44:35 UTC
Created attachment 120902 [details] [review]
Bug 7376: Clarify tests further

We want to prevent returns where the item cannot be transfered 'back to
where it came from (be that Homebranch or Holdingbranch as defined
respectively in the circulation rules).

This patch clarifies the tests further by testing this transfer
functionality distinctly from the 'AllowReturnToBranch' preference
functionality.
Comment 109 Victor Grousset/tuxayo 2021-05-12 19:44:45 UTC
Created attachment 120903 [details] [review]
Bug 7376: Allow passing 'transferbranch' to CanBookBeReturned

This patch adds the optional 'tranferbranch' parameter to the
CanBookBeReturned routine. This allows for passing the correct
'ReturnToX' transfer to be tested at return time for transfer limit
restrictions.
Comment 110 Victor Grousset/tuxayo 2021-05-12 19:49:29 UTC
Conflict fixed. It can be tested now.

Parallel question: are the calls to diag() temporary or they have a long term use?
Comment 111 Martin Renvoize 2021-05-13 07:11:45 UTC
(In reply to Victor Grousset/tuxayo from comment #110)
> Conflict fixed. It can be tested now.
> 
> Parallel question: are the calls to diag() temporary or they have a long
> term use?

The diag() are there deliberately.. they're just to clarify the test output for future people working in this area.

I've not got time to revisit this one this cycle.. I was hoping Arthur would chip in and test the final solution so clarify if we're doing what was intended.
Comment 112 Julian Maurice 2022-12-06 10:13:10 UTC
(In reply to Katrin Fischer from comment #96)
> Testing this:
> 
> - Activate branch transfers
> - Library Centerville DISALLOWS transfer to Fairview
> - Library Fairview ALLOWS transfer to Centerville
> 
> Without previous checkout! Just doing a checkin.
> 
> Test A:
> Checkin of Fairview item in Centerville
> Transfer is allowed, correct.

Returning a Fairview item in Centerville should trigger a transfer from Centerville to Fairview, right ? That transfer is forbidden. I don't understand why you expect the transfer to be allowed in that case.

> 
> Test B:
> Checkin of Centerville item in Fairview.
> Transfer is allowed - that wasn't the result I expected.

This makes sense to me, the transfer from Fairview to Centerville is allowed

> 
> This is a Centerville item. Centerville hasn't permitted it's items to go to
> Fairview - so should the transfer not be blocked?

I think there is a little bit of confusion over two different things:
1) Is it allowed to return item I to library L ? (which is controlled by syspref AllowReturnToBranch)
2) Is item I allowed to be transferred from library L1 to library L2 ? (controlled by transfer limits and related sysprefs)

It looks like you are testing 1 whereas the patch is about 2.

It worked fine for me. Here's what I did:

1. Set syspref AllowReturnToBranch = "to any library"
2. Set syspref UseBranchTransferLimits = "Enforce"
3. Set syspref BranchTransferLimitsType = "item type"
4. Create an item (itemtype BOOK, homebranch Centerville, holdingbranch Centerville)
5. Disable all transfers for item type BOOK
6. Set library to Fairfield
7. Checkin the item => checkin is blocked (message: "This item must be checked in at following library: Centerville)
   On master, the checkin is allowed and it starts a transfer from Fairfield to Centerville, which is forbidden)
8. If I enable the transfer from Fairfield to Centerville for BOOK, the checkin is allowed.
Comment 113 Julian Maurice 2022-12-06 10:16:11 UTC
Created attachment 144435 [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)

Sponsored-by: National Library of Finland

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 114 Julian Maurice 2022-12-06 10:16:20 UTC
Created attachment 144436 [details] [review]
Bug 7376: Unit test

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 115 Julian Maurice 2022-12-06 10:16:28 UTC
Created attachment 144437 [details] [review]
Bug 7376: (follow-up) Extend test coverage and document

I'm not sure about the requirements for this bug. This patch extends the
tests to cover the new combinations introduced and highlights exactly
what changes in the logic.. which appears to be fairly little.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 116 Julian Maurice 2022-12-06 10:16:36 UTC
Created attachment 144438 [details] [review]
Bug 7376: (follow-up) Make extended tests mock the actual desired behaviour

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 117 Julian Maurice 2022-12-06 10:16:44 UTC
Created attachment 144439 [details] [review]
Bug 7376: (follow-up) Fix C4/Circulation.pm

This follow-up patch actually fixes the issue.
This patch forbids a patron to return a book to a library
from where the book may not return to its home or holding library.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 118 Julian Maurice 2022-12-06 10:16:52 UTC
Created attachment 144440 [details] [review]
Bug 7376: Clarify tests further

We want to prevent returns where the item cannot be transfered 'back to
where it came from (be that Homebranch or Holdingbranch as defined
respectively in the circulation rules).

This patch clarifies the tests further by testing this transfer
functionality distinctly from the 'AllowReturnToBranch' preference
functionality.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 119 Julian Maurice 2022-12-06 10:17:01 UTC
Created attachment 144441 [details] [review]
Bug 7376: Allow passing 'transferbranch' to CanBookBeReturned

This patch adds the optional 'tranferbranch' parameter to the
CanBookBeReturned routine. This allows for passing the correct
'ReturnToX' transfer to be tested at return time for transfer limit
restrictions.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 120 Marcel de Rooy 2023-01-27 09:54:06 UTC
Looking here now
Comment 121 Marcel de Rooy 2023-01-27 10:17:10 UTC
In the course of time, both module and tests have been modified quite a bit over these patches. I feel that it needs a squash. Do you agree?
Comment 122 Marcel de Rooy 2023-01-27 10:24:12 UTC
($returnallowed, $message) = CanBookBeReturned($item, $branch, [$transferbranch])
=> Please explain transferbranch parameter in POD.

        my $to_library   = Koha::Libraries->find($transferbranch);
        if ( !$item->can_be_transferred({ from => $from_library, to => $to_library }) ) {
            $allowed = 0;
            $message = $transferbranch;
        }
$message looks a bit odd. You can't transfer to transfer here. And you are suggesting it actually in message?
Comment 123 Marcel de Rooy 2023-01-27 10:40:07 UTC
    # identify cases where transfer rules prohibit return

Please clarify this comment a bit further. It seems to be quite crucial here.
During the development it looks like both directions were examined, later on this was reverted again? Why is the check as we do now?
Comment 124 Marcel de Rooy 2023-01-27 10:41:29 UTC
    my $hbr = Koha::CirculationRules->get_return_branch_policy($item);
        # get the proper branch to which to return the item
    my $returnbranch = $hbr ne 'noreturn' ? $item->$hbr : $branch;
        # if $hbr was "noreturn" or any other non-item table value, then it should 'float' (i.e. stay at this branch)

[...]

    # check if the return is allowed at this branch
    my ($returnallowed, $message) = CanBookBeReturned($item, $branch, $returnbranch);

=> Looks like we should not pass $returnbranch if it equals $branch, right?
Comment 125 Marcel de Rooy 2023-01-27 10:52:13 UTC
Would it be useful to include a test case for:
get_return_branch_policy
  noreturn: do not return, let item remain where checked in (floating collections)
?
Comment 126 Marcel de Rooy 2023-01-27 10:57:23 UTC
I am moving this to FQA for a few minor remarks as noted.
But what I do miss here, is a test plan that provides more confidence that returning books from the interface will still work as expected in various cases.
A lot of attention went to the unit test. And thats great, but if a change like this is not tested thoroughly now on the interface, we might overlook stuff..
In that regard an additional outside sign-off would be welcome.
Comment 127 Fridolin Somers 2024-04-18 08:06:41 UTC
Linked to Bug 33075  ?