From dcf942e3a2bbc9e8440fe2315e998c274ed44b9b Mon Sep 17 00:00:00 2001 From: Michael Kohl Date: Tue, 8 Jun 2021 10:13:55 +0700 Subject: [PATCH] Add Admin:Settings controllers (#13851) * Add Admin:Settings::BaseController and refactor * Further simplify controllers * Add comments, more cleanup * Update mandatory settings controller * Update spec * Update service object and spec * Update controller * Update app/services/settings/general/upsert.rb Co-authored-by: Jamie Gaskins Co-authored-by: Jamie Gaskins --- app/controllers/admin/configs_controller.rb | 16 --- .../settings/authentications_controller.rb | 13 +- .../admin/settings/base_controller.rb | 51 ++++++++ .../admin/settings/campaigns_controller.rb | 18 +-- .../admin/settings/communities_controller.rb | 32 +---- .../settings/general_settings_controller.rb | 47 +++++++ .../settings/mandatory_settings_controller.rb | 20 ++- .../admin/settings/rate_limits_controller.rb | 18 +-- .../settings/user_experiences_controller.rb | 30 +---- app/controllers/admin/settings_controller.rb | 27 ++-- .../api/v0/admin/configs_controller.rb | 23 +++- app/controllers/concerns/settings_params.rb | 27 ---- .../authentication/settings_upsert.rb | 68 ---------- app/services/campaigns/settings_upsert.rb | 41 ------ app/services/rate_limits/settings_upsert.rb | 41 ------ .../settings/authentication/upsert.rb | 41 ++++++ app/services/settings/general/upsert.rb | 34 +++++ app/services/settings/upsert.rb | 66 +++------- .../_apple_auth_provider_settings.html.erb | 0 .../_auth_provider_settings.html.erb | 0 .../_form_submission.html.erb | 0 .../admin/{configs => settings}/show.html.erb | 28 ++--- config/routes/admin.rb | 4 +- spec/requests/admin/configs_spec.rb | 117 ++++++++++-------- 24 files changed, 317 insertions(+), 445 deletions(-) delete mode 100644 app/controllers/admin/configs_controller.rb create mode 100644 app/controllers/admin/settings/base_controller.rb create mode 100644 app/controllers/admin/settings/general_settings_controller.rb delete mode 100644 app/controllers/concerns/settings_params.rb delete mode 100644 app/services/authentication/settings_upsert.rb delete mode 100644 app/services/campaigns/settings_upsert.rb delete mode 100644 app/services/rate_limits/settings_upsert.rb create mode 100644 app/services/settings/authentication/upsert.rb create mode 100644 app/services/settings/general/upsert.rb rename app/views/admin/{configs => settings}/_apple_auth_provider_settings.html.erb (100%) rename app/views/admin/{configs => settings}/_auth_provider_settings.html.erb (100%) rename app/views/admin/{configs => settings}/_form_submission.html.erb (100%) rename app/views/admin/{configs => settings}/show.html.erb (97%) diff --git a/app/controllers/admin/configs_controller.rb b/app/controllers/admin/configs_controller.rb deleted file mode 100644 index ed0d9ee2d..000000000 --- a/app/controllers/admin/configs_controller.rb +++ /dev/null @@ -1,16 +0,0 @@ -module Admin - class ConfigsController < Admin::SettingsController - include SettingsParams - - def create - result = ::Settings::Upsert.call(settings_params) - if result.success? - Audit::Logger.log(:internal, current_user, params.dup) - bust_content_change_caches - redirect_to admin_config_path, notice: "Successfully updated settings." - else - redirect_to admin_config_path, alert: "😭 #{result.errors.to_sentence}" - end - end - end -end diff --git a/app/controllers/admin/settings/authentications_controller.rb b/app/controllers/admin/settings/authentications_controller.rb index 495dcc767..31db9030c 100644 --- a/app/controllers/admin/settings/authentications_controller.rb +++ b/app/controllers/admin/settings/authentications_controller.rb @@ -1,15 +1,10 @@ module Admin module Settings - class AuthenticationsController < Admin::ApplicationController - def create - result = ::Authentication::SettingsUpsert.call(settings_params) + class AuthenticationsController < Admin::Settings::BaseController + private - if result.success? - Audit::Logger.log(:internal, current_user, params.dup) - redirect_to admin_config_path, notice: "Successfully updated settings." - else - redirect_to admin_config_path, alert: "😭 #{result.errors.to_sentence}" - end + def upsert_config(settings) + ::Settings::Authentication::Upsert.call(settings) end def settings_params diff --git a/app/controllers/admin/settings/base_controller.rb b/app/controllers/admin/settings/base_controller.rb new file mode 100644 index 000000000..3fbc35833 --- /dev/null +++ b/app/controllers/admin/settings/base_controller.rb @@ -0,0 +1,51 @@ +module Admin + module Settings + class BaseController < Admin::ApplicationController + MISMATCH_ERROR = "The confirmation key does not match".freeze + + before_action :extra_authorization_and_confirmation, only: [:create] + + def create + result = upsert_config(settings_params) + + if result.success? + Audit::Logger.log(:internal, current_user, params.dup) + redirect_to admin_config_path, notice: "Successfully updated settings." + else + redirect_to admin_config_path, alert: "😭 #{result.errors.to_sentence}" + end + end + + private + + def extra_authorization_and_confirmation + not_authorized unless current_user.has_role?(:super_admin) + raise_confirmation_mismatch_error unless confirmation_text_valid? + end + + def confirmation_text_valid? + params.require(:confirmation) == + "My username is @#{current_user.username} and this action is 100% safe and appropriate." + end + + def raise_confirmation_mismatch_error + raise ActionController::BadRequest.new, MISMATCH_ERROR + end + + # Override this method if you need to call a custom class for upserting. + # Ideally such a class eventually calls out to Settings::Upsert and returns + # the result of that service. + def upsert_config(settings) + ::Settings::Upsert.call(settings, authorization_resource) + end + + # Override this if you need additional params or need to make other changes, + # e.g. a different require key. + def settings_params + params + .require(:"settings_#{authorization_resource.name.demodulize.underscore}") + .permit(*authorization_resource.keys) + end + end + end +end diff --git a/app/controllers/admin/settings/campaigns_controller.rb b/app/controllers/admin/settings/campaigns_controller.rb index 2e5995e20..f8afc10b5 100644 --- a/app/controllers/admin/settings/campaigns_controller.rb +++ b/app/controllers/admin/settings/campaigns_controller.rb @@ -1,22 +1,6 @@ module Admin module Settings - class CampaignsController < Admin::ApplicationController - def create - result = ::Campaigns::SettingsUpsert.call(settings_params) - - if result.success? - Audit::Logger.log(:internal, current_user, params.dup) - redirect_to admin_config_path, notice: "Successfully updated settings." - else - redirect_to admin_config_path, alert: "😭 #{result.errors.to_sentence}" - end - end - - def settings_params - params - .require(:settings_campaign) - .permit(*::Settings::Campaign.keys) - end + class CampaignsController < Admin::Settings::BaseController end end end diff --git a/app/controllers/admin/settings/communities_controller.rb b/app/controllers/admin/settings/communities_controller.rb index e511b9a0b..8657315ed 100644 --- a/app/controllers/admin/settings/communities_controller.rb +++ b/app/controllers/admin/settings/communities_controller.rb @@ -1,36 +1,6 @@ module Admin module Settings - class CommunitiesController < Admin::ApplicationController - def create - errors = upsert_config(settings_params) - - if errors.none? - Audit::Logger.log(:internal, current_user, params.dup) - redirect_to admin_config_path, notice: "Successfully updated settings." - else - redirect_to admin_config_path, alert: "😭 #{errors.to_sentence}" - end - end - - private - - def settings_params - params - .require(:settings_community) - .permit(*::Settings::Community.keys) - end - - def upsert_config(configs) - errors = [] - configs.each do |key, value| - ::Settings::Community.public_send("#{key}=", value) if value.present? - rescue ActiveRecord::RecordInvalid => e - errors << e.message - next - end - - errors - end + class CommunitiesController < Admin::Settings::BaseController end end end diff --git a/app/controllers/admin/settings/general_settings_controller.rb b/app/controllers/admin/settings/general_settings_controller.rb new file mode 100644 index 000000000..3a7b2f039 --- /dev/null +++ b/app/controllers/admin/settings/general_settings_controller.rb @@ -0,0 +1,47 @@ +module Admin + module Settings + class GeneralSettingsController < Admin::Settings::BaseController + SPECIAL_PARAMS_TO_ADD = %w[ + credit_prices_in_cents + email_addresses + meta_keywords + ].freeze + + def create + result = ::Settings::General::Upsert.call(settings_params) + if result.success? + Audit::Logger.log(:internal, current_user, params.dup) + bust_content_change_caches + redirect_to admin_config_path, notice: "Successfully updated settings." + else + redirect_to admin_config_path, alert: "😭 #{result.errors.to_sentence}" + end + end + + private + + # NOTE: we need to override this since the controller name doesn't reflect + # the model name + def authorization_resource + ::Settings::General + end + + def settings_params + has_emails = params.dig(:settings_general, :email_addresses).present? + params[:settings_general][:email_addresses][:default] = ApplicationConfig["DEFAULT_EMAIL"] if has_emails + + params.require(:settings_general)&.permit( + settings_keys.map(&:to_sym), + social_media_handles: ::Settings::General.social_media_handles.keys, + email_addresses: ::Settings::General.email_addresses.keys, + meta_keywords: ::Settings::General.meta_keywords.keys, + credit_prices_in_cents: ::Settings::General.credit_prices_in_cents.keys, + ) + end + + def settings_keys + ::Settings::General.keys + SPECIAL_PARAMS_TO_ADD + end + end + end +end diff --git a/app/controllers/admin/settings/mandatory_settings_controller.rb b/app/controllers/admin/settings/mandatory_settings_controller.rb index 71c214bce..e49be3482 100644 --- a/app/controllers/admin/settings/mandatory_settings_controller.rb +++ b/app/controllers/admin/settings/mandatory_settings_controller.rb @@ -1,17 +1,14 @@ module Admin module Settings - class MandatorySettingsController < Admin::ApplicationController - def create - errors = upsert_config(settings_params) - - if errors.none? - Audit::Logger.log(:internal, current_user, params.dup) - redirect_to admin_config_path, notice: "Successfully updated settings." - else - redirect_to admin_config_path, alert: "😭 #{errors.to_sentence}" + class MandatorySettingsController < Admin::Settings::BaseController + Result = Struct.new(:errors) do + def success? + errors.none? end end + private + def upsert_config(configs) errors = [] configs.each do |key, value| @@ -25,12 +22,9 @@ module Admin errors << e.message next end - - errors + Result.new(errors) end - private - # NOTE: we need to override this since the controller name doesn't reflect # the model name def authorization_resource diff --git a/app/controllers/admin/settings/rate_limits_controller.rb b/app/controllers/admin/settings/rate_limits_controller.rb index 6a95f3570..ce0ca3947 100644 --- a/app/controllers/admin/settings/rate_limits_controller.rb +++ b/app/controllers/admin/settings/rate_limits_controller.rb @@ -1,22 +1,6 @@ module Admin module Settings - class RateLimitsController < Admin::ApplicationController - def create - result = ::RateLimits::SettingsUpsert.call(settings_params) - - if result.success? - Audit::Logger.log(:internal, current_user, params.dup) - redirect_to admin_config_path, notice: "Successfully updated settings." - else - redirect_to admin_config_path, alert: "😭 #{result.errors.to_sentence}" - end - end - - def settings_params - params - .require(:settings_rate_limit) - .permit(*::Settings::RateLimit.keys) - end + class RateLimitsController < Admin::Settings::BaseController end end end diff --git a/app/controllers/admin/settings/user_experiences_controller.rb b/app/controllers/admin/settings/user_experiences_controller.rb index cba921581..324c543ce 100644 --- a/app/controllers/admin/settings/user_experiences_controller.rb +++ b/app/controllers/admin/settings/user_experiences_controller.rb @@ -1,34 +1,6 @@ module Admin module Settings - class UserExperiencesController < Admin::ApplicationController - def create - errors = upsert_config(settings_params) - - if errors.none? - Audit::Logger.log(:internal, current_user, params.dup) - redirect_to admin_config_path, notice: "Successfully updated settings." - else - redirect_to admin_config_path, alert: "😭 #{errors.to_sentence}" - end - end - - def settings_params - params - .require(:settings_user_experience) - .permit(*::Settings::UserExperience.keys) - end - - def upsert_config(configs) - errors = [] - configs.each do |key, value| - ::Settings::UserExperience.public_send("#{key}=", value) if value.present? - rescue ActiveRecord::RecordInvalid => e - errors << e.message - next - end - - errors - end + class UserExperiencesController < Admin::Settings::BaseController end end end diff --git a/app/controllers/admin/settings_controller.rb b/app/controllers/admin/settings_controller.rb index 6cd297df6..b4bb604ae 100644 --- a/app/controllers/admin/settings_controller.rb +++ b/app/controllers/admin/settings_controller.rb @@ -1,30 +1,19 @@ module Admin + # This controller is solely responsible for rendering the settings page at + # /admin/customization/config. The actual updates get handled by the settings + # controllers in the Admin::Settings namespace. class SettingsController < Admin::ApplicationController - MISMATCH_ERROR = "The confirmation key does not match".freeze - - before_action :extra_authorization_and_confirmation, only: [:create] - layout "admin" - def create; end - def show - @confirmation_text = confirmation_text + @confirmation_text = + "My username is @#{current_user.username} and this action is 100% safe and appropriate." end private - def extra_authorization_and_confirmation - not_authorized unless current_user.has_role?(:super_admin) - raise_confirmation_mismatch_error if params.require(:confirmation) != confirmation_text - end - - def confirmation_text - "My username is @#{current_user.username} and this action is 100% safe and appropriate." - end - - def raise_confirmation_mismatch_error - raise ActionController::BadRequest.new, MISMATCH_ERROR - end + # We need to override this method from Admin::ApplicationController since + # there is no resource to authorize. + def authorization_resource; end end end diff --git a/app/controllers/api/v0/admin/configs_controller.rb b/app/controllers/api/v0/admin/configs_controller.rb index f535a91bf..79a801039 100644 --- a/app/controllers/api/v0/admin/configs_controller.rb +++ b/app/controllers/api/v0/admin/configs_controller.rb @@ -2,7 +2,11 @@ module Api module V0 module Admin class ConfigsController < ApiController - include SettingsParams + SPECIAL_PARAMS_TO_ADD = %w[ + credit_prices_in_cents + email_addresses + meta_keywords + ].freeze before_action :authenticate_with_api_key_or_current_user! before_action :authorize_super_admin @@ -39,6 +43,23 @@ module Api private + def settings_params + has_emails = params.dig(:settings_general, :email_addresses).present? + params[:settings_general][:email_addresses][:default] = ApplicationConfig["DEFAULT_EMAIL"] if has_emails + + params.require(:settings_general)&.permit( + settings_keys.map(&:to_sym), + social_media_handles: ::Settings::General.social_media_handles.keys, + email_addresses: ::Settings::General.email_addresses.keys, + meta_keywords: ::Settings::General.meta_keywords.keys, + credit_prices_in_cents: ::Settings::General.credit_prices_in_cents.keys, + ) + end + + def settings_keys + ::Settings::General.keys + SPECIAL_PARAMS_TO_ADD + end + def auth_settings_params params .require(:site_config) diff --git a/app/controllers/concerns/settings_params.rb b/app/controllers/concerns/settings_params.rb deleted file mode 100644 index ba7d06e04..000000000 --- a/app/controllers/concerns/settings_params.rb +++ /dev/null @@ -1,27 +0,0 @@ -# Helper method for controllers interacting with Settings::General -module SettingsParams - SPECIAL_PARAMS_TO_ADD = %w[ - credit_prices_in_cents - email_addresses - meta_keywords - ].freeze - - def settings_params - has_emails = params.dig(:settings_general, :email_addresses).present? - params[:settings_general][:email_addresses][:default] = ApplicationConfig["DEFAULT_EMAIL"] if has_emails - - params.require(:settings_general)&.permit( - settings_keys.map(&:to_sym), - social_media_handles: Settings::General.social_media_handles.keys, - email_addresses: Settings::General.email_addresses.keys, - meta_keywords: Settings::General.meta_keywords.keys, - credit_prices_in_cents: Settings::General.credit_prices_in_cents.keys, - ) - end - - private - - def settings_keys - Settings::General.keys + SPECIAL_PARAMS_TO_ADD - end -end diff --git a/app/services/authentication/settings_upsert.rb b/app/services/authentication/settings_upsert.rb deleted file mode 100644 index e712e40be..000000000 --- a/app/services/authentication/settings_upsert.rb +++ /dev/null @@ -1,68 +0,0 @@ -module Authentication - class SettingsUpsert - attr_reader :errors - - def self.call(configs) - new(configs).call - end - - def initialize(configs) - @configs = configs - @errors = [] - end - - def call - upsert_configs - self - end - - def success? - @errors.none? - end - - private - - # NOTE: @citizen428 - This was adapted from Settings::Upsert. I'll see if - # a pattern for refactoring emerges in the future, but for now I'll leave - # this as-is. - def upsert_configs - @configs.each do |key, value| - if key == "auth_providers_to_enable" - update_enabled_providers(value) unless value.class.name != "String" - elsif value.is_a?(Array) && value.any? - Settings::Authentication.public_send("#{key}=", value.reject(&:blank?)) - elsif value.present? - Settings::Authentication.public_send("#{key}=", value.strip) - end - rescue ActiveRecord::RecordInvalid => e - @errors << e.message - next - end - end - - def update_enabled_providers(value) - enabled_providers = value.split(",").filter_map do |entry| - entry unless invalid_provider_entry(entry) - end - return if email_login_disabled_with_one_or_less_auth_providers(enabled_providers) - - Settings::Authentication.providers = enabled_providers - end - - def invalid_provider_entry(entry) - entry.blank? || - Authentication::Providers.available.map(&:to_s).exclude?(entry) || - provider_keys_missing(entry) - end - - def provider_keys_missing(entry) - Settings::Authentication.public_send("#{entry}_key").blank? || - Settings::Authentication.public_send("#{entry}_secret").blank? - end - - def email_login_disabled_with_one_or_less_auth_providers(enabled_providers) - !Settings::Authentication.allow_email_password_login && - enabled_providers.count <= 1 - end - end -end diff --git a/app/services/campaigns/settings_upsert.rb b/app/services/campaigns/settings_upsert.rb deleted file mode 100644 index e9c8dc6af..000000000 --- a/app/services/campaigns/settings_upsert.rb +++ /dev/null @@ -1,41 +0,0 @@ -module Campaigns - class SettingsUpsert - attr_reader :errors - - def self.call(configs) - new(configs).call - end - - def initialize(configs) - @configs = configs - @errors = [] - end - - def call - upsert_configs - self - end - - def success? - @errors.none? - end - - private - - # NOTE: @citizen428 - This was adapted from Settings::Upsert. I'll see if - # a pattern for refactoring emerges in the future, but for now I'll leave - # this as-is. - def upsert_configs - @configs.each do |key, value| - if value.is_a?(Array) && value.any? - Settings::Campaign.public_send("#{key}=", value.reject(&:blank?)) - elsif value.present? - Settings::Campaign.public_send("#{key}=", value.strip) - end - rescue ActiveRecord::RecordInvalid => e - @errors << e.message - next - end - end - end -end diff --git a/app/services/rate_limits/settings_upsert.rb b/app/services/rate_limits/settings_upsert.rb deleted file mode 100644 index 27f86c41d..000000000 --- a/app/services/rate_limits/settings_upsert.rb +++ /dev/null @@ -1,41 +0,0 @@ -module RateLimits - class SettingsUpsert - attr_reader :errors - - def self.call(configs) - new(configs).call - end - - def initialize(configs) - @configs = configs - @errors = [] - end - - def call - upsert_configs - self - end - - def success? - @errors.none? - end - - private - - # NOTE: @citizen428 - This was adapted from Settings::Upsert. I'll see if - # a pattern for refactoring emerges in the future, but for now I'll leave - # this as-is. - def upsert_configs - @configs.each do |key, value| - if value.is_a?(Array) && value.any? - Settings::RateLimit.public_send("#{key}=", value.reject(&:blank?)) - elsif value.present? - Settings::RateLimit.public_send("#{key}=", value.strip) - end - rescue ActiveRecord::RecordInvalid => e - @errors << e.message - next - end - end - end -end diff --git a/app/services/settings/authentication/upsert.rb b/app/services/settings/authentication/upsert.rb new file mode 100644 index 000000000..114947df3 --- /dev/null +++ b/app/services/settings/authentication/upsert.rb @@ -0,0 +1,41 @@ +module Settings + class Authentication + module Upsert + attr_reader :errors + + def self.call(settings) + auth_providers_to_enable = settings.delete("auth_providers_to_enable") + result = Settings::Upsert.call(settings, ::Settings::Authentication) + update_enabled_providers(auth_providers_to_enable) + result + end + + def self.update_enabled_providers(value) + return if value.blank? + + enabled_providers = value.split(",").filter_map do |entry| + entry unless invalid_provider_entry(entry) + end + return if email_login_disabled_with_one_or_less_auth_providers(enabled_providers) + + Settings::Authentication.providers = enabled_providers + end + + def self.invalid_provider_entry(entry) + entry.blank? || + ::Authentication::Providers.available.map(&:to_s).exclude?(entry) || + provider_keys_missing(entry) + end + + def self.provider_keys_missing(entry) + ::Settings::Authentication.public_send("#{entry}_key").blank? || + ::Settings::Authentication.public_send("#{entry}_secret").blank? + end + + def self.email_login_disabled_with_one_or_less_auth_providers(enabled_providers) + !::Settings::Authentication.allow_email_password_login && + enabled_providers.count <= 1 + end + end + end +end diff --git a/app/services/settings/general/upsert.rb b/app/services/settings/general/upsert.rb new file mode 100644 index 000000000..6bdbf9bd6 --- /dev/null +++ b/app/services/settings/general/upsert.rb @@ -0,0 +1,34 @@ +module Settings + class General + module Upsert + PARAMS_TO_BE_CLEANED = %i[sidebar_tags suggested_tags suggested_users].freeze + TAG_PARAMS = %w[sidebar_tags suggested_tags].freeze + + def self.call(settings) + cleaned_params = clean_params(settings) + result = ::Settings::Upsert.call(cleaned_params, ::Settings::General) + return result unless result.success? + + create_tags_if_necessary(settings) + result + end + + def self.clean_params(settings) + PARAMS_TO_BE_CLEANED.each do |param| + settings[param] = settings[param]&.downcase&.delete(" ") if settings[param] + end + settings[:credit_prices_in_cents]&.transform_values!(&:to_i) + settings + end + + # Bulk create tags if they should exist. + # This is an acts-as-taggable-on as used on saving of an Article, etc. + def self.create_tags_if_necessary(settings) + return unless (settings.keys & TAG_PARAMS).any? + + tags = Settings::General.suggested_tags + Settings::General.sidebar_tags + Tag.find_or_create_all_with_like_by_name(tags) + end + end + end +end diff --git a/app/services/settings/upsert.rb b/app/services/settings/upsert.rb index cdd44aca8..d9e427215 100644 --- a/app/services/settings/upsert.rb +++ b/app/services/settings/upsert.rb @@ -1,68 +1,42 @@ module Settings + # This service ensures that settings upserts to the database happen in a + # standardized way. Instead of subclassing this service I recommend wrapping + # it, see e.g. Settings::General::Upsert. class Upsert - VALID_DOMAIN = /^[a-zA-Z0-9]{1,61}[a-zA-Z0-9](?:\.[a-zA-Z]{2,})+$/.freeze + attr_reader :errors, :settings_class - PARAMS_TO_BE_CLEANED = %i[sidebar_tags suggested_tags suggested_users].freeze - - attr_reader :errors - - def self.call(configs) - new(configs).call + def self.call(settings, settings_class) + new(settings, settings_class).call end - def initialize(configs) - @configs = configs - @success = false + def initialize(settings, settings_class) + @settings = settings + @settings_class = settings_class + @errors = [] end def call - clean_up_params - - @errors = [] - upsert_configs - return self if @errors.any? - - @success = true - after_upsert_tasks + upsert_settings self end def success? - @success + @errors.none? end - def upsert_configs - @configs.each do |key, value| - if value.is_a?(Array) - Settings::General.public_send("#{key}=", value.reject(&:blank?)) unless value.empty? - elsif value.respond_to?(:to_h) - Settings::General.public_send("#{key}=", value.to_h) unless value.empty? - else - Settings::General.public_send("#{key}=", value.strip) unless value.nil? + def upsert_settings + @settings.each do |key, value| + if value.is_a?(Array) && value.any? + settings_class.public_send("#{key}=", value.reject(&:blank?)) + elsif value.respond_to?(:to_h) && value.present? + settings_class.public_send("#{key}=", value.to_h) + elsif value.present? + settings_class.public_send("#{key}=", value.strip) end rescue ActiveRecord::RecordInvalid => e @errors << e.message next end end - - def after_upsert_tasks - create_tags_if_not_created - end - - def clean_up_params - PARAMS_TO_BE_CLEANED.each do |param| - @configs[param] = @configs[param]&.downcase&.delete(" ") if @configs[param] - end - @configs[:credit_prices_in_cents]&.transform_values!(&:to_i) - end - - def create_tags_if_not_created - # Bulk create tags if they should exist. - # This is an acts-as-taggable-on as used on saving of an Article, etc. - return unless (@configs.keys & %w[suggested_tags sidebar_tags]).any? - - Tag.find_or_create_all_with_like_by_name(Settings::General.suggested_tags + Settings::General.sidebar_tags) - end end end diff --git a/app/views/admin/configs/_apple_auth_provider_settings.html.erb b/app/views/admin/settings/_apple_auth_provider_settings.html.erb similarity index 100% rename from app/views/admin/configs/_apple_auth_provider_settings.html.erb rename to app/views/admin/settings/_apple_auth_provider_settings.html.erb diff --git a/app/views/admin/configs/_auth_provider_settings.html.erb b/app/views/admin/settings/_auth_provider_settings.html.erb similarity index 100% rename from app/views/admin/configs/_auth_provider_settings.html.erb rename to app/views/admin/settings/_auth_provider_settings.html.erb diff --git a/app/views/admin/configs/_form_submission.html.erb b/app/views/admin/settings/_form_submission.html.erb similarity index 100% rename from app/views/admin/configs/_form_submission.html.erb rename to app/views/admin/settings/_form_submission.html.erb diff --git a/app/views/admin/configs/show.html.erb b/app/views/admin/settings/show.html.erb similarity index 97% rename from app/views/admin/configs/show.html.erb rename to app/views/admin/settings/show.html.erb index 6dd4a93b4..3c463fe23 100644 --- a/app/views/admin/configs/show.html.erb +++ b/app/views/admin/settings/show.html.erb @@ -115,7 +115,7 @@
- <%= form_for(Settings::General.new, url: admin_config_path) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -533,7 +533,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -584,7 +584,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -609,7 +609,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path, html: { data: { testid: "emailDigestSectionForm" } }) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path, html: { data: { testid: "emailDigestSectionForm" } }) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -635,7 +635,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -660,7 +660,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -726,7 +726,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path, html: { data: { testid: "mascotSectionForm" } }) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path, html: { data: { testid: "mascotSectionForm" } }) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -766,7 +766,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -798,7 +798,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -843,7 +843,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -900,7 +900,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -998,7 +998,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -1038,7 +1038,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { @@ -1064,7 +1064,7 @@
<% end %> - <%= form_for(Settings::General.new, url: admin_config_path) do |f| %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", locals: { diff --git a/config/routes/admin.rb b/config/routes/admin.rb index bc8dc5c8e..536e1d5c4 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -18,6 +18,7 @@ namespace :admin do resources :authentications, only: [:create] resources :campaigns, only: [:create] resources :communities, only: [:create] + resources :general_settings, only: [:create] resources :mandatory_settings, only: [:create] resources :rate_limits, only: [:create] resources :user_experiences, only: [:create] @@ -69,7 +70,8 @@ namespace :admin do end scope :customization do - resource :config + # We renamed the controller but don't want to change the route (yet) + resource :config, controller: "settings" resources :display_ads, only: %i[index edit update new create destroy] resources :html_variants, only: %i[index edit update new create show destroy] resources :navigation_links, only: %i[index update create destroy] diff --git a/spec/requests/admin/configs_spec.rb b/spec/requests/admin/configs_spec.rb index 7dbcb641b..079ef9d4f 100644 --- a/spec/requests/admin/configs_spec.rb +++ b/spec/requests/admin/configs_spec.rb @@ -14,7 +14,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "bars the regular user to access" do - expect { post admin_config_path, params: {} }.to raise_error(Pundit::NotAuthorizedError) + expect { post admin_settings_general_settings_path, params: {} }.to raise_error(Pundit::NotAuthorizedError) end end @@ -28,7 +28,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "does not allow user to update config if they have proper confirmation" do expected_image_url = "https://dummyimage.com/300x300.png" expect do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { favicon_url: expected_image_url }, confirmation: confirmation_message } @@ -38,7 +38,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "does not allow user to update config if they do not have proper confirmation" do expected_image_url = "https://dummyimage.com/300x300.png" expect do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { favicon_url: expected_image_url }, confirmation: "Not proper" } @@ -53,7 +53,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "updates settings admin action taken" do expect do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { health_check_token: "token" }, confirmation: confirmation_message } @@ -63,7 +63,7 @@ RSpec.describe "/admin/customization/config", type: :request do describe "API tokens" do it "updates the health_check_token" do token = rand(20).to_s - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { health_check_token: token }, confirmation: confirmation_message } @@ -71,7 +71,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "sets video_encoder_key" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { video_encoder_key: "123abc" }, confirmation: confirmation_message } @@ -80,21 +80,25 @@ RSpec.describe "/admin/customization/config", type: :request do end describe "Authentication" do + let(:provider) { "twitter" } + + before do + allow(Authentication::Providers).to receive(:available).and_return([provider]) + end + it "updates enabled authentication providers" do - enabled = Authentication::Providers.available.last.to_s post admin_settings_authentications_path, params: { settings_authentication: { - "#{enabled}_key": "someKey", - "#{enabled}_secret": "someSecret", - auth_providers_to_enable: enabled + "#{provider}_key": "someKey", + "#{provider}_secret": "someSecret", + auth_providers_to_enable: provider }, confirmation: confirmation_message } - expect(Settings::Authentication.providers).to eq([enabled]) + expect(Settings::Authentication.providers).to eq([provider]) end it "strips empty elements" do - provider = Authentication::Providers.available.last.to_s enabled = "#{provider}, '', nil" post admin_settings_authentications_path, params: { settings_authentication: { @@ -108,12 +112,13 @@ RSpec.describe "/admin/customization/config", type: :request do end it "does not update enabled authentication providers if any associated key missing" do - enabled = Authentication::Providers.available.first.to_s + allow(Settings::Authentication).to receive(:"#{provider}_secret").and_return(nil) + post admin_settings_authentications_path, params: { settings_authentication: { - "#{enabled}_key": "someKey", - "#{enabled}_secret": "", - auth_providers_to_enable: enabled + "#{provider}_key": "someKey", + "#{provider}_secret": "", + auth_providers_to_enable: provider }, confirmation: confirmation_message } @@ -279,7 +284,7 @@ RSpec.describe "/admin/customization/config", type: :request do describe "Emails" do it "does not update the default email address" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { email_addresses: { default: "random@example.com" } }, confirmation: confirmation_message } @@ -290,7 +295,7 @@ RSpec.describe "/admin/customization/config", type: :request do describe "Email digest frequency" do it "updates periodic_email_digest" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { periodic_email_digest: 1 }, confirmation: confirmation_message } @@ -299,7 +304,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "rejects update without proper confirmation" do expect do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { periodic_email_digest: 6 }, confirmation: "Incorrect yo!" } @@ -310,7 +315,7 @@ RSpec.describe "/admin/customization/config", type: :request do describe "Google Analytics Reporting API v4" do it "updates ga_tracking_id" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { ga_tracking_id: "abc" }, confirmation: confirmation_message } @@ -324,7 +329,7 @@ RSpec.describe "/admin/customization/config", type: :request do expect(Settings::General.main_social_image).to eq(expected_default_image_url) expected_image_url = "https://dummyimage.com/300x300.png" - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { main_social_image: expected_image_url }, confirmation: confirmation_message } @@ -334,7 +339,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "updates main_social_image with a valid image" do expected_image = "https://dummyimage.com/300x300" - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { main_social_image: expected_image }, confirmation: confirmation_message } @@ -344,7 +349,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "only updates the main_social_image if given a valid image URL" do invalid_image_url = "![logo_lowres]https://dummyimage.com/300x300" expect do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { main_social_image: invalid_image_url }, confirmation: confirmation_message } @@ -353,7 +358,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "updates favicon_url" do expected_image_url = "https://dummyimage.com/300x300.png" - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { favicon_url: expected_image_url }, confirmation: confirmation_message } @@ -364,7 +369,7 @@ RSpec.describe "/admin/customization/config", type: :request do expected_default_image_url = Settings::General.get_default(:logo_png) expected_image_url = "https://dummyimage.com/300x300.png" expect do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { logo_png: expected_image_url }, confirmation: confirmation_message } @@ -374,7 +379,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "updates logo_png with a valid image" do expected_image = "https://dummyimage.com/300x300" - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { logo_png: expected_image }, confirmation: confirmation_message } @@ -384,7 +389,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "only updates the logo_png if given a valid image URL" do invalid_image_url = "![logo_lowres]https://dummyimage.com/300x300.png" expect do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { logo_png: invalid_image_url }, confirmation: confirmation_message } @@ -393,7 +398,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "updates logo_svg" do expected_image_url = "https://dummyimage.com/300x300.png" - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { logo_svg: expected_image_url }, confirmation: confirmation_message } @@ -403,7 +408,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "rejects update without proper confirmation" do expected_image_url = "https://dummyimage.com/300x300.png" expect do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { logo_svg: expected_image_url }, confirmation: "Incorrect yo!" } @@ -413,7 +418,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "rejects update without any confirmation" do expected_image_url = "https://dummyimage.com/300x300.png" expect do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { logo_svg: expected_image_url }, confirmation: "" } @@ -424,7 +429,7 @@ RSpec.describe "/admin/customization/config", type: :request do describe "Mascot" do it "updates the mascot_user_id" do expected_mascot_user_id = 2 - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { mascot_user_id: expected_mascot_user_id }, confirmation: confirmation_message } @@ -435,7 +440,7 @@ RSpec.describe "/admin/customization/config", type: :request do expected_default_image_url = Settings::General.get_default(:mascot_image_url) expected_image_url = "https://dummyimage.com/300x300.png" expect do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { mascot_image_url: expected_image_url }, confirmation: confirmation_message } @@ -446,7 +451,7 @@ RSpec.describe "/admin/customization/config", type: :request do describe "Meta Keywords" do it "updates meta keywords" do expected_keywords = { "default" => "software, people", "article" => "user, experience", "tag" => "bye" } - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { meta_keywords: expected_keywords }, confirmation: confirmation_message } @@ -458,7 +463,7 @@ RSpec.describe "/admin/customization/config", type: :request do describe "Monetization" do it "updates payment pointer" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { payment_pointer: "$pay.yo" }, confirmation: confirmation_message } @@ -466,7 +471,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "updates stripe configs" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { stripe_api_key: "sk_live_yo", stripe_publishable_key: "pk_live_haha" @@ -480,7 +485,7 @@ RSpec.describe "/admin/customization/config", type: :request do describe "Newsletter" do it "updates mailchimp_api_key" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { mailchimp_api_key: "abc" }, confirmation: confirmation_message } @@ -488,7 +493,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "updates mailchimp_newsletter_id" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { mailchimp_newsletter_id: "abc" }, confirmation: confirmation_message } @@ -496,7 +501,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "updates mailchimp_sustaining_members_id" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { mailchimp_sustaining_members_id: "abc" }, confirmation: confirmation_message } @@ -504,7 +509,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "updates mailchimp_tag_moderators_id" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { mailchimp_tag_moderators_id: "abc" }, confirmation: confirmation_message } @@ -512,7 +517,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "updates mailchimp_community_moderators_id" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { mailchimp_community_moderators_id: "abc" }, confirmation: confirmation_message } @@ -523,7 +528,7 @@ RSpec.describe "/admin/customization/config", type: :request do describe "Onboarding" do it "updates onboarding_background_image" do expected_image_url = "https://dummyimage.com/300x300.png" - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { onboarding_background_image: expected_image_url }, confirmation: confirmation_message @@ -532,7 +537,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "removes space suggested_tags" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { suggested_tags: "hey, haha,hoho, bobo fofo" }, confirmation: confirmation_message } @@ -540,7 +545,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "downcases suggested_tags" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { suggested_tags: "hey, haha,hoHo, Bobo Fofo" }, confirmation: confirmation_message } @@ -548,7 +553,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "removes space suggested_users" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { suggested_users: "piglet, tigger,eeyore, Christopher Robin, kanga,roo" }, @@ -558,7 +563,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "downcases suggested_users" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { suggested_users: "piglet, tigger,EEYORE, Christopher Robin, KANGA,RoO" }, @@ -569,7 +574,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "updates prefer_manual_suggested_users to true" do prefer_manual = true - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { prefer_manual_suggested_users: prefer_manual }, confirmation: confirmation_message } @@ -578,7 +583,7 @@ RSpec.describe "/admin/customization/config", type: :request do it "updates prefer_manual_suggested_users to false" do prefer_manual = false - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { prefer_manual_suggested_users: prefer_manual }, confirmation: confirmation_message } @@ -760,7 +765,7 @@ RSpec.describe "/admin/customization/config", type: :request do describe "Social Media" do it "updates social_media_handles" do expected_handle = { "facebook" => "tpd", "github" => "", "instagram" => "", "twitch" => "", "twitter" => "" } - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { social_media_handles: expected_handle }, confirmation: confirmation_message } @@ -775,12 +780,14 @@ RSpec.describe "/admin/customization/config", type: :request do } it "does not update the twitter hashtag without the correct confirmation text" do - expect { post admin_config_path, params: params }.to raise_error ActionController::BadRequest + expect do + post admin_settings_general_settings_path, params: params + end.to raise_error ActionController::BadRequest end it "updates the twitter hashtag" do params["confirmation"] = confirmation_message - post admin_config_path, params: params + post admin_settings_general_settings_path, params: params expect(Settings::General.twitter_hashtag.to_s).to eq twitter_hashtag end end @@ -789,7 +796,7 @@ RSpec.describe "/admin/customization/config", type: :request do describe "Sponsors" do it "updates the sponsor_headline" do headline = "basic" - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { sponsor_headline: headline }, confirmation: confirmation_message } @@ -799,7 +806,7 @@ RSpec.describe "/admin/customization/config", type: :request do describe "Tags" do it "removes space sidebar_tags" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { sidebar_tags: "hey, haha,hoho, bobo fofo" }, confirmation: confirmation_message } @@ -807,7 +814,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "downcases sidebar_tags" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { sidebar_tags: "hey, haha,hoHo, Bobo Fofo" }, confirmation: confirmation_message } @@ -815,7 +822,7 @@ RSpec.describe "/admin/customization/config", type: :request do end it "creates tags if they do not exist" do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { sidebar_tags: "bobofogololo, spla, bla" }, confirmation: confirmation_message } @@ -915,7 +922,7 @@ RSpec.describe "/admin/customization/config", type: :request do Settings::General.credit_prices_in_cents.each_key do |size| new_prices = original_prices.merge(size => 123) expect do - post admin_config_path, params: { + post admin_settings_general_settings_path, params: { settings_general: { credit_prices_in_cents: new_prices }, confirmation: confirmation_message }