Revert "Merge branch 'fix_repo_storage_weights_admin' into 'master'"

This reverts merge request !53803

app/controllers/admin/application_settings_controller.rb

@@ -237,6 +237,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
     [
       *::ApplicationSettingsHelper.visible_attributes,
       *::ApplicationSettingsHelper.external_authorization_service_attributes,
+      *ApplicationSetting.repository_storages_weighted_attributes,
       *ApplicationSetting.kroki_formats_attributes.keys.map { |key| "kroki_formats_#{key}".to_sym },
       :lets_encrypt_notification_email,
       :lets_encrypt_terms_of_service_accepted,
@@ -247,8 +248,8 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       :default_branch_name,
       disabled_oauth_sign_in_sources: [],
       import_sources: [],
-      restricted_visibility_levels: [],
-      repository_storages_weighted: {}
+      repository_storages: [],
+      restricted_visibility_levels: []
     ]
   end
 

app/helpers/application_settings_helper.rb

@@ -37,8 +37,13 @@ module ApplicationSettingsHelper
   end
 
   def storage_weights
-    Gitlab.config.repositories.storages.keys.each_with_object(OpenStruct.new) do |storage, weights|
-      weights[storage.to_sym] = @application_setting.repository_storages_weighted[storage] || 0
+    ApplicationSetting.repository_storages_weighted_attributes.map do |attribute|
+      storage = attribute.to_s.delete_prefix('repository_storages_weighted_')
+      {
+        name: attribute,
+        label: storage,
+        value: @application_setting.repository_storages_weighted[storage] || 0
+      }
     end
   end
 

app/models/application_setting.rb

@@ -25,6 +25,10 @@ class ApplicationSetting < ApplicationRecord
   alias_attribute :instance_group_id, :instance_administrators_group_id
   alias_attribute :instance_administrators_group, :instance_group
 
