From 5841543bd941e6119ba2f641ee340b922eacfb64 Mon Sep 17 00:00:00 2001 From: Keegan George Date: Wed, 16 Jul 2025 14:51:32 -0700 Subject: [PATCH] FIX: validator --- config/locales/server.en.yml | 2 +- lib/configuration/llm_dependency_validator.rb | 3 +- lib/configuration/llm_validator.rb | 38 ++++++++-------- spec/configuration/llm_validator_spec.rb | 43 +++++++++++++------ 4 files changed, 54 insertions(+), 32 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 06179978..40602236 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -583,7 +583,7 @@ en: llm: configuration: create_llm: "You need to setup an LLM before enabling this feature" - disable_module_first: "You have to disable %{setting} first." + disable_modules_first: "You must disable these modules first: %{settings}" set_llm_first: "Set %{setting} first" model_unreachable: "We couldn't get a response from this model. Check your settings first." invalid_seeded_model: "You can't use this model with this feature" diff --git a/lib/configuration/llm_dependency_validator.rb b/lib/configuration/llm_dependency_validator.rb index c3b1a24f..8ca890be 100644 --- a/lib/configuration/llm_dependency_validator.rb +++ b/lib/configuration/llm_dependency_validator.rb @@ -10,8 +10,7 @@ module DiscourseAi def valid_value?(val) return true if val == "f" - @llm_dependency_setting_name = - DiscourseAi::Configuration::LlmValidator.new.choose_llm_setting_for(@opts[:name]) + @llm_dependency_setting_name = :ai_default_llm_model SiteSetting.public_send(@llm_dependency_setting_name).present? end diff --git a/lib/configuration/llm_validator.rb b/lib/configuration/llm_validator.rb index 26211946..ff63d8d3 100644 --- a/lib/configuration/llm_validator.rb +++ b/lib/configuration/llm_validator.rb @@ -9,10 +9,18 @@ module DiscourseAi def valid_value?(val) if val == "" - @parent_module_name = modules_and_choose_llm_settings.invert[@opts[:name]] + if @opts[:name] == :ai_default_llm_model + @parent_module_names = [] - @parent_enabled = SiteSetting.public_send(@parent_module_name) - return !@parent_enabled + enabled_settings.each do |setting_name| + if SiteSetting.public_send(setting_name) == true + @parent_module_names << setting_name + @parent_enabled = true + end + end + + return !@parent_enabled + end end run_test(val).tap { |result| @unreachable = result } @@ -43,11 +51,11 @@ module DiscourseAi end def error_message - if @parent_enabled + if @parent_enabled && @parent_module_names.present? return( I18n.t( - "discourse_ai.llm.configuration.disable_module_first", - setting: @parent_module_name, + "discourse_ai.llm.configuration.disable_modules_first", + settings: @parent_module_names.join(", "), ) ) end @@ -57,17 +65,13 @@ module DiscourseAi I18n.t("discourse_ai.llm.configuration.model_unreachable") end - def choose_llm_setting_for(module_enabler_setting) - modules_and_choose_llm_settings[module_enabler_setting] - end - - def modules_and_choose_llm_settings - { - ai_embeddings_semantic_search_enabled: :ai_default_llm_model, - ai_helper_enabled: :ai_default_llm_model, - ai_summarization_enabled: :ai_default_llm_model, - ai_translation_enabled: :ai_default_llm_model, - } + def enabled_settings + %i[ + ai_embeddings_semantic_search_enabled + ai_helper_enabled + ai_summarization_enabled + ai_translation_enabled + ] end end end diff --git a/spec/configuration/llm_validator_spec.rb b/spec/configuration/llm_validator_spec.rb index eb1aaad2..9e91a883 100644 --- a/spec/configuration/llm_validator_spec.rb +++ b/spec/configuration/llm_validator_spec.rb @@ -1,21 +1,40 @@ # frozen_string_literal: true -RSpec.describe DiscourseAi::Configuration::LlmValidator do +require "rails_helper" + +describe DiscourseAi::Configuration::LlmValidator do describe "#valid_value?" do - context "when the parent module is enabled and we try to reset the selected model" do - before do - assign_fake_provider_to(:ai_default_llm_model) - SiteSetting.ai_summarization_enabled = true - end + let(:validator) { described_class.new(name: :ai_default_llm_model) } - it "returns false and displays an error message" do - validator = described_class.new(name: :ai_summarization_model) + before do + assign_fake_provider_to(:ai_default_llm_model) + SiteSetting.ai_helper_enabled = false + SiteSetting.ai_summarization_enabled = false + SiteSetting.ai_embeddings_semantic_search_enabled = false + SiteSetting.ai_translation_enabled = false + end - value = validator.valid_value?("") + it "returns true when no modules are enabled and value is empty string" do + expect(validator.valid_value?("")).to eq(true) + end - expect(value).to eq(false) - expect(validator.error_message).to include("ai_summarization_enabled") - end + it "returns false when a module is enabled and value is empty string" do + SiteSetting.ai_helper_enabled = true + expect(validator.valid_value?("")).to eq(false) + expect(validator.error_message).to include("ai_helper_enabled") + end + + it "returns false when multiple modules are enabled and value is empty string" do + SiteSetting.ai_helper_enabled = true + SiteSetting.ai_summarization_enabled = true + expect(validator.valid_value?("")).to eq(false) + expect(validator.error_message).to include("ai_helper_enabled, ai_summarization_enabled") + end + + it "returns true for non-empty values regardless of module state" do + SiteSetting.ai_helper_enabled = true + SiteSetting.ai_summarization_enabled = true + expect(validator.valid_value?("some_model")).to eq(true) end end end