human-essentials icon indicating copy to clipboard operation
human-essentials copied to clipboard

WIP disallow entering numbers with commas

Open fchatterji opened this issue 2 years ago • 9 comments

Resolves #3373

Description

Change the input type of the line item quantity from number to string

Before this change, if the user added a comma to the number, it was considered invalid. The value was not sent to the backend, the save failed silently.

Now, with the string input type, the value is sent to the backend, and there is always an explicit error.

Note that this affects multiple places where the issue also occurred, for example when creating a distribution.

WIP because some barcode tests are still failing.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

fchatterji avatar Feb 24 '23 12:02 fchatterji

Hey @fchatterji -- I notice that you say it's WIP because some barcode tests are still failing. If they fail sometimes, but not always, that's a known issue we have here.

If that's so, and it's the only reason you're holding it in WIP, let's remove the WIP and see if we can get this reviewed and moved forward!

cielf avatar Mar 10 '23 02:03 cielf

hi @cielf , no unfortunately its tests regularly failing, because of changes made here. i'll try to debug in the next few days

fchatterji avatar Mar 20 '23 11:03 fchatterji

Hey @fchatterji -- Just checking in because it's been a couple of weeks -- how is this coming along?

cielf avatar Apr 04 '23 22:04 cielf

Note: I rebased onto main due to an issue with the sidebar's entry expanders that I was seeing with this branch locally but not on main.

scooter-dangle avatar Apr 16 '23 15:04 scooter-dangle

Hmm. Unfortunately this seems to break the barcode workflow (tested on Firefox on macOs with a USB barcode scanner).

I'll try to do a screen recording of the working version on main vs on this branch and upload those.


Here are the recordings:

main:

https://user-images.githubusercontent.com/934707/232325689-bab5663e-f84b-4418-8fe6-583f40d21bc7.mov

this PR:

https://user-images.githubusercontent.com/934707/232325698-e6ea6e00-6dd3-4419-84f9-8c5175511bbb.mov

scooter-dangle avatar Apr 16 '23 15:04 scooter-dangle

Ok. So a bunch of barcode stuff is depending on that field input type being integer, then, it would seem. The question then becomes do we a) back off from changing the field type to string and find a different way to trap the comma if entered, or b) find all the places that are depending on it being integer and address them one by one. Could be an excellent opportunity to learn more about how we handle barcodes for someone...(or c) some other option I haven't expressed.)

cielf avatar Apr 16 '23 22:04 cielf

Seems like it would be good to truly fix it, but I'd understand if that's more than @fchatterji had time for. If they aren't sure how to continue from here, it might make sense to tweak/regroom the issue with the additional information?

scooter-dangle avatar Apr 17 '23 00:04 scooter-dangle

To a certain extent, the barcode stuff is -- well, we don't currently have an expert on it. So it will be a learning experience for whoever takes it on.

cielf avatar Apr 17 '23 00:04 cielf

We have had this issue come up again in the wild, it doesn't happen often, but when it does it is very confusing.

cielf avatar Jul 29 '23 19:07 cielf

Closing due to inactivity.

dorner avatar Mar 08 '24 21:03 dorner