Admin settings controller refactor, add specs, cleanup (#2225)

* Add render_views for admin/settings spec

* Add coverage for admin/settings#update

* Add coverage for admin/settings typecasting open_registrations setting

* Simplify how admin/settings finds the value for updating

* Rely on activerecord to not update a value that hasnt changed

* Add coverage for non-existent setting

* Use a constant for boolean settings

app/controllers/admin/settings_controller.rb

@@ -2,21 +2,15 @@
 
 module Admin
   class SettingsController < BaseController
+    BOOLEAN_SETTINGS = %w(open_registrations).freeze
+
     def index
       @settings = Setting.all_as_records
     end
 
     def update
       @setting = Setting.where(var: params[:id]).first_or_initialize(var: params[:id])
-      value    = settings_params[:value]
+      @setting.update(value: value_for_update)
-
-      # Special cases
-      value = value == 'true' if @setting.var == 'open_registrations'
-
-      if @setting.value != value
-        @setting.value = value
-        @setting.save
-      end
 
       respond_to do |format|
         format.html { redirect_to admin_settings_path }
@@ -29,5 +23,17 @@ module Admin
     def settings_params
       params.require(:setting).permit(:value)
     end
+
+    def value_for_update
+      if updating_boolean_setting?
+        settings_params[:value] == 'true'
+      else
+        settings_params[:value]
+      end
+    end
+
+    def updating_boolean_setting?
+      BOOLEAN_SETTINGS.include?(params[:id])
+    end
   end
 end

spec/controllers/admin/settings_controller_spec.rb

@@ -1,14 +1,53 @@
 require 'rails_helper'
 
 RSpec.describe Admin::SettingsController, type: :controller do
-  describe 'GET #index' do
+  render_views
+
+  describe 'When signed in as an admin' do
     before do
       sign_in Fabricate(:user, admin: true), scope: :user
     end
 
+    describe 'GET #index' do
       it 'returns http success' do
         get :index
+
         expect(response).to have_http_status(:success)
       end
     end
+
+    describe 'PUT #update' do
+
+      describe 'for a record that doesnt exist' do
+        after do
+          Setting.new_setting_key = nil
+        end
+
+        it 'creates a settings value that didnt exist before' do
+          expect(Setting.new_setting_key).to be_nil
+
+          patch :update, params: { id: 'new_setting_key', setting: { value: 'New key value' } }
+
+          expect(response).to redirect_to(admin_settings_path)
+          expect(Setting.new_setting_key).to eq 'New key value'
+        end
+      end
+
+      it 'updates a settings value' do
+        Setting.site_title = 'Original'
+        patch :update, params: { id: 'site_title', setting: { value: 'New title' } }
+
+        expect(response).to redirect_to(admin_settings_path)
+        expect(Setting.site_title).to eq 'New title'
+      end
+
+      it 'typecasts open_registrations to boolean' do
+        Setting.open_registrations = false
+        patch :update, params: { id: 'open_registrations', setting: { value: 'true' } }
+
+        expect(response).to redirect_to(admin_settings_path)
+        expect(Setting.open_registrations).to eq true
+      end
+    end
+  end
 end