Bug 12768

Summary: Replacement cost and processing fee management
Product: Koha Reporter: Francois Charbonnier <francois.charbonnier>
Component: System AdministrationAssignee: Maxime Beaulieu <maxime.beaulieu>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andreas.hedstrom.mace, charles.farmer, chris, eivin, eric.begin, gmcharlt, jonathan.druart, jsasse, katrin.fischer, kyle.m.hall, kyle, marjorie.barry-vila, maxime.beaulieu, mtompset, nick, p.thrasher, patrick.robitaille, philippe.blouin, tmisilo, viktor.sarge
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10021
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14826    
Bug Blocks: 20216, 21196    
Attachments: Bug 12768 - Replacement cost and processing fee management
Bug 12768 - Replacement cost and processing fee management
Bug 12768 - Replacement cost and processing fee management
Bug 12768 - Replacement cost and processing fee management
Bug 12768 - Replacement cost and processing fee management
Bug 12768 [QA Followup] - Fix unit tests
Bug 12768 [QA Followup] - Fix unit tests
[SIGNED-OFF] Bug 12768 - Replacement cost and processing fee management
Bug 12768 [QA Followup] - Fix unit tests
Bug 12768 - Replacement cost and processing fee management
Bug 12768 - Updated schema files
Bug 12768 - Replacement cost and processing fee management
Bug 12768 [QA Followup] - Fix unit tests
Bug 12768 - Updated schema files
Bug 12768 - Replacement cost and processing fee management
Bug 12768 [QA Followup] - Fix unit tests
Bug 12768 [QA Followup] - Fix unit tests
Bug 12768 - Updated schema files
Bug 12768 - Replacement cost and processing fee management
Bug 12768 [QA Followup] - Fix unit tests
Bug 12768: qa follow-up
Bug 12768: Use the Price TT plugin to display prices
Bug 12768: Use the Price TT plugin to display prices
Bug 12768: Fixed tests 44 and 45 of t/db_dependant/Circulation.t
Bug 12768 - Updated schema files
Bug 12768 - Replacement cost and processing fee management
Bug 12768 - Updated schema files
Bug 12768 - Replacement cost and processing fee management
Bug 12768 - Updated schema files
Bug 12768 - Replacement cost and processing fee management
Bug 12768 - Fix up unit tests
Bug 12768 - Revert changes to getcharges
Bug 12768 - DB changes
Bug 12768 - Replacement cost and processing fee management
Bug 12768 - Fix up unit tests
Bug 12768 - Revert changes to getcharges
Bug 12768: Add missing IGNORE clauses
Bug 12768: Fix YAML syntax in circulation.pref
Bug 12768: DBIC Schema changes
Bug 12768 - DB changes
Bug 12768 - Replacement cost and processing fee management
Bug 12768 - Fix up unit tests
Bug 12768 - Revert changes to getcharges
Bug 12768: Add missing IGNORE clauses
Bug 12768: Fix YAML syntax in circulation.pref
Bug 12768: DBIC Schema changes
Bug 12768 - Fix up unit tests a bit more
Bug 12768 [QA Followup] - Add 'Processing fee' to templates
Bug 12768 - (QA Followup) Fix doubled code
Bug 12768 - DB changes
Bug 12768 - Replacement cost and processing fee management
Bug 12768 - Fix up unit tests
Bug 12768 - Revert changes to getcharges
Bug 12768: Add missing IGNORE clauses
Bug 12768: Fix YAML syntax in circulation.pref
Bug 12768: DBIC Schema changes
Bug 12768 - Fix up unit tests a bit more
Bug 12768 [QA Followup] - Add 'Processing fee' to templates
Bug 12768 - (QA Followup) Fix doubled code
Bug 12768: (follow-up) Add 'Processing fee' to templates
Bug 12768 [QA Followup] - Use specific account offset types for Processing Fee and Lost Item
Bug 12768: Fix tests - itemtypes.rentalcharge is now decimal(28,6)
Bug 12768 [QA Followup] - Use specific account offset types for Processing Fee and Lost Item
Bug 12768 [QA Followup] - Only set non-Manual Invoice offset type if manualinvoice isn't actually being used for manual invoices
Bug 12768 [QA Followup] - Don't use manualinvoice for non-manual invoices in chargelostitem

