Bug 12768

This feature will apply when the "longoverdue.pl" script is run OR when an item is marked "Lost" (the value in items.itemlost> 0).

To test:

Apply patch and update the database.

Set up :
On the item type administration page, default replacement price and a processing fee should be set up for on or more item types. When set up to 0, no processing fee will be charged to patrons for lost items.
The "UseDefaultReplacementCost" system preference can be switched to « Use » or « Don't use ». If the lost item doesn't have a replacement cost catalogued, Koha will charge the default replacement cost.
The "ProcessingFeeNote" system preference can be edited with the value to be used as a note alongside the processing fee when charged to the patron. The value will be displayed in the patron page fines tab.

These tests apply when :
* an item is manually marked as lost
* an item is marked lost by the longoverdue.pl script

Test scenario 1 : "UseDefaultReplacementCost" is set to « Use » and items.replacementcost = 0

* the item is manually marked lost

1. Set up the default replacement cost for a specific item type.
2. Set up the processing fee for this item type as well.
3. Search for an item of this item type and with no replacement cost.
4. Checkout the item to a patron.
5. Go to the item page.
6. Mark the item as lost.
7. Go to the patron page and open the fines tab.
Check that :
8. there is a new entry for the item marked as lost.
9. the value match the default replacement cost for this item type.
10. there is another entry for the processing fee.
11. This entry should display the value from the « ProcessingFeeNote » system preference.
Note : If you set up a processing fee to « 0 », there should be no entry for the processing fee.

* the item is marked lost by the longoverdue.pl script

1. Set up the default replacement cost for a specific item type.
2. Set up the processing fee for this item type as well.
3. Search for an item of this item type and with no replacement cost.
4. Checkout the item to a patron specifying a 30 days late due date.
5. Run the script longoverdue.pl (/misc/cronjobs/) ./longoverdue.pl --lost 30=2 --charge 2 --confirm (for example)
6. Go to the item page.
Check that :
7. the item is marked as lost.
8. Go to the patron page and open the fine tab.
Check that :
9. there is a new entry for the item marked as lost.
10. the value match the default replacement cost for this item type.
11. there is another entry for the processing fee.
12. This entry should display the value from the « ProcessingFeeNote » system preference.
Note : If you set up a processing fee to « 0 », there should be no entry for the processing fee.

Test scenario 2  "UseDefaultReplacementCost" is set to « Use » and items.replacementcost > 0

* the item is manually marked lost

1. Set up the default replacement cost for a specific item type.
2. Set up the processing fee for this item type as well.
3. Search for an item of this item type and with a replacement cost.
4. Checkout the item to a patron.
5. Go to the item page.
6. Mark the item lost.
7. Go to the patron page and open the fine tab.
Check that :
8. there is a new entry for the item marked as lost.
9. the value match the item replacement cost (it's Koha's default behaviour).
10. there is another entry for the processing fee.. This entry should display the value from the « ProcessingFeeNote » system preference.
Note : If you set up a processing fee to « 0 », there should be no entry for the processing fee.

* the item is marked lost by the longoverdue.pl script

1. Set up the default replacement cost for a specific item type.
2. Set up the processing fee for this item type as well.
3. Search for an item of this item type and with a replacement cost.
4. Checkout the item to a patron specifying a 30 days late due date.
5. Run the script longoverdue.pl (/misc/cronjobs/) ./longoverdue.pl --lost 30=2 --charge 2 --confirm (for example)
6. Go to the item page
Check that :
7. the item is marked as lost.
8. Go to the patron page and open the fine tab.
Check that :
9. there is a new entry for the item marked as lost.
10 the value match the item replacement cost (it's Koha's default behaviour).
11. there is another entry for the processing fee. This entry should display the value from the « ProcessingFeeNote » system preference.
Note : If you set up a processing fee to « 0 », there should be no entry for the processing fee.

Test scenario 3 :  "UseDefaultReplacementCost" is set to « Don't use »

Koha's default behaviour apply.

It means :

If items.replacementcost = 0 :
The default replacement cost value for the item type will not be used.
The processing fee (if greater than 0) will be charged.

If items.replacement >0.
The item replacement cost will be used.
The processing fee (if greater than 0) will be charged.

I want to sign off on this - but can only test the staff client part of things.  Everything there works great!! Maybe someone else could test the long over due cron part?

