Bug 22200 - Forgiving a fine (FOR) does not create a FORGIVEN credit line
Summary: Forgiving a fine (FOR) does not create a FORGIVEN credit line
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Martin Renvoize
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 22511
Blocks: 22563
  Show dependency treegraph
 
Reported: 2019-01-23 19:53 UTC by Kyle M Hall
Modified: 2019-04-26 15:56 UTC (History)
8 users (show)

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


Attachments
Bug 22200 - Forgiving a fine (FFOR) does not update the accountline date (2.48 KB, patch)
2019-01-23 19:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22200: Forgiving a fine (FFOR) does not update the accountline date (2.54 KB, patch)
2019-02-17 11:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22200: Add credit when forgiving overdue (2.45 KB, patch)
2019-04-18 08:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22200: Add Tests for change (1.80 KB, patch)
2019-04-18 08:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22200: Add Tests for change (1.99 KB, patch)
2019-04-18 09:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22200: Add credit when forgiving overdue (2.85 KB, patch)
2019-04-18 09:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22200: Add Tests for change (2.67 KB, patch)
2019-04-18 15:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22200: Add credit when forgiving overdue (2.85 KB, patch)
2019-04-18 15:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22200: Add Tests for change (2.67 KB, patch)
2019-04-25 09:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22200: Add credit when forgiving overdue (2.85 KB, patch)
2019-04-25 09:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22200: (follow-up) Wrap accountline creation in a transaction (3.91 KB, patch)
2019-04-25 11:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22200: Add Tests for change (2.75 KB, patch)
2019-04-25 12:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22200: Add credit when forgiving overdue (2.92 KB, patch)
2019-04-25 12:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22200: (follow-up) Wrap accountline creation in a transaction (3.94 KB, patch)
2019-04-25 12:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22200: Add Tests for change (2.78 KB, patch)
2019-04-25 13:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22200: Add credit when forgiving overdue (2.96 KB, patch)
2019-04-25 13:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22200: (follow-up) Wrap accountline creation in a transaction (3.98 KB, patch)
2019-04-25 13:28 UTC, Tomás Cohen Arazi
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 2019-01-23 19:53:51 UTC
Each time a fine is incremented, the date column is updated. When that fine is forgiven ( often via the "Forgive fines" option on the checkin page ), it does not update the column. It makes sense that if updating a fine sets the date column, forgiving it should do so as well.
Comment 1 Kyle M Hall 2019-01-23 19:57:53 UTC
Created attachment 84339 [details] [review]
Bug 22200 - Forgiving a fine (FFOR) does not update the accountline date

Each time a fine is incremented, the date column is updated. When that fine is forgiven ( often via the "Forgive fines" option on the checkin page ), it does not update the column. It makes sense that if updating a fine sets the date column, forgiving it should do so as well.

Test Plan:
1) Find a checkout that is overdue with fines, where the date is not today
2) Note the date in the accountlines table
3) Check it in, note the date is not updated
4) Apply this patch
5) Repeat steps 1-2
6) Note the date is updated!
Comment 2 Pierre-Marc Thibault 2019-01-25 19:32:00 UTC
I created a fine and changed it so its date is not today. I went to Pay fines and paid, but the date did not update. Am I doing something wrong?
Comment 3 Pierre-Marc Thibault 2019-01-25 22:16:19 UTC
Sorry, I didn't follow the steps precisely. I didn't test on the checking in page, but I did by the Create manual credit way and it wasn't still working.
Comment 4 Kyle M Hall 2019-01-29 13:44:41 UTC
(In reply to Pierre-Marc Thibault from comment #2)
> I created a fine and changed it so its date is not today. I went to Pay
> fines and paid, but the date did not update. Am I doing something wrong?

That is not the action I meant. You'll need to check in the item with the "forgive fine" option from the checkins page.
Comment 5 Kyle M Hall 2019-01-29 13:45:14 UTC
(In reply to Pierre-Marc Thibault from comment #3)
> Sorry, I didn't follow the steps precisely. I didn't test on the checking in
> page, but I did by the Create manual credit way and it wasn't still working.

To test, you'll need a checked out overdue item with fines. Nothing else will work.
Comment 6 Genevieve Beaudry 2019-02-15 20:02:30 UTC
patch working
Comment 7 Katrin Fischer 2019-02-17 11:13:38 UTC
Created attachment 85219 [details] [review]
Bug 22200: Forgiving a fine (FFOR) does not update the accountline date

Each time a fine is incremented, the date column is updated. When that fine is forgiven ( often via the "Forgive fines" option on the checkin page ), it does not update the column. It makes sense that if updating a fine sets the date column, forgiving it should do so as well.

Test Plan:
1) Find a checkout that is overdue with fines, where the date is not today
2) Note the date in the accountlines table
3) Check it in, note the date is not updated
4) Apply this patch
5) Repeat steps 1-2
6) Note the date is updated!

