From 1ada494bb21658c92a58f8bd9e5cc4d7ebf59b6e Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Thu, 20 Apr 2017 11:18:09 -0400 Subject: [PATCH] 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 | 24 ++++++---- .../admin/settings_controller_spec.rb | 47 +++++++++++++++++-- 2 files changed, 58 insertions(+), 13 deletions(-) diff --git a/app/controllers/admin/settings_controller.rb b/app/controllers/admin/settings_controller.rb index 6cca5c3e3f..fc9064068c 100644 --- a/app/controllers/admin/settings_controller.rb +++ b/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] - - # Special cases - value = value == 'true' if @setting.var == 'open_registrations' - - if @setting.value != value - @setting.value = value - @setting.save - end + @setting.update(value: value_for_update) 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 diff --git a/spec/controllers/admin/settings_controller_spec.rb b/spec/controllers/admin/settings_controller_spec.rb index c126b645be..889f78bc13 100644 --- a/spec/controllers/admin/settings_controller_spec.rb +++ b/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 - it 'returns http success' do - get :index - expect(response).to have_http_status(:success) + 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