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

If someone *does* put in an invalid date range, it should be handled gently (shouldn't do a 500) #4922

Open
2 tasks
cielf opened this issue Jan 12, 2025 · 9 comments · May be fixed by #4925
Open
2 tasks

If someone *does* put in an invalid date range, it should be handled gently (shouldn't do a 500) #4922

cielf opened this issue Jan 12, 2025 · 9 comments · May be fixed by #4925

Comments

@cielf
Copy link
Collaborator

cielf commented Jan 12, 2025

Summary

If someone puts in an invalid date range, they should receive a friendly error, rather than getting a 500 error

Why?

Reduce user frustration

Details

To produce this situation
sign in as [email protected]
Click on Distributions, then "All Distribuitons"
type "nov 08 to feb 08" in the date range box and hit return
You currently get a 500 error
We should instead display an error "date range not properly formatted."

Some things just use a helper, but some others are more complicated.

Criteria for completion

  • range formatting errors handled on all the screens that use date ranges
  • tests to support this behaviour
@gabeparra01
Copy link
Contributor

Hi, can I take this one? I'll start by making a list of places that use date ranges and show the 500 error. I'll post that list here to make sure I am not missing any important use cases.

@cielf
Copy link
Collaborator Author

cielf commented Jan 14, 2025

Sounds good. I'm hoping this can be handled in one place, but not counting on it.

@github-actions github-actions bot removed the Help Wanted Groomed + open to all! label Jan 14, 2025
@gabeparra01
Copy link
Contributor

@cielf I am not able to consistently reproduce this issue. I have only been able to trigger the error once or twice out of 20 or so attempts. I have tried using Chrome in incognito mode and I have also tried using Safari. Neither of those changes seem to make a difference.

I am attaching a screenshot of the error and a screen recording of how I am trying reproduce the issue.

Please let me know if there is a step that I am missing. Right now, it looks to me like it could be a race condition which is tough to debug.

Error screenshot

Testing Date Range.zip

@coalest
Copy link
Collaborator

coalest commented Jan 14, 2025

@gabeparra01 I think there are two issues at play here.

  1. The issue described in this ticket. Passing in an invalid date range shouldn't produce a 500, i.e. http://localhost:3000/product_drives?filters[date_range]=foobar should display an error instead of crashing.

  2. There is a bug somewhere that this is causing invalid date params to be sent quite often. If you go to a page that shows a date range filter, like http://localhost:3000/product_drives. Every time you click the "Filter" button, you will see the the value in the date range fliter flash for a split second (Because we send back an HTML value for the date range that is incorrect which is then updated to be correct via JS. You can turn off JS in your browser to see the original incorrect value and have it not be updated). I suspect there is an issue with the @selected_date_range_label variable setting.

@cielf
Copy link
Collaborator Author

cielf commented Jan 14, 2025

If I, instead of what's described, type nov 08 to feb 08 and click filter, I get it 5 times out of 5 on staging .

@cielf
Copy link
Collaborator Author

cielf commented Jan 14, 2025

Actually, I get it 5 times out of 5 when I do it as described too (on staging).

@cielf
Copy link
Collaborator Author

cielf commented Jan 14, 2025

And I tried it in incognito mode on local with the most up to date main -- same result.
@gabeparra01 Are you working from the latest main? I don't think we've changed anything in this area recently, but just to make sure we're on the same page.

@gabeparra01
Copy link
Contributor

Thank you all for helping me with this investigation!

@cielf My fork is up to date and I ran bin/setup before bin/start but I am still having the same problem with reproducing the issue.

The approach @coalest provided, manually updating the URL in the browser, does give me the 500 error every time. If it sounds good to both of you, I will fix that issue first and then we can have a few people try the normal way to reproduce the issue in my PR branch to confirm?

@cielf
Copy link
Collaborator Author

cielf commented Jan 14, 2025

If I understand correctly, that should give us something better than we have now, so please proceed. I am puzzled, though, about how you are not experiencing this issue.

@gabeparra01 gabeparra01 linked a pull request Jan 15, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants