Bug 10490 - Overdue fines cap (amount) in circulation rules is saved incorrectly (rounded)
Summary: Overdue fines cap (amount) in circulation rules is saved incorrectly (rounded)
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Katrin Fischer
QA Contact:
Depends on: 7420
  Show dependency treegraph
Reported: 2013-06-18 16:49 UTC by Katrin Fischer
Modified: 2014-12-07 20:02 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:

Bug 10490: Overdue fines cap can't store decimal values (3.48 KB, patch)
2013-07-01 20:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10490: Overdue fines cap can't store decimal values (3.52 KB, patch)
2013-07-10 03:52 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 10490: Overdue fines cap can't store decimal values (3.57 KB, patch)
2013-07-12 15:12 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 Katrin Fischer 2013-06-18 16:49:49 UTC
To test:

1) Add a new circulation rule, give 4.50 for overdue fines cap.

> Verify the system saves it to be 5.00

2) Edit an existing circulation rule, give 3.40 for overdue fines cap.

> Verify the system saves it to be 3.00

Obviously, that's not good and especially bad because most of our libraries have standardized on 4.50.

Maybe there is a problem with the datatype definition in the kohastructure.sql?

We have:
overduefinescap decimal default NULL, -- the maximum amount of an overdue fine

In opposite to:
reservecharge decimal(28,6) default NULL,

I couldn't test it, but the definition is the same for kohastructure and update database. So a bit worried that we might change the data here on update instead of copying it correctly from the system preference into issuingrules.

The feature was added with bug 7420.
Comment 1 Galen Charlton 2013-06-19 19:14:59 UTC
Nice catch.  Also, oops.  Also, we really ought to be counting everything in pennies and not use decimal data types for currencies.

Regardless, I think altering the column type to decimal(28,6) should do the trick and preserve existing values:

MariaDB [koha]> create table tmp (overduefinescap decimal null);
Query OK, 0 rows affected (0.05 sec)

MariaDB [koha]> insert into tmp values (1.25);
Query OK, 1 row affected, 1 warning (0.00 sec)

MariaDB [koha]> select * from tmp;
| overduefinescap |
|               1 |
1 row in set (0.00 sec)

MariaDB [koha]> alter table tmp modify column overduefinescap decimal(28,6) null; 
Query OK, 1 row affected (0.06 sec)                
Records: 1  Duplicates: 0  Warnings: 0

MariaDB [koha]> select * from tmp;
| overduefinescap |
|        1.000000 |
1 row in set (0.00 sec)
Comment 2 Katrin Fischer 2013-07-01 20:12:53 UTC Comment hidden (obsolete)
Comment 3 Srdjan Jankovic 2013-07-10 03:52:11 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2013-07-12 14:47:26 UTC
I don't think the patch should be altering a past database update the way it is. I don't believe it would have any negative effects, but I've had patches fail qa for this reason in the past.
Comment 5 Katrin Fischer 2013-07-12 14:55:40 UTC
Kyle, if we don't alter the old database update we will have people losing data. If your maxfine was 4.5 it will be 5 in the circulation matrix on update with the maxfine being lost. That's why I chose to also update the old updatedatabase. Running the alter database for already correct installations won't change a thing for them.
Comment 6 Kyle M Hall 2013-07-12 15:12:36 UTC
Created attachment 19601 [details] [review]
Bug 10490: Overdue fines cap can't store decimal values

When overduefinescap was added to the issuingrules the datatype
given was decimal. This translates in MySQL to decimal(10,0).
This doesn't allow you to store decimal values and therefore
values like 4.5 are saved as 5 in the database.

To test:

On a current installation:
1) Try to enter 4.5 as Overdue fines cap. Verify that the value
   is not correctly saved.
2) Apply patch and run database update.
3) Try adding/changing an issuing rule setting Overdue fines cap
   to 4.5 again.
4) Verify the value is saved correctly.

Create a new Koha installation from scratch:
1) Verify that the issuingrules table has been created correctly
   and that you can add/mofidy issuingrules correctly.

Because this bug can create data loss, the old database update
has also been changed to avoid this problem for people updating
at a later point in time. Checkout an older version of Koha

1) Run the database updates.
2) Verify again, that adding/modifying issuingrules works correctly.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Galen Charlton 2013-07-12 20:16:26 UTC
Pushed to master.  Thanks, Katrin!
Comment 8 Tomás Cohen Arazi 2013-07-16 19:10:19 UTC
This patch has been pushed to 3.12.x, will be in 3.12.2.

Thanks Katrin!
Comment 9 Bernardo Gonzalez Kriegel 2013-08-04 19:57:34 UTC
Pushed to 3.10.x, will be in 3.10.10
Comment 10 Chris Hall 2013-08-18 05:33:29 UTC
Pushed to 3.8.x, will be in 3.8.17
Comment 11 Jonathan Druart 2013-09-02 12:16:22 UTC
(In reply to Chris Hall from comment #10)
> Pushed to 3.8.x, will be in 3.8.17

Chris, This patch produces an error (see http://lists.koha-community.org/pipermail/koha-devel/2013-September/039649.html) updating the database. Since bug 7420 is not pushed in 3.8.x, I think this fix should not be pushed.