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.

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

@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.

Testing Date Range.zip

@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.

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 .

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

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.

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?

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.

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.

Automatically unassigned after 7 days of inactivity.

There is an in-flight PR on this that the author can't finish for awhile-- so it is available to be taken across the finish line.

I could pick this up.

I can look at the existing PR, and also investigate the root cause. Ideally it would be possible to use standard Rails model validations rather than having to rescue an invalid date later. But there might be some existing complexity preventing this.

btw - I think that litepicker.js project is archived on GitHub. Also the domain might have expired because the "Documentation" link from the GitHub repo goes to a suspicious looking page.

Longer term (not necessarily as part of this issue), it might be good to move to an actively maintained library for the date range functionality.

Please do pick this up. And please feel free to look into the possibilities.

@cielf Is it critical that the user be allowed to enter any text they want into that date range picker? It seems to me that this is where problems can happen.

If user is restricted to only interacting with the date range picker (such as any calendar date or select from one of the range options), then it works well.

But if we open up free-form text entry to the user, then they can possibly enter invalid date formats (or anything at all), which causes an error.

Just to put it out here as a simple solution, we can add a readonly attribute to that daterange input. This prevents user from typing into it, but still leaves the date range interaction functional:

Let me talk about it with the planning group tomorrow -- it's certainly taking a function that they already have away -- and I know that when I'm testing. I like to interact with the text. But critical? Probably not.

It's not critical but would be irritating to folk who are currently using the text component. We also played around with it for a bit and discovered that it's really a quite small range of things that cause it to reliably go boom (Scott couldn't make it die on his machine at all).

It was considered puzzling that this is producing a 500 rather than a 400? (Only Scott and I were in the room this week)

@dorner -- what do you think of the concept of finding an alternate library, given the dependency on the archived litepicker.js?

The reason it sometimes results in a 500 is because of this line in DateRangeHelper:

  def selected_interval
    date_range_params.split(" - ").map do |d|
      Date.strptime(d, "%B %d, %Y")
    rescue
      # === UNHANDLED ERROR THEREFORE A 500 PAGE RESULTS ===
      raise "Invalid date: #{d} in #{date_range_params}"
    end
  end

What's mysterious is that even when I enter intentionally invalid input, it only sometimes goes there. More often, the view kind of "flashes", and my invalid input is replaced with the default input, then the form submitted with that as valid data. I haven't yet found what's causing that flash.

To get a 400, i.e. standard validation error - but also remember all the other filter fields user has filled in - this could be done by introducing a model for the filter input, using ActiveModel (i.e. not backed by a database table). Simply a model to gather up all the user's filter fields into an object, then apply standard or custom Rails validation. Then the controller index could create an instance of this model and check if it's valid. If yes, go ahead and apply filters, if not, render the error view with that instance of the model (which would remember what the user typed in, and have errors).

The above would require redoing the filter forms everywhere these are used.

Another possibility, if want to keep things only in javascript-land: That litepicker exposes an onSelect event - we could try to hook in there and verify the text input is valid, then somehow provide some feedback client side that there's an issue. Would also have to prevent form submission though. Otherwise user could still submit invalid data.

Update: I think litepicker already has logic to prevent invalid dates in it's updateInput method. That's why sometimes when you enter an invalid range and then click away, it resets back to the default range. But there may be a race condition with form submission where the JS reset code doesn't run in time, and the invalid data gets submitted.

So if we want to keep allowing user to free-form enter anything into that input, this may require server-side validation (a good idea in any case).

I think looking into an alternative to litepicker is a good exercise. But the extra features seem kind of niche - I'm not sure you'll find a reliable one that does exactly what we want.

I think a form model seems like a pretty reasonable approach. I don't think the immediate feedback is that important, especially since users can always just use the UI to pick their date if they're unsure about it.

Figured out how to reliably reproduce this - the key is to tab into the field rather than clicking in to it:

Starting from a page that has date range filtering, keep hitting the tab key until the date range field is focused, then type in any invalid value, then hit enter. Then you can see from Rails server output that the invalid value gets submitted.

Otherwise if using the mouse to click into the field, that sets up a number of event listeners such that even if hitting enter afterwards, the litepicker.js will run parseInput and updateInput functions which check if invalid, it doesn't update the values, so the form gets submitted with whatever valid values were there before (such as the default ones).

@danielabar Good find! I was curious why only sometimes it was happening.

So maybe we could just add an event listener to validate/update the input whenever the value changes (maybe with a small delay)?

Since there's a few different ways the user could interact with the date range, including tabbing in, tabbing out, or clicking in/out, or hitting enter, I think the solution requires two parts:

1. Server side validation

No matter what's added on client side, there's always some way invalid data might get submitted. To this end, the solution @coalest mentioned on an earlier PR works very nicely.

If user tabs into the field, types something invalid, then hits Enter (which submits the form), they will get this:

2. Client side validation

For the most part, the litepicker.js library being used for the date range already has this - if given invalid input (i.e. not in the form of validfrom - validto), it will not update the value, which has the effect of reverting to whatever was there before. In our case, since the input is initialized with a default range, that's what it gets put back to.

But if user tabs into the field, then the litepicker.js events don't run. If they hit enter right away, then we're in server-side validation territory. But if user then tabs or clicks away from this invalid input, then we have an opportunity to add some client side JS to validate.

To deal with this, a Stimulus controller can be added to the input, to perform a similar validation that litepicker.js would have done (uses luxon for datetime parsing behind the scenes). Then the custom JavaScript can display an alert (or perhaps some other interaction TBD).

This would run if user tabs into the field, types something invalid, then tabs away (triggering a blur event, which then runs a custom validate method). It would look something like this (we can discuss if it should look differently):

@cielf I've implemented the above on my branch: main...danielabar:human-essentials:4922-v2-handle-invalid-date-range-filter

Would that be ok to resolve the issue?

As far as the functionality goes, it sounds good to me (haven't tried it out). For consistency with the rest of the app, the pop-up should be black on white, though? (for an example, enter a million items in a distribution and save). For technical aspects, i defer, as always to @dorner's view.

@cielf re: popup color

For consistency with the rest of the app, the pop-up should be black on white, though?

I couldn't see any popup happening for distribution, but it does happen for donation, when quantity > 100000. For me it's still in dark mode - this is probably coming from OS settings:

Whoops - yes, I meant donation