a) _insert_accontlines() has a typo, it should be _insert_accountlines()

b) Can you please either rewrite chargelostitem() to use manualinvoice() for creating the accountline, or change manualinvoice() to use _insert_accontlines() as well?

c) You have chargelostitem() returning an accountline as a value, which can be either the processing fee or the lost fee, but not both even though both may be charged. This return value is not even used by any code. The simplest resolution would be to delete the line.

Thanks!

I read _insert_accountlines again and adapted manualinvoice to take its place. I also removed the *return* instruction at the end of chargelostitem() because, as you said, it wasn't used anywhere (except once in ChargeLostItem.t)

Thank you for your time!

(In reply to Charles Farmer from comment #8)
> I read _insert_accountlines again and adapted manualinvoice to take its
> place. I also removed the *return* instruction at the end of
> chargelostitem() because, as you said, it wasn't used anywhere (except once
> in ChargeLostItem.t)
> 
> Thank you for your time!

Thanks for those fixes!

Hi Simith,

starting with a code review here:

Results of the QA script:

 FAIL	C4/Accounts.pm
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 404)
		forbidden pattern: tab char (line 410)
		forbidden pattern: tab char (line 409)
		forbidden pattern: tab char (line 469)
		forbidden pattern: tab char (line 408)
		forbidden pattern: tab char (line 407)
		forbidden pattern: tab char (line 406)
   OK	  pod
   OK	  valid

Also some tests seem to be failing:

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 512 Tests: 60 Failed: 2)
  Failed tests:  43-44
  Non-zero exit status: 2


'Manual' code review:

1) Don't use double as the type, but decimal, as we are already using decimal in all other places where amounts are stored and double should not be used to store monetary values:

+  defaultreplacecost double(16,4) default NULL, -- default replacement cost
+  processfee double(16,4) default NULL, -- default text be recorded in the column note when the processing fee is applied

+ updatedatabase.pl as well (blocker)

2) Please amend POD of manualinvoice with the new type PF and the new parameter.
Can you explain what notifyid is used for? (normal)

3) Spotting a translation problem here:

 +            C4::Accounts::chargelostitem($issues, "Lost Item $issues->{'title'} $issues->{'barcode'}");

... but it was there before your patch.

4) Please include the updated schema files in a separate patch file.

Hi Katrin,

Following your advice, I changed a few things from the previous patches:

    * the double fields are now decimals
    * the method description now has PF with a short description.
    * a new patch now changes the database structure (atomicupdate + kohastruct + schema)

FWIW, the failing test you mentioned also fails on the current master. I believe this is out of the scope of these patches.

The tests pass ok for me on current master - can you try again on a database with your patch and all the sample data installed?

I just tried applying this patch to a branch up to date with the current master and there are merge conflicts.

Hi Eivin, if you find a problem while testing the patch you can set to 'failed qa' - or 'patch doesn't apply' if there are conflicts.

I set this back to Needs Signoff, as resolving the conflict was relatively trivial.

Test results:

After applying the patch I was unable to set "Charge", "Default replacement cost" or "Processing fee" for any item.
When clicking save the values were not changed but stayed 0.00.

I checked out master again to verify and I could then change the value for "Charge".

After manually altering the table with the commands in a/installer/data/mysql/atomicupdate/bug_12768-update_schema_files.sql
I got it to work. This may have been a local problem with my devbox.

Test scenario 1:
When setting to "lost" manually this seems to work as advertised. 
But as a side note: if changing the status for the item from "lost" back to "choose" the fines stays put.
So by setting an item to "lost" by mistake and then changing back the fines must also be manually handled.

When using the longoverdue.pl script the item is marked as Long overdue (lost) but there is no fine shown for the patron.

Test scenario 2:
Manual work as advertised.

When using the longoverdue.pl script the result is the same as in scenario 1. There is no fine.

Perhaps someone else could test the part using the script? If this works for them there  might be an issue with my dev env and I would be happy to sign off.

