From 6ac3f1f7b593538f5e96666ad7909dea6083823c Mon Sep 17 00:00:00 2001
From: Blake Erickson <o.blakeerickson@gmail.com>
Date: Fri, 23 Jul 2021 14:58:10 -0600
Subject: [PATCH] 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
---
 app/controllers/list_controller.rb    |  1 +
 spec/requests/list_controller_spec.rb | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb
index b0d3f6bd100..1fc891ea767 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 e84888a00a8..9bd7c6f8596 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) }