Bug 7981 - Remove HomeOrHoldingBranchReturn syspref
Summary: Remove HomeOrHoldingBranchReturn syspref
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Benjamin Rokseth
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 14842
  Show dependency treegraph
 
Reported: 2012-04-19 00:10 UTC by Chris Hall
Modified: 2016-06-21 21:40 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref (12.94 KB, patch)
2014-04-01 10:15 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref (13.01 KB, patch)
2014-04-23 14:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref (21.84 KB, patch)
2014-05-06 01:56 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref (21.45 KB, patch)
2014-05-06 17:22 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 7981 - Remove HomeOrHoldingBranchReturn syspref (21.50 KB, patch)
2014-05-13 08:26 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref (6.42 KB, patch)
2014-11-11 12:38 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 7981 - Remove HomeOrHoldingBranchReturn syspref (21.49 KB, patch)
2015-03-05 15:14 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 7981 - Remove HomeOrHoldingBranchReturn syspref (21.55 KB, patch)
2015-04-13 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7981: Remove HomeOrHoldingBranchReturn syspref from C4::UsageStats (757 bytes, patch)
2015-04-13 11:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7981: Use The Branches TT plugin instead of GetBranchName (3.10 KB, patch)
2015-04-13 11:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7981: Follow-up - Fix qa script complaints (3.30 KB, patch)
2015-04-20 05:26 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7981 [QA Followup] - Rename TT vars (2.33 KB, patch)
2015-04-20 10:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7981 - Remove HomeOrHoldingBranchReturn syspref (21.55 KB, patch)
2015-04-20 11:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7981: Remove HomeOrHoldingBranchReturn syspref from C4::UsageStats (848 bytes, patch)
2015-04-20 11:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7981: Use The Branches TT plugin instead of GetBranchName (3.17 KB, patch)
2015-04-20 11:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7981: Follow-up - Fix qa script complaints (3.36 KB, patch)
2015-04-20 11:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7981 [QA Followup] - Rename TT vars (2.40 KB, patch)
2015-04-20 11:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7981: Follow-up - Fix qa script complaints (3.35 KB, patch)
2015-04-21 09:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7981 [QA Followup] - Rename TT vars (2.39 KB, patch)
2015-04-21 09:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7981 - Remove HomeOrHoldingBranchReturn syspref (21.61 KB, patch)
2015-04-21 13:15 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 7981: Remove HomeOrHoldingBranchReturn syspref from C4::UsageStats (875 bytes, patch)
2015-04-21 13:15 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 7981: Use The Branches TT plugin instead of GetBranchName (3.22 KB, patch)
2015-04-21 13:15 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 7981: Follow-up - Fix qa script complaints (3.41 KB, patch)
2015-04-21 13:15 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 7981 [QA Followup] - Rename TT vars (2.45 KB, patch)
2015-04-21 13:15 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 7981: Reintroduce dropboxdate (920 bytes, patch)
2015-05-26 09:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7981 - Remove HomeOrHoldingBranchReturn syspref (21.67 KB, patch)
2015-06-04 11:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7981: Remove HomeOrHoldingBranchReturn syspref from C4::UsageStats (938 bytes, patch)
2015-06-04 11:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7981: Use The Branches TT plugin instead of GetBranchName (3.28 KB, patch)
2015-06-04 11:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7981: Follow-up - Fix qa script complaints (3.47 KB, patch)
2015-06-04 11:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7981 [QA Followup] - Rename TT vars (2.51 KB, patch)
2015-06-04 11:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7981: Reintroduce dropboxdate (989 bytes, patch)
2015-06-04 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Hall 2012-04-19 00:10:24 UTC
When returning a book it is said to require a transfer if the home branch is not the same as the current branch (even if HomeOrHoldingBranchReturn specifies it should be returned to the holding branch)

