Use hstore more directly in repository storages form

Running the admin form off of the hstore directly should allow us to
synchronise the database more easily with any storage config changes.

app/controllers/admin/application_settings_controller.rb

@@ -249,7 +249,8 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       disabled_oauth_sign_in_sources: [],
       import_sources: [],
       repository_storages: [],
-      restricted_visibility_levels: []
+      restricted_visibility_levels: [],
+      repository_storages_weighted: {}
     ]
   end
 

app/helpers/application_settings_helper.rb

@@ -37,13 +37,8 @@ module ApplicationSettingsHelper
   end
 
   def storage_weights
-    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
-      }
+    Gitlab.config.repositories.storages.keys.each_with_object(OpenStruct.new) do |storage, weights|
+      weights[storage.to_sym] = @application_setting.repository_storages_weighted[storage] || 0
     end
   end
 

app/models/application_setting.rb

@@ -502,6 +502,7 @@ 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

app/models/application_setting_implementation.rb

@@ -465,16 +465,20 @@ 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_#{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)
+      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)
     end
   end
 

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

@@ -18,8 +18,9 @@
             = _('Enter weights for storages for new repositories.')
             = link_to sprite_icon('question-o'), help_page_path('administration/repository_storage_paths')
         .form-check
-          - 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'
+          = 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
   = 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

@@ -34882,6 +34882,9 @@ 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 ""
 
@@ -36288,6 +36291,12 @@ 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/features/admin/admin_settings_spec.rb

@@ -384,7 +384,20 @@ RSpec.describe 'Admin updates settings' do
           click_button 'Save changes'
         end
 
-        expect(current_settings.repository_storages_weighted_default).to be 50
+        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)
       end
     end
 

spec/helpers/application_settings_helper_spec.rb

@@ -130,20 +130,15 @@ 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([
-          { 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 }
-        ])
+      expect(helper.storage_weights).to eq(OpenStruct.new(
+                                             default: 100,
+                                             storage_1: 50,
+                                             storage_2: 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.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('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.to allow_value(400).for(:notes_create_limit) }
     it { is_expected.not_to allow_value('two').for(:notes_create_limit) }
@@ -1007,11 +1007,4 @@ 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/views/admin/application_settings/_repository_storage.html.haml_spec.rb

@@ -3,34 +3,49 @@
 require 'spec_helper'
 
 RSpec.describe 'admin/application_settings/_repository_storage.html.haml' do
-  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
+  let(:app_settings) { build(:application_setting, repository_storages_weighted: repository_storages_weighted) }
 
   before do
-    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)
+    stub_storage_settings({ 'default': {}, 'mepmep': {}, 'foobar': {} })
     assign(:application_setting, app_settings)
-    allow(ApplicationSetting).to receive(:repository_storages_weighted_attributes).and_return(repository_storages_weighted_attributes)
   end
 
-  context 'when multiple storages are available' do
+  context 'additional storage config' do
+    let(:repository_storages_weighted) do
+      {
+        'default' => 100,
+        'mepmep' => 50
+      }
+    end
+
     it 'lists them all' do
       render
 
-      # lists storages that are saved with weights
-      repository_storages_weighted.each do |storage_name, storage_weight|
+      Gitlab.config.repositories.storages.keys.each do |storage_name|
         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