rubyforgood / human-essentials

Human Essentials is an inventory management system for diaper, incontinence, and period-supply banks. It supports them in distributing to partners, tracking inventory, and reporting stats and analytics.
https://humanessentials.app
MIT License
436 stars 450 forks source link

3373 numeric input safari #4288

Closed danielabar closed 2 months ago

danielabar commented 2 months ago

Resolves #3373

Description

This changes the item quantity field to be text rather than numeric. Safari allows entry of non-numeric characters, but then submits a blank value. When combined with the feature that the system doesn't associate items when either the name or quantity is left blank (due to use of custom reject_if proc option on accepts_nested_attributes_for in Itemizable concern ), this creates confusion for Safari users because their donation will be saved without the item they added. (the server gets a blank quantity field and assumes they didn't want to add an item).

It was decided to resolve this by converting the item field to text. This ensures that for all browsers, whatever value the user types in gets submitted, and then the Item numeric validation runs server side, which will report back to the user if the value they entered isn't numeric.

Note that other options were considered including modifying the Itemizable concern, this may become future work.

Changing the input type had some knock-on effects that were fixed:

  1. Barcode javascript (that updates quantity field based on xhr lookup results) was using input[type=number] as selector, this was modified to use a data dash attribute.
  2. Large donation check javascript was using valueAsNumber which only works when the field is numeric, for text this returns NaN. Fixed to use parseInt.
  3. create.js.erb also uses input[type=number] selector, was fixed in the same way as barcode js.
  4. Some system tests were using .numeric as a selector, which is. a class added by simple_form on numeric inputs. As this class is no longer added, these tests started breaking, fixed to use the data dash selector.
  5. The numeric css class is also used for styling, added the data dash attribute to have the same style. Intentionally did not remove the existing numeric from css because there are other uses of it.
  6. Tiny scope creep: System test spec/system/purchase_system_spec.rb was failing even before my changes - because it expects a quantity field to be updated, but that happens via JS and the existing way the test was written wasn't waiting for that, fixed.

Type of change

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

How Has This Been Tested?

Maintained existing system tests, and added new system test to verify entering a non numeric value in quantity field will error.

Can also exercise this manually - try in Safari:

Screenshots

From Safari: image

cielf commented 2 months ago

Note after the merge -- we'll want to change the error message from "Quantity is not a number" to "Quantity is not a number -- note: commas are not allowed" -- because anyone who puts commas in will take umbrage at the idea that 1,300 is not a number. Not incredibly urgent - will make a good first issue.

github-actions[bot] commented 2 months ago

@danielabar: Your PR 3373 numeric input safari is part of today's Human Essentials production release: 2024.04.28. Thank you very much for your contribution!