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

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>

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

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.

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

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

This fix was included in the 3.6.x branch prior to 3.6.4.