Bug 15675

*** Bug 15334 has been marked as a duplicate of this bug. ***

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.

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.

(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.

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.

(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!

(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.

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.

The "NOT IN" will be terribly slow on this table. Try using a left join instead.

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>

Created attachment 48469 [details] [review]
Bug 15675 - Update Schema file

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

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>

Created attachment 48471 [details] [review]
Bug 15675 [QA Followup] - Close out accruing fines that are not really accruing, add issue_id to accruing fines

Created attachment 48472 [details] [review]
Bug 15675 [QA Followup] - Switch from NOT IN to LEFT JOIN

Created attachment 48473 [details] [review]
Bug 15675: Fix conflict with bug 15446 (type vs _type)

Pushed to Master - Should be in the May 2016.  Thanks

It looks like an enhancement rather than a bug. Should it be backported to 3.22 ?

*** Bug 15310 has been marked as a duplicate of this bug. ***

Hm, feels a bit big for a maintenance release, might be worth the wait.

(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

One pretty nasty regression got introduced by this bug, see Bug 16378 for details.

Please also take a look at bug 17135 - glad this didn't go into stable yet.

(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?

You are correct - I got my versions confused :(

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 ;)