Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: CaseContact form date input #5974

Merged
merged 8 commits into from
Aug 14, 2024

Conversation

thejonroberts
Copy link
Contributor

@thejonroberts thejonroberts commented Aug 7, 2024

What github issue is this PR for, if any?

Addresses #5860

What changed, and why?

Javascript is running (on any page load app wide) that manipulated #case_contact_occurred_at field value if it was present. This was probably to ensure it matched previous date picker library format, but app uses browser date pickers now. The manipulation may have been the cause of bug - we have not been able to reproduce the issue. Even if not, this brings date selection in line with standard validation.

  • Remove offending javascript
  • Swap date range picker component (partial) that worked with it, in favor of standard rails form.date_field , plus proper attributes for browser validation, using iso8601 date format standard for browsers
  • Change occurred_at 'not in future' validation from custom method to comparison (as we validate minimum date)
  • Use MINIMUM_DATE model class constant to ensure form and model use same date.
  • Remove the components/dead javascript that aren't used after the change

How is this tested? (please write tests!) 💖💪

Unsure how to test as not able to reproduce/browser specific. I think existing system new/edit specs do a lot of date inputting, so I think that's sufficient, but open to suggestions.

Feelings gif (optional)

alt text

@github-actions github-actions bot added javascript for use by Github Labeler to mark pull requests that update Javascript code ruby Pull requests that update Ruby code Tests! 🎉💖👏 erb labels Aug 7, 2024
use min date constant & I18n in validation
@thejonroberts thejonroberts force-pushed the contact-form-date-bug branch from 07d08ba to b6c2a50 Compare August 8, 2024 03:42
Comment on lines +61 to +62
max: current_date.to_fs(:iso8601),
min: min_date.to_fs(:iso8601),
Copy link
Contributor Author

@thejonroberts thejonroberts Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could drop these min/max attributes to handle with model validations only (allow submit & show validation errors). This will prevent selecting bad dates via UI, but user can still type them in, afaik.

Copy link
Collaborator

@elasticspoon elasticspoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I good with this as if, thanks. Love removing unnecessary dependencies.

There is existing issue where the current inputs don't take into account the timezone of the user is not getting used. So if I enter 8/10/2024 that is saved at 8/10/24 UTC and if I later convert that to EST it becomes 8/9/24 EST.

It would be cool if that could be covered in this PR but I'm totally fine with that being a different PR as well. (Also I'm not 100% if we use the occurred_at with a timezone)

@thejonroberts
Copy link
Contributor Author

I good with this as if, thanks. Love removing unnecessary dependencies.

There is existing issue where the current inputs don't take into account the timezone of the user is not getting used. So if I enter 8/10/2024 that is saved at 8/10/24 UTC and if I later convert that to EST it becomes 8/9/24 EST.

It would be cool if that could be covered in this PR but I'm totally fine with that being a different PR as well. (Also I'm not 100% if we use the occurred_at with a timezone)

Yes, feels great when a PR is more red than green!

There is some code that grabs time zone from a browser cookie (browser_time_zone from Users::TimeZone concern), but I don't think it's being used here (or CaseContact show). It's set by app/javascript/src/time_zone.js.
Definitely worth thinking about and testing a little better, but I think it would take some time to dig around and be really consistent.

Time is always tricky, I like to refer to this thoughtbot article a lot... Might be worth adding a user timezone column for the user to be able to choose? Or having a good fallback.

@elasticspoon
Copy link
Collaborator

I think this PR is kind of the idea of how we would do it in this code base but I guesstimate there are more places incorrectly using timezones than correctly at the moment.

@thejonroberts
Copy link
Contributor Author

thejonroberts commented Aug 12, 2024

The occurred_at param is a plain date, sent without any tz info, so saving to UTC, which is good. Potential issues if user is on the 'other side' of the date line from UTC.

  • User Behind => form prefilled date is a day later than expected (tomorrow from user perspective).
  • User Ahead => validation fails because they are 'in the future' (tomorrow from server perspective).

America/New York is 4 hours behind UTC, so the first scenario could be happening after 8pm Eastern for users. It may be annoying but not blocking creation, and doesn't sound like this bug as reported.

I think incorporating browser_time_zone would be beneficial, but perhaps separate from this work?

@elasticspoon
Copy link
Collaborator

The occurred_at param is a plain date, sent without any tz info, so saving to UTC, which is good. Potential issues if user is on the 'other side' of the date line from UTC.

* User Behind => form prefilled date is a day later than expected (tomorrow from user perspective).

* User Ahead => validation fails because they are 'in the future' (tomorrow from server perspective).

America/New York is 4 hours behind UTC, so the first scenario could be happening after 8pm Eastern for users. It may be annoying but not blocking creation, and doesn't sound like this bug as reported.

I think incorporating browser_time_zone would be beneficial, but perhaps separate from this work?

Yea for sure not part of this. I was just mentioning it in general. And I has been reported with regards to some notification and email tables in the past.

@elasticspoon elasticspoon merged commit 3f6c32c into rubyforgood:main Aug 14, 2024
19 checks passed
@thejonroberts thejonroberts deleted the contact-form-date-bug branch August 14, 2024 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
erb javascript for use by Github Labeler to mark pull requests that update Javascript code ruby Pull requests that update Ruby code Tests! 🎉💖👏
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants