Bug 19919 - Writing off a Lost Item Fee marks as "Paid for by patron"
Summary: Writing off a Lost Item Fee marks as "Paid for by patron"
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Whiteboard:
Keywords: RM_priority
: 12817 18868 (view as bug list)
Depends on: 22563
Blocks: 14825
  Show dependency treegraph
 
Reported: 2018-01-05 15:12 UTC by Lisette Scheer
Modified: 2019-06-14 17:12 UTC (History)
19 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
An example (37.57 KB, image/jpeg)
2018-01-05 15:12 UTC, Lisette Scheer
Details
Bug 19919: Do not record payments for a writeoff (2.28 KB, patch)
2019-05-03 14:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19919: Do not record payments for a writeoff (2.27 KB, patch)
2019-05-05 10:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19919: Stop using paidfor altogether (5.55 KB, patch)
2019-05-08 10:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19919: Stop using paidfor altogether (6.60 KB, patch)
2019-05-08 12:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19919: Stop using paidfor altogether (6.65 KB, patch)
2019-05-15 18:03 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 19919: Stop using paidfor altogether (6.65 KB, patch)
2019-05-16 07:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19919: Unit Tests (1.91 KB, patch)
2019-05-16 07:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19919: Stop using paidfor altogether (6.65 KB, patch)
2019-05-16 08:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19919: Unit Tests (2.01 KB, patch)
2019-05-16 08:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19919: Stop using paidfor altogether (6.72 KB, patch)
2019-05-31 11:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19919: Stop using paidfor altogether (6.72 KB, patch)
2019-05-31 11:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19919: Unit Tests (2.08 KB, patch)
2019-05-31 11:11 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 Lisette Scheer 2018-01-05 15:12:47 UTC
Created attachment 70293 [details]
An example

A replacement copy was ordered for an item that was "long lost overdue" on a patron's account. 
The librarian wanted the old copy deleted from the system to keep the record clean. 
We checked in the item, marked it as lost, and then wrote off the fee associated with the item. Finally we reapplied the fee manually, no longer associated with the item. 
We noticed that at the point of the write off, the item gets a line that says paid for by (Patron name and barcode) DATE TIME.


This also happens if the book is checked in and no longer marked as lost (say it was returned by the borrower so we wrote off the lost item fee.)

The borrower has not paid for the book in either situation.
Comment 1 Christopher Davis 2018-01-11 18:06:19 UTC
Thanks Lisette,

We have seen this behavior in Koha at our institution as well. This bug is more annoying and confusing than anything, but it would be good to have it patched.
Comment 2 Barton Chittenden 2018-03-01 16:43:44 UTC
Marking as major, simply because this is going to cause a lot of confusion; I foresee many support tickets.
Comment 3 Jonathan Druart 2018-04-19 13:07:12 UTC
What could be a solution?
Comment 4 Katrin Fischer 2018-04-21 11:30:27 UTC
I think that might be some underdocumented feature here that doesn't work in all cases. 

The message is set by ReturnLostItem:
http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=C4/Circulation.pm;hb=95031b035b5de87211d0de2040eb685ae005606c#l3601

ReturnLostItem is only used by pay (at least in master):

http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=Koha/Account.pm;hb=95031b035b5de87211d0de2040eb685ae005606c#l114

Then there is also the template:
http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt;hb=95031b035b5de87211d0de2040eb685ae005606c#l237

So when a fine of type L or Rep is paid, the line will be added to items.paidfor.

Of course this is problematic in multiple ways:
- the whole note is untranslatable
- we store firstname surname and not the borrowernumber, which can lead to confusion with common names.
- we store a badly formatted date (note the missing leading zeros) in ISO format.

We could fix the behaviour and only add the note for payments (not write offs). But maybe we should rethink the whole thing? Is this information really helpful when you can get it from the cleaned up accountlines and account_offsets table now in a clean way?
Comment 5 Magnus Enger 2018-05-02 09:48:38 UTC
Storing this info on the item level is also problematic in light of GDPR. 

