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.

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.

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.

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.

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.

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?

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.

Sorry for being slow... Are they mutually exclusive (competing solutions for the same problem) or could they co-exist?

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 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 !)

for those doing testing - this patch uses an atomicupdate - don't forget to run it (like I did. :))

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.

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.

QA comments: 

Code clean with comments.

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

Marked as passed QA.

(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.

(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 :)

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.

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)

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>

Just switching status back to "Passed QA" based on Paul's note from 2/27.

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.

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.

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 ;-)

(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

Ooops, sorry, this moved on quite a bit. Please disregard my input.

There have been no further reports of problems so I am marking this bug resolved.

Can anyone say what is actually going on with this bug?

Was anything ever pushed?

Yes, check the possible return settings on the circulation conditions page :)

(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?

(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...