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.
sponsored by Lyon3 University.
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)
Created attachment 6954 [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)
(BibLibre MT8601)
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.
By the way, I tested this patch on top of the patches for Bug 6842 since otherwise I couldn't change library transfer limits.
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
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
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
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.
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.
It's been a while on this, but can we get an updated patch?
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?
Jonathan, if you can have a look to this patch
Created attachment 21715 [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)
Rebased patch.
Patch applied cleanly, go forth and signoff
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.
Created attachment 21805 [details] When failing UseBranchTransferLimits-check, don't display the "Thanks for returning this book"-message.
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.
(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?
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.
I follow the Test Plan, it's Ok. But the librarian can't override, it should be better.
Agreed to fix this bug for Joubu, in exchange for QA stuff.
sponsored by Joensuu Regional Library
Created attachment 22016 [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 CheckBranchTransferAllowed() call IsBranchTransferAllowed(). *Made a unit test library out of CheckBranchTransferAllowed.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
Created attachment 22017 [details] Manual test cases
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 !
Created attachment 22323 [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
"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.
Created attachment 22357 [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
(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.
Created attachment 22696 [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
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
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
Be care, AddReserve prototype changed by bug 9809 (contrainst param removed).
Still a critical bug? - It was reported 2011-12-22.
Does not apply. Is this bug still valid?
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?
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)
Original patch rebased.
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.
(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 :(
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)
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>
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 ;)
Please add unit tests
(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.
(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 ?
(In reply to Julian Maurice from comment #49) > Are you asking for a 2nd signoff ? Yes.
(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.
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...
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?
(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?
Moving to new bug that encompasses multiple old bugs. *** This bug has been marked as a duplicate of bug 22330 ***
Hi, are you sure that bug 22330 encompasses this one? Because it(22330) seems to concert only holds.
*** Bug 23505 has been marked as a duplicate of this bug. ***
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
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
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.
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.
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
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.
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
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
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)
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
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
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
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
(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.
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
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
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 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.
(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 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
last patch still doesn't apply anyway...
Too late for 20.05 I feel.. updating target to 20.11
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>
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>
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>
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.
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>
Created attachment 115370 [details] [review] Bug 7376: Unit test
I did a rebase of the patches + added unit tests
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.
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.
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.
> 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!
The patch does the oposite
Created attachment 116927 [details] [review] Bug 7376: (follow-up) Make extended tests mock the actual desired behaviour
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.
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.
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.
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.
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?
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'.
(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.
Hmm, are your items set to 'Float' ?.. that's the only case I can think of that isn't handled.
(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.
Just tried, patch doesn't apply : git diff ++<<<<<<< HEAD +use Test::More tests => 54; ++======= + use Test::More tests => 53; ++>>>>>>> Bug 7376: Unit test
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>
Created attachment 120898 [details] [review] Bug 7376: Unit test
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.
Created attachment 120900 [details] [review] Bug 7376: (follow-up) Make extended tests mock the actual desired behaviour
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.
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.
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.
Conflict fixed. It can be tested now. Parallel question: are the calls to diag() temporary or they have a long term use?
(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.
(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.
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>
Created attachment 144436 [details] [review] Bug 7376: Unit test Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
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>
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>
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>
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>
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>
Looking here now
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?
($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?
# 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?
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?
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) ?
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.
Linked to Bug 33075 ?