Signed-off-by: Genevieve Beaudry <genevieve.beaudry@inlibro.com>
Comment 8 Katrin Fischer 2019-02-17 11:14:34 UTC
I've fixed the commit subject and added Genevieve's sign-off line manually.

But I am a little confused here :(

I started my tests with verifying the current behavior and noticed that accountlines.date is already updated to the current date when the entry is changed to FFOR in my testing. 

Can you please take a look?
Comment 9 Kyle M Hall 2019-03-05 15:48:14 UTC
I think we're barking up the wrong tree here. At this time, I think updating the date column is a *bad* idea. It is our 'created on' date, and timestamp is our 'last updated on' date. Really, we shouldn't be changing the fee type to FFOR, we should be generating a FFOR payment and applying it to the fee. I'm going to ask Tomas to chime in.
Comment 10 Martin Renvoize 2019-04-18 07:41:37 UTC
Funny.. I was just thinking "Why are we not creating an offsetting writeoff/forgiven whatever credit here instead of updating the overdue amounts directly?"

I'm going to adopt this bug and push it into the appropriate spot in the accountlines status tree.
Comment 11 Martin Renvoize 2019-04-18 08:57:32 UTC
Created attachment 88269 [details] [review]
Bug 22200: Add credit when forgiving overdue

Prior to this patch the exemptfine option for_FixOverduesOnReturn would
directly set the overdue amount to 0 rather than creating an appropriate
credit of the type forgiven and offseting it against the debt.
Comment 12 Martin Renvoize 2019-04-18 08:57:35 UTC
Created attachment 88270 [details] [review]
Bug 22200: Add Tests for change
Comment 13 Martin Renvoize 2019-04-18 09:03:01 UTC
Test Plan:
1) Find a checkout that is overdue with fines
2) Check the item in with the forgive fines option checked
3) Note that the users account details now shows the overdue as forgiven and a forgiven credit is added which matches the overdue amount (and is applied against it, i.e. both overdue and forgiven lines have 0 amountoutstanding).
Comment 14 Martin Renvoize 2019-04-18 09:10:28 UTC
Created attachment 88271 [details] [review]
Bug 22200: Add Tests for change

Unit tests to check for the addition of a credit of type 'FOR' when
_FixOverduesOnReturn is called with $exemptfine set to true.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2019-04-18 09:10:32 UTC
Created attachment 88273 [details] [review]
Bug 22200: Add credit when forgiving overdue

Prior to this patch the exemptfine option for_FixOverduesOnReturn would
directly set the overdue amount to 0 rather than creating an appropriate
credit of the type forgiven and offseting it against the debt.

Test Plan:
1) Find a checkout that is overdue with fines
2) Check the item in with the forgive fines option checked
3) Note that the users account details now shows the overdue as forgiven
and a forgiven credit is added which matches the overdue amount (and is
applied against it, i.e. both overdue and forgiven lines have 0
amountoutstanding).

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2019-04-18 15:35:55 UTC
Created attachment 88304 [details] [review]
Bug 22200: Add Tests for change

