If a patron loses an item and is charged a lost fee, returns the item, and loses it again, the patron will not be charged a new lost fee! Steps to reproduce: 1) Check out item with a replacement fee to a patron 2) Mark the item as lost 3) Note the lost fee, write it off 4) Return the item 5) Check out item to the patron a second time 6) Mark the item as lost a second time 7) Note there is no second lost item fee
I believe bug 6427 will resolve this bug.
Created attachment 80507 [details] [review] Bug 13098: Tests for _FixAccountForLostAndReturned on writeoff When a a write off is done on lost fees, but then the item is returned, the current behaviour is that a credit is added to the patron, for the amount the original lost fee was. This behaviour is wrong and this tests reproduce the situation. To test: - Run: $ kshell k$ prove t/db_dependent/Circulation.t => FAIL: Tests fail because a credit (CR) is added for the original amount, without taking into account the fact the amount was not payed, but written off
Created attachment 80678 [details] [review] Bug 13098: Tests for _FixAccountForLostAndReturned on writeoff When a a write off is done on lost fees, but then the item is returned, the current behaviour is that a credit is added to the patron, for the amount the original lost fee was. This behaviour is wrong and this tests reproduce the situation. To test: - Run: $ kshell k$ prove t/db_dependent/Circulation.t => FAIL: Tests fail because a credit (CR) is added for the original amount, without taking into account the fact the amount was not payed, but written off
Ok, this is the more complete tests I came up with. Use cases: - PF + L + Full write off - PF + L + Full payment - PF + L + No write off or payment - PF + L + Partial write off, partial payment, some remaining debt (L, amountoutstanding) The idea is that _FixAccountForLostAndFound should: - Consider write offs as good enough (so no CR for writeoffs) - Generate a single CR for all payments that reduced amountoutstanding for L - Write offs should be identified by looking at the account_offsets table, specifically when credit_id IS NOT NULL, debit_id is the L accountlines_id and type <> 'Writeoff'.
Created attachment 80862 [details] [review] Bug 13098: Add Koha::Account::Offsets->total This patch introduces this trivial method for returning the sum0 result of the amount columns of a Koha::Account::Offsets set. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Koha/Account/Offsets.t - Sign off :-D
Created attachment 80863 [details] [review] Bug 13098: Tests for all _FixAccountForLostAndReturned use cases This patch introduces tests for all the use cases for _FixAccountForLostAndReturned. This includes the following scenarios before _FixAccountForLostAndReturned is called: - Full write off (the original issue) - Full payment - No payment or write off - Partial payment and partial write off Written off debts (L lines) are just skipped, any kind of payment is added and then refund as a single credit (CR). Amount outstanding is set to 0. Offsets need to be generated for paper trail of this decisions. To test: - Run: $ kshell k$ prove t/db_dependent/Circulation.t => FAIL: Tests fail because _FixAccountForLostAndReturned is not doing the right thing :-D
Created attachment 80864 [details] [review] Bug 13098: Make _FixAccountForLostAndReturned refund the right thing This patch changes the logic inside the method, to make it match the behaviour described on the tests. It uses the existing offsets on the account_offsets table to gather information about the right things to refund. To test: - Run: $ kshell k$ prove t/db_dependent/Circulation.t => FAIL: Tests don't pass! - Apply this patch - Run k$ prove t/db_dependent/Circulation.t => SUCCESS: Tests pass! - Sign off :-D
Created attachment 81105 [details] [review] Bug 13098: Add Koha::Account::Offsets->total This patch introduces this trivial method for returning the sum0 result of the amount columns of a Koha::Account::Offsets set. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Koha/Account/Offsets.t - Sign off :-D Followed test plan, patch worked as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 81106 [details] [review] Bug 13098: Tests for all _FixAccountForLostAndReturned use cases This patch introduces tests for all the use cases for _FixAccountForLostAndReturned. This includes the following scenarios before _FixAccountForLostAndReturned is called: - Full write off (the original issue) - Full payment - No payment or write off - Partial payment and partial write off Written off debts (L lines) are just skipped, any kind of payment is added and then refund as a single credit (CR). Amount outstanding is set to 0. Offsets need to be generated for paper trail of this decisions. To test: - Run: $ kshell k$ prove t/db_dependent/Circulation.t => FAIL: Tests fail because _FixAccountForLostAndReturned is not doing the right thing :-D Followed test plan, patch works as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 81107 [details] [review] Bug 13098: Make _FixAccountForLostAndReturned refund the right thing This patch changes the logic inside the method, to make it match the behaviour described on the tests. It uses the existing offsets on the account_offsets table to gather information about the right things to refund. To test: - Run: $ kshell k$ prove t/db_dependent/Circulation.t => FAIL: Tests don't pass! - Apply this patch - Run k$ prove t/db_dependent/Circulation.t => SUCCESS: Tests pass! - Sign off :-D Followed test plan, patch works as described. All three patches pass QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
1. +=head3 total + +=cut => You are cheating, please provide POD. 2. I would suggest to use the same pattern as Koha::Account->balance for Koha::Account::Offsets->total, i.e. sum at DBMS level. 3. In tests: $price + 0.00 => I do not understand the trick, seems useless. I will need more free brain to finish here.
Created attachment 81194 [details] [review] Bug 13098: Do not compare with precision Formating will be done TT side
Created attachment 81195 [details] [review] Bug 13098: Sum at DBMS level
Created attachment 81354 [details] [review] Bug 13098: Fix POD for Koha::Account::Offsets::total Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 81538 [details] [review] Bug 13098: Add Koha::Account::Offsets->total This patch introduces this trivial method for returning the sum0 result of the amount columns of a Koha::Account::Offsets set. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Koha/Account/Offsets.t - Sign off :-D Followed test plan, patch worked as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81539 [details] [review] Bug 13098: Tests for all _FixAccountForLostAndReturned use cases This patch introduces tests for all the use cases for _FixAccountForLostAndReturned. This includes the following scenarios before _FixAccountForLostAndReturned is called: - Full write off (the original issue) - Full payment - No payment or write off - Partial payment and partial write off Written off debts (L lines) are just skipped, any kind of payment is added and then refund as a single credit (CR). Amount outstanding is set to 0. Offsets need to be generated for paper trail of this decisions. To test: - Run: $ kshell k$ prove t/db_dependent/Circulation.t => FAIL: Tests fail because _FixAccountForLostAndReturned is not doing the right thing :-D Followed test plan, patch works as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81540 [details] [review] Bug 13098: Make _FixAccountForLostAndReturned refund the right thing This patch changes the logic inside the method, to make it match the behaviour described on the tests. It uses the existing offsets on the account_offsets table to gather information about the right things to refund. To test: - Run: $ kshell k$ prove t/db_dependent/Circulation.t => FAIL: Tests don't pass! - Apply this patch - Run k$ prove t/db_dependent/Circulation.t => SUCCESS: Tests pass! - Sign off :-D Followed test plan, patch works as described. All three patches pass QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81541 [details] [review] Bug 13098: Do not compare with precision Formating will be done TT side Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81542 [details] [review] Bug 13098: Sum at DBMS level Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 81543 [details] [review] Bug 13098: Fix POD for Koha::Account::Offsets::total Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Awesome work all! Pushed to master for 18.11
Dependencies not in 18.05.x.