Bug 7144

Summary: Floating collection
Product: Koha Reporter: Srdjan Jankovic <srdjan>
Component: CirculationAssignee: Srdjan Jankovic <srdjan>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: enhancement    
Priority: P1 - high CC: brendan, chris, dcook, gmcharlt, katrin.fischer, koha.sekjal, kyle.m.hall, magnus, melia, paul.poulain, wizzyrea
Version: 3.8   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 7830, 14422    
Attachments: patch
bug_7144: renamed HomeOrHoldingBranchReturn to returnBranch and added returningbranch option for floating collections
Enhancement 7144: Floating Collections (per branch/itemtype)
Revised Patch
[Signed Off] Enhancement 7144: Floating Collections (per branch/itemtype)
Bug 7144 follow-up cleaning for HTML4 coding guideline
[Signed Off] Bug 7144 follow-up cleaning for HTML4 coding guideline

Description Srdjan Jankovic 2011-11-03 02:57:17 UTC
Make a system preference called 'floating collection' if you turn this on, then only reserves will cause a transfer, not just returning an item at a branch other than it's homebranch

Work to do:
* Add a system preference 'FloatingCollection'
* If this is enable, items only prompt to be transferred if they could fill a reserve there
* Otherwise they stay at their current branch

How to test
* Switch system preference on
* Return some items at a branch other than its home branch, you should get no prompt to transfer
* Return one that has a reserve at another branch, should get a prompt to transfer.
Comment 1 Srdjan Jankovic 2011-11-04 05:37:19 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2011-12-01 02:06:58 UTC Comment hidden (obsolete)
Comment 3 Ian Walls 2011-12-01 15:04:14 UTC
This is working a global, system preference level.  It would be better if the mechanism to determine whether an item "floats" or not occurred at the itemtype/homebranch level in the circ rules.  Often, any given library will have it's Special Stuff that it will lend, but always want returned home.
Comment 4 Chris Cormack 2011-12-01 15:44:59 UTC
The library who commissioned it, specifically want it at a global level.
But accepting it doesn't mean an itemtype level one can't be created in the future.
Comment 5 Ian Walls 2011-12-12 14:44:05 UTC
This can be done at a global level in the default_circ_rules table; it if's also then added to  branch_item_rules, default_branch_circ_rules and default_branch_item_rules, then it can be controlled on a per branch and item type basis, as well.  The preference can piggyback on the GetBranchItemRule subroutine.
Comment 6 Chris Cormack 2011-12-12 17:00:26 UTC
I feel you are scope creeping the bug, outside of the clients requirements. As such you are welcome to send a counter patch.

But I do not think that it is harmful to accept this patch and extend it later. It will change no existing behaviour without turning the systempreference on. And will work for libraries who want it to work this way.
Comment 7 Ian Walls 2011-12-12 17:30:42 UTC
Chris,

I suppose I should have been more explicit; I've also got a couple partner libraries that need this, but on a per itemtype basis.  Hence my interest in the generalization.  A patch is forthcoming that should meet both our needs, and we can hopefully soon get rid of HomeOrHoldingBranchReturn entirely.
Comment 8 Ian Walls 2011-12-20 20:04:23 UTC Comment hidden (obsolete)
Comment 9 Ian Walls 2012-01-03 20:12:07 UTC Comment hidden (obsolete)
Comment 10 Magnus Enger 2012-01-06 11:03:31 UTC
What's the relationship between these two patches? Should the one from Ian be applied on top of the one from Chris, or are they independent?
Comment 11 Ian Walls 2012-01-06 13:53:35 UTC
Magnus,

The two patches are independent, alternate implementations.  Chris's works at a global level, while mine works at the branch/itemtype level as well.
Comment 12 Magnus Enger 2012-01-06 14:43:12 UTC
Sorry for being slow... Are they mutually exclusive (competing solutions for the same problem) or could they co-exist?
Comment 13 Ian Walls 2012-01-06 15:16:55 UTC
I believe mutually exclusive; my version should cover all the same end functionality as Chris's, just using a different method (circ rules instead of syspref).  I'm pretty sure you'd get a nasty merge conflict if you tried to apply both.
Comment 14 Paul Poulain 2012-01-24 09:42:44 UTC
Comment on attachment 6482 [details] [review]
bug_7144: renamed HomeOrHoldingBranchReturn to returnBranch and added returningbranch option for floating collections