Unit tests to check for the addition of a credit of type 'FOR' when
_FixOverduesOnReturn is called with $exemptfine set to true.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2019-04-18 15:35:58 UTC
Created attachment 88305 [details] [review]
Bug 22200: Add credit when forgiving overdue

Prior to this patch the exemptfine option for_FixOverduesOnReturn would
directly set the overdue amount to 0 rather than creating an appropriate
credit of the type forgiven and offseting it against the debt.

Test Plan:
1) Find a checkout that is overdue with fines
2) Check the item in with the forgive fines option checked
3) Note that the users account details now shows the overdue as forgiven
and a forgiven credit is added which matches the overdue amount (and is
applied against it, i.e. both overdue and forgiven lines have 0
amountoutstanding).

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2019-04-25 09:10:57 UTC
Created attachment 88718 [details] [review]
Bug 22200: Add Tests for change

Unit tests to check for the addition of a credit of type 'FOR' when
_FixOverduesOnReturn is called with $exemptfine set to true.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2019-04-25 09:11:00 UTC
Created attachment 88719 [details] [review]
Bug 22200: Add credit when forgiving overdue

Prior to this patch the exemptfine option for_FixOverduesOnReturn would
directly set the overdue amount to 0 rather than creating an appropriate
credit of the type forgiven and offseting it against the debt.

Test Plan:
1) Find a checkout that is overdue with fines
2) Check the item in with the forgive fines option checked
3) Note that the users account details now shows the overdue as forgiven
and a forgiven credit is added which matches the overdue amount (and is
applied against it, i.e. both overdue and forgiven lines have 0
amountoutstanding).

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2019-04-25 11:36:18 UTC
Created attachment 88750 [details] [review]
Bug 22200: (follow-up) Wrap accountline creation in a transaction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Kyle M Hall 2019-04-25 12:09:01 UTC
Created attachment 88759 [details] [review]
Bug 22200: Add Tests for change

Unit tests to check for the addition of a credit of type 'FOR' when
_FixOverduesOnReturn is called with $exemptfine set to true.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2019-04-25 12:09:09 UTC
Created attachment 88760 [details] [review]
Bug 22200: Add credit when forgiving overdue

Prior to this patch the exemptfine option for_FixOverduesOnReturn would
directly set the overdue amount to 0 rather than creating an appropriate
credit of the type forgiven and offseting it against the debt.

Test Plan:
1) Find a checkout that is overdue with fines
2) Check the item in with the forgive fines option checked
3) Note that the users account details now shows the overdue as forgiven
and a forgiven credit is added which matches the overdue amount (and is
applied against it, i.e. both overdue and forgiven lines have 0
amountoutstanding).

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2019-04-25 12:09:12 UTC
Created attachment 88761 [details] [review]
Bug 22200: (follow-up) Wrap accountline creation in a transaction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Tomás Cohen Arazi 2019-04-25 13:28:42 UTC
Created attachment 88779 [details] [review]
Bug 22200: Add Tests for change

Unit tests to check for the addition of a credit of type 'FOR' when
_FixOverduesOnReturn is called with $exemptfine set to true.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2019-04-25 13:28:46 UTC
Created attachment 88780 [details] [review]
Bug 22200: Add credit when forgiving overdue

Prior to this patch the exemptfine option for_FixOverduesOnReturn would
directly set the overdue amount to 0 rather than creating an appropriate
credit of the type forgiven and offseting it against the debt.

Test Plan:
1) Find a checkout that is overdue with fines
2) Check the item in with the forgive fines option checked
3) Note that the users account details now shows the overdue as forgiven
and a forgiven credit is added which matches the overdue amount (and is
applied against it, i.e. both overdue and forgiven lines have 0
amountoutstanding).

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2019-04-25 13:28:50 UTC
Created attachment 88781 [details] [review]
Bug 22200: (follow-up) Wrap accountline creation in a transaction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Nick Clemens 2019-04-26 12:41:24 UTC
Awesome work all!

Pushed to master for 19.05
Comment 28 Martin Renvoize 2019-04-26 15:56:47 UTC
Depends on enhancements not in 18.11.x series.