Merge pull request #10508 from opf/bug/41947-fresh-instances-have-empty-strings-as-setting-value-for-feature_storages_module_active

[#41947] Fresh instances should have feature_storages_module_active false

app/models/setting.rb

@@ -97,7 +97,7 @@ class Setting < ApplicationRecord
           end
 
           value = self[:#{name}]
-          ActiveRecord::Type::Boolean.new.cast(value)
+          ActiveRecord::Type::Boolean.new.cast(value) || false
         end
 
         def self.#{name}=(value)
@@ -157,7 +157,7 @@ class Setting < ApplicationRecord
   end
 
   def value=(val)
-    unless Settings::Definition[name].writable?
+    unless definition.writable?
       raise NoMethodError, "#{name} is not writable but can be set through env vars or configuration.yml file."
     end
 
@@ -167,7 +167,7 @@ class Setting < ApplicationRecord
   def formatted_value(value)
     return value if value.blank?
 
-    if config.serialized?
+    if definition.serialized?
       return value.to_yaml
     end
 
@@ -180,7 +180,7 @@ class Setting < ApplicationRecord
   end
 
   def self.[]=(name, value)
-    old_setting = cached_or_default(name)
+    old_value = cached_or_default(name)
     new_setting = find_or_initialize_by(name: name)
     new_setting.value = value
 
@@ -195,11 +195,11 @@ class Setting < ApplicationRecord
       clear_cache(old_cache_key)
 
       # fire callbacks for name and pass as much information as possible
-      fire_callbacks(name, new_value, old_setting)
+      fire_callbacks(name, new_value, old_value)
 
       new_value
     else
-      old_setting
+      old_value
     end
   end
 
@@ -273,7 +273,7 @@ class Setting < ApplicationRecord
     definition = Settings::Definition[name]
 
     value = if definition.writable?
-              cached_settings.fetch(name) { Settings::Definition[name].value }
+              cached_settings.fetch(name) { definition.value }
             else
               definition.value
             end
@@ -320,16 +320,16 @@ class Setting < ApplicationRecord
     @settings_table_exists_yet ||= connection.data_source_exists?(table_name)
   end
 
-  # Unserialize a serialized settings value
+  # Deserialize a serialized settings value
   def self.deserialize(name, value)
     definition = Settings::Definition[name]
 
     if definition.serialized? && value.is_a?(String)
       YAML::safe_load(value, permitted_classes: [Symbol, ActiveSupport::HashWithIndifferentAccess, Date, Time])
-    elsif value.present?
-      read_formatted_setting value, definition.format
+    elsif value != '' && !value.nil?
+      read_formatted_setting(value, definition.format)
     else
-      value
+      definition.format == :string ? value : nil
     end
   end
 
@@ -352,10 +352,10 @@ class Setting < ApplicationRecord
 
   protected
 
-  def config
-    @config ||= Settings::Definition[name]
+  def definition
+    @definition ||= Settings::Definition[name]
   end
 
   delegate :format,
-           to: :config
+           to: :definition
 end

app/seeders/basic_data/setting_seeder.rb

@@ -38,7 +38,7 @@ module BasicData
     end
 
     def applicable?
-      !settings_not_in_db.empty?
+      settings_not_in_db.any?
     end
 
     def not_applicable_message
@@ -46,38 +46,33 @@ module BasicData
     end
 
     def data
-      @settings ||= begin
-        settings = Setting.definitions.select(&:writable?).each_with_object({}) do |definition, hash|
-          hash[definition.name] = definition.value || ''
+      @data ||= begin
+        settings = seedable_setting_definitions.each_with_object({}) do |definition, hash|
+          hash[definition.name] = definition.value
         end
 
         # deviate from the defaults specified in the settings definition here
         # to set a default role. The role cannot be specified in the definition as
         # that would mean to know the ID upfront.
-        update_unless_present(settings, 'new_project_user_role_id') do
-          Role.find_by(name: I18n.t(:default_role_project_admin)).try(:id)
-        end
+        settings['new_project_user_role_id'] = Role.find_by(name: I18n.t(:default_role_project_admin)).try(:id)
 
         # Set the closed status for repository commit references
-        update_unless_present(settings, 'commit_fix_status_id') do
-          Status.find_by(name: I18n.t(:default_status_closed)).try(:id)
-        end
+        settings['commit_fix_status_id'] = Status.find_by(name: I18n.t(:default_status_closed)).try(:id)
 
-        settings
+        settings.compact
       end
     end
 
     private
 
-    def update_unless_present(settings, key)
-      if !settings_in_db.include?(key)
-        value = yield
-        settings[key] = value unless value.nil?
-      end
+    def seedable_setting_definitions
+      Setting.definitions
+             .select(&:writable?)
+             .reject { |definition| definition.value.nil? }
     end
 
     def settings_in_db
-      @settings_in_db ||= Setting.all.pluck(:name)
+      Setting.all.pluck(:name)
     end
 
     def settings_not_in_db

config/constants/settings/definition.rb

@@ -57,7 +57,7 @@ module Settings
       when :float
         @value.to_f
       when :boolean
-        @value.is_a?(Integer) ? ActiveRecord::Type::Boolean.new.cast(@value) : @value
+        ActiveRecord::Type::Boolean.new.cast(@value)
       when :symbol
         @value.to_sym
       else

spec/lib/open_project/configuration_spec.rb

@@ -28,18 +28,7 @@
 
 require 'spec_helper'
 
-describe OpenProject::Configuration do
-  let!(:definitions_before) { Settings::Definition.all.dup }
-
-  before do
-    Settings::Definition.send(:reset)
-  end
-
-  after do
-    Settings::Definition.send(:reset)
-    Settings::Definition.instance_variable_set(:@all, definitions_before)
-  end
-
+describe OpenProject::Configuration, :settings_reset do
   describe '.[setting]' do
     it 'fetches the value' do
       expect(described_class.app_title)

spec/models/setting_spec.rb

@@ -111,6 +111,47 @@ describe Setting, type: :model do
       expect(described_class.app_title)
         .to eql('OpenProject')
     end
+
+    context 'when value is blank but not nil' do
+      it 'is read correctly for array' do
+        expect(Settings::Definition['apiv3_cors_origins'].format).to eq(:array) # safeguard
+        expect(described_class['apiv3_cors_origins']).to eq([])
+      end
+
+      it 'is read correctly for hash' do
+        expect(Settings::Definition['fog'].format).to eq(:hash) # safeguard
+        expect(described_class['fog']).to eq({})
+      end
+    end
+
+    context 'when value was seeded as empty string in database', :settings_reset do
+      let(:setting_name) { "my_setting" }
+
+      subject { described_class[setting_name] }
+
+      before do
+        Settings::Definition.add(
+          setting_name,
+          value: nil,
+          format: setting_format
+        )
+        described_class.create!(name: setting_name, value: '')
+      end
+
+      %i[array boolean date datetime hash symbol].each do |setting_format|
+        context "for a #{setting_format} setting" do
+          let(:setting_format) { setting_format }
+
+          it { is_expected.to be_nil }
+        end
+      end
+
+      context 'for a string setting' do
+        let(:setting_format) { :string }
+
+        it { is_expected.to eq('') }
+      end
+    end
   end
 
   describe '.[setting]?' do
@@ -295,7 +336,7 @@ describe Setting, type: :model do
       end
     end
 
-    context 'cache is not empty' do
+    context 'when cache is not empty' do
       let(:cached_hash) do
         { 'available_languages' => "---\n- en\n- de\n" }
       end

spec/seeders/demo_data_seeder_spec.rb

@@ -32,7 +32,11 @@ describe RootSeeder,
          'standard edition',
          with_config: { edition: 'standard' },
          with_settings: { journal_aggregation_time_minutes: 0 } do
-  it 'create the demo data' do
+  before do
+    allow($stdout).to receive(:puts) { |msg| Rails.logger.info(msg) }
+  end
+
+  it 'creates the demo data' do
     expect { described_class.new.do_seed! }.not_to raise_error
 
     expect(User.where(admin: true).count).to eq 1

spec/seeders/setting_seeder_spec.rb

@@ -28,44 +28,52 @@
 
 require 'spec_helper'
 
-describe 'SettingSeeder' do
-  subject { ::BasicData::SettingSeeder.new }
+describe ::BasicData::SettingSeeder do
+  subject { described_class.new }
 
   let(:new_project_role) { Role.find_by(name: I18n.t(:default_role_project_admin)) }
   let(:closed_status) { Status.find_by(name: I18n.t(:default_status_closed)) }
 
   before do
-    allow(STDOUT).to receive(:puts)
     allow(ActionMailer::Base).to receive(:perform_deliveries).and_return(false)
     allow(Delayed::Worker).to receive(:delay_jobs).and_return(false)
 
-    expect { BasicDataSeeder.new.seed! }.not_to raise_error
+    BasicData::BuiltinRolesSeeder.new.seed!
+    BasicData::RoleSeeder.new.seed!
+    BasicData::ColorSchemeSeeder.new.seed!
+    StandardSeeder::BasicData::StatusSeeder.new.seed!
+    described_class.new.seed!
   end
 
   def reseed!
-    expect(subject).to receive(:update_unless_present).twice.and_call_original
-    expect(subject).to be_applicable
-    expect { subject.seed! }.not_to raise_error
+    subject.seed!
   end
 
-  shared_examples 'settings' do
-    it 'applies initial settings' do
-      Setting.where(name: %w(commit_fix_status_id new_project_user_role_id)).delete_all
+  it 'applies initial settings' do
+    Setting.where(name: %w(commit_fix_status_id new_project_user_role_id)).delete_all
+    expect(subject).to be_applicable
 
-      reseed!
+    reseed!
 
-      expect(Setting.commit_fix_status_id).to eq closed_status.id
-      expect(Setting.new_project_user_role_id).to eq new_project_role.id
-    end
+    expect(subject).not_to be_applicable
+    expect(Setting.commit_fix_status_id).to eq closed_status.id
+    expect(Setting.new_project_user_role_id).to eq new_project_role.id
+  end
 
-    it 'does not override settings' do
-      Setting.commit_fix_status_id = 1337
-      Setting.where(name: 'new_project_user_role_id').delete_all
+  it 'does not override settings' do
+    Setting.commit_fix_status_id = 1337
+    Setting.where(name: 'new_project_user_role_id').delete_all
 
-      reseed!
+    reseed!
+
+    expect(Setting.commit_fix_status_id).to eq 1337
+    expect(Setting.new_project_user_role_id).to eq new_project_role.id
+  end
 
-      expect(Setting.commit_fix_status_id).to eq 1337
-      expect(Setting.new_project_user_role_id).to eq new_project_role.id
-    end
+  it 'does not seed settings whose default value is undefined' do
+    names_of_undefined_settings = Settings::Definition.all.select { _1.value == nil }.map(&:name)
+    # these ones are special as their value is set based on database ids
+    names_of_undefined_settings -= ["new_project_user_role_id", "commit_fix_status_id"]
+    expect(Setting.where(name: names_of_undefined_settings).pluck(:name)).to be_empty
   end
 end

spec/support/shared/with_settings.rb

@@ -38,8 +38,29 @@ def aggregate_mocked_settings(example, settings)
   settings
 end
 
+RSpec.shared_context 'with settings reset' do
+  let(:storages_module_active) { true }
+
+  around do |example|
+    definitions_before = Settings::Definition.all.dup
+    Settings::Definition.send(:reset)
+    example.run
+  ensure
+    Settings::Definition.send(:reset)
+    Settings::Definition.instance_variable_set(:@all, definitions_before)
+  end
+
+  before do
+    allow(OpenProject::FeatureDecisions).to receive(:storages_module_active?).and_return(storages_module_active)
+  end
+end
+
 RSpec.configure do |config|
-  config.before(:each) do |example|
+  # examples tagged with `:settings_reset` will automatically have the settings
+  # reset before the example, and restored after.
+  config.include_context "with settings reset", :settings_reset
+
+  config.before do |example|
     settings = example.metadata[:with_settings]
     if settings.present?
       settings = aggregate_mocked_settings(example, settings)