+  def self.repository_storages_weighted_attributes
+    @repository_storages_weighted_atributes ||= Gitlab.config.repositories.storages.keys.map { |k| "repository_storages_weighted_#{k}".to_sym }.freeze
+  end
+
   def self.kroki_formats_attributes
     {
       blockdiag: {
@@ -40,6 +44,7 @@ class ApplicationSetting < ApplicationRecord
   end
 
   store_accessor :kroki_formats, *ApplicationSetting.kroki_formats_attributes.keys, prefix: true
+  store_accessor :repository_storages_weighted, *Gitlab.config.repositories.storages.keys, prefix: true
 
   # Include here so it can override methods from
   # `add_authentication_token_field`
@@ -498,7 +503,6 @@ class ApplicationSetting < ApplicationRecord
             inclusion: { in: [true, false], message: _('must be a boolean value') }
 
   before_validation :ensure_uuid!
-  before_validation :coerce_repository_storages_weighted, if: :repository_storages_weighted_changed?
 
   before_save :ensure_runners_registration_token
   before_save :ensure_health_check_access_token
@@ -579,6 +583,12 @@ class ApplicationSetting < ApplicationRecord
     recaptcha_enabled || login_recaptcha_protection_enabled
   end
 
+  repository_storages_weighted_attributes.each do |attribute|
+    define_method :"#{attribute}=" do |value|
+      super(value.to_i)
+    end
+  end
+
   kroki_formats_attributes.keys.each do |key|
     define_method :"kroki_formats_#{key}=" do |value|
       super(::Gitlab::Utils.to_boolean(value))

app/models/application_setting_implementation.rb

@@ -298,6 +298,10 @@ module ApplicationSettingImplementation
     Array(read_attribute(:repository_storages))
   end
 
+  def repository_storages_weighted
+    read_attribute(:repository_storages_weighted)
+  end
+
   def commit_email_hostname
     super.presence || self.class.default_commit_email_hostname
   end
@@ -329,10 +333,9 @@ module ApplicationSettingImplementation
 
   def normalized_repository_storage_weights
     strong_memoize(:normalized_repository_storage_weights) do
-      repository_storages_weights = repository_storages_weighted.slice(*Gitlab.config.repositories.storages.keys)
-      weights_total = repository_storages_weights.values.reduce(:+)
+      weights_total = repository_storages_weighted.values.reduce(:+)
 
-      repository_storages_weights.transform_values do |w|
+      repository_storages_weighted.transform_values do |w|
         next w if weights_total == 0
 
         w.to_f / weights_total
@@ -470,20 +473,16 @@ module ApplicationSettingImplementation
       invalid.empty?
   end
 
-  def coerce_repository_storages_weighted
-    repository_storages_weighted.transform_values!(&:to_i)
-  end
-
   def check_repository_storages_weighted
     invalid = repository_storages_weighted.keys - Gitlab.config.repositories.storages.keys
-    errors.add(:repository_storages_weighted, _("can't include: %{invalid_storages}") % { invalid_storages: invalid.join(", ") }) unless
+    errors.add(:repository_storages_weighted, "can't include: %{invalid_storages}" % { invalid_storages: invalid.join(", ") }) unless
       invalid.empty?
 
     repository_storages_weighted.each do |key, val|
       next unless val.present?
 
-      errors.add(:repository_storages_weighted, _("value for '%{storage}' must be an integer") % { storage: key }) unless val.is_a?(Integer)
-      errors.add(:repository_storages_weighted, _("value for '%{storage}' must be between 0 and 100") % { storage: key }) unless val.between?(0, 100)
+      errors.add(:"repository_storages_weighted_#{key}", "value must be an integer") unless val.is_a?(Integer)
+      errors.add(:"repository_storages_weighted_#{key}", "value must be between 0 and 100") unless val.between?(0, 100)
     end
   end
 

app/views/admin/application_settings/_repository_storage.html.haml

@@ -18,9 +18,8 @@
             = _('Enter weights for storages for new repositories.')
             = link_to sprite_icon('question-o'), help_page_path('administration/repository_storage_paths')
         .form-check
-          = f.fields_for :repository_storages_weighted, storage_weights do |storage_form|
-            - Gitlab.config.repositories.storages.keys.each do |storage|
-              = storage_form.text_field storage, class: 'form-text-input'
-              = storage_form.label storage, storage, class: 'label-bold form-check-label'
-              %br
+          - storage_weights.each do |attribute|
+            = f.text_field attribute[:name], class: 'form-text-input', value: attribute[:value]
+            = f.label attribute[:label], attribute[:label], class: 'label-bold form-check-label'
+            %br
   = f.submit _('Save changes'), class: "gl-button btn btn-success qa-save-changes-button"

changelogs/unreleased/fix_repo_storage_weights_admin.yml

----
-title: Allow saving repository weights after a storage has been removed
-merge_request: 53803
-author:
-type: fixed

locale/gitlab.pot

@@ -34923,9 +34923,6 @@ msgstr ""
 msgid "can't be enabled because signed commits are required for this project"
 msgstr ""
 
-msgid "can't include: %{invalid_storages}"
-msgstr ""
-
 msgid "cannot be a date in the past"
 msgstr ""
 
@@ -36332,12 +36329,6 @@ msgstr ""
 msgid "v%{version} published %{timeAgo}"
 msgstr ""
 
-msgid "value for '%{storage}' must be an integer"
-msgstr ""
-
-msgid "value for '%{storage}' must be between 0 and 100"
-msgstr ""
-
 msgid "verify ownership"
 msgstr ""
 

spec/controllers/admin/application_settings_controller_spec.rb

@@ -144,10 +144,10 @@ RSpec.describe Admin::ApplicationSettingsController do
     end
 
     it 'updates repository_storages_weighted setting' do
-      put :update, params: { application_setting: { repository_storages_weighted: { default: 75 } } }
+      put :update, params: { application_setting: { repository_storages_weighted_default: 75 } }
 
       expect(response).to redirect_to(general_admin_application_settings_path)
-      expect(ApplicationSetting.current.repository_storages_weighted).to eq('default' => 75)
+      expect(ApplicationSetting.current.repository_storages_weighted_default).to eq(75)
     end
 
     it 'updates kroki_formats setting' do

spec/features/admin/admin_settings_spec.rb

@@ -384,20 +384,7 @@ RSpec.describe 'Admin updates settings' do
           click_button 'Save changes'
         end
 
-        expect(current_settings.repository_storages_weighted).to eq('default' => 50)
-      end
-
-      it 'still saves when settings are outdated' do
-        current_settings.update_attribute :repository_storages_weighted, { 'default' => 100, 'outdated' => 100 }
-
-        visit repository_admin_application_settings_path
-
-        page.within('.as-repository-storage') do
-          fill_in 'application_setting_repository_storages_weighted_default', with: 50
-          click_button 'Save changes'
-        end
-
-        expect(current_settings.repository_storages_weighted).to eq('default' => 50)
+        expect(current_settings.repository_storages_weighted_default).to be 50
       end
     end
 

spec/helpers/application_settings_helper_spec.rb

@@ -130,15 +130,20 @@ RSpec.describe ApplicationSettingsHelper do
     before do
       helper.instance_variable_set(:@application_setting, application_setting)
       stub_storage_settings({ 'default': {}, 'storage_1': {}, 'storage_2': {} })
+      allow(ApplicationSetting).to receive(:repository_storages_weighted_attributes).and_return(
+        [:repository_storages_weighted_default,
+         :repository_storages_weighted_storage_1,
+         :repository_storages_weighted_storage_2])
+
       stub_application_setting(repository_storages_weighted: { 'default' => 100, 'storage_1' => 50, 'storage_2' => nil })
     end
 
     it 'returns storages correctly' do
-      expect(helper.storage_weights).to eq(OpenStruct.new(
-                                             default: 100,
-                                             storage_1: 50,
-                                             storage_2: 0
-                                           ))
+      expect(helper.storage_weights).to eq([
+          { name: :repository_storages_weighted_default, label: 'default', value: 100 },
+          { name: :repository_storages_weighted_storage_1, label: 'storage_1', value: 50 },
+          { name: :repository_storages_weighted_storage_2, label: 'storage_2', value: 0 }
+        ])
     end
   end
 

spec/models/application_setting_spec.rb

@@ -105,14 +105,14 @@ RSpec.describe ApplicationSetting do
 
     it { is_expected.not_to allow_value(false).for(:hashed_storage_enabled) }
 
-    it { is_expected.to allow_value('default' => 0).for(:repository_storages_weighted) }
-    it { is_expected.to allow_value('default' => 50).for(:repository_storages_weighted) }
-    it { is_expected.to allow_value('default' => 100).for(:repository_storages_weighted) }
-    it { is_expected.to allow_value('default' => '90').for(:repository_storages_weighted) }
-    it { is_expected.to allow_value('default' => nil).for(:repository_storages_weighted) }
-    it { is_expected.not_to allow_value('default' => -1).for(:repository_storages_weighted).with_message("value for 'default' must be between 0 and 100") }
-    it { is_expected.not_to allow_value('default' => 101).for(:repository_storages_weighted).with_message("value for 'default' must be between 0 and 100") }
-    it { is_expected.not_to allow_value('default' => 100, shouldntexist: 50).for(:repository_storages_weighted).with_message("can't include: shouldntexist") }
+    it { is_expected.not_to allow_value(101).for(:repository_storages_weighted_default) }
+    it { is_expected.to allow_value('90').for(:repository_storages_weighted_default) }
+    it { is_expected.not_to allow_value(-1).for(:repository_storages_weighted_default) }
+    it { is_expected.to allow_value(100).for(:repository_storages_weighted_default) }
+    it { is_expected.to allow_value(0).for(:repository_storages_weighted_default) }
+    it { is_expected.to allow_value(50).for(:repository_storages_weighted_default) }
+    it { is_expected.to allow_value(nil).for(:repository_storages_weighted_default) }
+    it { is_expected.not_to allow_value({ default: 100, shouldntexist: 50 }).for(:repository_storages_weighted) }
 
     it { is_expected.to allow_value(400).for(:notes_create_limit) }
     it { is_expected.not_to allow_value('two').for(:notes_create_limit) }
@@ -984,6 +984,12 @@ RSpec.describe ApplicationSetting do
 
   it_behaves_like 'application settings examples'
 
+  describe 'repository_storages_weighted_attributes' do
+    it 'returns the keys for repository_storages_weighted' do
+      expect(subject.class.repository_storages_weighted_attributes).to eq([:repository_storages_weighted_default])
+    end
+  end
+
   describe 'kroki_format_supported?' do
     it 'returns true when Excalidraw is enabled' do
       subject.kroki_formats_excalidraw = true
@@ -1027,4 +1033,11 @@ RSpec.describe ApplicationSetting do
       expect(subject.kroki_formats_excalidraw).to eq(true)
     end
   end
+
+  it 'does not allow to set weight for non existing storage' do
+    setting.repository_storages_weighted = { invalid_storage: 100 }
+
+    expect(setting).not_to be_valid
+    expect(setting.errors.messages[:repository_storages_weighted]).to match_array(["can't include: invalid_storage"])
+  end
 end

spec/support/shared_examples/models/application_setting_shared_examples.rb

@@ -289,7 +289,6 @@ RSpec.shared_examples 'application settings examples' do
 
   describe '#pick_repository_storage' do
     before do
-      allow(Gitlab.config.repositories.storages).to receive(:keys).and_return(%w(default backup))
       allow(setting).to receive(:repository_storages_weighted).and_return({ 'default' => 20, 'backup' => 80 })
     end
 
@@ -305,19 +304,15 @@ RSpec.shared_examples 'application settings examples' do
   describe '#normalized_repository_storage_weights' do
     using RSpec::Parameterized::TableSyntax
 
-    where(:config_storages, :storages, :normalized) do
-      %w(default backup) | { 'default' => 0, 'backup' => 100 }   | { 'default' => 0.0, 'backup' => 1.0 }
-      %w(default backup) | { 'default' => 100, 'backup' => 100 } | { 'default' => 0.5, 'backup' => 0.5 }
-      %w(default backup) | { 'default' => 20, 'backup' => 80 }   | { 'default' => 0.2, 'backup' => 0.8 }
-      %w(default backup) | { 'default' => 0, 'backup' => 0 }     | { 'default' => 0.0, 'backup' => 0.0 }
-      %w(default)        | { 'default' => 0, 'backup' => 100 }   | { 'default' => 0.0 }
-      %w(default)        | { 'default' => 100, 'backup' => 100 } | { 'default' => 1.0 }
-      %w(default)        | { 'default' => 20, 'backup' => 80 }   | { 'default' => 1.0 }
+    where(:storages, :normalized) do
+      { 'default' => 0, 'backup' => 100 }   | { 'default' => 0.0, 'backup' => 1.0 }
+      { 'default' => 100, 'backup' => 100 } | { 'default' => 0.5, 'backup' => 0.5 }
+      { 'default' => 20, 'backup' => 80 }   | { 'default' => 0.2, 'backup' => 0.8 }
+      { 'default' => 0, 'backup' => 0 }     | { 'default' => 0.0, 'backup' => 0.0 }
     end
 
     with_them do
       before do
-        allow(Gitlab.config.repositories.storages).to receive(:keys).and_return(config_storages)
         allow(setting).to receive(:repository_storages_weighted).and_return(storages)
       end
 

spec/views/admin/application_settings/_repository_storage.html.haml_spec.rb

@@ -3,49 +3,34 @@
 require 'spec_helper'
 
 RSpec.describe 'admin/application_settings/_repository_storage.html.haml' do
-  let(:app_settings) { build(:application_setting, repository_storages_weighted: repository_storages_weighted) }
+  let(:app_settings) { create(:application_setting) }
+  let(:repository_storages_weighted_attributes) { [:repository_storages_weighted_default, :repository_storages_weighted_mepmep, :repository_storages_weighted_foobar]}
+  let(:repository_storages_weighted) do
+    {
+      "default" => 100,
+      "mepmep" => 50
+    }
+  end
 
   before do
-    stub_storage_settings({ 'default': {}, 'mepmep': {}, 'foobar': {} })
+    allow(app_settings).to receive(:repository_storages_weighted).and_return(repository_storages_weighted)
+    allow(app_settings).to receive(:repository_storages_weighted_mepmep).and_return(100)
+    allow(app_settings).to receive(:repository_storages_weighted_foobar).and_return(50)
     assign(:application_setting, app_settings)
+    allow(ApplicationSetting).to receive(:repository_storages_weighted_attributes).and_return(repository_storages_weighted_attributes)
   end
 
-  context 'additional storage config' do
-    let(:repository_storages_weighted) do
-      {
-        'default' => 100,
-        'mepmep' => 50
-      }
-    end
-
+  context 'when multiple storages are available' do
     it 'lists them all' do
       render
 
-      Gitlab.config.repositories.storages.keys.each do |storage_name|
+      # lists storages that are saved with weights
+      repository_storages_weighted.each do |storage_name, storage_weight|
         expect(rendered).to have_content(storage_name)
       end
 
+      # lists storage not saved with weight
       expect(rendered).to have_content('foobar')
     end
   end
-
-  context 'fewer storage configs' do
-    let(:repository_storages_weighted) do
-      {
-        'default' => 100,
-        'mepmep' => 50,
-        'something_old' => 100
-      }
-    end
-
-    it 'lists only configured storages' do
-      render
-
-      Gitlab.config.repositories.storages.keys.each do |storage_name|
-        expect(rendered).to have_content(storage_name)
-      end
-
-      expect(rendered).not_to have_content('something_old')
-    end
-  end
 end