Description Francois Charbonnier 2014-08-15 13:00:48 UTC
Sponsored by Halland County Library

RFC : http://wiki.koha-community.org/wiki/Overdue_Notice_Enhancement#Replacement_cost_and_processing_fee_management

Description

A default replacement cost and a processing fee will be added to the item type page. The input form and the table that displays all iten types will have these two new field/column :

    "Default replacement cost"
    "Processing fee (When lost)" 

Two system preferences will be added.

    "UseDefaultReplacementCost" to enable the use of the value set on the item type level.
    "ProcessingFeeNote" to set the value that will populate the note in the table "accountlines" when the processing fee are applied. 

These elements will be used as follows :

When the "longoverdue.pl" script is run :

If "UseDefaultReplacementCost" is enabled and the items.replacementcost = 0 :

    The value of "Default replacement cost" set up in the itemtype populate the "accountlines" table.
    If the value of "Processing fee (When lost)" is > 0, it is also used to populate the table "accountlines." The value in the system preference "ProcessingFeeNote" is used to update the note in accountlines for this item. Otherwise, no processing fees are added. 

If "UseDefaultReplacementCost" is enabled and the value in items.replacementcost> 0

    The default koha behavior is used. The value in items.replacementcost populate the "accountlines" table.
    If the value of "Processing fee (When lost)" defined in the document type is > 0, it is also used to populate the table "accountlines." The value in the system preference "ProcessingFeeNote" is used to update the note in accountlines for this item. Otherwise, no processing fees are added. 

If "UseDefaultReplacementCost" is disabled :

    The default koha behavior is used.
    If the value of "Processing fee (When lost)" defined in the document type is > 0, it is also used to populate the "accountlines" table. The value in the system preference "ProcessingFeeNote" is used to update the note in accountlines for this item. Otherwise, no processing fees are added.
