Bug 7597 - fines not recording the right info in the stats table
Summary: fines not recording the right info in the stats table
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.6
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-24 20:48 UTC by Nicole C. Engard
Modified: 2019-06-27 09:24 UTC (History)
2 users (show)

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


Attachments
Bug 7597 - Part 2 - move sub writeoff to C4::Accounts (5.73 KB, patch)
2012-02-28 14:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7597 - Part 1 - fines not recording the right info in the stats table (942 bytes, patch)
2012-02-28 14:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7597 - Part 1 - fines not recording the right info in the stats table (942 bytes, patch)
2012-02-28 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7597 - Part 2 - move sub writeoff to C4::Accounts (5.73 KB, patch)
2012-02-28 14:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7597 - Part 3 - fines not recording the right info in the stats table (1.06 KB, patch)
2012-02-28 14:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7597 - Part 1 - fines not recording the right info in the stats table (1.12 KB, patch)
2012-02-28 14:56 UTC, Nicole C. Engard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7597 - Part 2 - move sub writeoff to C4::Accounts (5.93 KB, patch)
2012-02-28 14:56 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7597 - Part 3 - fines not recording the right info in the stats table (1.06 KB, patch)
2012-02-28 14:57 UTC, Nicole C. Engard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7597 - Part 3 - fines not recording the right info in the stats table (1.26 KB, patch)
2012-02-28 14:58 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7597 - Part 4 - Change C4::Acounts::WriteOff to WriteOffFee (1.71 KB, patch)
2012-03-05 20:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7597 - Part 4 - Change C4::Acounts::WriteOff to WriteOffFee (1.90 KB, patch)
2012-03-05 20:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7597 - Part 4 - Change C4::Acounts::WriteOff to WriteOffFee (1.97 KB, patch)
2012-03-06 04:33 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7597 - Part 5 - Remove my from $branch, which was masking previous declaration (872 bytes, patch)
2012-03-08 19:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7597 - Part 5 - Remove my from $branch, which was masking previous declaration (948 bytes, patch)
2012-03-14 00:59 UTC, Nicole C. Engard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2012-02-24 20:48:20 UTC
I did some testing today of paying fines and watched my DB only to find that the data being recorded when writing off a fine is wrong.

http://screencast.com/t/XuhUtQRMccr

When you write off a fine it's using the wrong branch code and it seems to be writing off the wrong amount altogether.
Comment 1 Kyle M Hall 2012-02-28 14:20:53 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-02-28 14:23:08 UTC Comment hidden (obsolete)
Comment 3 Nicole C. Engard 2012-02-28 14:24:26 UTC
Kyle,

To test do I apply 1 and test it and then apply 2 and test it - or do I apply both and test them together?

Nicole
Comment 4 Kyle M Hall 2012-02-28 14:25:48 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2012-02-28 14:26:22 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-02-28 14:27:58 UTC
Either way should work fine.

(In reply to comment #3)
> Kyle,
> 
> To test do I apply 1 and test it and then apply 2 and test it - or do I apply
> both and test them together?
> 
> Nicole
Comment 7 Kyle M Hall 2012-02-28 14:36:00 UTC Comment hidden (obsolete)
Comment 8 Nicole C. Engard 2012-02-28 14:56:08 UTC
Created attachment 7919 [details] [review]
[SIGNED-OFF] Bug 7597 - Part 1 - fines not recording the right info in the stats table

Koha was writing off the value of accountlines.amount rather
than the value of accountlines.ammountoutstanding.

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>

Tested all three patches together and was able to write off single
and multiple fines and the data was correct in the stats table.
Comment 9 Nicole C. Engard 2012-02-28 14:56:52 UTC
Created attachment 7920 [details] [review]
[SIGNED-OFF] Bug 7597 - Part 2 - move sub writeoff to C4::Accounts

Currently, the subroutine writeoff lives in pay.pl, which
is a violation of the Koha coding guidelines, as it writes
to the database. This commit restructures and moves writeoff
to C4::Accounts::WriteOff(), and modifies pay.pl to use it.

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>

Tested all three patches together and was able to write off single
and multiple fines and the data was correct in the stats table.
Comment 10 Nicole C. Engard 2012-02-28 14:57:21 UTC Comment hidden (obsolete)
Comment 11 Nicole C. Engard 2012-02-28 14:58:03 UTC
Created attachment 7922 [details] [review]
[SIGNED-OFF] Bug 7597 - Part 3 - fines not recording the right info in the stats table

This commit fixes writeoff_all to write off only the amount outstanding
for each fine being written off, rather than the original amount owed.

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>

Tested all three patches together and was able to write off single
and multiple fines and the data was correct in the stats table.
Comment 12 Ian Walls 2012-03-01 20:20:49 UTC
Part 1:  just changes which field is recorded in writeoff from amount to amountoutstanding

Part 2:  moves code out of pay.pl and into C4/Accounts, which is very good.  Unfortunately, the new subroutine, WriteOff, does not meet coding guideline PERL9... we've got the verb, but not the noun.  WriteOffFee or WriteOffFine would be appropriate substitutions.

Part 3:  correspondingly fixes amount to amountoutstanding in writeoffall.

Parts 1 and 3 pass QA, but part 2 does not.  It's a small change to make, though, and once the WriteOff subroutine is renamed to follow PERL9, it will pass QA.
Comment 13 Kyle M Hall 2012-03-05 20:30:33 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2012-03-05 20:32:16 UTC Comment hidden (obsolete)
Comment 15 Nicole C. Engard 2012-03-06 04:33:37 UTC
Created attachment 8026 [details] [review]
[SIGNED-OFF] Bug 7597 - Part 4 - Change C4::Acounts::WriteOff to WriteOffFee

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Comment 16 Paul Poulain 2012-03-08 17:07:29 UTC
QA comment:

some variable declared twice
"my" variable $branch masks earlier declaration in same scope at /home/paul/koha.dev/koha-community//C4/Accounts.pm line 783.

=> branch is created at line 782, removing the my should be enough

(the problem appears in patch 2)
Comment 17 Kyle M Hall 2012-03-08 19:36:17 UTC Comment hidden (obsolete)
Comment 18 Nicole C. Engard 2012-03-14 00:59:02 UTC
Created attachment 8198 [details] [review]
[SIGNED-OFF] Bug 7597 - Part 5 - Remove my from $branch, which was masking previous declaration

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Comment 19 Paul Poulain 2012-03-21 16:48:07 UTC
QA comment: clean code, improving coding guidelines followance
passed QA
Comment 20 Jared Camins-Esakov 2012-05-09 16:50:07 UTC
This bug will be included in the Koha 3.6.5 release.