Bug 15675

Summary: Add issue_id column to accountlines and use it for updating fines
Product: Koha Reporter: Kyle M Hall <kyle>
Component: CirculationAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: abl, barton, brendan, gmcharlt, jonathan.druart+koha, josef.moravec, julian.maurice, katrin.fischer, kyle.m.hall, martin.renvoize, mirko, nengard, nicole, veron
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16378
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5216
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15732, 15895, 16378, 16496, 17135    
Attachments: Bug 15675 - Add issue_id column to accountlines and use it for updating fines
Bug 15675 - Update Schema file
Bug 15675 - Fix some missed tab characters
Bug 15675 - Fix some missed tab characters, remove old FIXMEs
Bug 15675 - Add issue_id column to accountlines and use it for updating fines
Bug 15675 - Update Schema file
Bug 15675 - Fix some missed tab characters, remove old FIXMEs
Bug 15675 - Fix some missed tab characters, remove old FIXMEs
Bug 15675 - Add issue_id column to accountlines and use it for updating fines
Bug 15675 - Update Schema file
Bug 15675 - Fix some missed tab characters, remove old FIXMEs
Bug 15675 [QA Followup] - Close out accruing fines that are not really accruing, add issue_id to accruing fines
Bug 15675 [QA Followup] - Close out accruing fines that are not really accruing, add issue_id to accruing fines
Bug 15675 [QA Followup] - Switch from NOT IN to LEFT JOIN
Bug 15675 - Add issue_id column to accountlines and use it for updating fines
Bug 15675 - Update Schema file
Bug 15675 - Fix some missed tab characters, remove old FIXMEs
Bug 15675 [QA Followup] - Close out accruing fines that are not really accruing, add issue_id to accruing fines
Bug 15675 [QA Followup] - Switch from NOT IN to LEFT JOIN
Bug 15675: Fix conflict with bug 15446 (type vs _type)

