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.
Created attachment 7913 [details] [review] 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.
Created attachment 7914 [details] [review] 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.
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
Created attachment 7915 [details] [review] 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.
Created attachment 7916 [details] [review] 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.
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
Created attachment 7917 [details] [review] 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.
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.
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.
Created attachment 7921 [details] [review] 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.
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.
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.
Created attachment 8020 [details] [review] Bug 7597 - Part 4 - Change C4::Acounts::WriteOff to WriteOffFee
Created attachment 8021 [details] [review] Bug 7597 - Part 4 - Change C4::Acounts::WriteOff to WriteOffFee
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>
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)
Created attachment 8100 [details] [review] Bug 7597 - Part 5 - Remove my from $branch, which was masking previous declaration
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>
QA comment: clean code, improving coding guidelines followance passed QA
This bug will be included in the Koha 3.6.5 release.