[72685] User can input anything in WP unit costs

[HDinger]

Ticket

https://community.openproject.org/wp/72685

What are you trying to accomplish?

Allow only numbers for logged units. This needed to be done with a regex because number fields are per definition not sensitive to locales resulting in wrong results with other locales then English.

[cbliard]

It works but I don't find the message pretty:

And as the "Costs" amount is automatically updated each time the number of unit is changed, would it be possible to run the validation on every keystroke as well? (instead of having to click "Save" button to see it's wrong)

Also there is a regression: before it was possible to enter an amount with spaces in it (like "10 000"), but it does not work anymore. What feels wrong is that the calculated cost updates itself correctly, but it can't be saved:

Also as my language is set to English, , is interpreted as the thousand separator, and . as the decimal separator.

And when I switch to French (and probably other languages), it's reversed: . is the thousand separator and , is the decimal separator.

That contradicts a bit our hour input in time tracking where both , and . are always decimal separators.

Is it ok? (I suppose as it's already how it was behaving before. We probably already have a existing bug if that's a problem for users).

So there are 2 things to change, and 2 optional:

• bad formatting of input validation message
  • could it validate the field each time it changes?
• it should allow spaces in the number'
  • is it ok that . and , are treated differently depending on locale?

[myabc]

to reply to @cbliard's review:

could it validate the field each time it changes?

HTML 5 validations work on input change, However, they're only exposed to the user on submit – unless we introduce styling with CSS pseudo-classes. I believe the following would work:

input:valid { border-color: green; }
input:invalid { border-color: red; }

I think we probably need to live with this until we get around to primerising the page.

it should allow spaces in the number'

as thousand separators? or strip them out?

The input type="number" should strip out numbers on paste, but won't allow them to be enterred.

[myabc]

And when I switch to French (and probably other languages), it's reversed: . is the thousand separator and , is the decimal separator.

I'm honestly not sure what our non-EN users' expectations are here. @HDinger I ended up commenting on this PR rather than pushing to the branch directly, because I wanted to discuss first.

[cbliard]

to reply to @cbliard's review:

could it validate the field each time it changes?

HTML 5 validations work on input change, However, they're only exposed to the user on submit – unless we introduce styling with CSS pseudo-classes. I believe the following would work:

input:valid { border-color: green; }
input:invalid { border-color: red; }

I think we probably need to live with this until we get around to primerising the page.

it should allow spaces in the number'

as thousand separators? or strip them out?

to reply to @myabc

could it validate the field each time it changes?

HTML 5 validations work on input change, However, they're only exposed to the user on submit – unless we introduce styling with CSS pseudo-classes. I believe the following would work:

input:valid { border-color: green; }
input:invalid { border-color: red; }

I think we probably need to live with this until we get around to primerising the page.

I think that's perfectly fine. It would have been a nice-to-have thing, but not having it still works.

My point was more about the validation message which overlaps the text, and that's not good (It appears on other places too: I just got it today for instance when creating a ldap connection, and not inputing the name)

it should allow spaces in the number'

as thousand separators? or strip them out?

The input type="number" should strip out numbers on paste, but won't allow them to be enterred.

In the existing implementation, it just ignores them.
I think the use case is copy pasting the value from somewhere else: if you take the amount and paste it, it should be interpreted the same. More over, if the input value of "Units" is interpreted client-side to compute the cost (nb units x unit cost), then the value "Units" should be interpreted the same server-side or it will be confusing for the user.

And when I switch to French (and probably other languages), it's reversed: . is the thousand separator and , is the decimal separator.

I'm honestly not sure what our non-EN users' expectations are here. @HDinger I ended up commenting on this PR rather than pushing to the branch directly, because I wanted to discuss first.

That's how it has been working. If nobody complained about it so far, it means it's fine like this (or it means that nobody uses it :D). I think it's a bug fix about user being able to input anything without being warned, and it should be limited to it. If we think the en/fr ,/. interpretation is problematic, that's worth creating another bug wp but it should not be changed here: that's out of scope.

[cbliard]

to sum it up, the real blockers in my review are:

• blank spaces should be allowed and ignored
• the validation message overlaps text, and it's not readable (and "Please match the requested format" is a poor error message, because it's using a regexp, but we can live with it)

[Copilot]

Pull request overview

This PR aims to prevent arbitrary (non-numeric) input for logged unit costs by adding an HTML pattern constraint to the :units input field in the cost entry edit form, with the intent to better support locale-specific number formats.

Changes:

• Add an HTML pattern attribute to the cost_entry[units] text field (both with and without a cost type suffix).

---

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

The new pattern is both too permissive and not aligned with server-side parsing/validation:

• Parsing (Costs::NumberHelper.parse_number_string, used via Rate.parse_number_string) is locale-sensitive (single decimal separator from I18n) and supports thousands delimiters such as spaces (see modules/costs/spec/helpers/costs/number_helper_spec.rb). Allowing both . and , here means e.g. in :de locale a user can enter 1.42 (matches) but it will be interpreted as 142 because . is treated as a thousands delimiter.
• CostEntry rejects negative units (units&.negative?), but the pattern currently allows a leading -.
  Consider generating the regex from the current locale separator (same lookup as parse_number_string), allowing supported thousands delimiters (including spaces), allowing at most one decimal separator, and disallowing negative values so browser validation matches backend behavior. (This applies to both :units fields in this view.)

[HDinger]

to sum it up, the real blockers in my review are:

* blank spaces should be allowed and ignored

I fixed the regex to support blank spaces. ✅

* the validation message overlaps text, and it's not readable (and "Please match the requested format" is a poor error message, because it's using a regexp, but we can live with it)

This is a browser native warning, which we cannot influence. I assume this is a generic issue in the browser you are using(?)

In general, I think we should be rather pragmatic about this. The current change is an improvement, although this is of course not a final state of this page.