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

Smart Time entry doesn't seem to handle 12-hour time entries for hours ≥10 without a space #994

Closed
lukestein opened this issue May 23, 2024 · 4 comments · Fixed by #998

Comments

@lukestein
Copy link
Collaborator

Installation: v3.0.3 via Docker (Synology NAS)


Times with "a", "am", "p", or "pm" exhibit some strange behavior. I think there is no smart time entry documentation on 12 hour times, but some things work and some things… don't.

For example:

  • 9a, 9am, 9 a, 9 am, 9p, 9pm, 9 p, 9 pm all parse as expected: 09:00:00 or 21:00:00
  • 10 a, 10 am, 10 p, 10 pm all parse as expected: 10:00:00 or 22:00:00
  • 10a, 10am, 10p, 10pm (no space) do not parse
@cpvalente
Copy link
Owner

Thank you @lukestein,

The smart time entry feature aims to allow some shorthand on time input.
I think it works pretty well for the most case, however I realise I have neglected users that use am/pm

I am happy to fix this as it seems a simple cleanup job, but I am not sure what is the standard to aim for here.

I was wondering if you would have some opinions on what are the reasonable expectations for a string that contains am/pm?

  • should we consider a or am as entries?
  • what is the expected value of 10 10 am
  • what is the expected value of 23 am
  • is it safe to assume that am / pm are always at the end? so no 10 am 10?

We have a few tests locking in this part of the feature
https://github.com/cpvalente/ontime/blob/master/apps/client/src/common/utils/__tests__/dateConfig.test.js#L96
if we could come up with a few test scenarios that give us the reasonable expectation for what Ontime handle it would be very much helpful.
Would you have availability to help?

@jwetzell
Copy link
Collaborator

I've got a fix for "standard" am/pm formats i.e 10a, 10pm, 12:01am, 9:12pm, etc. And tests that test that all strings between 1 and 12:59:59 parse right with either a/am/p/pm after them with and without a space.

@lukestein
Copy link
Collaborator Author

I am happy to try to do a bit of thinking, but my baseline thought here (as a user, not a developer) is that what's important is to give expected output with expected input.

Obviously the code has to make a decision about what to do with unexpected input, but imho it's not critical to decide what to do with an entry like 23 am since I can't think of why a user should expect to get something reasonable there.

Hopefully @jwetzell and #998 have us covered.

I was wondering if you would have some opinions on what are the reasonable expectations for a string that contains am/pm?

  • should we consider a or am as entries?
  • what is the expected value of 10 10 am
  • what is the expected value of 23 am
  • is it safe to assume that am / pm are always at the end? so no 10 am 10?
  • Of the items on this list, I think a am p or pm (any capitalization, with or without leading spaces) should be valid. A purist might like e.g. a.m. too but I think it's good to skip that since periods are already a valid separator.
  • 10 10 am ideally parses as 10:10:00 (and 10 10 pm as 22:10:00) but seem's less important that these work than colon separated or hours only without minutes
  • 23 am invalid input so don't care
  • am/pm etc. only valid at the end seems fine

@jwetzell
Copy link
Collaborator

jwetzell commented May 25, 2024

I will have to double check on the space between hour/minutes which I am fairly certain is handled as there are a couple valid "separators" other than a colon, but other than that #998 handles all these cases now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants