In short, it is possible to 'over-writeoff' a fee such that the value of the fee is now a credit.
Created attachment 71113 [details] [review] Bug 20120 - Writeoffs for individual fees allows an amount to be written off more than the amount of the fee In short, it is possible to 'over-writeoff' a fee such that the value of the fee is now a credit. Test Plan: 1) Attempt to writeoff a single fee, set the amount input to more than the amount of the fee. 2) Note the writeoff makes the fee go negative 3) Apply this patch 4) Attempt to repeat step 1, you should not be able to over-writeoff the fee
Patch tested with a sandbox, by George Williams <george@nekls.org>
Created attachment 71152 [details] [review] Bug 20120 - Writeoffs for individual fees allows an amount to be written off more than the amount of the fee In short, it is possible to 'over-writeoff' a fee such that the value of the fee is now a credit. Test Plan: 1) Attempt to writeoff a single fee, set the amount input to more than the amount of the fee. 2) Note the writeoff makes the fee go negative 3) Apply this patch 4) Attempt to repeat step 1, you should not be able to over-writeoff the fee Signed-off-by: George Williams <george@nekls.org>
Created attachment 71176 [details] [review] Bug 20120: Writeoffs for individual fees allows an amount to be written off more than the amount of the fee In short, it is possible to 'over-writeoff' a fee such that the value of the fee is now a credit. Test Plan: 1) Attempt to writeoff a single fee, set the amount input to more than the amount of the fee. 2) Note the writeoff makes the fee go negative 3) Apply this patch 4) Attempt to repeat step 1, you should not be able to over-writeoff the fee Signed-off-by: George Williams <george@nekls.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 71177 [details] [review] Bug 20120: Prevent individual writeoffs to have an amount more than the amount of the fee In short, it is possible to 'over-writeoff' a fee such that the value of the fee is now a credit. Test Plan: 1) Attempt to writeoff a single fee, set the amount input to more than the amount of the fee. 2) Note the writeoff makes the fee go negative 3) Apply this patch 4) Attempt to repeat step 1, you should not be able to over-writeoff the fee Signed-off-by: George Williams <george@nekls.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Fixed commit message, please keep on mind: https://wiki.koha-community.org/wiki/Commit_messages
1. How bad is that? 2. If it should lead to a credit, what would be the procedure? write off then credit, so 2 lines? 3. in pay.pl, maybe we should refuse instead of assuming it is what the user wanted. 4. Should use Price TT plugin instead of format
Created attachment 71358 [details] [review] Bug 20120: Use the Price TT plugin to format amounts
I don't think we should imply it's a credit, at least not silently. I think the change will avoid errors for now and is a bug fix. I'd treat an option to create a credit as a separate enhancement.
Hm, thinking about it: when you writeoff, you don't get any money from the customer, so even more reason NOT to assume a credit.
(In reply to Katrin Fischer from comment #10) > Hm, thinking about it: when you writeoff, you don't get any money from the > customer, so even more reason NOT to assume a credit. Right. I think we've addressed everything sufficiently. If I'm wrong, please update the bug status of course!
(In reply to Jonathan Druart from comment #7) > 3. in pay.pl, maybe we should refuse instead of assuming it is what the user > wanted. I think this is still valid.
Created attachment 72598 [details] [review] Bug 20120: Writeoffs for individual fees allows an amount to be written off more than the amount of the fee
This version prevents over-writeoffs completely.
Created attachment 72984 [details] [review] Bug 20120: Writeoffs for individual fees allows an amount to be written off more than the amount of the fee Signed-off-by: George Williams <george@nekls.org>
Hi Kyle, can you please add an updated test plan and description to the commit message? Thx!
Oh, and also change the subject to reflect what the patch does (and maybe bug title too?)
Created attachment 74713 [details] [review] Bug 20120: Prevent writeoffs of more than the amount owed for a fee In short, it is possible to 'over-writeoff' a fee such that the value of the fee is now a credit. Test Plan: 1) Attempt to writeoff a single fee, set the amount input to more than the amount of the fee. 2) Note the writeoff makes the fee go negative. 3) Apply this patch 4) Attempt to repeat step 1, you should be unable to write-off an amount more than the amount outstanding for the fee! Signed-off-by: George Williams <george@nekls.org>
Created attachment 74732 [details] [review] Bug 20120: Prevent writeoffs of more than the amount owed for a fee In short, it is possible to 'over-writeoff' a fee such that the value of the fee is now a credit. Test Plan: 1) Attempt to writeoff a single fee, set the amount input to more than the amount of the fee. 2) Note the writeoff makes the fee go negative. 3) Apply this patch 4) Attempt to repeat step 1, you should be unable to write-off an amount more than the amount outstanding for the fee! Signed-off-by: George Williams <george@nekls.org> Signed-off-by: George Williams <george@nekls.org>
I don't think the use of the Price TT is correct here. If you change the CurrencyFormat to FR (comma separator) it will prefill the input field with something like 10,00 - which won't be accepted correctly by Koha yet.
For inputs you have to set on_editing: [% your_amount | $Price on_editing => 1 %]
Created attachment 74910 [details] [review] Bug 20120 [QA Followup]: Pass on_editing parameter for Price plugin
Created attachment 74920 [details] [review] Bug 20120: Prevent writeoffs of more than the amount owed for a fee In short, it is possible to 'over-writeoff' a fee such that the value of the fee is now a credit. Test Plan: 1) Attempt to writeoff a single fee, set the amount input to more than the amount of the fee. 2) Note the writeoff makes the fee go negative. 3) Apply this patch 4) Attempt to repeat step 1, you should be unable to write-off an amount more than the amount outstanding for the fee! Signed-off-by: George Williams <george@nekls.org> Signed-off-by: George Williams <george@nekls.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 74921 [details] [review] Bug 20120: (QA follow-up) Pass on_editing parameter for Price plugin Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
How do we get the redirect? Sounds like impossible as we have the JS check.
Created attachment 75016 [details] [review] Bug 20120: force scalar context for CGI->param
(In reply to Jonathan Druart from comment #27) > How do we get the redirect? Sounds like impossible as we have the JS check. Never rely on client side validation ; )
(In reply to Kyle M Hall from comment #29) > (In reply to Jonathan Druart from comment #27) > > How do we get the redirect? Sounds like impossible as we have the JS check. > > Never rely on client side validation ; ) Yes you are right, the problem is that it does not work. It's easy to get the redirect: 98 $amount=999; Then you see: https://screenshots.firefox.com/8sNfHqHqxHpnMt45/pro.kohadev.org
(In reply to Jonathan Druart from comment #30) > (In reply to Kyle M Hall from comment #29) > > (In reply to Jonathan Druart from comment #27) > > > How do we get the redirect? Sounds like impossible as we have the JS check. > > > > Never rely on client side validation ; ) > > Yes you are right, the problem is that it does not work. > It's easy to get the redirect: > 98 $amount=999; > > Then you see: > https://screenshots.firefox.com/8sNfHqHqxHpnMt45/pro.kohadev.org I don't get why you are failing qa for something that has nothing to do with my code. Did you even test it? If you fake a larger amount, then try to overpay the real amount, my code will prevent the payment from going though as expected. I seriously don't understand how you can say it doesn't work. Please post a video demonstration. Your screenshot gives no useful information. Setting back to PQA for now.
I was testing the redirect, to make sure it worked. Changing the line (as noted in the comment) to bypass the JS check and the "999" appears on the table, I guess it's a variable name conflict.
Created attachment 75917 [details] [review] Bug 20120: (QA follow-up) display correct amount in redirect
Pushed to master for 18.11, awesome work all!
This patch has been pushed to 18.05.x and will be in 18.05.01
Patches does not apply on 17.11.x. Can you provide a rebase ? (maybe squash commits)