Bug 7190 - written off fines being refunded
Summary: written off fines being refunded
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.6
Hardware: All All
: P5 - low normal (vote)
Assignee: Srdjan Jankovic
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-07 16:34 UTC by Nicole C. Engard
Modified: 2013-12-05 19:59 UTC (History)
2 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
patch (1.20 KB, patch)
2012-01-24 03:49 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (3.98 KB, patch)
2012-01-31 02:40 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] bug_7190: Do not reverse writeoffs when item is returned (4.05 KB, patch)
2012-02-01 01:00 UTC, Nicole C. Engard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2011-11-07 16:34:43 UTC
When a lost charge is written off (using the write off function) and then returned at a later date the patron is refunded the fee for the book - this means the patron makes out with extra money that they shouldn't be getting.  

If a fine line was written off then when returning the item the fees should not be refunded.
Comment 1 Srdjan Jankovic 2012-01-24 03:49:11 UTC Comment hidden (obsolete)
Comment 2 Nicole C. Engard 2012-01-30 14:47:20 UTC
I have tested and the behavior has not changed after applying the patch.  Test plan:

1. check a book out
2. mark it lost
3. confirm that patron is charged lost fee
4. writeoff lost fine line
5. return the book
6. confirm that patron was not refunded the price of the lost item [FAILED]

In my test the patron is still refunded even though the accountline was written off.

Nicole
Comment 3 Srdjan Jankovic 2012-01-31 02:40:05 UTC Comment hidden (obsolete)
Comment 4 Nicole C. Engard 2012-02-01 01:00:18 UTC
Created attachment 7404 [details] [review]
[SIGNED-OFF] bug_7190: Do not reverse writeoffs when item is returned

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Comment 5 Paul Poulain 2012-02-02 17:17:03 UTC
QA comment:

Srdjan, I couldn't understand why you write
    my $sth = $dbh->prepare("SELECT * FROM accountlines WHERE itemnumber = ? AND accounttype IN ('L', 'Rep', 'W') ORDER BY date DESC, accountno DESC");

and a few lines below:
    $data or return;    # bail if there is nothing to do
    $data->{accounttype} eq 'W' and return;    # Written off

It's equivalent to 
    my $sth = $dbh->prepare("SELECT * FROM accountlines WHERE itemnumber = ? AND accounttype IN ('L', 'Rep') ORDER BY date DESC, accountno DESC");
followed by
    $data or return;    # bail if there is nothing to do
?

Thinking of it, a little bit more, the date DESC is important. As the sub returns only 1 line, you can have more than 1 line in the SQL, we assume the W is the most recent one (that sound reasonable: you writeoff a fine you've already set)

At the end, my feeling is that the sub is quite strange (could I say clumsy ? probably...), but it was before your patch, so it's OK

Otherwise, small patch, passed QA
Comment 6 Srdjan Jankovic 2012-02-03 00:26:05 UTC
Paul,

It is not equivalent. The purpose is to catch 'W'.
* With "SELECT * FROM accountlines WHERE itemnumber = ? AND accounttype IN ('L', 'Rep') ORDER BY date DESC, accountno DESC" 'W' is ignored. It says "The latest fine" and the latest fine is acted upon.
* With "accounttype IN ('L', 'Rep', 'W')" it says "Latest fine or writeoff", and if it is a writeoff we walk away.

Re clumsiness, well it is not just this sub, it is the accounting side as a whole. There's no proper accounting/billing implemented, and the best thing would be to integrate a third party solution, but that is another subject.
Comment 7 Paul Poulain 2012-02-03 08:11:18 UTC
(In reply to comment #6)
> Paul,
> 
> It is not equivalent. The purpose is to catch 'W'.
> * With "SELECT * FROM accountlines WHERE itemnumber = ? AND accounttype IN
> ('L', 'Rep') ORDER BY date DESC, accountno DESC" 'W' is ignored. It says "The
> latest fine" and the latest fine is acted upon.
> * With "accounttype IN ('L', 'Rep', 'W')" it says "Latest fine or writeoff",
> and if it is a writeoff we walk away.

Yes, that's what I understood at the end. But relying on the date to discover if there is a writeoff is quite dangerous. And it's more as the date field is only a date.
Suppose the fine and the writeoff are made the same day. We are not sure of the order mySQL return.
Or suppose someone or something changes the date.

However, your patch relies on how Koha works today, so I have pushed it.
> 
> Re clumsiness, well it is not just this sub, it is the accounting side as a
> whole.
You're right. Its a kind of "oldies but NOT goodies"...

> There's no proper accounting/billing implemented, and the best thing
> would be to integrate a third party solution, but that is another subject.
agreed (to all part of this sentence : no proper & third party solution & that's another subject)
Comment 8 Srdjan Jankovic 2012-02-03 09:37:43 UTC
"Suppose the fine and the writeoff are made the same day. We are not sure of the"
order mySQL return."
Correct. That's why I seconded it with accountnumber.

"Or suppose someone or something changes the date."
That would really screw things up. But I don't think anything does. Let's hope for the best :)
Comment 9 Jared Camins-Esakov 2012-05-23 12:58:12 UTC
This fix was included in the 3.6.x branch prior to 3.6.4.