Comment 1 Francois Charbonnier 2014-08-15 13:19:57 UTC
This feature will apply when the "longoverdue.pl" script is run OR when an item is marked "Lost" (the value in items.itemlost> 0).
Comment 2 simith.doliveira 2014-12-11 18:18:38 UTC
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.
Comment 3 simith.doliveira 2014-12-11 18:19:26 UTC Comment hidden (obsolete)
Comment 4 simith.doliveira 2014-12-11 19:07:24 UTC Comment hidden (obsolete)
Comment 5 Charles Farmer 2015-03-02 19:03:22 UTC Comment hidden (obsolete)
Comment 6 Nicole C. Engard 2015-03-04 10:03:23 UTC
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?
Comment 7 Kyle M Hall 2015-03-04 12:56:27 UTC
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!
Comment 8 Charles Farmer 2015-03-04 17:05:17 UTC
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!
Comment 9 Charles Farmer 2015-03-04 17:05:44 UTC Comment hidden (obsolete)
Comment 10 Charles Farmer 2015-03-04 17:08:30 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2015-03-05 08:04:21 UTC
(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!
Comment 12 Kyle M Hall 2015-03-05 10:47:20 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2015-03-05 10:48:37 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-03-05 10:49:29 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2015-03-05 10:50:03 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2015-04-21 22:18:25 UTC
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.
Comment 17 Charles Farmer 2015-04-22 20:15:50 UTC Comment hidden (obsolete)
Comment 18 Charles Farmer 2015-04-22 20:16:11 UTC Comment hidden (obsolete)
Comment 19 Charles Farmer 2015-04-22 20:23:47 UTC Comment hidden (obsolete)
Comment 20 Charles Farmer 2015-04-22 20:45:40 UTC Comment hidden (obsolete)
Comment 21 Charles Farmer 2015-04-22 20:56:19 UTC
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.
Comment 22 Katrin Fischer 2015-04-22 21:00:42 UTC
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?
Comment 23 Eivin Giske Skaaren 2015-05-17 20:39:22 UTC
I just tried applying this patch to a branch up to date with the current master and there are merge conflicts.
Comment 24 Katrin Fischer 2015-05-18 20:06:03 UTC
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.
Comment 25 Mark Tompsett 2015-05-19 13:41:46 UTC Comment hidden (obsolete)
Comment 26 Mark Tompsett 2015-05-19 13:41:54 UTC Comment hidden (obsolete)
Comment 27 Mark Tompsett 2015-05-19 13:42:02 UTC Comment hidden (obsolete)
Comment 28 Mark Tompsett 2015-05-19 13:43:36 UTC
I set this back to Needs Signoff, as resolving the conflict was relatively trivial.
Comment 29 Eivin Giske Skaaren 2015-06-01 18:48:57 UTC
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.
Comment 30 Maxime Beaulieu 2015-06-05 19:38:14 UTC
(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';
Comment 31 Eivin Giske Skaaren 2015-06-08 17:36:30 UTC
Thank you Maxime. The problem was that I used the same patron and the same item for the tests.
Comment 32 Eivin Giske Skaaren 2015-06-08 17:48:58 UTC Comment hidden (obsolete)
Comment 33 Eivin Giske Skaaren 2015-06-08 17:51:10 UTC
Created attachment 40000 [details] [review]
Bug 12768 - Updated schema files

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>
Comment 34 Eivin Giske Skaaren 2015-06-08 17:51:19 UTC
Created attachment 40001 [details] [review]
Bug 12768 - Replacement cost and processing fee management

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>
Comment 35 Eivin Giske Skaaren 2015-06-08 17:51:27 UTC
Created attachment 40002 [details] [review]
Bug 12768 [QA Followup] - Fix unit tests

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>
Comment 36 Jonathan Druart 2015-09-01 14:12:30 UTC
Created attachment 42157 [details] [review]
Bug 12768: qa follow-up
Comment 37 Jonathan Druart 2015-09-01 14:12:35 UTC Comment hidden (obsolete)
Comment 38 Jonathan Druart 2015-09-01 14:14:18 UTC
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).
Comment 39 Peggy Thrasher 2015-09-01 14:19:02 UTC
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
Comment 40 Eivin Giske Skaaren 2015-10-06 21:18:26 UTC
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>
Comment 41 Eivin Giske Skaaren 2015-10-06 21:28:48 UTC
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.
Comment 42 Jonathan Druart 2015-10-15 09:46:34 UTC
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 :)
Comment 43 Eivin Giske Skaaren 2015-10-15 10:48:58 UTC
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.
Comment 44 Charles Farmer 2015-10-16 21:44:27 UTC
Created attachment 43546 [details] [review]
Bug 12768: Fixed tests 44 and 45 of t/db_dependant/Circulation.t
Comment 45 Charles Farmer 2015-10-16 21:47:42 UTC
(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 ?
Comment 46 Jonathan Druart 2015-10-21 10:44:39 UTC
(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 47 Kyle M Hall 2015-10-21 11:50:43 UTC
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
Comment 48 Kyle M Hall 2015-10-21 12:00:04 UTC
> 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.
Comment 49 Kyle M Hall 2015-10-21 12:00:09 UTC
> 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.
Comment 50 Nick Clemens 2017-04-14 10:44:05 UTC
Is this one still being worked on?
Comment 51 Jonathan Druart 2017-04-17 12:31:58 UTC
Failing QA according to previous comments.
Comment 52 Nick Clemens 2017-07-03 14:29:12 UTC
Created attachment 64775 [details] [review]
Bug 12768 - Updated schema files

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>
Comment 53 Nick Clemens 2017-07-03 14:29:18 UTC
Created attachment 64776 [details] [review]
Bug 12768 - Replacement cost and processing fee management

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>
Comment 54 Nick Clemens 2017-07-03 14:33:40 UTC
Rebased, addressed QA concerns, squashed for readability, added more tests
Comment 55 Kyle M Hall 2017-09-01 16:18:18 UTC
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>
Comment 56 Kyle M Hall 2017-09-01 16:18:28 UTC
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>
Comment 57 Jonathan Druart 2017-09-04 15:19:51 UTC
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.
Comment 58 Kyle M Hall 2017-10-10 14:45:14 UTC
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>
Comment 59 Kyle M Hall 2017-10-10 14:45:24 UTC
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>
Comment 60 Kyle M Hall 2017-10-10 14:45:29 UTC
Created attachment 67877 [details] [review]
Bug 12768 - Fix up unit tests
Comment 61 Kyle M Hall 2017-10-10 14:45:34 UTC
Created attachment 67878 [details] [review]
Bug 12768 - Revert changes to getcharges
Comment 62 Kyle M Hall 2017-10-10 14:46:43 UTC
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!
Comment 63 Jonathan Druart 2017-10-10 20:30:10 UTC
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>
Comment 64 Jonathan Druart 2017-10-10 20:30:20 UTC
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>
Comment 65 Jonathan Druart 2017-10-10 20:30:26 UTC
Created attachment 67893 [details] [review]
Bug 12768 - Fix up unit tests
Comment 66 Jonathan Druart 2017-10-10 20:30:34 UTC
Created attachment 67894 [details] [review]
Bug 12768 - Revert changes to getcharges
Comment 67 Jonathan Druart 2017-10-10 20:30:41 UTC
Created attachment 67895 [details] [review]
Bug 12768: Add missing IGNORE clauses
Comment 68 Jonathan Druart 2017-10-10 20:30:49 UTC
Created attachment 67896 [details] [review]
Bug 12768: Fix YAML syntax in circulation.pref
Comment 69 Jonathan Druart 2017-10-10 20:33:19 UTC
Created attachment 67897 [details] [review]
Bug 12768: DBIC Schema changes
Comment 70 Jonathan Druart 2017-10-10 20:42:22 UTC
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.
Comment 71 Jonathan Druart 2017-10-10 21:34:25 UTC
Note: sounds like the "notify" code is not used (see bug 10021).
Comment 72 Kyle M Hall 2017-10-12 16:56:35 UTC
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>
Comment 73 Kyle M Hall 2017-10-12 16:56:45 UTC
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>
Comment 74 Kyle M Hall 2017-10-12 16:56:50 UTC
Created attachment 68037 [details] [review]
Bug 12768 - Fix up unit tests
Comment 75 Kyle M Hall 2017-10-12 16:56:55 UTC
Created attachment 68038 [details] [review]
Bug 12768 - Revert changes to getcharges
Comment 76 Kyle M Hall 2017-10-12 16:57:00 UTC
Created attachment 68039 [details] [review]
Bug 12768: Add missing IGNORE clauses
Comment 77 Kyle M Hall 2017-10-12 16:57:04 UTC
Created attachment 68040 [details] [review]
Bug 12768: Fix YAML syntax in circulation.pref
Comment 78 Kyle M Hall 2017-10-12 16:57:09 UTC
Created attachment 68041 [details] [review]
Bug 12768: DBIC Schema changes
Comment 79 Kyle M Hall 2017-10-12 16:57:14 UTC
Created attachment 68042 [details] [review]
Bug 12768 - Fix up unit tests a bit more
Comment 80 Kyle M Hall 2017-10-13 14:03:02 UTC
Created attachment 68076 [details] [review]
Bug 12768 [QA Followup] - Add 'Processing fee' to templates
Comment 81 Kyle M Hall 2017-10-13 14:16:32 UTC
At this point I feel that a second qa'er is needed. I'm moving this back to signed-off.
Comment 82 Jonathan Druart 2017-10-17 14:52:26 UTC
I'd like to push these patches along with bug 14826.
Comment 83 Nick Clemens 2017-10-18 15:52:24 UTC
Created attachment 68264 [details] [review]
Bug 12768 - (QA Followup) Fix doubled code
Comment 84 Nick Clemens 2017-10-18 16:43:13 UTC
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>
Comment 85 Nick Clemens 2017-10-18 16:43:19 UTC
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>
Comment 86 Nick Clemens 2017-10-18 16:43:25 UTC
Created attachment 68267 [details] [review]
Bug 12768 - Fix up unit tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 87 Nick Clemens 2017-10-18 16:43:30 UTC
Created attachment 68268 [details] [review]
Bug 12768 - Revert changes to getcharges

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 88 Nick Clemens 2017-10-18 16:43:35 UTC
Created attachment 68269 [details] [review]
Bug 12768: Add missing IGNORE clauses

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 89 Nick Clemens 2017-10-18 16:43:40 UTC
Created attachment 68270 [details] [review]
Bug 12768: Fix YAML syntax in circulation.pref

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 90 Nick Clemens 2017-10-18 16:43:45 UTC
Created attachment 68271 [details] [review]
Bug 12768: DBIC Schema changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 91 Nick Clemens 2017-10-18 16:43:49 UTC
Created attachment 68272 [details] [review]
Bug 12768 - Fix up unit tests a bit more

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 92 Nick Clemens 2017-10-18 16:43:55 UTC
Created attachment 68273 [details] [review]
Bug 12768 [QA Followup] - Add 'Processing fee' to templates

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 93 Nick Clemens 2017-10-18 16:43:59 UTC
Created attachment 68274 [details] [review]
Bug 12768 - (QA Followup) Fix doubled code

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 94 Nick Clemens 2017-10-18 16:45:03 UTC
Tests passed, code makes sense, worked in my testing, passes QA tools.

Moving to PQA
Comment 95 Jonathan Druart 2017-10-20 19:35:35 UTC
Created attachment 68332 [details] [review]
Bug 12768: (follow-up) Add 'Processing fee' to templates
Comment 96 Jonathan Druart 2017-10-20 19:50:00 UTC
(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?
Comment 97 Katrin Fischer 2017-10-20 19:51:06 UTC
Fix capitalization? :)
Comment 98 Jonathan Druart 2017-10-20 20:15:23 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 99 Jonathan Druart 2017-10-20 20:23:30 UTC
(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.
Comment 100 Katrin Fischer 2017-10-20 21:33:23 UTC
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.
Comment 101 Katrin Fischer 2017-10-22 10:00:31 UTC
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!
Comment 102 Jonathan Druart 2017-10-23 14:53:30 UTC
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
Comment 103 Kyle M Hall 2017-10-23 15:02:14 UTC
Created attachment 68415 [details] [review]
Bug 12768 [QA Followup] - Use specific account offset types for Processing Fee and Lost Item
Comment 104 Jonathan Druart 2017-10-23 15:05:34 UTC
Created attachment 68416 [details] [review]
Bug 12768: Fix tests - itemtypes.rentalcharge is now decimal(28,6)
Comment 105 Kyle M Hall 2017-10-23 15:18:34 UTC
Created attachment 68417 [details] [review]
Bug 12768 [QA Followup] - Use specific account offset types for Processing Fee and Lost Item
Comment 106 Kyle M Hall 2017-10-23 16:36:00 UTC
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
Comment 107 Kyle M Hall 2017-10-23 17:09:04 UTC
Created attachment 68427 [details] [review]
Bug 12768 [QA Followup] - Don't use manualinvoice for non-manual invoices in chargelostitem
Comment 108 Jonathan Druart 2017-10-23 17:50:17 UTC
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