We have set up Koha to prevent people owing more than x amount of fines. When we issue rental items (DVDs for instance) the amount is entered as a fine so if the patron issues more than one DVD their account is blocked until we pay the rental amount off. Work to do: * C4::Members::GetMemberAccountRecords() - add another boolean parameter $no_charges. If set to true disregard non-fine charges: - Rent - Res - authorised_values MANUAL_INV * C4::Circulation::CanBookBeIssued() - pass TRUE as no_charges when calling C4::Members::GetMemberAccountRecords() How to test: * Set syspref noissuescharge to some small value * Set rental charge for some item to be higher than noissuescharge * For a patron without fines: - issue an item with rental charges (above) - issue another item of the same type - there should be no warnings or stoppers
Created attachment 6335 [details] [review] patch This proved to be a bit wider then originally expected, changed patronflags() not to raise "CHARGES" if owing amount is not fines. I've also noticed some other possible issues, but will raise them in a separate bug request
Created attachment 6709 [details] [review] Tested patch, correctly solves the problem, requires further sign off.
I am not sure I agree with this change for several reasons: I am not sure MAN_INV should be included in the list of fines that don't block. I am not sure if this change makes sense, as the MAN_INV values can be used for a lot of different things and libraries might want to block patrons from checkout here. Having 'Res' not block checkout is also a change to current behaviour, although I agree that it might make sense. I think this needs a system preference. Perhaps a third option for 'issuinginprocess' that makes rental fees never block circ. This way we don't change the behaviour of this for all libraries, which is something that libraries may not want.
Created attachment 7127 [details] [review] patch I have introduced RentalsInNoissueCharges and ManInvInNoissueCharges sys prefs to fine control issue blocking charge. Those are set to "Yes" initially to have current behaviour. 'Res' charges are not in the fines total unconditionally.
Hi Srdjan, thx a lot for taking my comments into account. Reading through this again I wonder if a better solution would be to make the fine types that can block checkouts a list? We have some system preferences now that take | or comma separated lists of values. It would allow for more flexibility and it would work with 1 additional system preference instead of 2. I am really not sure here and perhaps your solution is what others think is the best way to do it. Some more opinions about this would be good.
I have no personal preferences in terms how we store sysprefs. Two sysprefs were suggested, and I went with that. I'm still kind of learning how community operates. Having one syspref with comma or | delimited values (as I saw it elsewhere) makes sense, but some people may find it confusing. From my point of view it is trivial how we store it, so if you think it's better to have one syspref I can change it. One syspref has following advantages: * it scales better, ie if a new charge code needs to be added * MAN_INV charges can be fine grained Disadvantage is that is maybe harder to understand/use. Your call.
I agree with Katrin. This is the kind of thing that will cause trouble for those who have been long time Koha users. There should be some way to say this fine is a FINE and this fine is a CHARGE or whatever we need to distinguish between them. Nicole
Fine types need to be broken out into their own table in the database. Right now, they're hardcoded in the system, and that makes for all kinds of confusion and overlap of features. We need to be able to indicate, on configurable level, which kind of fines count against patron checkout and against patron holds. We should also be able to indicate whether those fines are capped or unlimited (Overdues should stop at $5, let's say, but lost book fees and card renewals fees should accrue infinitely) I'm moving this to the new In Discussion value, since I think we've got more to discuss before a workable change can be made.
I think we will end up with a total rewrite of accounts if we try to find a real perfect solution for this. In fact this has already been talked about and there is bug 6427 for it. The feature as described here could be helpful for now and get into the next release, while we get the big picture right and a total rewrite done. For example MAN_INV is helpful - but I don't really like to have it as a authorised value. But it shows there is a need for user configurable charges and fines. This bug shows there is a need to distinguish between different fines and charges. For this bug I think the question is about making an enhancement without adding regression or confusion for libraries being used to the current behaviour.
My intention was to make it work as it is now out of the box.
Reviving the discussion... One thing I'm not sure to understand: if this patch were pushed, the current behaviour would still be applicable with the syspref RentalsInNoissueCharges=0 isn't it ? In this case, I don't understand why we should reject this patch, the only thing that we could request is to have the syspref be 0 when upgrading (which is not the case atm) We could rewrite a lot of things on account management, but that would be another topic !
Created attachment 11943 [details] [review] bug_7243: Be selective when summing up charges for blocking issues Added RentalsInNoissueCharges and ManInvlsInNoissueCharges sys prefs Created C4::Members::cwGetMemberAccountBallance() * A wrapper for GetMemberAccountRecords that gives info on non-issue and other charges * Other charges are: 'Res' 'Rent' if RentalsInNoissueCharges is Mo authorised_values MANUAL_INV if ManInvlsInNoissueCharges is No C4::Members::GetMemberAccountRecords() changes: * Dropped input param $date, it is not used Use split charges in C4::Circulation::CanBookBeIssued() and C4::Members::patronflags(). That way only fines decide whether an item can be issued, and not other non-fine charges
A slightly changed approach. Should be very backward compatible.
Created attachment 12452 [details] [review] bug_7243: Be selective when summing up charges for blocking issues Added RentalsInNoissueCharges and ManInvlsInNoissueCharges sys prefs Created C4::Members::cwGetMemberAccountBallance() * A wrapper for GetMemberAccountRecords that gives info on non-issue and other charges * Other charges are: 'Res' 'Rent' if RentalsInNoissueCharges is Mo authorised_values MANUAL_INV if ManInvlsInNoissueCharges is No C4::Members::GetMemberAccountRecords() changes: * Dropped input param $date, it is not used Use split charges in C4::Circulation::CanBookBeIssued() and C4::Members::patronflags(). That way only fines decide whether an item can be issued, and not other non-fine charges
Created attachment 12454 [details] [review] [SIGNED-OFF] bug_7243: Be selective when summing up charges for blocking issues Added RentalsInNoissueCharges and ManInvlsInNoissueCharges sys prefs Created C4::Members::cwGetMemberAccountBallance() * A wrapper for GetMemberAccountRecords that gives info on non-issue and other charges * Other charges are: 'Res' 'Rent' if RentalsInNoissueCharges is Mo authorised_values MANUAL_INV if ManInvlsInNoissueCharges is No C4::Members::GetMemberAccountRecords() changes: * Dropped input param $date, it is not used Use split charges in C4::Circulation::CanBookBeIssued() and C4::Members::patronflags(). That way only fines decide whether an item can be issued, and not other non-fine charges Signed-off-by: Marc Veron <veron@veron.ch> Rebased (updatedatabase.pl) ManInvInNoissueCharges and RentalsInNoissueCharges ar both included by default (= behaviour as before) All variants tested: Both included, none included, manual invoice included, rentals included. Works fine, blocks/does not blok as appropirate, messages appear as expected.
Created attachment 12781 [details] [review] Signed patch 7243 Rebased for updatedatabase. Automerging of last version went wrong.
Preliminary QA comment (result of qa-tools on Member.pm): * C4/Members.pm FAIL pod OK forbidden patterns OK valid FAIL Subroutine GetMemberAccountBallance redefined critic OK Please note that a routine name with the misspelled Ballance (read: Balance) should be corrected too. Such spelling errors (even for a non-native speaker like me) do not increase the trust level of corresponding code :-)
QA Comment: Code looks good to me. Worked as described during limited testing. The above warning seems to be false; the remark on spelling remains! The enhancement is written such a way that it does not interfere with current behavior. By default rentals and manual invoices are included in the patron balance used when checking out. Only by explicitly changing the prefs behavior changes. (Exception here could be the amounts marked with Res.) I only have a few non-blocker comments. Followups are always welcome! You mention NoissueCharge in some descriptions. Please correct. Looking for noissuescharge will help find the pref much easier ;) Note the additional s in the middle. The description of the prefs could be improved (see below). t/db_dependent/lib/KohaTest/Members.pm looks to me like an unused test module; I once asked the RM already to remove such files Not a blocker, but just mentioning: The names of the prefs and also the names of the variables (returned values) by the new GetMemberAccountBallance are confusing. E.g. What means Do not include .. in No issues .. (double no)? Not intuitively clear. Could you say something like Consider rentals as fines? And explain in one sentence that they do or do not block checkout. And similarly Consider other charges as fines? Note that this is just to stimulate the process of finding more understandable names :) E.g. what is $non_issue_ballance (note double l ..)? Actually you mean here total fines. It would be easier if you say total amount= fines + other charges, and work with such names in code. Passed QA
Changes Circulation.pm, Members.pm and circulation. Adds a column to the database I won't push this patch for 3.10 because of the Feature Freeze. Affecting rel_3_12
Srdjan, Is there any chance we could get a follow-up (not a squashed patch) correcting the spelling errors Marcel identified?
Created attachment 13261 [details] [review] bug_7243: Rewording and renaming
Sorry about the delay, just slipped.
Srdjan, Could you please revise the follow-up to change "ballance" => "balance"? The other changes in the follow-up look good, though, thanks.
Created attachment 13664 [details] [review] bug_7243: Rewording and renaming
Created attachment 13666 [details] [review] bug_7243: Rewording and renaming
Hi Srdjan, can you check your last patch again? I can still spot some 'Ballances' =head2 GetMemberAccountBallance - ($total_ballance, $non_issue_ballance, $other_charges) = &GetMemberAccountBallance($borrowernumber); + ($total_balance, $non_issue_balance, $other_charges) = &GetMemberAccountBallance($borrowernumber);
Created attachment 13693 [details] [review] bug_7243: Rewording and renaming
Sorry, substandard grep.
Created attachment 13917 [details] [review] [SIGNED-OFF] bug_7243: Rewording and renaming Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Marking as Passed QA as the code has met all the requirements set forth by QA.
Kyle, do you think you could rebase and reattach these two patches? The second will not apply for me.
Created attachment 14741 [details] [review] Bug 7243: Be selective when summing up charges for blocking issues Added RentalsInNoissueCharges and ManInvlsInNoissueCharges sys prefs Created C4::Members::cwGetMemberAccountBallance() * A wrapper for GetMemberAccountRecords that gives info on non-issue and other charges * Other charges are: 'Res' 'Rent' if RentalsInNoissueCharges is Mo authorised_values MANUAL_INV if ManInvlsInNoissueCharges is No C4::Members::GetMemberAccountRecords() changes: * Dropped input param $date, it is not used Use split charges in C4::Circulation::CanBookBeIssued() and C4::Members::patronflags(). That way only fines decide whether an item can be issued, and not other non-fine charges Signed-off-by: Marc Veron <veron@veron.ch> Rebased (updatedatabase.pl) ManInvInNoissueCharges and RentalsInNoissueCharges ar both included by default (= behaviour as before) All variants tested: Both included, none included, manual invoice included, rentals included. Works fine, blocks/does not blok as appropirate, messages appear as expected. [Oct 12, 2012 marcelr:] Amended for updatedatabase.pl
Created attachment 14742 [details] [review] Bug 7243: Rewording and renaming Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This patch has been pushed to master.