Bug 7243 - Do not take rentals as fines
Summary: Do not take rentals as fines
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.12
Hardware: All All
: P3 enhancement (vote)
Assignee: Srdjan Jankovic
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 7244
  Show dependency treegraph
 
Reported: 2011-11-18 03:48 UTC by Srdjan Jankovic
Modified: 2014-05-26 21:04 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
patch (4.92 KB, patch)
2011-11-18 08:59 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Tested patch, correctly solves the problem, requires further sign off. (4.99 KB, patch)
2011-12-11 21:35 UTC, Duncan
Details | Diff | Splinter Review
patch (9.41 KB, patch)
2012-01-13 07:13 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_7243: Be selective when summing up charges for blocking issues (16.22 KB, patch)
2012-09-03 05:05 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_7243: Be selective when summing up charges for blocking issues (16.65 KB, patch)
2012-09-22 18:31 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] bug_7243: Be selective when summing up charges for blocking issues (17.00 KB, patch)
2012-09-22 18:34 UTC, Marc Véron
Details | Diff | Splinter Review
Signed patch 7243 (17.40 KB, patch)
2012-10-12 13:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
bug_7243: Rewording and renaming (9.29 KB, patch)
2012-11-06 01:18 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_7243: Rewording and renaming (2.63 KB, patch)
2012-11-26 01:34 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_7243: Rewording and renaming (10.67 KB, patch)
2012-11-26 01:41 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_7243: Rewording and renaming (11.69 KB, patch)
2012-11-26 23:26 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] bug_7243: Rewording and renaming (11.74 KB, patch)
2012-12-07 13:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7243: Be selective when summing up charges for blocking issues (17.32 KB, patch)
2013-01-22 13:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7243: Rewording and renaming (11.71 KB, patch)
2013-01-22 13:18 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Srdjan Jankovic 2011-11-18 03:48:19 UTC
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
Comment 1 Srdjan Jankovic 2011-11-18 08:59:35 UTC Comment hidden (obsolete)
Comment 2 Duncan 2011-12-11 21:35:53 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2011-12-31 07:58:01 UTC
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.
Comment 4 Srdjan Jankovic 2012-01-13 07:13:03 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2012-01-15 18:45:30 UTC
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.
Comment 6 Srdjan Jankovic 2012-01-15 23:42:15 UTC
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.
Comment 7 Nicole C. Engard 2012-01-16 14:30:45 UTC
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
Comment 8 Ian Walls 2012-01-16 14:43:04 UTC
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.
Comment 9 Katrin Fischer 2012-01-16 14:59:31 UTC
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.
Comment 10 Srdjan Jankovic 2012-01-16 23:56:08 UTC
My intention was to make it work as it is now out of the box.
Comment 11 Paul Poulain 2012-03-30 10:16:57 UTC
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 !
Comment 12 Srdjan Jankovic 2012-09-03 05:05:50 UTC Comment hidden (obsolete)
Comment 13 Srdjan Jankovic 2012-09-03 05:07:23 UTC
A slightly changed approach. Should be very backward compatible.
Comment 14 Marc Véron 2012-09-22 18:31:02 UTC Comment hidden (obsolete)
Comment 15 Marc Véron 2012-09-22 18:34:32 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2012-10-12 13:09:39 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2012-10-12 13:16:55 UTC
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 :-)
Comment 18 Marcel de Rooy 2012-10-12 14:48:39 UTC
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
Comment 19 Paul Poulain 2012-10-17 16:12:54 UTC
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
Comment 20 Jared Camins-Esakov 2012-11-05 19:29:25 UTC
Srdjan,

Is there any chance we could get a follow-up (not a squashed patch) correcting the spelling errors Marcel identified?
Comment 21 Srdjan Jankovic 2012-11-06 01:18:13 UTC Comment hidden (obsolete)
Comment 22 Srdjan Jankovic 2012-11-06 01:19:43 UTC
Sorry about the delay, just slipped.
Comment 23 Jared Camins-Esakov 2012-11-25 23:55:30 UTC
Srdjan,

Could you please revise the follow-up to change "ballance" => "balance"? The other changes in the follow-up look good, though, thanks.
Comment 24 Srdjan Jankovic 2012-11-26 01:34:55 UTC Comment hidden (obsolete)
Comment 25 Srdjan Jankovic 2012-11-26 01:41:22 UTC Comment hidden (obsolete)
Comment 26 Katrin Fischer 2012-11-26 06:53:08 UTC
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);
Comment 27 Srdjan Jankovic 2012-11-26 23:26:43 UTC Comment hidden (obsolete)
Comment 28 Srdjan Jankovic 2012-11-26 23:28:50 UTC
Sorry, substandard grep.
Comment 29 Kyle M Hall 2012-12-07 13:26:18 UTC Comment hidden (obsolete)
Comment 30 Jared Camins-Esakov 2013-01-16 03:28:19 UTC
Marking as Passed QA as the code has met all the requirements set forth by QA.
Comment 31 Jared Camins-Esakov 2013-01-18 03:19:53 UTC
Kyle, do you think you could rebase and reattach these two patches? The second will not apply for me.
Comment 32 Kyle M Hall 2013-01-22 13:17:44 UTC
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
Comment 33 Kyle M Hall 2013-01-22 13:18:17 UTC
Created attachment 14742 [details] [review]
Bug 7243: Rewording and renaming

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Jared Camins-Esakov 2013-01-24 14:17:23 UTC
This patch has been pushed to master.