Later on a message is generated in returns.pl if the first test was true, and this message respects HomeOrHoldingBranchReturn and will put the correct branch name into the message.
Comment 1 Benjamin Rokseth 2014-03-31 11:46:44 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.
Comment 2 Benjamin Rokseth 2014-04-01 09:07:14 UTC
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.
Comment 3 Benjamin Rokseth 2014-04-01 10:15:15 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2014-04-23 14:15:01 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2014-04-27 21:27:04 UTC
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.
Comment 6 Benjamin Rokseth 2014-05-06 01:56:33 UTC Comment hidden (obsolete)
Comment 7 Benjamin Rokseth 2014-05-06 17:22:57 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-05-12 15:11:53 UTC
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.
Comment 9 Benjamin Rokseth 2014-05-13 08:26:14 UTC Comment hidden (obsolete)
Comment 10 Benjamin Rokseth 2014-05-13 08:33:10 UTC
> 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.
Comment 11 Colin Campbell 2014-08-26 11:05:51 UTC
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.
Comment 12 Katrin Fischer 2014-09-14 19:58:43 UTC
Hi Colin,
Can you explain a bit what needs to be changed to move this forward? The patch applies for on master tonight.
Comment 13 Colin Campbell 2014-09-16 14:29:33 UTC
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.
Comment 14 Benjamin Rokseth 2014-09-16 20:19:05 UTC
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!
Comment 15 Katrin Fischer 2014-09-17 07:21:43 UTC
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?
Comment 16 Colin Campbell 2014-09-17 14:38:52 UTC
(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
Comment 17 Katrin Fischer 2014-09-17 14:59:10 UTC
Hi Colin, could you maybe suggest a possible wording? Maybe it will be clearer then.
Comment 18 Colin Campbell 2014-09-17 15:11:16 UTC
(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.
Comment 19 Benjamin Rokseth 2014-09-17 19:31:48 UTC
(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 20 Benjamin Rokseth 2014-09-17 19:34:59 UTC
Comment on attachment 28199 [details] [review]
Bug 7981 - Remove HomeOrHoldingBranchReturn syspref

Renamed subject to better match what the patch does
Comment 21 Chris Cormack 2014-11-11 08:43:45 UTC
Conflict

CONFLICT (content): Merge conflict in C4/Circulation.pm
Failed to merge in the changes.
Comment 22 Benjamin Rokseth 2014-11-11 12:38:04 UTC Comment hidden (obsolete)
Comment 23 Benjamin Rokseth 2014-11-11 12:39:51 UTC
Resolved conflict in C4/Circulation.pm
Comment 24 Benjamin Rokseth 2014-11-11 16:41:46 UTC
Comment on attachment 33455 [details] [review]
Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref

Obsoleted wrong patch. fixed now.
Comment 25 M. Tompsett 2014-12-16 06:48:13 UTC
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.
Comment 26 Benjamin Rokseth 2015-03-05 15:14:18 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2015-04-13 10:58:51 UTC
HomeOrHoldingBranchReturn has been introduced by bug 3536.
Comment 28 Jonathan Druart 2015-04-13 11:23:22 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2015-04-13 11:24:29 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2015-04-13 11:24:35 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2015-04-13 11:25:47 UTC
This should be highlighted in the release notes.
Comment 32 Katrin Fischer 2015-04-19 22:37:00 UTC
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
Comment 33 Katrin Fischer 2015-04-20 05:26:34 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2015-04-20 10:43:49 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2015-04-20 11:47:53 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2015-04-20 11:48:01 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2015-04-20 11:48:04 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2015-04-20 11:48:08 UTC Comment hidden (obsolete)
Comment 39 Kyle M Hall 2015-04-20 11:48:11 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2015-04-21 09:58:26 UTC Comment hidden (obsolete)
Comment 41 Jonathan Druart 2015-04-21 09:58:33 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2015-04-21 09:59:30 UTC
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).
Comment 43 Mirko Tietgen 2015-04-21 13:15:31 UTC Comment hidden (obsolete)
Comment 44 Mirko Tietgen 2015-04-21 13:15:40 UTC Comment hidden (obsolete)
Comment 45 Mirko Tietgen 2015-04-21 13:15:45 UTC Comment hidden (obsolete)
Comment 46 Mirko Tietgen 2015-04-21 13:15:51 UTC Comment hidden (obsolete)
Comment 47 Mirko Tietgen 2015-04-21 13:15:57 UTC Comment hidden (obsolete)
Comment 48 Katrin Fischer 2015-04-22 22:27:10 UTC
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.
Comment 49 Katrin Fischer 2015-05-24 21:57:20 UTC
Small follow-up needed... someone? :)
Comment 50 Jonathan Druart 2015-05-26 09:56:46 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2015-06-04 11:36:31 UTC
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>
Comment 52 Jonathan Druart 2015-06-04 11:36:39 UTC
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>
Comment 53 Jonathan Druart 2015-06-04 11:36:46 UTC
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>
Comment 54 Jonathan Druart 2015-06-04 11:36:53 UTC
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>
Comment 55 Jonathan Druart 2015-06-04 11:36:58 UTC
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>
Comment 56 Jonathan Druart 2015-06-04 11:37:03 UTC
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>
Comment 57 Tomás Cohen Arazi 2015-06-11 14:22:07 UTC
Patches pushed to master.

Thanks Benjamin and pals!
Comment 58 Jonathan Druart 2015-09-15 15:52:51 UTC
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.
Comment 59 Benjamin Rokseth 2015-09-17 07:14:51 UTC
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
Comment 60 Jonathan Druart 2015-09-17 08:08:25 UTC
(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.
Comment 61 Jonathan Druart 2015-09-17 08:09:05 UTC
(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.