|
Description
Julian Maurice
2020-02-06 14:20:31 UTC
Created attachment 98534 [details] [review] Bug 24603: Allow to cancel charges in patron accounting There is already a button to void a payment. It should be possible to cancel a charge too. This patch adds a button in patron's accounting section (Transactions tab) that allow to cancel charges. Charges that have been fully or partially paid cannot be cancelled. It also fixes Koha::Account::Line::is_credit by looking at credit_type_code instead of amount (amount can be 0 for voided payments) It also fixes the tests for Koha::Account::Line::void when database does not contain the borrowernumber 51 (the default in t::lib::Mocks::mock_userenv) Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. Create another manual invoice 5. Pay it (partially or fully) 6. Notice that the 'Cancel charge' button is not available 7. Void the payment 8. 'Cancel charge' button is available again. Click on it and verify that it still works 9. prove t/db_dependent/Koha/Account/Lines.t Created attachment 98551 [details] [review] Bug 24603: Allow to cancel charges in patron accounting There is already a button to void a payment. It should be possible to cancel a charge too. This patch adds a button in patron's accounting section (Transactions tab) that allow to cancel charges. Charges that have been fully or partially paid cannot be cancelled. It also fixes Koha::Account::Line::is_credit by looking at credit_type_code instead of amount (amount can be 0 for voided payments) It also fixes the tests for Koha::Account::Line::void when database does not contain the borrowernumber 51 (the default in t::lib::Mocks::mock_userenv) Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. Create another manual invoice 5. Pay it (partially or fully) 6. Notice that the 'Cancel charge' button is not available 7. Void the payment 8. 'Cancel charge' button is available again. Click on it and verify that it still works 9. prove t/db_dependent/Koha/Account/Lines.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 102005 [details] [review] Bug 24603: Allow to cancel charges in patron accounting There is already a button to void a payment. It should be possible to cancel a charge too. This patch adds a button in patron's accounting section (Transactions tab) that allow to cancel charges. Charges that have been fully or partially paid cannot be cancelled. It also fixes Koha::Account::Line::is_credit by looking at credit_type_code instead of amount (amount can be 0 for voided payments) It also fixes the tests for Koha::Account::Line::void when database does not contain the borrowernumber 51 (the default in t::lib::Mocks::mock_userenv) Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. Create another manual invoice 5. Pay it (partially or fully) 6. Notice that the 'Cancel charge' button is not available 7. Void the payment 8. 'Cancel charge' button is available again. Click on it and verify that it still works 9. prove t/db_dependent/Koha/Account/Lines.t Signed-off-by: David Nind <david@davidnind.com> Looking here... The logic here doesn't feel right to me I'm afraid. I've worked hard in recent releases to improve the consistency of our accounts records.. taking the 'double-entry accounting' approach. In such an approach accountlines, once set, should not be altered and instead, all alterations/updates should be recorded as additional lines and offset against the original. The exception to that rule is where the final amount is not yet set when the line is initially created (the only case so far is fines where we have accruing fines which are updated via the 'adjust' method). I was intending to eventually update the void method to prevent it from updating the original payment line as losing that detail in the line itself can be a red flag when accountants come to audit the system. Could we perhaps either update VOID to allow voiding of charges and apply a similar treatment here.. or utilise the 'reduce' method I added recently? Created attachment 106772 [details] [review] Bug 24603: (follow-up) Update to double entry accounting This patch updates the logic to create a cancellation accountline and apply it to the charge line so we correction record the transaction in terms of double entry accounting standards. Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. A cancellation line should be associated with the original charge. 5. Create another manual invoice 6. Pay it (partially or fully) 7. Notice that the 'Cancel charge' button is not available 8. Void the payment 9. 'Cancel charge' button is available again. Click on it and verify that it still works 10. prove t/db_dependent/Koha/Account/Lines.t Setting back to NSO to get Julians take on my follow-up Thanks for the patch Martin, but I cannot apply it ("could not build fake ancestor" error).
Can you rebase it please ?
Created attachment 111217 [details] [review] Bug 24603: Allow to cancel charges in patron accounting There is already a button to void a payment. It should be possible to cancel a charge too. This patch adds a button in patron's accounting section (Transactions tab) that allow to cancel charges. Charges that have been fully or partially paid cannot be cancelled. It also fixes Koha::Account::Line::is_credit by looking at credit_type_code instead of amount (amount can be 0 for voided payments) It also fixes the tests for Koha::Account::Line::void when database does not contain the borrowernumber 51 (the default in t::lib::Mocks::mock_userenv) Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. Create another manual invoice 5. Pay it (partially or fully) 6. Notice that the 'Cancel charge' button is not available 7. Void the payment 8. 'Cancel charge' button is available again. Click on it and verify that it still works 9. prove t/db_dependent/Koha/Account/Lines.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 111218 [details] [review] Bug 24603: (follow-up) Update to double entry accounting This patch updates the logic to create a cancellation accountline and apply it to the charge line so we correction record the transaction in terms of double entry accounting standards. Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. A cancellation line should be associated with the original charge. 5. Create another manual invoice 6. Pay it (partially or fully) 7. Notice that the 'Cancel charge' button is not available 8. Void the payment 9. 'Cancel charge' button is available again. Click on it and verify that it still works 10. prove t/db_dependent/Koha/Account/Lines.t I've rebased, but just in case there's also a branch on gitlab: https://gitlab.com/mrenvoize/Koha/-/commits/bug_24603 Created attachment 111255 [details] [review] Bug 24603: Allow to cancel charges in patron accounting There is already a button to void a payment. It should be possible to cancel a charge too. This patch adds a button in patron's accounting section (Transactions tab) that allow to cancel charges. Charges that have been fully or partially paid cannot be cancelled. It also fixes Koha::Account::Line::is_credit by looking at credit_type_code instead of amount (amount can be 0 for voided payments) It also fixes the tests for Koha::Account::Line::void when database does not contain the borrowernumber 51 (the default in t::lib::Mocks::mock_userenv) Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. Create another manual invoice 5. Pay it (partially or fully) 6. Notice that the 'Cancel charge' button is not available 7. Void the payment 8. 'Cancel charge' button is available again. Click on it and verify that it still works 9. prove t/db_dependent/Koha/Account/Lines.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 111256 [details] [review] Bug 24603: (follow-up) Update to double entry accounting This patch updates the logic to create a cancellation accountline and apply it to the charge line so we correction record the transaction in terms of double entry accounting standards. Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. A cancellation line should be associated with the original charge. 5. Create another manual invoice 6. Pay it (partially or fully) 7. Notice that the 'Cancel charge' button is not available 8. Void the payment 9. 'Cancel charge' button is available again. Click on it and verify that it still works 10. prove t/db_dependent/Koha/Account/Lines.t Signed-off-by: David Nind <david@davidnind.com> This might be an edge case, but if you: - Partially pay a charge (pay $5 off a $15 charge) - Pay the rest of the charge (pay $10 off a $15 charge) - Void one of the payments for the charge - Then you cannot cancel the charge - This option comes back when you void the other partial payment My notes from testing; - updatedatabase required (In reply to David Nind from comment #14) > This might be an edge case, but if you: > - Partially pay a charge (pay $5 off a $15 charge) > - Pay the rest of the charge (pay $10 off a $15 charge) > - Void one of the payments for the charge > - Then you cannot cancel the charge > - This option comes back when you void the other partial payment > > My notes from testing; > - updatedatabase required I believe this is the expected behaviour. You can only cancel a charge if it has not been paid off (partially or fully). So the action of voiding all payments against it would make it act as though that was the case and as such it should be cancellable. Thanks for testing :) Thanks Martin! I am sorry, but this currently no longer applies: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 24603: Allow to cancel charges in patron accounting Applying: Bug 24603: (follow-up) Update to double entry accounting error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt). error: could not build fake ancestor Patch failed at 0001 Bug 24603: (follow-up) Update to double entry accounting The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-24603-follow-up-Update-to-double-entry-account-FBn5S8.patch I'd also love an explanation on how void, cancel and other options differ! (little confused there) Grr, that sha1 issue is a nightmare to solve.. I'll stick it on a branch for you instead, that easier: https://gitlab.com/mrenvoize/Koha/-/tree/bug_24603 (In reply to Katrin Fischer from comment #18) > > I'd also love an explanation on how void, cancel and other options differ! > (little confused there) OK, I must admit, I did wonder this myself and had a conversation with Julian about it.. must have been IRC as I don't see it in the comments above. So.. Void: This can only be applied to 'payments' and it will reverse the whole payment process and affect the payment line amounts itself. If the payment has been applied against any debts, then those debts will have their amounts reset to the value they had before the payment was applied to them. The final payment line will read 'VOID' as it's status and will have an amount and amountoutstanding of '0'.. It's basically meant for "correcting mistakes". (I don't like this method as at present it's the only one that doesn't respect double-entry accounting as it changes the accountline in question directly without adding a distinct 'VOID' line to the system to offset it.) Cancel: This can only be applied to 'charges' and only charges that have not yet had any payment/credit applied against them. Other Options (May as well list them all): Forgive - Is automatically applied to 'overdue charges' depending on settings; for example, if the item is marked as lost and lost charge is added instead. Discount - Can only be applied to 'charges', records a discount line that is applied against the outstanding amount on the charge. Used to give a patron an agree discount on the list price of the charge. (Currently this is manually added) Pay - Can only be applied to 'charges', used to pay either the whole or part of the outstanding debt Writeoff - Can only be applied to 'charges', records a 'writeoff' line that is applied against the list price of the charge. In British libraries at least, this signifies when they've 'Given up' on getting the money from the patron. Payout - Can only be applied to 'credits', where the patron has excess credit on their account for one reason or another (Perhaps they paid a fine in full before a backdated return was added). Allows the library to pay the credit back out to the patron. Refund - Can be applied to a debt that has been paid off already. This is used to grant a patron credit/payout a cash amount when the library has decided they have wrongly charged a patron for some reason and the patron has already paid. Hi Martin, thx! Adding the manual keyword here as I tihnk your explanations would be great to have in there. Created attachment 111794 [details] [review] Bug 24603: Allow to cancel charges in patron accounting There is already a button to void a payment. It should be possible to cancel a charge too. This patch adds a button in patron's accounting section (Transactions tab) that allow to cancel charges. Charges that have been fully or partially paid cannot be cancelled. It also fixes Koha::Account::Line::is_credit by looking at credit_type_code instead of amount (amount can be 0 for voided payments) It also fixes the tests for Koha::Account::Line::void when database does not contain the borrowernumber 51 (the default in t::lib::Mocks::mock_userenv) Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. Create another manual invoice 5. Pay it (partially or fully) 6. Notice that the 'Cancel charge' button is not available 7. Void the payment 8. 'Cancel charge' button is available again. Click on it and verify that it still works 9. prove t/db_dependent/Koha/Account/Lines.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=24063 Created attachment 111795 [details] [review] Bug 24603: (follow-up) Update to double entry accounting This patch updates the logic to create a cancellation accountline and apply it to the charge line so we correction record the transaction in terms of double entry accounting standards. Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. A cancellation line should be associated with the original charge. 5. Create another manual invoice 6. Pay it (partially or fully) 7. Notice that the 'Cancel charge' button is not available 8. Void the payment 9. 'Cancel charge' button is available again. Click on it and verify that it still works 10. prove t/db_dependent/Koha/Account/Lines.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=24063 Created attachment 111796 [details] [review] Bug 24603: Fix number of unit tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=24063 Created attachment 111797 [details] [review] Bug 24603: (QA follow-up) Make the new credit type translatable and shorten description a bit Adding the new offset type and credit type to the include so it can be translated. Also suggest shortening the db description a bit. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=24063 Created attachment 111914 [details] [review] Bug 24603: (QA follow-up) Add translation to OPAC Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Please rebase error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/includes/accounts.inc). Created attachment 113048 [details] [review] Bug 24603: Allow to cancel charges in patron accounting There is already a button to void a payment. It should be possible to cancel a charge too. This patch adds a button in patron's accounting section (Transactions tab) that allow to cancel charges. Charges that have been fully or partially paid cannot be cancelled. It also fixes Koha::Account::Line::is_credit by looking at credit_type_code instead of amount (amount can be 0 for voided payments) It also fixes the tests for Koha::Account::Line::void when database does not contain the borrowernumber 51 (the default in t::lib::Mocks::mock_userenv) Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. Create another manual invoice 5. Pay it (partially or fully) 6. Notice that the 'Cancel charge' button is not available 7. Void the payment 8. 'Cancel charge' button is available again. Click on it and verify that it still works 9. prove t/db_dependent/Koha/Account/Lines.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=24063 Created attachment 113049 [details] [review] Bug 24603: (follow-up) Update to double entry accounting This patch updates the logic to create a cancellation accountline and apply it to the charge line so we correction record the transaction in terms of double entry accounting standards. Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. A cancellation line should be associated with the original charge. 5. Create another manual invoice 6. Pay it (partially or fully) 7. Notice that the 'Cancel charge' button is not available 8. Void the payment 9. 'Cancel charge' button is available again. Click on it and verify that it still works 10. prove t/db_dependent/Koha/Account/Lines.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=24063 Created attachment 113050 [details] [review] Bug 24603: Fix number of unit tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=24063 Created attachment 113051 [details] [review] Bug 24603: (QA follow-up) Make the new credit type translatable and shorten description a bit Adding the new offset type and credit type to the include so it can be translated. Also suggest shortening the db description a bit. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> https://bugs.koha-community.org/show_bug.cgi?id=24063 Created attachment 113052 [details] [review] Bug 24603: (QA follow-up) Add translation to OPAC Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Rebased as requested... soooo many rebases at the minute.. great to see master catching up on the queue. Created attachment 113113 [details] [review] Bug 24603: Adjust permissions for cancel-charge.pl members/cancel-charge.pl should have the same permissions check than boraccount (borrowers => edit_borrowers was missing) Created attachment 113114 [details] [review] Bug 24603: Simplify tests using TestBuilder Comment on attachment 113049 [details] [review] Bug 24603: (follow-up) Update to double entry accounting Review of attachment 113049 [details] [review]: ----------------------------------------------------------------- ::: Koha/Account/Line.pm @@ +324,4 @@ > } > > + # Check for mandatory parameters > + my @mandatory = ( 'staff_id', 'branch' ); Should not be "branch", but "branchcode" I'd say. + # Make sure it has not be paid yet + return if $self->amount != $self->amountoutstanding; Won't this fail when comparing floats? Julian, can you review the last 2 patches and answer the last 2 comments please? (In reply to Jonathan Druart from comment #37) > + # Make sure it has not be paid yet > + return if $self->amount != $self->amountoutstanding; > > Won't this fail when comparing floats? I think we should be safe, as they're stored as DECIMAL (a fixed-point type, not floating-point), and the initial value of those two columns are exactly the same. So when they come from the database they should still be equal, unless a payment have been made. (In reply to Jonathan Druart from comment #36) > Should not be "branch", but "branchcode" I'd say. I think that's a question for Martin Follow-up patches look reasonable to me. branch Vs branchcode ... Branch is consistent with all other methods in the Koha::Account~ area at the moment.. I wouldn't be against a distinct bug to convert all to branchcode (or library_id for a strict consistency) but I would prefer not to change a single case here for consistency. Pushed to master for 20.11, thanks to everybody involved! enhancement will not be backported to 20.05.x |