|
Description
Chris Hall
2012-04-19 00:10:24 UTC
Seems to me HomeOrHoldingBranchReturn SysPref is a residue or at least not any more in use. Circulation rules in 'Default checkout, hold and return policy' seems to be the preferred setting. At least they are honored in C4::Circulation. This also affects SIP transactions as returns gets no return branch. I will make a small patch to remove syspref HomeOrHoldingBranchReturn and make C4::Circulation::AddReturn return message about transfer branch on checkin. Actually, on second look, circ/returns.pl uses C4::Context->preference('HomeOrHoldingBranchReturn');
Others should probably decide, but there cannot be two conflicting return policies, and I believe C4::Circulation should be the preferred setting.
C4::GetBranchItemRule returns noreturn|holdingbranch|homebranch and defaults to homebranch if no default circulation rule is set.
Created attachment 26723 [details] [review] Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref This patch removes HomeOrHoldingBranchReturn syspref and makes circ/returns.pl respect branch circulation rules from C4::Circulation::GetBranchItemRule. Also transfer slip notice should reflect this. Default should always be to return item to home branch. Test plan: - make sure syspref 'AutomaticItemReturn' is set to 'false' - unset 'Default checkout, hold and return policy' or set 'Return policy' to 'Item returns home' - checkout an item and do a checkin from different branch than items homebranch - verify that you're prompted with a transfer message to item's home branch and that print slip matches - set 'Return policy' to 'Item returns to issuing library' - do a checkout and a checkin from branch different than item's home branch - verify that you're not prompted with a transfer message and that holding library is your current branch Created attachment 27487 [details] [review] [SIGNED-OFF] Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref This patch removes HomeOrHoldingBranchReturn syspref and makes circ/returns.pl respect branch circulation rules from C4::Circulation::GetBranchItemRule. Also transfer slip notice should reflect this. Default should always be to return item to home branch. Test plan: - make sure syspref 'AutomaticItemReturn' is set to 'false' - unset 'Default checkout, hold and return policy' or set 'Return policy' to 'Item returns home' - checkout an item and do a checkin from different branch than items homebranch - verify that you're prompted with a transfer message to item's home branch and that print slip matches - set 'Return policy' to 'Item returns to issuing library' - do a checkout and a checkin from branch different than item's home branch - verify that you're not prompted with a transfer message and that holding library is your current branch Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Comment on attachment 27487 [details] [review] [SIGNED-OFF] Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref Review of attachment 27487 [details] [review]: ----------------------------------------------------------------- Quick code review: - patch should also delete HomeOrHoldingBranchReturn from sysprefs.sql - I think the updatedatabase message could note that we remove HomeOrHoldingBranchReturn in favor of the circulation rules - Is there an existing test for the return policies? ... and now... sleep. Created attachment 27950 [details] [review] Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref Created attachment 27992 [details] [review] Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref This patch removes HomeOrHoldingBranchReturn syspref and makes circ/returns.pl respect branch circulation rules from C4::Circulation::GetBranchItemRule. Also transfer slip notice should reflect this. Default should always be to return item to home branch. Test plan: - make sure syspref 'AutomaticItemReturn' is set to 'false' - unset 'Default checkout, hold and return policy' or set 'Return policy' to 'Item returns home' - checkout an item and do a checkin from different branch than items homebranch - verify that you're prompted with a transfer message to item's home branch and that print slip matches - set 'Return policy' to 'Item returns to issuing library' - do a checkout and a checkin from branch different than item's home branch - verify that you're not prompted with a transfer message and that holding library is your current branch Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Follow-up: - Added 3 tests in t/db_dependent/Circulation_Branches.t to test AddReturn policies - Removed HomeOrHoldingBranchReturn from sysprefs.sql - Added notice on removing syspref in updatedatabase Darn... lost original commit message in previos push Comment on attachment 27992 [details] [review] Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref Review of attachment 27992 [details] [review]: ----------------------------------------------------------------- And some trailing spaces FAIL t/db_dependent/Circulation_Branch.t OK pod FAIL forbidden patterns forbidden pattern: trailing space char (line 319) forbidden pattern: trailing space char (line 311) forbidden pattern: trailing space char (line 323) forbidden pattern: trailing space char (line 327) forbidden pattern: trailing space char (line 315) Marked as Failed QA. ::: C4/Circulation.pm @@ +1976,3 @@ > $messages->{'WasTransfered'} = 1; > } else { > + $messages->{'NeedsTransfer'} = $returnbranch; Why don't you keep needstransfer = 1 ? You don't use this value later. ::: circ/returns.pl @@ +272,5 @@ > title => $biblio->{'title'}, > homebranch => $biblio->{'homebranch'}, > + homebranchname => GetBranchName( $biblio->{'homebranch'} ), > + returnbranch => $returnbranch, > + returnbranchname => GetBranchName( $returnbranch ), Please use the Branches TT plugin to get a branch name from its code. Created attachment 28199 [details] [review] Bug 7981 - Remove HomeOrHoldingBranchReturn syspref This patch removes HomeOrHoldingBranchReturn syspref and makes circ/returns.pl respect branch circulation rules from C4::Circulation::GetBranchItemRule. Also transfer slip notice should reflect this. Default should always be to return item to home branch. Test plan: - make sure syspref 'AutomaticItemReturn' is set to 'false' - unset 'Default checkout, hold and return policy' or set 'Return policy' to 'Item returns home' - checkout an item and do a checkin from different branch than items homebranch - verify that you're prompted with a transfer message to item's home branch and that print slip matches - set 'Return policy' to 'Item returns to issuing library' - do a checkout and a checkin from branch different than item's home branch - verify that you're not prompted with a transfer message and that holding library is your current branch Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Follow-up: - Added 3 tests in t/db_dependent/Circulation_Branches.t to test AddReturn policies - Removed HomeOrHoldingBranchReturn from sysprefs.sql - Added notice on removing syspref in updatedatabase QA edits: - removed trailing whitespace in tests - moved branchname lookup from returns.pl to template > Why don't you keep needstransfer = 1 ? > You don't use this value later. This is why it's blocking bug 12021. AddReturn has been modified to add transfer branch to $message. I don't see any better way to pass the transfer branch to the SIP controller. NeedsTransfer in returns.pl was modified accordingly to avvoid confusion only. Path doesnt apply against master also. The first like of the path should indicate what it does not reiterate the original bug report. Which if applied is irrelevant as the option gets removed. Hi Colin, Can you explain a bit what needs to be changed to move this forward? The patch applies for on master tonight. The subject line does not give an indication of what the patch does. That line should convey a summary of the change made by the patch. Its meant to give the user some indication in the git logs etc of what changes. The current line is not only uninformative but misleading in this resepect. I see the objections. Perhaps should it be moved to a new bug? The patch deletes HomeOrHoldingBranchReturn syspref and makes the logics of C4::Circulation::GetBranchItemRule decide return policy, which makes more sense than a syspref. Also the return message for transfers is fixed to allow SIP2 returns to alert branch on transfers. I have no experience in this som suggestins welcome! Hi Benjamin and Colin, maybe it's a misunderstanding here - from Colin's last comment I think what needs to be changes is not the code, but the documentation of it in the patch? (In reply to Katrin Fischer from comment #15) > Hi Benjamin and Colin, > maybe it's a misunderstanding here - from Colin's last comment I think what > needs to be changes is not the code, but the documentation of it in the > patch? Yes. The first line should be a succinct summary of what the patch changes. As such its useful for future developers and others viewing the history when merging, debugging or learning about the code Hi Colin, could you maybe suggest a possible wording? Maybe it will be clearer then. (In reply to Katrin Fischer from comment #17) > Hi Colin, could you maybe suggest a possible wording? Maybe it will be > clearer then. Make the Subject line Bug 7981 Remove HomeOrHoldingBranchReturn syspref The rest of the commit message explains the implications well enough. (In reply to Colin Campbell from comment #18) > (In reply to Katrin Fischer from comment #17) > > Hi Colin, could you maybe suggest a possible wording? Maybe it will be > > clearer then. > > Make the Subject line > > Bug 7981 Remove HomeOrHoldingBranchReturn syspref > > > The rest of the commit message explains the implications well enough. I'm quite happy with that, as it is what the patch basically does. I took the liberty of changing. The attachment should mayhaps be changed accordingly? For the record, the old subject line was: "Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref" Comment on attachment 28199 [details] [review] Bug 7981 - Remove HomeOrHoldingBranchReturn syspref Renamed subject to better match what the patch does Conflict CONFLICT (content): Merge conflict in C4/Circulation.pm Failed to merge in the changes. Created attachment 33455 [details] [review] Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref This patch removes HomeOrHoldingBranchReturn syspref and makes circ/returns.pl respect branch circulation rules from C4::Circulation::GetBranchItemRule. Also transfer slip notice should reflect this. Default should always be to return item to home branch. Test plan: - make sure syspref 'AutomaticItemReturn' is set to 'false' - unset 'Default checkout, hold and return policy' or set 'Return policy' to 'Item returns home' - checkout an item and do a checkin from different branch than items homebranch - verify that you're prompted with a transfer message to item's home branch and that print slip matches - set 'Return policy' to 'Item returns to issuing library' - do a checkout and a checkin from branch different than item's home branch - verify that you're not prompted with a transfer message and that holding library is your current branch Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Follow-up: - Added 3 tests in t/db_dependent/Circulation_Branches.t to test AddReturn policies - Removed HomeOrHoldingBranchReturn from sysprefs.sql - Added notice on removing syspref in updatedatabase QA edits: - removed trailing whitespace in tests - moved branchname lookup from returns.pl to template Resolved conflict in C4/Circulation.pm Comment on attachment 33455 [details] [review] Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref Obsoleted wrong patch. fixed now. I'm moving this back to assigned, because I noticed some rotating collections variable was lost. This might not reflect that changes made in bug 8836. Please check, and either change to needs sign off if the changes are reflected OR fix and then change to needs sign off. This was just a quick eyeball. Created attachment 36615 [details] [review] Bug 7981 - Remove HomeOrHoldingBranchReturn syspref This patch removes HomeOrHoldingBranchReturn syspref and makes circ/returns.pl respect branch circulation rules from C4::Circulation::GetBranchItemRule. Also transfer slip notice should reflect this. Default should always be to return item to home branch. Test plan: - make sure syspref 'AutomaticItemReturn' is set to 'false' - unset 'Default checkout, hold and return policy' or set 'Return policy' to 'Item returns home' - checkout an item and do a checkin from different branch than items homebranch - verify that you're prompted with a transfer message to item's home branch and that print slip matches - set 'Return policy' to 'Item returns to issuing library' - do a checkout and a checkin from branch different than item's home branch - verify that you're not prompted with a transfer message and that holding library is your current branch Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Follow-up: - Added 3 tests in t/db_dependent/Circulation_Branches.t to test AddReturn policies - Removed HomeOrHoldingBranchReturn from sysprefs.sql - Added notice on removing syspref in updatedatabase QA edits: - removed trailing whitespace in tests - moved branchname lookup from returns.pl to template HomeOrHoldingBranchReturn has been introduced by bug 3536. Created attachment 37724 [details] [review] Bug 7981 - Remove HomeOrHoldingBranchReturn syspref Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 37725 [details] [review] Bug 7981: Remove HomeOrHoldingBranchReturn syspref from C4::UsageStats Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 37726 [details] [review] Bug 7981: Use The Branches TT plugin instead of GetBranchName Note that homebranchname is never used in the template. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> This should be highlighted in the release notes. Some first notes - please fix! FAIL installer/data/mysql/updatedatabase.pl OK critic OK forbidden patterns OK pod FAIL valid Missing right curly or square bracket installer/data/mysql/updatedatabase.pl had compilation errors. syntax error OK t/db_dependent/Circulation_Branch.t OK critic OK forbidden patterns OK pod OK valid FAIL koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt FAIL forbidden patterns forbidden pattern: tab char (line 287) forbidden pattern: tab char (line 279) OK tt_valid OK valid_template OK installer/data/mysql/sysprefs.sql OK sysprefs_order Created attachment 38151 [details] [review] Bug 7981: Follow-up - Fix qa script complaints - Fix updatedatabase, moving the update entry into the right spot - Fix some tabs Created attachment 38174 [details] [review] Bug 7981 [QA Followup] - Rename TT vars Created attachment 38176 [details] [review] Bug 7981 - Remove HomeOrHoldingBranchReturn syspref This patch removes HomeOrHoldingBranchReturn syspref and makes circ/returns.pl respect branch circulation rules from C4::Circulation::GetBranchItemRule. Also transfer slip notice should reflect this. Default should always be to return item to home branch. Test plan: - make sure syspref 'AutomaticItemReturn' is set to 'false' - unset 'Default checkout, hold and return policy' or set 'Return policy' to 'Item returns home' - checkout an item and do a checkin from different branch than items homebranch - verify that you're prompted with a transfer message to item's home branch and that print slip matches - set 'Return policy' to 'Item returns to issuing library' - do a checkout and a checkin from branch different than item's home branch - verify that you're not prompted with a transfer message and that holding library is your current branch Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Follow-up: - Added 3 tests in t/db_dependent/Circulation_Branches.t to test AddReturn policies - Removed HomeOrHoldingBranchReturn from sysprefs.sql - Added notice on removing syspref in updatedatabase QA edits: - removed trailing whitespace in tests - moved branchname lookup from returns.pl to template Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 38177 [details] [review] [SIGNED-OFF] Bug 7981: Remove HomeOrHoldingBranchReturn syspref from C4::UsageStats Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38178 [details] [review] [SIGNED-OFF] Bug 7981: Use The Branches TT plugin instead of GetBranchName Note that homebranchname is never used in the template. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38179 [details] [review] [SIGNED-OFF] Bug 7981: Follow-up - Fix qa script complaints - Fix updatedatabase, moving the update entry into the right spot - Fix some tabs Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38180 [details] [review] [SIGNED-OFF] Bug 7981 [QA Followup] - Rename TT vars Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38237 [details] [review] Bug 7981: Follow-up - Fix qa script complaints - Fix updatedatabase, moving the update entry into the right spot - Fix some tabs Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38238 [details] [review] Bug 7981 [QA Followup] - Rename TT vars Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Last 2 patches fix: Applying: Bug 7981: Follow-up - Fix qa script complaints fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl). Created attachment 38241 [details] [review] Bug 7981 - Remove HomeOrHoldingBranchReturn syspref This patch removes HomeOrHoldingBranchReturn syspref and makes circ/returns.pl respect branch circulation rules from C4::Circulation::GetBranchItemRule. Also transfer slip notice should reflect this. Default should always be to return item to home branch. Test plan: - make sure syspref 'AutomaticItemReturn' is set to 'false' - unset 'Default checkout, hold and return policy' or set 'Return policy' to 'Item returns home' - checkout an item and do a checkin from different branch than items homebranch - verify that you're prompted with a transfer message to item's home branch and that print slip matches - set 'Return policy' to 'Item returns to issuing library' - do a checkout and a checkin from branch different than item's home branch - verify that you're not prompted with a transfer message and that holding library is your current branch Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Follow-up: - Added 3 tests in t/db_dependent/Circulation_Branches.t to test AddReturn policies - Removed HomeOrHoldingBranchReturn from sysprefs.sql - Added notice on removing syspref in updatedatabase QA edits: - removed trailing whitespace in tests - moved branchname lookup from returns.pl to template Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Created attachment 38242 [details] [review] Bug 7981: Remove HomeOrHoldingBranchReturn syspref from C4::UsageStats Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Created attachment 38243 [details] [review] Bug 7981: Use The Branches TT plugin instead of GetBranchName Note that homebranchname is never used in the template. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Created attachment 38244 [details] [review] Bug 7981: Follow-up - Fix qa script complaints - Fix updatedatabase, moving the update entry into the right spot - Fix some tabs Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Created attachment 38245 [details] [review] Bug 7981 [QA Followup] - Rename TT vars Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> From reading the code: + # do the return + ( $returned, $messages, $issueinformation, $borrower ) = + AddReturn( $barcode, $userenv_branch, $exemptfine, $dropboxmode, $return_date_override ); + I think we miss the dropboxdate here - Please check and fix. Small follow-up needed... someone? :) Created attachment 39504 [details] [review] Bug 7981: Reintroduce dropboxdate The parameter was lost in previous commit. Created attachment 39848 [details] [review] Bug 7981 - Remove HomeOrHoldingBranchReturn syspref This patch removes HomeOrHoldingBranchReturn syspref and makes circ/returns.pl respect branch circulation rules from C4::Circulation::GetBranchItemRule. Also transfer slip notice should reflect this. Default should always be to return item to home branch. Test plan: - make sure syspref 'AutomaticItemReturn' is set to 'false' - unset 'Default checkout, hold and return policy' or set 'Return policy' to 'Item returns home' - checkout an item and do a checkin from different branch than items homebranch - verify that you're prompted with a transfer message to item's home branch and that print slip matches - set 'Return policy' to 'Item returns to issuing library' - do a checkout and a checkin from branch different than item's home branch - verify that you're not prompted with a transfer message and that holding library is your current branch Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Follow-up: - Added 3 tests in t/db_dependent/Circulation_Branches.t to test AddReturn policies - Removed HomeOrHoldingBranchReturn from sysprefs.sql - Added notice on removing syspref in updatedatabase QA edits: - removed trailing whitespace in tests - moved branchname lookup from returns.pl to template Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Created attachment 39849 [details] [review] Bug 7981: Remove HomeOrHoldingBranchReturn syspref from C4::UsageStats Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Created attachment 39850 [details] [review] Bug 7981: Use The Branches TT plugin instead of GetBranchName Note that homebranchname is never used in the template. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Created attachment 39851 [details] [review] Bug 7981: Follow-up - Fix qa script complaints - Fix updatedatabase, moving the update entry into the right spot - Fix some tabs Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Created attachment 39852 [details] [review] Bug 7981 [QA Followup] - Rename TT vars Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Created attachment 39853 [details] [review] Bug 7981: Reintroduce dropboxdate The parameter was lost in previous commit. Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Patches pushed to master. Thanks Benjamin and pals! Comment on attachment 39848 [details] [review] Bug 7981 - Remove HomeOrHoldingBranchReturn syspref Review of attachment 39848 [details] [review]: ----------------------------------------------------------------- ::: circ/returns.pl @@ +265,5 @@ > ); > } > > + # make sure return branch respects home branch circulation rules, default to homebranch > + my $hbr = GetBranchItemRule($biblio->{'homebranch'}, $itemtype)->{'returnbranch'} || "homebranch"; I think there is a problem here. $itemtype is a hashref returned by C4::ItemType->get, but GetBranchItemRule expects a string. Well spotted, Jonathan! Since it is already pushed to master, could you apply a patch? My env is a bit off at the moment. (silly excuse ;) Maybe this is the reason for the strange fault in print slip on transfer on http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12043 (In reply to Benjamin Rokseth from comment #59) > Well spotted, Jonathan! Since it is already pushed to master, could you > apply a patch? My env is a bit off at the moment. (silly excuse ;) > > Maybe this is the reason for the strange fault in print slip on transfer on > http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12043 See bug 14842. (In reply to Jonathan Druart from comment #60) > > Maybe this is the reason for the strange fault in print slip on transfer on > > http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12043 I don't think so, 7981 is present in master only for 5 months. |