(In reply to Eivin Giske Skaaren from comment #29)

> After applying the patch I was unable to set "Charge", "Default replacement cost" or "Processing fee" for any item.
> After manually altering the table with the commands in a/installer/data/mysql/atomicupdate/bug_12768-update_schema_files.sql

The update worked fine on my end and I was able to edit the values.
Have you run the installer/data/mysql/updatedatabase.pl script?

> When using the longoverdue.pl script the item is marked as Long overdue
> (lost) but there is no fine shown for the patron.

> When using the longoverdue.pl script the result is the same as in scenario
> 1. There is no fine.

Did you run longoverdue.pl with the -c option?

--charge | -c This specifies what lost value triggers Koha to charge the account for the lost item.  Replacement costs are not charged if this is not specified.

Also, the patron can not be charged multiple times for losing the same item.
Even if you writeoff / pay the fine, it's still there in the DB.

C4/Accounts.pm
368    # first make sure the borrower hasn't already been charged for this item
369    my $sth1=$dbh->prepare("SELECT * from accountlines
370    WHERE borrowernumber=? AND itemnumber=? and accounttype='L'");

You will need to remove the lost status from the item and run something like this in MySQL if you want to test multiple times with the same borrower and item combination.
DELETE from accountlines WHERE borrowernumber=#BORROWERNUMBER# AND itemnumber=#ITEMNUMBER# and accounttype='L';

Thank you Maxime. The problem was that I used the same patron and the same item for the tests.

Created attachment 40000 [details] [review]
Bug 12768 - Updated schema files

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Created attachment 40001 [details] [review]
Bug 12768 - Replacement cost and processing fee management

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Created attachment 40002 [details] [review]
Bug 12768 [QA Followup] - Fix unit tests

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Created attachment 42157 [details] [review]
Bug 12768: qa follow-up

It does not seem to work if item-level_itypes  is set to biblio.
You should also provide tests for C4::Circulation::LostItem (I suspect that the problem comes from the query executed in this subroutine).

It may be out of scope for this bug, but it would be great if this could be set on a per branch basis.  My consortium includes a public library (which does charge a processing fee, and school libraries (who do not).

Thanks for considering this
Peggy

Created attachment 43181 [details] [review]
Bug 12768: Use the Price TT plugin to display prices

Retested also with item-level_itypes set to biblio and made sure to delete relevant rows from accountlines table when using same item and patron:

Scenario 1 works
Scenario 2 works

I have also manually tested the sql in C4::Circulation::LostItem and it seems to work from what I can see.
Adding a test for that in t/db_dependent/Circulation/Chargelostitem.t was not trivial.
I am not a test developer but if it really is needed someone with more experience developing mockups with the db should probably be able to do it.

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

I am setting this to signed off again even though there are no tests written for the C4::Circulation::LostItem subroutine. The test seems to need to mock several tables and someone experienced writing that type of tests for Koha should probably do it if it really is needed. I have tested thoroughly manually in GUI, console and mysql client.

Peggy: This patch has already consumed considerable time and perhaps it is better to not increase its scope at this stage. My suggestion is that you file a new bug with the wanted enhancements and refer to this one.

Eivin,
It seems that my concerns on comment 38 have not been addressed.

(In reply to Eivin Giske Skaaren from comment #40)
> Adding a test for that in t/db_dependent/Circulation/Chargelostitem.t was
> not trivial.

You can also have a look at t/db_dependent/Circulation.t

> I am not a test developer

Of course you are, if you are a developer, you can are a test developer :)

Hi Jonathan,

Perhaps a clarification:

Yes I did look at that example but to develop tests for C4::Circulation::LostItem seemed more complex.

Anyway I am not the original dev of this patch and will need to talk to the sponsor before putting more time into it.

Created attachment 43546 [details] [review]
Bug 12768: Fixed tests 44 and 45 of t/db_dependant/Circulation.t

(In reply to Jonathan Druart from comment #38)
> It does not seem to work if item-level_itypes  is set to biblio.
> You should also provide tests for C4::Circulation::LostItem (I suspect that
> the problem comes from the query executed in this subroutine).

When you say "provide tests for LostItem", do you mean that a separate file should be written under t/db_dependent/Circulation/ for that purpose, or would you accept if the tests were added to everything else in Circulation.t ?

(In reply to Charles Farmer from comment #45)
> (In reply to Jonathan Druart from comment #38)
> > It does not seem to work if item-level_itypes  is set to biblio.
> > You should also provide tests for C4::Circulation::LostItem (I suspect that
> > the problem comes from the query executed in this subroutine).
> 
> When you say "provide tests for LostItem", do you mean that a separate file
> should be written under t/db_dependent/Circulation/ for that purpose, or
> would you accept if the tests were added to everything else in Circulation.t
> ?

The last patch could do the job, if the itemtype type you used (BK) was created (tests should create their own data).
But thinking about it, I am wondering why you pass the processfee to chargelostitem and don't retrieve it in the chargelostitem subroutine.
It also a bit weird to pass an issue, the name of the sub is chargelost*item* :)

Kyle, could I get your opinion on that please?

Comment on attachment 40000 [details] [review]
Bug 12768 - Updated schema files

Review of attachment 40000 [details] [review]:
-----------------------------------------------------------------

::: Koha/Schema/Result/Itemtype.pm
@@ +37,4 @@
>  
>  =head2 rentalcharge
>  
> +  data_type: 'decimal'

itemtypes.rentalcharge is changed from double to decimal in Schema and kohastructure.pl, but no corrosponding change is made in bug_12768-update_schema_files.sql

> Kyle, could I get your opinion on that please?

I agree. This change makes the LostItem far more complicated. Using the plural form "issues" adds even more confusion. Please restore the prototype for LostItem to its original form and fetch the processing fee from the itemtype within chargelostitem.

> Kyle, could I get your opinion on that please?

I agree. This change makes the LostItem far more complicated. Using the plural form "issues" adds even more confusion. Please restore the prototype for LostItem to its original form and fetch the processing fee from the itemtype within chargelostitem.

Is this one still being worked on?

Failing QA according to previous comments.

Created attachment 64775 [details] [review]
Bug 12768 - Updated schema files

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Created attachment 64776 [details] [review]
Bug 12768 - Replacement cost and processing fee management

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Rebased, addressed QA concerns, squashed for readability, added more tests

Created attachment 66760 [details] [review]
Bug 12768 - Updated schema files

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 66761 [details] [review]
Bug 12768 - Replacement cost and processing fee management

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Tests do not pass!

t/db_dependent/Circulation/Chargelostitem.t .. Can't locate C4/Branch.pm in @INC (you may need to install the C4::Branch module)

C4::Branch has been removed months ago.

Created attachment 67875 [details] [review]
Bug 12768 - Updated schema files

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 67876 [details] [review]
Bug 12768 - Replacement cost and processing fee management

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 67877 [details] [review]
Bug 12768 - Fix up unit tests

Created attachment 67878 [details] [review]
Bug 12768 - Revert changes to getcharges

RM: I did a fair amount of fixing up on this ( though I didn't modify any of the main patch code, mostly unit tests ). So feel free to require another qa'er if you feel it is needed!

Created attachment 67891 [details] [review]
Bug 12768 - DB changes

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 67892 [details] [review]
Bug 12768 - Replacement cost and processing fee management

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 67893 [details] [review]
Bug 12768 - Fix up unit tests

Created attachment 67894 [details] [review]
Bug 12768 - Revert changes to getcharges

Created attachment 67895 [details] [review]
Bug 12768: Add missing IGNORE clauses

Created attachment 67896 [details] [review]
Bug 12768: Fix YAML syntax in circulation.pref

Created attachment 67897 [details] [review]
Bug 12768: DBIC Schema changes

1. accounttype "PF" must be handle in the SWITCH block in templates
2. Any chances to see the tests cleaned a bit? There are a lot of c/p
3. I got an error, to reproduce:
 useDefaultReplacementCost set to "Use"
 itemtype.defaultreplacecost = 1
 itemtype.processfee = 2
Check an item out
Mark it as lost
=> 2 fines are created (L=1, PF=2)
Use the "pay selected" button

{UNKNOWN}: Can't call method "unblessed" on an undefined value at /home/vagrant/kohaclone/C4/Circulation.pm line 2177. at /home/vagrant/kohaclone/C4/Circulation.pm line 2194

May be a bug in master.

Note: sounds like the "notify" code is not used (see bug 10021).

Created attachment 68035 [details] [review]
Bug 12768 - DB changes

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 68036 [details] [review]
Bug 12768 - Replacement cost and processing fee management

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 68037 [details] [review]
Bug 12768 - Fix up unit tests

Created attachment 68038 [details] [review]
Bug 12768 - Revert changes to getcharges

Created attachment 68039 [details] [review]
Bug 12768: Add missing IGNORE clauses

Created attachment 68040 [details] [review]
Bug 12768: Fix YAML syntax in circulation.pref

Created attachment 68041 [details] [review]
Bug 12768: DBIC Schema changes

Created attachment 68042 [details] [review]
Bug 12768 - Fix up unit tests a bit more

Created attachment 68076 [details] [review]
Bug 12768 [QA Followup] - Add 'Processing fee' to templates

At this point I feel that a second qa'er is needed. I'm moving this back to signed-off.

I'd like to push these patches along with bug 14826.

Created attachment 68264 [details] [review]
Bug 12768 - (QA Followup) Fix doubled code

Created attachment 68265 [details] [review]
Bug 12768 - DB changes

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 68266 [details] [review]
Bug 12768 - Replacement cost and processing fee management

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 68267 [details] [review]
Bug 12768 - Fix up unit tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 68268 [details] [review]
Bug 12768 - Revert changes to getcharges

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 68269 [details] [review]
Bug 12768: Add missing IGNORE clauses

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 68270 [details] [review]
Bug 12768: Fix YAML syntax in circulation.pref

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 68271 [details] [review]
Bug 12768: DBIC Schema changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 68272 [details] [review]
Bug 12768 - Fix up unit tests a bit more

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 68273 [details] [review]
Bug 12768 [QA Followup] - Add 'Processing fee' to templates

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 68274 [details] [review]
Bug 12768 - (QA Followup) Fix doubled code

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Tests passed, code makes sense, worked in my testing, passes QA tools.

Moving to PQA

Created attachment 68332 [details] [review]
Bug 12768: (follow-up) Add 'Processing fee' to templates

(In reply to Nick Clemens from comment #93)
> Created attachment 68274 [details] [review] [review]
> Bug 12768 - (QA Followup) Fix doubled code
> 
> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

This last patches changes a behaviour:
Before, chargelostitem created a Koha::Account::Offset with type="Lost Item", now it is "Manual Debit".

What do we want?

Fix capitalization? :)

Pushed to master for 17.11, thanks to everybody involved!

(In reply to Jonathan Druart from comment #96)
> (In reply to Nick Clemens from comment #93)
> > Created attachment 68274 [details] [review] [review] [review]
> > Bug 12768 - (QA Followup) Fix doubled code
> > 
> > Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
> 
> This last patches changes a behaviour:
> Before, chargelostitem created a Koha::Account::Offset with type="Lost
> Item", now it is "Manual Debit".
> 
> What do we want?

(In reply to Katrin Fischer from comment #97)
> Fix capitalization? :)

Please take care of that with a follow-up asap.

Just wondering, if the values are not supposed to be translated, maybe an ENUM instead of the separate table or having a code that can later be translated in the templates? "Payment Reversed" seems like an odd PK and some of the values are codes in accountlines (F, FU, ...), so we might want them to match.
Can file a separate bug if this is not totally off.

Hi, could you please write up instructions for this for the manual? You can either do a merge request or send me text and screenshots:
https://wiki.koha-community.org/wiki/Editing_the_Koha_Manual

I am not sure yet how the note pref works, having an example there would be nice!

Was this change expected?

-  rentalcharge double(16,4) default NULL, -- the amount charged when this item is checked out/issued
+  rentalcharge decimal(28,6) default NULL, -- the amount charged when this item is checked out/issued

Created attachment 68415 [details] [review]
Bug 12768 [QA Followup] - Use specific account offset types for Processing Fee and Lost Item

Created attachment 68416 [details] [review]
Bug 12768: Fix tests - itemtypes.rentalcharge is now decimal(28,6)

Created attachment 68417 [details] [review]
Bug 12768 [QA Followup] - Use specific account offset types for Processing Fee and Lost Item

Created attachment 68426 [details] [review]
Bug 12768 [QA Followup] - Only set non-Manual Invoice offset type if manualinvoice isn't actually being used for manual invoices

Created attachment 68427 [details] [review]
Bug 12768 [QA Followup] - Don't use manualinvoice for non-manual invoices in chargelostitem

Last patches have been pushed to master:

276b0e2587 Bug 12768: Fix tests - itemtypes.rentalcharge is now decimal(28,6)
1ea5f9c82a Bug 12768: (QA follow-up) Use specific account offset types for Processing Fee and Lost Item
208500193e Bug 12768: DBRev 17.06.00.019
c63d560b04 Bug 12768: (QA follow-up) Don't use manualinvoice for non-manual invoices in chargelostitem