Bug 28168 - Manual invoice form pre-fills Amount field with invalid number
Summary: Manual invoice form pre-fills Amount field with invalid number
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Peter Vashchuk
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 27410
Blocks: 14825
  Show dependency treegraph
 
Reported: 2021-04-19 07:43 UTC by Joonas Kylmälä
Modified: 2021-04-29 14:32 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00


Attachments
Bug 28168: leave the field empty if amount variable is empty (2.08 KB, patch)
2021-04-21 11:51 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28168: leave the field empty if amount variable is empty (2.13 KB, patch)
2021-04-21 12:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28168: leave the field empty if amount variable is empty (2.19 KB, patch)
2021-04-22 14:51 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2021-04-19 07:43:35 UTC
The Amount field on Manual invoice form (/cgi-bin/koha/members/maninvoice.pl?borrowernumber=XXXX) used to be empty by default before bug 27410. Now we get a "0.00" or "0,00" value there. This adds one more click to the workflow but more importantly because the form only accepts X.XX (decimal separator being ".") values the pre-filled number is incorrectly in the wrong format if using a comma and it confuses the user because they might think it is okay to enter a comma there.
Comment 1 Peter Vashchuk 2021-04-21 11:51:46 UTC
Created attachment 119943 [details] [review]
Bug 28168: leave the field empty if amount variable is empty

Add "if" check to the template so that it uses the value of
"debit_type.default_amount" only if it's true, i.e. not null or 0.

The reason for this patch is there's requirement from librarians -
to have this field completely empty if it's 0, so they could paste the
amount (as they usually do) without the need to clear the field first.
And anyway if you try to save the form with 0.00 value,
it won't accept it saying "Debit amount passed is not positive" so
in my opinion there's no point to preset it with zeroes to begin with.

To reproduce:
    1) head to /cgi-bin/koha/members/maninvoice.pl?borrowernumber=XXXX
    and check that field "Amount" is prefilled with 0.00;
    2) apply patch;
    3) refresh page and ensure that field "Amount" is empty now;
    4) go to /cgi-bin/koha/admin/debit_types.pl and change default
    amount to some decimal amount;
    5) refresh manual invoice page again and ensure that "Amount"
    field is prefilled with that exact decimal number;
Comment 2 Owen Leonard 2021-04-21 12:28:12 UTC
Created attachment 119945 [details] [review]
Bug 28168: leave the field empty if amount variable is empty

Add "if" check to the template so that it uses the value of
"debit_type.default_amount" only if it's true, i.e. not null or 0.

The reason for this patch is there's requirement from librarians -
to have this field completely empty if it's 0, so they could paste the
amount (as they usually do) without the need to clear the field first.
And anyway if you try to save the form with 0.00 value,
it won't accept it saying "Debit amount passed is not positive" so
in my opinion there's no point to preset it with zeroes to begin with.

