Bug 20120 - Prevent writeoffs of more than the amount owed for a fee
Summary: Prevent writeoffs of more than the amount owed for a fee
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 11210
Blocks:
  Show dependency treegraph
 
Reported: 2018-02-01 14:49 UTC by Kyle M Hall
Modified: 2019-10-14 19:58 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 20120 - Writeoffs for individual fees allows an amount to be written off more than the amount of the fee (4.96 KB, patch)
2018-02-01 14:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20120 - Writeoffs for individual fees allows an amount to be written off more than the amount of the fee (5.00 KB, patch)
2018-02-02 16:19 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 20120: Writeoffs for individual fees allows an amount to be written off more than the amount of the fee (5.08 KB, patch)
2018-02-04 12:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20120: Prevent individual writeoffs to have an amount more than the amount of the fee (5.06 KB, patch)
2018-02-04 12:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20120: Use the Price TT plugin to format amounts (1.16 KB, patch)
2018-02-08 19:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20120: Writeoffs for individual fees allows an amount to be written off more than the amount of the fee (5.98 KB, patch)
2018-03-09 14:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20120: Writeoffs for individual fees allows an amount to be written off more than the amount of the fee (6.03 KB, patch)
2018-03-15 18:47 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 20120: Prevent writeoffs of more than the amount owed for a fee (6.37 KB, patch)
2018-04-23 11:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20120: Prevent writeoffs of more than the amount owed for a fee (6.38 KB, patch)
2018-04-23 14:38 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 20120 [QA Followup]: Pass on_editing parameter for Price plugin (1.15 KB, patch)
2018-04-27 10:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20120: Prevent writeoffs of more than the amount owed for a fee (6.44 KB, patch)
2018-04-28 00:50 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20120: (QA follow-up) Pass on_editing parameter for Price plugin (1.19 KB, patch)
2018-04-28 00:50 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20120: force scalar context for CGI->param (838 bytes, patch)
2018-05-03 16:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20120: (QA follow-up) display correct amount in redirect (928 bytes, patch)
2018-06-08 12:04 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-02-01 14:49:41 UTC
In short, it is possible to 'over-writeoff' a fee such that the value of the fee is now a credit.
Comment 1 Kyle M Hall 2018-02-01 14:51:49 UTC
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
Comment 2 Biblibre Sandboxes 2018-02-02 16:19:11 UTC
Patch tested with a sandbox, by George Williams <george@nekls.org>
Comment 3 Biblibre Sandboxes 2018-02-02 16:19:34 UTC
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>
Comment 4 Katrin Fischer 2018-02-04 12:56:58 UTC
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>
Comment 5 Katrin Fischer 2018-02-04 12:58:25 UTC
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>
Comment 6 Katrin Fischer 2018-02-04 12:59:09 UTC
Fixed commit message, please keep on mind:
https://wiki.koha-community.org/wiki/Commit_messages
Comment 7 Jonathan Druart 2018-02-08 19:35:46 UTC
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
Comment 8 Jonathan Druart 2018-02-08 19:37:09 UTC
Created attachment 71358 [details] [review]
Bug 20120: Use the Price TT plugin to format amounts
Comment 9 Katrin Fischer 2018-02-08 20:02:07 UTC
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.
Comment 10 Katrin Fischer 2018-02-08 20:02:38 UTC
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.
Comment 11 Kyle M Hall 2018-02-09 14:51:30 UTC
(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!
Comment 12 Jonathan Druart 2018-02-13 16:03:36 UTC
(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.
Comment 13 Kyle M Hall 2018-03-09 14:21:00 UTC
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
Comment 14 Kyle M Hall 2018-03-09 14:21:22 UTC
This version prevents over-writeoffs completely.
Comment 15 Biblibre Sandboxes 2018-03-15 18:47:11 UTC
Patch tested with a sandbox, by George Williams <george@nekls.org>
Comment 16 Biblibre Sandboxes 2018-03-15 18:47:34 UTC
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>
Comment 17 Katrin Fischer 2018-04-22 18:55:47 UTC
Hi Kyle, 

can you please add an updated test plan and description to the commit message? Thx!
Comment 18 Katrin Fischer 2018-04-22 18:56:18 UTC
Oh, and also change the subject to reflect what the patch does (and maybe bug title too?)
Comment 19 Kyle M Hall 2018-04-23 11:34:09 UTC
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>
Comment 20 Biblibre Sandboxes 2018-04-23 14:38:11 UTC
Patch tested with a sandbox, by George Williams <george@nekls.org>
Comment 21 Biblibre Sandboxes 2018-04-23 14:38:34 UTC
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>
Comment 22 Katrin Fischer 2018-04-24 06:17:57 UTC
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.
Comment 23 Jonathan Druart 2018-04-24 14:16:21 UTC
For inputs you have to set on_editing:
  [% your_amount | $Price on_editing => 1 %]
Comment 24 Kyle M Hall 2018-04-27 10:01:00 UTC
Created attachment 74910 [details] [review]
Bug 20120 [QA Followup]: Pass on_editing parameter for Price plugin
Comment 25 Katrin Fischer 2018-04-28 00:50:14 UTC
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>
Comment 26 Katrin Fischer 2018-04-28 00:50:41 UTC
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>
Comment 27 Jonathan Druart 2018-05-03 16:25:43 UTC
How do we get the redirect? Sounds like impossible as we have the JS check.
Comment 28 Jonathan Druart 2018-05-03 16:26:33 UTC
Created attachment 75016 [details] [review]
Bug 20120: force scalar context for CGI->param
Comment 29 Kyle M Hall 2018-05-07 15:51:33 UTC
(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 ; )
Comment 30 Jonathan Druart 2018-05-10 15:00:43 UTC
(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
Comment 31 Kyle M Hall 2018-05-18 14:44:19 UTC
(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.
Comment 32 Jonathan Druart 2018-05-18 15:08:19 UTC
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.
Comment 33 Nick Clemens 2018-06-08 12:04:53 UTC
Created attachment 75917 [details] [review]
Bug 20120: (QA follow-up) display correct amount in redirect
Comment 34 Nick Clemens 2018-06-08 13:56:59 UTC
Pushed to master for 18.11, awesome work all!
Comment 35 Martin Renvoize 2018-06-11 08:17:50 UTC
This patch has been pushed to 18.05.x and will be in 18.05.01
Comment 36 Fridolin Somers 2018-06-14 13:12:46 UTC
Patches does not apply on 17.11.x.
Can you provide a rebase ? (maybe squash commits)