|
Description
Martin Renvoize (ashimema)
2019-12-23 12:55:28 UTC
I'm thinking of adding a payout_amount method within Koha::Account that does similar to the offset_amount method I'm looking to add in bug 27636 but in the reverse direction.. i.e. it adds a PAYOUT debit line and offsets it against a set of passed CREDIT lines. Created attachment 116395 [details] [review] Bug 24300: Add payout_amount method to Koha::Account Created attachment 116412 [details] [review] Bug 24300: Add payout_amount method to Koha::Account Created attachment 116413 [details] [review] Bug 24300: Add payout amount to boraccount page Test plan 1/ Add some debts and credits to a patrons account, resulting in an overall credit 2/ Note the appearance of a 'Payout amount' button next to the total due line. 3/ Try paying out an amount with the button 4/ Signoff This will need some unit tests adding for QA, but it's ready for an initial testing phase. I'm not entirely sure I like how it works.. at the moment you can payout prior to paying off debts so you may result in an account that more in debt than before the payout. Feedback welcome. Created attachment 116837 [details] [review] Bug 24300: Add payout_amount method to Koha::Account Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Created attachment 116838 [details] [review] Bug 24300: Add payout amount to boraccount page Test plan 1/ Add some debts and credits to a patrons account, resulting in an overall credit 2/ Note the appearance of a 'Payout amount' button next to the total due line. 3/ Try paying out an amount with the button 4/ Signoff Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Patch works as described. An account being more in debt is ok - there are sometimes circumstances where a patron might owe money, but they also need a refund for an unrelated transaction. With that in mind, it might be nice to have the option to pay out the credit despite the patron being in debt. e.g. Patron owes £20.00 for lost book - but let's say this is in dispute. Patron borrows 5 x DVDs at £2 each. Patron is given a credit for all 5 DVDs. Patron then decides to have a refund rather than replacement DVDs. There is no payout button because the account says £10.00 (£20 owed minus £10 credit). It would be nice to have a payout button for the £10 credit, rather than having to issue payout across 5 individual lines. Equally, on the payment page, it would be nice to be able to use the credit against selected lines - a bit like 'Pay Selected' or 'Write off Selected', it'd be nice to 'Use Credit against Selected' - so again, we can use the credit against the DVDs and not against the lost book. I get the feeling that's all out of scope of this bug though. But the patch works as described, so I'm signing off. Hi Martin, please add unit tests! (In reply to Katrin Fischer from comment #9) > Hi Martin, please add unit tests! Dang, how did I forget that!.. Created attachment 117917 [details] [review] Bug 24300: Add payout_amount method to Koha::Account This patch adds a 'payout_amount' method to the Koha::Account class to allow payout of an amount against a list of credits (or any outstanding credits, oldest to newest). Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Created attachment 117918 [details] [review] Bug 24300: Add payout amount to boraccount page Test plan 1/ Add some debts and credits to a patrons account, resulting in an overall credit 2/ Note the appearance of a 'Payout amount' button next to the total due line. 3/ Try paying out an amount with the button 4/ Signoff Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Created attachment 117919 [details] [review] Bug 24300: (QA follow-up) Add Unit Tests Doh! I forgot the unit tests first time around! Created attachment 118335 [details] [review] Bug 24300: (follow-up) Set minimum for amount input Add a 'min' attribute for validation of the amount field in the payout modal so that negative amounts cannot be submitted. Works as expected. Tests pass. My QA concerns were promptly addressed by the follow-up. QA script happy. Good job Tomás I think you forgot to add the signed-off patches Created attachment 118392 [details] [review] Bug 24300: Add payout_amount method to Koha::Account This patch adds a 'payout_amount' method to the Koha::Account class to allow payout of an amount against a list of credits (or any outstanding credits, oldest to newest). Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 118393 [details] [review] Bug 24300: Add payout amount to boraccount page Test plan 1/ Add some debts and credits to a patrons account, resulting in an overall credit 2/ Note the appearance of a 'Payout amount' button next to the total due line. 3/ Try paying out an amount with the button 4/ Signoff Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 118394 [details] [review] Bug 24300: (QA follow-up) Add Unit Tests Doh! I forgot the unit tests first time around! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 118395 [details] [review] Bug 24300: (follow-up) Set minimum for amount input Add a 'min' attribute for validation of the amount field in the payout modal so that negative amounts cannot be submitted. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 118396 [details] [review] Bug 24300: (QA follow-up) Update payout_amount to use internal methods Tomas pointed out that we could, and probably should, use the internal 'add_debit' method within this function. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I took the liberty of attaching the patches with Tomas's SO lines ;) (In reply to Owen Leonard from comment #16) > Tomás I think you forgot to add the signed-off patches You are right! (In reply to Martin Renvoize from comment #22) > I took the liberty of attaching the patches with Tomas's SO lines ;) Thanks! You saved me some time! Oop, just found a bug in that final QA followup (In reply to Martin Renvoize from comment #25) > Oop, just found a bug in that final QA followup And.. I realised I'd missed tests for offset creations too.. Created attachment 118464 [details] [review] Bug 24300: (QA follow-up) Enhance test and catch failures The tests did not include testing for offset lines. This patch adds such tests and updates the function to pass them Created attachment 118503 [details] [review] Bug 24300: (QA follow-up) Enhance test and catch failures The tests did not include testing for offset lines. This patch adds such tests and updates the function to pass them Created attachment 118601 [details] [review] Bug 24300: Add payout_amount method to Koha::Account This patch adds a 'payout_amount' method to the Koha::Account class to allow payout of an amount against a list of credits (or any outstanding credits, oldest to newest). Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 118602 [details] [review] Bug 24300: Add payout amount to boraccount page Test plan 1/ Add some debts and credits to a patrons account, resulting in an overall credit 2/ Note the appearance of a 'Payout amount' button next to the total due line. 3/ Try paying out an amount with the button 4/ Signoff Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 118603 [details] [review] Bug 24300: (QA follow-up) Add Unit Tests Doh! I forgot the unit tests first time around! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 118604 [details] [review] Bug 24300: (follow-up) Set minimum for amount input Add a 'min' attribute for validation of the amount field in the payout modal so that negative amounts cannot be submitted. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 118605 [details] [review] Bug 24300: (QA follow-up) Update payout_amount to use internal methods Tomas pointed out that we could, and probably should, use the internal 'add_debit' method within this function. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 118606 [details] [review] Bug 24300: (QA follow-up) Enhance test and catch failures The tests did not include testing for offset lines. This patch adds such tests and updates the function to pass them Created attachment 118612 [details] [review] Bug 24300: (QA follow-up) Enhance test and catch failures The tests did not include testing for offset lines. This patch adds such tests and updates the function to pass them Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 119195 [details] [review] Bug 24300: Add payout_amount method to Koha::Account This patch adds a 'payout_amount' method to the Koha::Account class to allow payout of an amount against a list of credits (or any outstanding credits, oldest to newest). Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 119196 [details] [review] Bug 24300: Add payout amount to boraccount page Test plan 1/ Add some debts and credits to a patrons account, resulting in an overall credit 2/ Note the appearance of a 'Payout amount' button next to the total due line. 3/ Try paying out an amount with the button 4/ Signoff Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 119197 [details] [review] Bug 24300: (QA follow-up) Add Unit Tests Doh! I forgot the unit tests first time around! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 119198 [details] [review] Bug 24300: (follow-up) Set minimum for amount input Add a 'min' attribute for validation of the amount field in the payout modal so that negative amounts cannot be submitted. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 119199 [details] [review] Bug 24300: (QA follow-up) Update payout_amount to use internal methods Tomas pointed out that we could, and probably should, use the internal 'add_debit' method within this function. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 119200 [details] [review] Bug 24300: (QA follow-up) Enhance test and catch failures The tests did not include testing for offset lines. This patch adds such tests and updates the function to pass them Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Tree re-arranged slightly to put bugs that need backporting before these enhancements that don't. Rebased to accommodate the above. Blocked by bug 27927. Created attachment 120016 [details] [review] Bug 24300: Add payout_amount method to Koha::Account This patch adds a 'payout_amount' method to the Koha::Account class to allow payout of an amount against a list of credits (or any outstanding credits, oldest to newest). Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 120017 [details] [review] Bug 24300: Add payout amount to boraccount page Test plan 1/ Add some debts and credits to a patrons account, resulting in an overall credit 2/ Note the appearance of a 'Payout amount' button next to the total due line. 3/ Try paying out an amount with the button 4/ Signoff Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 120018 [details] [review] Bug 24300: (QA follow-up) Add Unit Tests Doh! I forgot the unit tests first time around! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 120019 [details] [review] Bug 24300: (follow-up) Set minimum for amount input Add a 'min' attribute for validation of the amount field in the payout modal so that negative amounts cannot be submitted. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 120020 [details] [review] Bug 24300: (QA follow-up) Update payout_amount to use internal methods Tomas pointed out that we could, and probably should, use the internal 'add_debit' method within this function. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 120021 [details] [review] Bug 24300: (QA follow-up) Enhance test and catch failures The tests did not include testing for offset lines. This patch adds such tests and updates the function to pass them Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 21.05, thanks to everybody involved! Enhancement not pushed to 20.11.x |