Can anyone describe a use case for storing this info in items.paidfor?
Comment 6 Katrin Fischer 2018-05-02 11:05:34 UTC
(In reply to Magnus Enger from comment #5)
> Storing this info on the item level is also problematic in light of GDPR. 
> 
> Can anyone describe a use case for storing this info in items.paidfor?

Good point, totally agree. We should not store patron information like names in item data ever, there is too much risk in exporting the data in some way or making it accidentally visible in the OPAC.

I'd vote for removing it.
Comment 7 Christopher Brannon 2019-03-13 16:31:42 UTC
(In reply to Katrin Fischer from comment #6)
> (In reply to Magnus Enger from comment #5)
> > Storing this info on the item level is also problematic in light of GDPR. 
> > 
> > Can anyone describe a use case for storing this info in items.paidfor?
> 
> Good point, totally agree. We should not store patron information like names
> in item data ever, there is too much risk in exporting the data in some way
> or making it accidentally visible in the OPAC.
> 
> I'd vote for removing it.

+1
Comment 8 Jonathan Druart 2019-03-14 21:33:33 UTC
(In reply to Magnus Enger from comment #5)

> Can anyone describe a use case for storing this info in items.paidfor?

I found that:

2001-05-17 15:36  rangi
 
   * database.mysql: Changed the interim field in the items table, to
   be paidfor The horowhenua librarians wanted a place to store
   information when a lost item was paid for. The information is
   removed if the item is ever returned.  The interim field was a
   hangover from the previous system and wasnt be used so I stole it
   :-)
Comment 9 George Williams (NEKLS) 2019-03-15 15:38:35 UTC
I'm in favor of removing this altogether if it's not needed.

If it is needed, then I'd like to see the patron information stored as the patron's borrowernumber of barcode number rather than as their name.
Comment 10 Christopher Brannon 2019-03-15 16:10:37 UTC
(In reply to George Williams (NEKLS) from comment #9)
> I'm in favor of removing this altogether if it's not needed.
> 
> If it is needed, then I'd like to see the patron information stored as the
> patron's borrowernumber of barcode number rather than as their name.

Never a barcode.  It should be the borrowernumber.
Comment 11 Katrin Fischer 2019-04-25 10:52:55 UTC
*** Bug 12817 has been marked as a duplicate of this bug. ***
Comment 12 Katrin Fischer 2019-04-25 10:54:15 UTC
*** Bug 18868 has been marked as a duplicate of this bug. ***
Comment 13 Martin Renvoize 2019-05-03 14:51:40 UTC
Created attachment 89320 [details] [review]
Bug 19919: Do not record payments for a writeoff
Comment 14 Martin Renvoize 2019-05-03 14:56:31 UTC
I need some guidance here.

I've attached a basic patch to resolve just the problem as described in the title of the bug..

However, it looks like people are also very keen to remove this paidfor field entirely (which is actually pretty simple to do at this point and would not lose any information as we already store enough data to reconstruct it via the accountlines).

So, as far as I can see there are two use cases for it, the first is for display in the item moredetails view (Does it matter if an item has been paid for, then returned and written off.. right now we remove the detail in paidfor so we don't display anything at this point, but we could show the payment and refund if we were so inclined)

The second use case is for reports. This one is more challenging, we can't easily know what reports are out there in the wild so the best we can really do is advise as to how to get the same information back via a join to the accountlines table.

Any guidance as to what we would hope to achieve in the long run would be greatly appreciated.
Comment 15 Jonathan Druart 2019-05-03 20:05:23 UTC
Martin, the patch does not apply:
  error: sha1 information is lacking or useless (C4/Circulation.pm).
Comment 16 Martin Renvoize 2019-05-05 10:24:04 UTC
Created attachment 89356 [details] [review]
Bug 19919: Do not record payments for a writeoff
Comment 17 Martin Renvoize 2019-05-05 10:27:13 UTC
I've uploaded a refreshed patch freshly based against master.

I've not written a test for it yet - I wasn't sure whether this was actually what people wanted here (and to split out the removal of paidfor as a separate bug) or not.. so it's more of a proof of concept at this point.  A test should however be pretty simple should we want to follow this route.
Comment 18 Liz Rea 2019-05-07 19:29:58 UTC
I confirm that this works, but I'd like to hear from Chris if Horowhenua Libraries still require this functionality before we totally get rid of it.
Comment 19 Katrin Fischer 2019-05-07 19:31:45 UTC
(In reply to Liz Rea from comment #18)
> I confirm that this works, but I'd like to hear from Chris if Horowhenua
> Libraries still require this functionality before we totally get rid of it.

Adding him as CC
Comment 20 Chris Cormack 2019-05-07 19:48:22 UTC
They still use it, so if it could be fixed rather than removed would be great.
Always a shame when we break things the Original Koha library use.
Comment 21 Martin Renvoize 2019-05-07 21:15:08 UTC
(In reply to Chris Cormack from comment #20)
> They still use it, so if it could be fixed rather than removed would be
> great.
> Always a shame when we break things the Original Koha library use.

Just to clarify, this patch only corrects the behaviour to not record 'paidfor' when the action is a writoff rather than a payment.  So, if I go ahead and write the tests for that here we should be good for this bug.

For a follow-up bug I believe people are interested in cleaning up the implimentation somewhat to fit with more recent practice.  Might pick your brains as to how this is used at Horowhenua some point soon Chris, be interested to fully understand the use cases before diving in.  As I said before, I'm confident we can still get all the data out but I don't want to miss something or reduce usability uneccesarily
Comment 22 Katrin Fischer 2019-05-07 21:19:09 UTC
My concerns here are the issues of translatability and privacy. We store a patron name in items... which we expose for export practically everywhere. I think it would be really nice to keep the information, but in another database table. As it's mapped in MARC excluding it everywhere might turn out a bit of a never-ending chase.
Comment 23 Katrin Fischer 2019-05-07 21:19:47 UTC
Would moving it out of items be ok?
Comment 24 Martin Renvoize 2019-05-08 10:50:05 UTC
Created attachment 89459 [details] [review]
Bug 19919: Stop using paidfor altogether
Comment 25 Martin Renvoize 2019-05-08 10:53:25 UTC
OK, the second patch is an alternative `Stop using paidfor altogether` (or beginning of one which I would like some feedback on).

This version removes the use of paidfor in the code entirely and instead derives the information for display within moredetails.pl (which is the only place I could find that it's displayed to the end users).

I noticed whilst doing this that we also ship with the option LOST authorized value of 'Lost and Paid for'.. should we not set that status as part of the payment routine as part of this perhaps too?

I've not added the code to actually remove the field yet (or the proper tests) as it's only a proof of concept at the minute which I'm seeking feedback.
Comment 26 Martin Renvoize 2019-05-08 12:38:58 UTC
Created attachment 89472 [details] [review]
Bug 19919: Stop using paidfor altogether

This patch removed references to setting and getting the items.paidfor
field. Where it was used for display, in moredetail.pl, we replace it
with a query on the accountlines.

Test plan:
1) Apply patch
2) Pay off a LOST item
3) Check for the associated display of 'Paidfor?:' on the itemdetails
page
4) Writeoff a LOST item
5) Check that a 'Paidfor?:' is not displayed on the itemdetails page.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 George Williams (NEKLS) 2019-05-08 17:56:28 UTC
(In reply to Chris Cormack from comment #20)
> They still use it, so if it could be fixed rather than removed would be
> great.
> Always a shame when we break things the Original Koha library use.

My objection is storing the patron's name.  If this could store the borrowernumber instead, that would be OK by me, but using the patron's name is a privacy issue for me.
Comment 28 Martin Renvoize 2019-05-08 17:58:42 UTC
(In reply to George Williams (NEKLS) from comment #27)
> (In reply to Chris Cormack from comment #20)
> > They still use it, so if it could be fixed rather than removed would be
> > great.
> > Always a shame when we break things the Original Koha library use.
> 
> My objection is storing the patron's name.  If this could store the
> borrowernumber instead, that would be OK by me, but using the patron's name
> is a privacy issue for me.

So, the final patch here drops the need for the field entirely and instead queries the data from the accountlines instead.  :)
Comment 29 Liz Rea 2019-05-08 20:01:51 UTC
The only problem i see here is that when we delete the patron, this paid for record will go away? or will it not I can't remember if we delete the accountlines when we delete a patron. Anyway it seems to work, and I'll sign off on it once you get a test for your new sub, providing everyone is OK with how it works now. :)
Comment 30 Katrin Fischer 2019-05-08 21:25:47 UTC
(In reply to Liz Rea from comment #29)
> The only problem i see here is that when we delete the patron, this paid for
> record will go away? or will it not I can't remember if we delete the
> accountlines when we delete a patron. Anyway it seems to work, and I'll sign
> off on it once you get a test for your new sub, providing everyone is OK
> with how it works now. :)

It used to go away, but we fixed that (it broke the accounting reports - mean). It should now set to NULL. I think as we can only delete a patron that has paid off all their fines, this should be ok?
Comment 31 Martin Renvoize 2019-05-10 11:12:46 UTC
So... are we close to a signoff.. I'm not sure how to read the last comments?
Comment 32 Martin Renvoize 2019-05-10 11:15:00 UTC
If I get a signoff that the concept is right, I'll throw together the outstanding unit test quickly for the QA step.
Comment 33 Liz Rea 2019-05-11 02:07:32 UTC
GO GO UNIT TEST MASTAH. :D We think the concept is good afaict.
Comment 34 Liz Rea 2019-05-15 17:54:13 UTC
Waiting for the promised tests (I know you are busy!)
Comment 35 Liz Rea 2019-05-15 18:03:02 UTC
Created attachment 89794 [details] [review]
Bug 19919: Stop using paidfor altogether

This patch removed references to setting and getting the items.paidfor
field. Where it was used for display, in moredetail.pl, we replace it
with a query on the accountlines.

Test plan:
1) Apply patch
2) Pay off a LOST item
3) Check for the associated display of 'Paidfor?:' on the itemdetails
page
4) Writeoff a LOST item
5) Check that a 'Paidfor?:' is not displayed on the itemdetails page.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 36 Martin Renvoize 2019-05-16 07:50:02 UTC
Created attachment 89814 [details] [review]
Bug 19919: Stop using paidfor altogether

