Bug 15675 - Add issue_id column to accountlines and use it for updating fines
Summary: Add issue_id column to accountlines and use it for updating fines
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
: 15310 15334 (view as bug list)
Depends on:
Blocks: 15732 15895 16378 16496 17135
  Show dependency treegraph
 
Reported: 2016-01-26 16:45 UTC by Kyle M Hall
Modified: 2019-02-22 10:50 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15675 - Add issue_id column to accountlines and use it for updating fines (17.86 KB, patch)
2016-01-26 17:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15675 - Update Schema file (1.43 KB, patch)
2016-01-26 17:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15675 - Fix some missed tab characters (1.28 KB, patch)
2016-01-26 17:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15675 - Fix some missed tab characters, remove old FIXMEs (2.00 KB, patch)
2016-01-26 17:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15675 - Add issue_id column to accountlines and use it for updating fines (17.99 KB, patch)
2016-01-28 10:22 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15675 - Update Schema file (1.43 KB, patch)
2016-01-28 10:23 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15675 - Fix some missed tab characters, remove old FIXMEs (2.13 KB, patch)
2016-01-28 10:27 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15675 - Fix some missed tab characters, remove old FIXMEs (2.05 KB, patch)
2016-01-28 10:33 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 15675 - Add issue_id column to accountlines and use it for updating fines (18.04 KB, patch)
2016-01-28 10:41 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 15675 - Update Schema file (1.48 KB, patch)
2016-01-28 10:41 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 15675 - Fix some missed tab characters, remove old FIXMEs (2.18 KB, patch)
2016-01-28 10:41 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 15675 [QA Followup] - Close out accruing fines that are not really accruing, add issue_id to accruing fines (1.25 KB, patch)
2016-02-03 15:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15675 [QA Followup] - Close out accruing fines that are not really accruing, add issue_id to accruing fines (1.48 KB, patch)
2016-02-03 15:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15675 [QA Followup] - Switch from NOT IN to LEFT JOIN (1.77 KB, patch)
2016-02-09 14:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15675 - Add issue_id column to accountlines and use it for updating fines (18.05 KB, patch)
2016-02-29 17:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15675 - Update Schema file (1.48 KB, patch)
2016-02-29 17:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15675 - Fix some missed tab characters, remove old FIXMEs (2.18 KB, patch)
2016-02-29 17:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15675 [QA Followup] - Close out accruing fines that are not really accruing, add issue_id to accruing fines (1.48 KB, patch)
2016-02-29 17:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15675 [QA Followup] - Switch from NOT IN to LEFT JOIN (1.77 KB, patch)
2016-02-29 17:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15675: Fix conflict with bug 15446 (type vs _type) (1.28 KB, patch)
2016-02-29 17:02 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 ;)