DEV: Return 400 instead of 500 for invalid top period (#13828)

DEV: Return 400 instead of 500 for invalid top period (#13828)

  • DEV: Return 400 instead of 500 for invalid top period

This change will prevent a fatal 500 error when passing in an invalid period param value to the /top route.

  • Check if the method exists first

I couldn’t get ListController.respond_to? to work, but was still able to check if the method exists with ListController.action_methods.include?. This way we can avoid relying on the NoMethodError exception which may be raised during the course of executing the method.

  • Just check if the period param value is valid

  • Use the new TopTopic.validate_period method

diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb
index b0d3f6b..1fc891e 100644
--- a/app/controllers/list_controller.rb
+++ b/app/controllers/list_controller.rb
@@ -259,6 +259,7 @@ class ListController < ApplicationController
     options ||= {}
     period = params[:period]
     period ||= ListController.best_period_for(current_user.try(:previous_visit_at), options[:category])
+    TopTopic.validate_period(period)
     public_send("top_#{period}", options)
   end
 
diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb
index e84888a..9bd7c6f 100644
--- a/spec/requests/list_controller_spec.rb
+++ b/spec/requests/list_controller_spec.rb
@@ -424,6 +424,23 @@ RSpec.describe ListController do
     end
   end
 
+  describe 'Top' do
+    it 'renders top' do
+      get "/top"
+      expect(response.status).to eq(200)
+    end
+
+    it 'renders top with a period' do
+      get "/top?period=weekly"
+      expect(response.status).to eq(200)
+    end
+
+    it 'errors for invalid periods on top' do
+      get "/top?period=decadely"
+      expect(response.status).to eq(400)
+    end
+  end
+
   describe 'category' do
     context 'in a category' do
       let(:category) { Fabricate(:category_with_definition) }

GitHub sha: 6ac3f1f7b593538f5e96666ad7909dea6083823c

This commit appears in #13828 which was approved by danielwaterworth. It was merged by blake.