Obsoleting chris version : the feature will be more usefull at itemtype level, so we should test & work on integrating this one.

(PS: and it's a damn usefull feature !)
Comment 15 Liz Rea 2012-02-07 16:35:18 UTC
for those doing testing - this patch uses an atomicupdate - don't forget to run it (like I did. :))
Comment 16 Liz Rea 2012-02-07 22:31:50 UTC
Notes:

Functionality:
* seems to be working, but still testing.

Documentation:
* floating collection settings need to be added to the help descriptions on the page.
* libraries wanting to enforce a floating collection for a specific item type across an entire set of libraries will need to define that rule in all library definitions - fixing that issue is outside the scope of this dev but it will need to be documented.
Comment 17 Liz Rea 2012-02-09 15:42:49 UTC
Created attachment 7533 [details] [review]
[Signed Off] Enhancement 7144: Floating Collections (per branch/itemtype)

Adds support for Floating Collections (i.e. items that don't automatically return
home when checked in at another branch) on a per branchcode/itemtype basis.

This patch adds a new column (returnbranch) to the default_circ_rules, default_branch_item_rules,
default_branch_circ_rules and branch_item_rules tables, after the 'holdsallowed' column.  While
this is coded as a varchar(15), the only currently supported values are 'homebranch', 'holdingbranch',
'noreturn' and NULL.

On upgrade, the value of HomeOrHoldingBranchReturn is used to populate the global default (which is
stored in default_circ_rules.returnbranch).

To access this value, use C4::Circulation::GetBranchItemRule.  This subroutine is altered to supply
an additional key, "returnbranch", containing this value (or 'homebranch' as a default).  No existing
usage of GetBranchItemRule should need to be modified.

The use of HomeOrHoldingBranchReturn is removed in AddReturn to instead use this subroutine.  This will
determine, on a more granular level, where the item should be transferred, after all is said and done.  If
'noreturn' is specified, then the material will remain at the branch doing the checking in.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Passes prove t xt t/db_dependent

I was able to make this feature work as advertised.
As noted above, if you want a floating rule applied across all branches, adding a single default rule won't suffice, you'll need to add the rule to all branches. That issue is not related to the functioning of *this* patch however.
Comment 18 Jonathan Druart 2012-02-17 15:43:27 UTC
QA comments: 

Code clean with comments.

Note for RM: don't forget to create an entry in the updatedatabase file :-)

Marked as passed QA.
Comment 19 Paul Poulain 2012-02-20 15:02:45 UTC
(In reply to comment #16)
> Documentation:
> * floating collection settings need to be added to the help descriptions on the
> page.
You're perfectly right.

And I'd like to have more details about how to test this feature.
Here is my thought:
* I must have a branch called "Floating Collection" (FC, or anything else)
* "floating" items have homebranch=FC
* in circ rules, for branch LIB (my main branch), i'll set:
 - homebranch = if I want the item to be returned to his home branch
 - holdingbranch = if I want the item to be returned to his holdingbranch (in case of a book homebranch=BranchA, checked out at= BranchB, checked in at = BranchC)
 - no return = the book stays where it has been returned

Some questions:
* what does NULL mean ? in the circ rules ?
* which combinations of branch_transfer_limit have you tested ? (was it ON, OFF, which branch transfers are permitted/forbidden ?)


I don't change the bug status, I just feel I just need a few more informations.
Comment 20 Brendan Gallagher 2012-02-23 00:25:52 UTC
(In reply to comment #19)
> (In reply to comment #16)
> > Documentation:
> > * floating collection settings need to be added to the help descriptions on the
> > page.
> You're perfectly right.
> 
> And I'd like to have more details about how to test this feature.
> Here is my thought:
> * I must have a branch called "Floating Collection" (FC, or anything else)
> * "floating" items have homebranch=FC
> * in circ rules, for branch LIB (my main branch), i'll set:
>  - homebranch = if I want the item to be returned to his home branch
>  - holdingbranch = if I want the item to be returned to his holdingbranch (in
> case of a book homebranch=BranchA, checked out at= BranchB, checked in at =
> BranchC)
>  - no return = the book stays where it has been returned
> 
> Some questions:
> * what does NULL mean ? in the circ rules ?
> * which combinations of branch_transfer_limit have you tested ? (was it ON,
> OFF, which branch transfers are permitted/forbidden ?)
> 
> 
> I don't change the bug status, I just feel I just need a few more informations.

I'm not sure what these questions mean - perhaps Ian can answer these questions.  Passing QA is a great sign and the testing has already been completed on production servers - so Paul I know that it should be good and great to pass into master.  So I'll ping Ian too see if he can answer these - but I'll tell you it's good :)
Comment 21 Ian Walls 2012-02-23 21:33:43 UTC
Paul,


You don't need any special configured branches to use this feature.  All it does is move the AutomaticItemReturn system preference to a branch/itemtype level.  This allows you to specify whether certain kinds of materials (like DVDs) return to either the homebranch or holdingbranch, or, for cheap paperbacks not worth the transportation costs, if they "float" (which means remain at the branch at which they were returned).

A NULL value in the returnbranch field of the circ rules, or in fact any value that's not "homebranch" or "holdingbranch", will result in the item floating.  We hardcode "noreturn" in the system preferences to be explicit, but if somewhere we to manually edit the data or some error were to creep in, that's what the system would default to.

This code interacts with Branch Transfer Limits the exact same way that AutomaticItemReturn did; again, this just moves the logic from a global syspref to a branch/itemtype configurable rule.
Comment 22 Paul Poulain 2012-02-27 13:17:23 UTC
QA comment (continued)

* the [HTML4] coding guideline about upercase was not respected, attaching a follow-up fixing that, please counter-signoff welcomed
* I think the smart-rules documentation need a little update:
In the last section, it's written:

Holds policy by item type for Branch 1

For this library, you can edit rules for given itemtypes, regardless of the patron's category.

Currently, this means hold policies. The various policies have the following effects:

    From any library: Patrons from any library may put this item on hold. (default if none is defined)
    From home library: Only patrons from the item's home library may put this book on hold.
    No holds allowed: No patron may put this book on hold.

Note that if the system preference AllowHoldPolicyOverride is enabled, these policies can be overridden by your circulation staff. Also, these policies are based on the patron's home library, not the library where the hold is being placed.. 

There's not reference to return policy.

It's worth adding some documentation for it.
And, more specifically, how it interfere with the 
"Default checkout, checkin and hold policy for Branch 1"
which one is used first if both are specified ?

Note that in my follow-up, there's also a small addition to the documentation of the default rules section (the 2nd one, just after the main rules)

-    <h3>Default checkout and hold policy for [% IF ( humanbranch ) %][% humanbranch %][% ELSE %]all libraries[% END %]</h3>
-        <p>You can set a default maximum number of checkouts and hold policy that will be used if none is defined below for a particular item type or category.</p>
+    <h3>Default checkout, checkin and hold policy for [% IF ( humanbranch ) %][% humanbranch %][% ELSE %]all libraries[% END %]</h3>
+        <p>You can set a default maximum number of checkouts, hold policy and checkin policy that will be used if none is defined below for a particular item type or category.</p>


Please:
* signoff my follow-up
* add a small follow-up to document the default itemtype checkin policy & how it interfere with the default one

(switch back to passed QA when it's done)
Comment 23 Paul Poulain 2012-02-27 13:17:42 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2012-03-07 15:09:10 UTC
Created attachment 8056 [details] [review]
[Signed Off] Bug 7144 follow-up cleaning for HTML4 coding guideline

http://wiki.koha-community.org/wiki/Coding_Guidelines#Upper_and_Lower_cases_in_strings
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Melia Meggs 2012-03-09 02:09:22 UTC
Just switching status back to "Passed QA" based on Paul's note from 2/27.
Comment 26 Kyle M Hall 2012-03-13 14:25:04 UTC
Maybe I'm a bit late in asking this, but why use itemtype for the floating collection rules instead of collection code? It seems odd to require a separate item type for floating items that are really Books, Audiobooks, etc.

For example, at the Crawford County Federated Library System, there are a number of 'rotating collections'. The books and audiobooks move from from one library to the next each month. 

I have written a rotating collections feature for them that lets the librarian's create 'collections' of items, and set what branch the collection is currently at. Koha currently tracks who owns and item, and where an item is at, but not where an item should be. For example, Collection1 is at LibraryA, and is now to be moved to LibraryB. It my system, a librarian simply goes into the rotating collections system and changes the current library for Collection1 from LibraryA to LibraryB. LibraryA then packs up all the items that are currently on the shelf and ships them to LibraryB. When any books in Collection1 that are currently circulating are checked in, the librarian recieves a notice that the item is part of a rotating collection and should be transferred to LibraryB immediately.

Could this feature be used to replicate this scenario? If not, I would be more than happy to open a bug and submit my rotating collections code. I suppose rotating collections are different than floating collections in how they behave.
Comment 27 Ian Walls 2012-03-13 18:00:15 UTC
This code works off of item type because item types are the "What" parameter in circulation rules ("where" being branch, and "who" being patron category").  Collection codes to not factor into circulation rules, and I don't believe they should.  If you need materials to have a different set of behaviours, assign them different item types.  If that set of types isn't what you'd want your users to search on, that's okay, Collection Code can be used for searching instead.

Rotating Collections wouldn't be well covered by this code; as you say, we need to track not only who owns a material and where it currently is, but where it should be.  If you abstract this a little further, we could have "temporary" item information of any kind.  This would support Course Reserves, in that materials could have their item types and shelving locations changed for the term, then reverted back.
Comment 28 Paul Poulain 2012-03-21 10:06:39 UTC
Finally... I could test and make this patch work !

Patch pushed, but 2 questions:
* the string for items returned by patrons is usually "check in" in Koha (see mainpage.pl for example). Here, it's "return". Should it be fixed for consistency or is it an english subtilty i'm missing ?
* I'm not sure that the "all libraries" rule work. I could make the feature working by entering "item float" for a given library. Maybe I made something wrong, maybe it's how it's supposed to work, explanation welcomed ;-)
Comment 29 Srdjan Jankovic 2012-03-22 00:19:33 UTC
(In reply to comment #28)
> Finally... I could test and make this patch work !
> 
> Patch pushed, but 2 questions:
> * the string for items returned by patrons is usually "check in" in Koha
> (see mainpage.pl for example). Here, it's "return". Should it be fixed for
> consistency or is it an english subtilty i'm missing ?

Not being a native speaker I'm subtlety resistant myself. My train of thoughts was:
- "Check in" branch is where an item is *returned by the borrower* (checked-in); "Return" branch is where the item is supposed to be stored, ie *returned by the receiving librarian*
- The original syspref was named "HomeOrHoldingBranchReturn". I could have named it "BranchReturn", but "ReturnBranch" somehow appealed more.

> * I'm not sure that the "all libraries" rule work. I could make the feature
> working by entering "item float" for a given library. Maybe I made something
> wrong, maybe it's how it's supposed to work, explanation welcomed ;-)

I'm not sure what particular rule/operation you are referring to. Can you please tell me what to do to duplicate the issue.

Also, I've found a small bug there, will create a follow up
Comment 30 Srdjan Jankovic 2012-03-22 00:39:05 UTC
Ooops, sorry, this moved on quite a bit. Please disregard my input.
Comment 31 Jared Camins-Esakov 2012-12-31 00:47:41 UTC
There have been no further reports of problems so I am marking this bug resolved.
Comment 32 David Cook 2013-09-24 07:45:08 UTC
Can anyone say what is actually going on with this bug?

Was anything ever pushed?
Comment 33 Katrin Fischer 2013-09-24 08:01:55 UTC
Yes, check the possible return settings on the circulation conditions page :)
Comment 34 David Cook 2013-09-24 08:15:32 UTC
(In reply to Katrin Fischer from comment #33)
> Yes, check the possible return settings on the circulation conditions page :)

What do you mean by the circulation conditions page, Katrin?
Comment 35 David Cook 2013-09-24 08:18:09 UTC
(In reply to David Cook from comment #34)
> (In reply to Katrin Fischer from comment #33)
> > Yes, check the possible return settings on the circulation conditions page :)
> 
> What do you mean by the circulation conditions page, Katrin?

Ahh...the circulation and fines rules page. I'm seeing the "Item floats" return policies now.

Interesting...