Description Kyle M Hall 2016-01-26 16:45:00 UTC
Right now, fines are updated based on the fine description. There are a number of areas where this can go wrong ( date or time format changing, title being modified, etc ). Now that issues has a unique identifier, we should use that for selection and updating of fines.
Comment 1 Kyle M Hall 2016-01-26 16:45:25 UTC
*** Bug 15334 has been marked as a duplicate of this bug. ***
Comment 2 Kyle M Hall 2016-01-26 17:24:46 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2016-01-26 17:24:56 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2016-01-26 17:31:57 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2016-01-26 17:35:15 UTC Comment hidden (obsolete)
Comment 6 Marc Véron 2016-01-28 10:22:22 UTC Comment hidden (obsolete)
Comment 7 Marc Véron 2016-01-28 10:23:19 UTC Comment hidden (obsolete)
Comment 8 Marc Véron 2016-01-28 10:27:32 UTC Comment hidden (obsolete)
Comment 9 Mirko Tietgen 2016-01-28 10:33:58 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2016-01-28 10:35:44 UTC
Hi Kyle,
after thinking some about this, how are we going to deal with multiple fine lines for the same issue? If the item is renewed the due date in issues will change and a new fine line should be generated. The old due date information will then be lost. I wonder if we should store the due date that the fine was generated for additionally to the link in accountlines - in a separate date column.
Comment 11 Mirko Tietgen 2016-01-28 10:41:12 UTC Comment hidden (obsolete)
Comment 12 Mirko Tietgen 2016-01-28 10:41:18 UTC Comment hidden (obsolete)
Comment 13 Mirko Tietgen 2016-01-28 10:41:24 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2016-01-29 11:36:19 UTC
Could you please detail what do we need this new column?
1/ There is no fk on it
2/ can be null but C4::Overdues::UpdateFine requires it
3/ The prototype of UpdateFine is weird, you have to pass issue_id, itemnumber and borrowernumber. issue_id should be enough.
Comment 15 Jonathan Druart 2016-01-29 11:36:45 UTC
(In reply to Katrin Fischer from comment #10)
> Hi Kyle,
> after thinking some about this, how are we going to deal with multiple fine
> lines for the same issue? If the item is renewed the due date in issues will
> change and a new fine line should be generated. The old due date information
> will then be lost. I wonder if we should store the due date that the fine
> was generated for additionally to the link in accountlines - in a separate
> date column.

Katrin, since there is no constraint on this new column, I don't understand your comment.
Comment 16 Katrin Fischer 2016-01-29 12:03:27 UTC
Hi Joubu,
not really a database structure problem I was worried about, having the issue_id certainly makes sense and no FK seems fine as the id can be in issues or old_issues.

I was wondering as this is no 1:1 but a 1:n if the calcuation will be correct. If it will still add another separate line in accountlines if an item goes overdue - renew - overdue - renew... etc. If the due date is removed from the description or no longer used, this information will get lost etc.
Comment 17 Kyle M Hall 2016-01-29 16:13:37 UTC
(In reply to Jonathan Druart from comment #14)
> Could you please detail what do we need this new column?
> 1/ There is no fk on it

Right, as Katrin wrote, since the issue_id can be in issues or old_issues, we can't give it a key constraint.

> 2/ can be null but C4::Overdues::UpdateFine requires it

I suppose we *could* make it not null, but then how do we deal with adding an issue_id to all the already existing account lines? We can definitely update the ones that are currently accruing. Those are the only ones where it really matters.

> 3/ The prototype of UpdateFine is weird, you have to pass issue_id,
> itemnumber and borrowernumber. issue_id should be enough.

Agreed, I should change that. I was just trying to make the smallest change possible. It would make sense for it to just take issue_id ( or an issue object if possible ). If you'd like to see that change just set the status to failed qa and let me know!
Comment 18 Kyle M Hall 2016-01-29 16:17:38 UTC
(In reply to Katrin Fischer from comment #16)
> I was wondering as this is no 1:1 but a 1:n if the calcuation will be
> correct. If it will still add another separate line in accountlines if an
> item goes overdue - renew - overdue - renew... etc. If the due date is
> removed from the description or no longer used, this information will get
> lost etc.

You are right, it is a one to many join from issues to accountlines. One item may go overdue multiple times. However, there should only ever be one *accruing* fine for a given issue at a time, so it shouldn't be a problem. The current query already deals with that in that the FU fine will be the first selected and matched by issue_id.
Comment 19 Kyle M Hall 2016-01-29 16:44:02 UTC
Per conversation with Katrin, we should make sure to clean up any multiple FU fines that haven't been closed out in a db update for this. Setting to failed qa until I get that done.
Comment 20 Kyle M Hall 2016-02-03 15:28:09 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2016-02-03 15:32:23 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2016-02-09 11:32:03 UTC
The "NOT IN" will be terribly slow on this table. Try using a left join instead.
Comment 23 Kyle M Hall 2016-02-09 14:39:32 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2016-02-29 17:01:47 UTC
Created attachment 48468 [details] [review]
Bug 15675 - Add issue_id column to accountlines and use it for updating fines

Right now, fines are updated based on the fine description. There are a
number of areas where this can go wrong ( date or time format changing,
title being modified, etc ). Now that issues has a unique
identifier, we should use that for selection and updating of fines.

Test Plan:
1) Apply this patch
2) Test creating and updating fines via fines.pl
   and checking in overdue items. No changes should be noted.
3) prove t/db_dependent/Circulation.t

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 25 Jonathan Druart 2016-02-29 17:01:56 UTC
Created attachment 48469 [details] [review]
Bug 15675 - Update Schema file

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 26 Jonathan Druart 2016-02-29 17:02:03 UTC
Created attachment 48470 [details] [review]
Bug 15675 - Fix some missed tab characters, remove old FIXMEs

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 27 Jonathan Druart 2016-02-29 17:02:09 UTC
Created attachment 48471 [details] [review]
Bug 15675 [QA Followup] - Close out accruing fines that are not really accruing, add issue_id to accruing fines
Comment 28 Jonathan Druart 2016-02-29 17:02:18 UTC
Created attachment 48472 [details] [review]
Bug 15675 [QA Followup] - Switch from NOT IN to LEFT JOIN
Comment 29 Jonathan Druart 2016-02-29 17:02:32 UTC
Created attachment 48473 [details] [review]
Bug 15675: Fix conflict with bug 15446 (type vs _type)
Comment 30 Brendan Gallagher 2016-03-02 03:45:03 UTC
Pushed to Master - Should be in the May 2016.  Thanks
Comment 31 Julian Maurice 2016-03-07 11:00:46 UTC
It looks like an enhancement rather than a bug. Should it be backported to 3.22 ?
Comment 32 Kyle M Hall 2016-03-07 14:14:36 UTC
*** Bug 15310 has been marked as a duplicate of this bug. ***
Comment 33 Katrin Fischer 2016-03-07 20:50:41 UTC
Hm, feels a bit big for a maintenance release, might be worth the wait.
Comment 34 Julian Maurice 2016-03-08 06:22:26 UTC
(In reply to Katrin Fischer from comment #33)
> Hm, feels a bit big for a maintenance release, might be worth the wait.

That was my thinking too, thanks for the confirmation
Comment 35 Jacek Ablewicz 2016-04-28 10:49:50 UTC
One pretty nasty regression got introduced by this bug, see Bug 16378 for details.
Comment 36 Katrin Fischer 2016-08-19 09:06:57 UTC
Please also take a look at bug 17135 - glad this didn't go into stable yet.
Comment 37 Jacek Ablewicz 2016-08-19 09:23:45 UTC
(In reply to Katrin Fischer from comment #36)
> glad this didn't go into stable yet.

I was under the impression that it did (comment #30), stable == 16.05 right now?
Comment 38 Katrin Fischer 2016-08-19 09:25:09 UTC
You are correct - I got my versions confused :(
Comment 39 Jacek Ablewicz 2016-08-19 09:35:42 UTC
Had a look at 16.05.x branch on git to be sure, and yep, 16.05 is definitely affected by Bug 17135 too. Would be good to fix this issue ASAP, before angry mob of librarians shows up (with the pitchforks ;)