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

[BUG] You can enter an issued_at date without a time. Hijinks ensue. #4313

Open cielf opened 2 months ago

cielf commented 2 months ago

Summary

It is possible to enter an issued_at date without a time. If you do, the date is not properly saved.

Why investigate?

Possible reduction of support issues.

Details

See discussion in https://github.com/rubyforgood/human-essentials/pull/3530 Enter a new (or edit a) distribution. Change the date to a different day, but blank out the time. Save. It saves as today. Weird.

This is not incredibly high priority, as we've made it so the user has to go out of their way to enter an issued_at date without a time.

Criteria for completion

[] Figure out how we could fix this [] Either put in a PR to fix it, comment the heck out of it on this issue and raise it to the senior folks attention, or come to the Sunday meeting to discuss.

MohanThanigaivelan commented 1 month ago

Hi @cielf , Can I take up this one if available ?

dorner commented 1 month ago

Go for it!

MohanThanigaivelan commented 1 month ago

Thanks @dorner . As per my analysis so far, browser validation is set to false here. https://github.com/rubyforgood/human-essentials/blob/b583d7bfad46ed9db05bd96f14bdde9cb9485ac2/config/initializers/simple_form.rb#L124

If enabled it would have shown like this in chrome and prevented the form submission.

browser validation

Will try to find a way a solution of achieving something similar to this without updating browser_validations config .

MohanThanigaivelan commented 1 month ago

Hi @dorner ,

I think , we can solve this in three ways.

  1. Prevent users from manually typing in the datetime field and ensure they only use a date-time picker.
  2. Enable browser validations only for this form . Browser prevents from form submission and automatically displays an error message indicating which field needs to be corrected.
  3. When user clicks Save , before proceeding with the submission , validate the date time and display a custom error message in a popup if validation fails

Which one do you prefer is the best ? I prefer option 1 due to simplicity .

MohanThanigaivelan commented 1 month ago

@dorner / @cielf , Can you check my above comment ? thanks

cielf commented 1 month ago

Hey @MohanThanigaivelan -- I definitely want @dorner's take on this.

If we were to go with #1, we'd want to have the same restriction in some other places, for consistency's sake. There might be accessibility issues with not allowing the text input, though. (need to check if our current datepicker meets the appropriate standards)

The other two may introduce an inconsistent look and feel, which can be confusing to users.

dorner commented 1 month ago

Yeah, I wouldn't want to choose #1 - locking things down unless you have JS working is kind of a last resort.

If we can detect the bad date/time on the controller side, I'd rather validate it there and punt them back with an error. We should be able to do this in a reusable way so all date-time fields can make use of that logic.

github-actions[bot] commented 1 week ago

This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.

github-actions[bot] commented 1 day ago

Automatically unassigned after 7 days of inactivity.