To reproduce:
    1) head to /cgi-bin/koha/members/maninvoice.pl?borrowernumber=XXXX
    and check that field "Amount" is prefilled with 0.00;
    2) apply patch;
    3) refresh page and ensure that field "Amount" is empty now;
    4) go to /cgi-bin/koha/admin/debit_types.pl and change default
    amount to some decimal amount;
    5) refresh manual invoice page again and ensure that "Amount"
    field is prefilled with that exact decimal number;

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Joonas Kylmälä 2021-04-21 12:54:25 UTC
(In reply to Joonas Kylmälä from comment #0)
> The Amount field on Manual invoice form
> (/cgi-bin/koha/members/maninvoice.pl?borrowernumber=XXXX) used to be empty
> by default before bug 27410. Now we get a "0.00" or "0,00" value there. This
> adds one more click to the workflow but more importantly because the form
> only accepts X.XX (decimal separator being ".") values the pre-filled number
> is incorrectly in the wrong format if using a comma and it confuses the user
> because they might think it is okay to enter a comma there.

After doing more investigation I want to clarify that the default value used to be empty only with CurrencyFormat = "FR". After this patch the default value will be either empty or 0.00 depending whether the user has set the default fee in /cgi-bin/koha/admin/debit_types.pl to be empty/NULL or 0.00. So I think the proposed solution from Petro here is great because the old behaviour in US currency format can be changed via debit_types.pl page. Now with this patch both US and FR format users will be happy! :)
Comment 4 Martin Renvoize 2021-04-22 14:04:07 UTC
Right.. this is an improvement.. but i think we're still missing the localisation side of it... I'm not sure if we aught to handle that on it's own bug as I know it's been contentious in the past.
Comment 5 Katrin Fischer 2021-04-22 14:07:14 UTC
(In reply to Martin Renvoize from comment #4)
> Right.. this is an improvement.. but i think we're still missing the
> localisation side of it... I'm not sure if we aught to handle that on it's
> own bug as I know it's been contentious in the past.

Problem was, apart from other things, the thousands separator. That's why we had concluded to only allow 100,00.00 for now I think to keep things consistent all over Koha for now. 

I haven't tested this, just trying to summarize where discussion about this stopped to my knowledge!
Comment 6 Joonas Kylmälä 2021-04-22 14:08:15 UTC
(In reply to Martin Renvoize from comment #4)
> Right.. this is an improvement.. but i think we're still missing the
> localisation side of it... I'm not sure if we aught to handle that on it's
> own bug as I know it's been contentious in the past.

It can be handled as separate bug as this just fixes the regression introduced by bug 27410 (whether the field is empty or not). Actually I'm unsure if there is already a bug for the localization error because Petro said something about filing a bug report for that.
Comment 7 Andrew Nugged 2021-04-22 14:09:36 UTC
2 Martin / all:
 
Peter exactly started with second part of this ticket (actually we thought to create another next one ticket): localization

We today with him found that amount value in POST just added and treated as "Perl number", so in our case with comma record just NOT added (we still testing and researching).

Let's give a day more for Peter (Petro) to bring more research and seems solution: localization of result.

Please give any comments by the way which will help us to move efficiently and not take a wrong turn
Comment 8 Andrew Nugged 2021-04-22 14:19:21 UTC
But this not related to current solution in this ticket, it will be another ticket from Petro or me.

This ticket is better to be go through QA if possible.
Comment 9 Andrew Nugged 2021-04-22 14:20:50 UTC
P.S. We need this field to be empty if there's zeroes for usability
people should be able to paste values easier, without precleaning fields,

Also this won't affect our next ticket about localization.

Moreover: system won't allow 0,00 nor 0.00 to be saved later in POST server-side validation.
Comment 10 Martin Renvoize 2021-04-22 14:51:15 UTC
Created attachment 120007 [details] [review]
Bug 28168: leave the field empty if amount variable is empty

Add "if" check to the template so that it uses the value of
"debit_type.default_amount" only if it's true, i.e. not null or 0.

The reason for this patch is there's requirement from librarians -
to have this field completely empty if it's 0, so they could paste the
amount (as they usually do) without the need to clear the field first.
And anyway if you try to save the form with 0.00 value,
it won't accept it saying "Debit amount passed is not positive" so
in my opinion there's no point to preset it with zeroes to begin with.

To reproduce:
    1) head to /cgi-bin/koha/members/maninvoice.pl?borrowernumber=XXXX
    and check that field "Amount" is prefilled with 0.00;
    2) apply patch;
    3) refresh page and ensure that field "Amount" is empty now;
    4) go to /cgi-bin/koha/admin/debit_types.pl and change default
    amount to some decimal amount;
    5) refresh manual invoice page again and ensure that "Amount"
    field is prefilled with that exact decimal number;

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2021-04-22 14:55:52 UTC
OK, Agreed.. lets handle localization on another ticket (there are already a bunch of them open regarding decimal handling, comma's vs dots etc)

With that in mind, I'm going to PQA on this one.. it resolve the issue and is a clean solution.
Comment 12 Martin Renvoize 2021-04-22 14:58:20 UTC
Bug 21507 is one of the long-running bugs around decimal separators.  I'd love fresh eyes on that.. or even an entirely new approach.. it was opened after another attempt failed before it...
Comment 13 Jonathan Druart 2021-04-23 09:55:57 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 14 Fridolin Somers 2021-04-29 14:32:24 UTC
Depends on 27410 not in 20.11.x