As additional background -- for unknown reasons, @gabeparra01 was unable to easily recreate the issue in the way described in the issue -- we have no idea why at this point!

@gabeparra01 Thanks for taking this on.

It is a palpable improvement from a functional pov. I'm not keen on them losing all their filters (including but not exclusive to the badly formatted date) when there is an error in the date format. But it's better than a 500 error, IMO.

There are also some cases where I put in an improperly formatted date (say, November 10-16, 2024 on donations), and it doesn't give me an error, it just goes back to the default. Those don't lose the other filters, though.

There are also some cases where I put in an improperly formatted date (say, November 10-16, 2024 on donations), and it doesn't give me an error, it just goes back to the default. Those don't lose the other filters, though.

@cielf That's a great catch! I did not realize that the reports controller does not have an index action. So if I entered http://localhost:3000/reports/donations_summary?filters[date_range]=foobar as the URL, the 500 error would show up still. I updated the redirect code in this commit to fix this issue.

There are 2 problems with this fix though:

• Filters are now lost on the redirect. (I will create a new conversation thread in this PR to discuss issues I'm seeing when trying to pass the filters)
• The redirect is now less safe. If the original action was a POST/PUT/DELETE, I do not think a redirect would be the safest approach. Is it possible that the date_range control is being used in a POST/PUT/DELETE operation now or in the future?

Asking @dorner to jump in with advice on this one.

@gabeparra01 Thanks for taking this on.

It is a palpable improvement from a functional pov. I'm not keen on them losing all their filters (including but not exclusive to the badly formatted date) when there is an error in the date format. But it's better than a 500 error, IMO.

@cielf I am happy to help!

I tried passing filters in the redirect in this way:
redirect_to controller: params["controller"], action: params["action"], filters: params["filters"]
but then I run into this nil error:

I am not sure where the error is coming from. I debugged and confirmed that:

• helpers.default_date and params["filters] are both defined
• the filters are permitted (they pass the filter params check in distributions_controller.rb. I have not tested this approach in the other controllers yet)

The only other cause I can think of is maybe it is related to the JavaScript issue that @coalest identified here: #4922 (comment)

I can start looking into the JavaScript issue as well, but it will take some time. @cielf @coalest Please let me know what your thoughts are

Question: Do we have to redirect at all? If we fail to parse the date, can we just use the default date range and flash an error/warning that the date range couldnt be parsed so it was ignored? Then all the other filters are saved at least, and we don't have to worrrt about the redirection logic.

Flashing an error/warning does require a redirect. If you don't do a redirect then you end up in a weird browser state where refreshing the page resubmits the original form.

I think it's safe to say that we will not be using filters except in GET requests. So if your solution only works for GET, I think that's fine.

I think, if they are going to lose their other filters, we should mention that in our error -- Maybe something like "Date range '[what they entered]' not properly formatted. Filters reset."

Makes sense to me! Updated here and here

Including a few screenshots for the new error message below:

@dorner Maybe there's something I don't understand about flashes with GET requests. But to me, a user clicks the filter button and requests HTML. On the server we should be able to send back HTML that includes a flash message.

At a glance the following seems to work without any issues:

diff --git a/app/helpers/date_range_helper.rb b/app/helpers/date_range_helper.rb
index cc73af80..82a71c16 100644
--- a/app/helpers/date_range_helper.rb
+++ b/app/helpers/date_range_helper.rb
@@ -36,8 +36,12 @@ module DateRangeHelper
   def selected_interval
     date_range_params.split(" - ").map do |d|
       Date.strptime(d, "%B %d, %Y")
-    rescue
-      raise "Invalid date: #{d} in #{date_range_params}"
+    rescue Date::Error
+      flash.now[:notice] = "Invalid date param provided. Reset to default date range"
+      return default_date.split(" - ").map do |d|
+        Date.strptime(d.to_s, "%B %d, %Y")
+      end
     end
   end

I don't know if there would be any edge cases, but if it is possible to do the above I think it would provide the best user experience. Because all of their other filters are preserved, only the date range is lost but we have no way of parsing the date range anyways.

See screenshot below for what this would look like:

@gabeparra01 Regarding your question about the other related issue (why these invalid date params are appearing at least some of the time), I think it could probably be a separate PR to keep things easy for the team here to review.

Note: Although the javascript normally fixes the date range to be valid, I think the issue is strictly server side. Here is an example test that I believe should be passing but is currently failing. Should be helpful if you do want to tackle the other part of this issue.

diff --git a/spec/requests/distributions_requests_spec.rb b/spec/requests/distributions_requests_spec.rb
index a4346247..d33a9275 100644
--- a/spec/requests/distributions_requests_spec.rb
+++ b/spec/requests/distributions_requests_spec.rb
@@ -70,6 +70,17 @@ RSpec.describe "Distributions", type: :request do
         expect(response).to be_successful
       end

+      it "fills in the date range filter with a valid date range" do
+        get distributions_path
+
+        page = Nokogiri::HTML(response.body)
+        default_date_range = page.at_css("#filters_date_range")["value"]
+
+        expect {
+          default_date_range.split(" - ").map { |d| Date.strptime(d, "%B %d, %Y") }
+        }.to_not raise_error
+      end
+
       it "sums distribution totals accurately" do
         create(:distribution, :with_items, item_quantity: 5, organization: organization)
         create(:line_item, :distribution, itemizable_id: distribution.id, quantity: 7)