Resolves #4922

[gabeparra01]

Resolves #4922

Description

The code changes in this PR add error handling for the date range logic used in the following pages/workflows:

• All Donations
• All purchases
• Requests
• Distributions
• Inventory Adjustments
• Transfers
• All Product Drives
• History
• Distributions - Summary
• Distirubtions - By Country
• Distributions - Itemized
• Donations - Summary
• Donations - Itemized
• Donations - Manufacturer
• Product Drives - Summary
• Purchases - Summary

The approach I chose was to add a rescue method in application_controller.rb for the Date::Error that was being raised in date_range_helper.rb. The main reason that I chose this approach is because we need to redirect to the correct child controller's action method after handling the error. I think it is possible to redirect to the correct controller from the Helper class but it seems like an anti-pattern and would make future maintenance challenging.

One drawback of this approach is that the redirect is defaulting to the index action of the child controller. So if there is a future workflow that uses the date range logic in a different controller action other than index, this code could cause an unexpected redirect. If that happens, a possible fix could be adding a new method in the child controller and then calling this method with super.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I spent a lot of time trying to write new Rspec tests around the date range error handling but did not achieve much success. The TLDR summary is that I wanted to avoid refactoring any date range logic because of the long list of workflows that it is used in. Even though it would make the code much easier to unit test.

Here are some of the issues I ran into while trying to add unit tests:

• Controller params are accessed in the DateRangeHelper
  • Helpers do not implement params so I received an error when trying to stub them
  • The params are not passed to the Helper methods, they are accessed in context. Which prevented me from accessing the params by mocking them in the controller and then passing them to the Helper
• setup_date_range_picker method in application_controller.rb is a protected method. Calling this method directly will cause an error in RSpec
  • It might be possible to test this method in an integration test that sets up application_controller.rb, DateRangeHelper.rb and one of the child controllers. This would take a long time to setup properly and might be brittle.
• I was not able to trigger the new Date::Error rescue method in application_controller.rb using RSpec. I think it would require a full route/API test in one of the child controllers

Screenshots and Screen Recordings

Products Drive Manual Test.zip
Distributions Manual Test.zip

[cielf]

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!

[cielf]

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

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.

[gabeparra01]

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?

[cielf]

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

[gabeparra01]

@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

[coalest]

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.

[dorner]

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.

[cielf]

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

[coalest]

If we go with this approach, maybe could we rescue from a custom error for this purpose like InvalidDateRange or something? Because since Date::Error is from the standard library we might at some point raise this error in a different place and we may not want to respond in the same way.

[gabeparra01]

Yeah good point! I created the new custom error and updated the unit test here

[coalest]

Is this line necessary? Won't the default date range be selected after a redirect anyways?

[gabeparra01]

Thanks for catching that! Manually resetting the date_range is no longer necessary with our latest approach. Removed it here

[gabeparra01]

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:

[coalest]

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

[coalest]

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

addressed