This patch removed references to setting and getting the items.paidfor
field. Where it was used for display, in moredetail.pl, we replace it
with a query on the accountlines.

Test plan:
1) Apply patch
2) Pay off a LOST item
3) Check for the associated display of 'Paidfor?:' on the itemdetails
page
4) Writeoff a LOST item
5) Check that a 'Paidfor?:' is not displayed on the itemdetails page.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 37 Martin Renvoize 2019-05-16 07:50:06 UTC
Created attachment 89815 [details] [review]
Bug 19919: Unit Tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2019-05-16 08:22:50 UTC
Created attachment 89816 [details] [review]
Bug 19919: Stop using paidfor altogether

This patch removed references to setting and getting the items.paidfor
field. Where it was used for display, in moredetail.pl, we replace it
with a query on the accountlines.

Test plan:
1) Apply patch
2) Pay off a LOST item
3) Check for the associated display of 'Paidfor?:' on the itemdetails
page
4) Writeoff a LOST item
5) Check that a 'Paidfor?:' is not displayed on the itemdetails page.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 39 Martin Renvoize 2019-05-16 08:22:54 UTC
Created attachment 89817 [details] [review]
Bug 19919: Unit Tests

This patch adds unit tests for the addition of a patron accessor to the
Koha::Account::Line object.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Marcel de Rooy 2019-05-31 08:10:40 UTC
Depends on NSO. Please reset BLOCKED status as dependency has been signed off.
Comment 41 Kyle M Hall 2019-05-31 11:09:25 UTC
Created attachment 90228 [details] [review]
Bug 19919: Stop using paidfor altogether

This patch removed references to setting and getting the items.paidfor
field. Where it was used for display, in moredetail.pl, we replace it
with a query on the accountlines.

Test plan:
1) Apply patch
2) Pay off a LOST item
3) Check for the associated display of 'Paidfor?:' on the itemdetails
page
4) Writeoff a LOST item
5) Check that a 'Paidfor?:' is not displayed on the itemdetails page.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Kyle M Hall 2019-05-31 11:11:11 UTC
Created attachment 90229 [details] [review]
Bug 19919: Stop using paidfor altogether

This patch removed references to setting and getting the items.paidfor
field. Where it was used for display, in moredetail.pl, we replace it
with a query on the accountlines.

Test plan:
1) Apply patch
2) Pay off a LOST item
3) Check for the associated display of 'Paidfor?:' on the itemdetails
page
4) Writeoff a LOST item
5) Check that a 'Paidfor?:' is not displayed on the itemdetails page.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Kyle M Hall 2019-05-31 11:11:22 UTC
Created attachment 90230 [details] [review]
Bug 19919: Unit Tests

This patch adds unit tests for the addition of a patron accessor to the
Koha::Account::Line object.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 44 Nick Clemens 2019-06-14 17:12:45 UTC
I think Kyle Qaed? marking as such