From 333b331eb9afa70cd426ad69042cf78eaf1743e5 Mon Sep 17 00:00:00 2001
From: Roman Rizzi <roman@discourse.org>
Date: Mon, 27 May 2024 16:44:08 -0300
Subject: [PATCH] FEATURE: Allow deleting custom LLMs. (#643)

This change allows us to delete custom models. It checks if there is no module using them.

It also fixes a bug where the after-create transition wasn't working. While this prevents a model from being saved multiple times, endpoint validations are still needed (will be added in a separate PR).:
---
 .../discourse_ai/admin/ai_llms_controller.rb  | 30 +++++++++++++++++
 .../discourse/components/ai-llm-editor.gjs    | 32 ++++++++++++++++---
 config/locales/client.en.yml                  |  5 ++-
 config/locales/server.en.yml                  |  4 +++
 config/routes.rb                              |  2 +-
 lib/configuration/llm_validator.rb            |  8 ++---
 .../requests/admin/ai_llms_controller_spec.rb | 21 ++++++++++++
 7 files changed, 92 insertions(+), 10 deletions(-)

diff --git a/app/controllers/discourse_ai/admin/ai_llms_controller.rb b/app/controllers/discourse_ai/admin/ai_llms_controller.rb
index 897fb152..54230fe0 100644
--- a/app/controllers/discourse_ai/admin/ai_llms_controller.rb
+++ b/app/controllers/discourse_ai/admin/ai_llms_controller.rb
@@ -49,6 +49,36 @@ module DiscourseAi
         end
       end
 
+      def destroy
+        llm_model = LlmModel.find(params[:id])
+
+        dependant_settings = %i[ai_helper_model ai_embeddings_semantic_search_hyde_model]
+
+        in_use_by = []
+        dependant_settings.each do |s_name|
+          in_use_by << s_name if SiteSetting.public_send(s_name) == "custom:#{llm_model.id}"
+        end
+
+        if !in_use_by.empty?
+          return(
+            render_json_error(
+              I18n.t(
+                "discourse_ai.llm.delete_failed",
+                settings: in_use_by.join(", "),
+                count: in_use_by.length,
+              ),
+              status: 409,
+            )
+          )
+        end
+
+        if llm_model.destroy
+          head :no_content
+        else
+          render_json_error llm_model
+        end
+      end
+
       def test
         RateLimiter.new(current_user, "llm_test_#{current_user.id}", 3, 1.minute).performed!
 
diff --git a/assets/javascripts/discourse/components/ai-llm-editor.gjs b/assets/javascripts/discourse/components/ai-llm-editor.gjs
index dd92abc2..e1f16db0 100644
--- a/assets/javascripts/discourse/components/ai-llm-editor.gjs
+++ b/assets/javascripts/discourse/components/ai-llm-editor.gjs
@@ -16,6 +16,7 @@ import DTooltip from "float-kit/components/d-tooltip";
 export default class AiLlmEditor extends Component {
   @service toasts;
   @service router;
+  @service dialog;
 
   @tracked isSaving = false;
 
@@ -45,10 +46,7 @@ export default class AiLlmEditor extends Component {
 
       if (isNew) {
         this.args.llms.addObject(this.args.model);
-        this.router.transitionTo(
-          "adminPlugins.show.discourse-ai-llms.show",
-          this.args.model
-        );
+        this.router.transitionTo("adminPlugins.show.discourse-ai-llms.index");
       } else {
         this.toasts.success({
           data: { message: I18n.t("discourse_ai.llms.saved") },
@@ -94,6 +92,24 @@ export default class AiLlmEditor extends Component {
     return this.testRunning || this.testResult !== null;
   }
 
+  @action
+  delete() {
+    return this.dialog.confirm({
+      message: I18n.t("discourse_ai.llms.confirm_delete"),
+      didConfirm: () => {
+        return this.args.model
+          .destroyRecord()
+          .then(() => {
+            this.args.llms.removeObject(this.args.model);
+            this.router.transitionTo(
+              "adminPlugins.show.discourse-ai-llms.index"
+            );
+          })
+          .catch(popupAjaxError);
+      },
+    });
+  }
+
   <template>
     <BackButton
       @route="adminPlugins.show.discourse-ai-llms"
@@ -182,6 +198,14 @@ export default class AiLlmEditor extends Component {
         >
           {{I18n.t "discourse_ai.llms.save"}}
         </DButton>
+        {{#unless @model.isNew}}
+          <DButton
+            @action={{this.delete}}
+            class="btn-danger ai-llm-editor__delete"
+          >
+            {{I18n.t "discourse_ai.llms.delete"}}
+          </DButton>
+        {{/unless}}
       </div>
 
       <div class="control-group ai-llm-editor-tests">
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index ee54016d..3c334a19 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -217,7 +217,10 @@ en:
         edit: "Edit"
         saved: "LLM Model Saved"
         back: "Back"
-        tests:
+        confirm_delete: Are you sure you want to delete this model?
+        delete: Delete
+
+        tests: 
           title: "Run Test"
           running: "Running test..."
           success: "Success!"
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index d65f4bde..b065bb43 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -319,6 +319,10 @@ en:
         configuration_hint:
           one: "Make sure the `%{settings}` setting was configured."
           other: "Make sure these settings were configured: %{settings}"
+      
+      delete_failed:
+        one: "We couldn't delete this model because %{settings} is using it. Update the setting and try again."
+        other: "We couldn't delete this model because %{settings} are using it. Update the settings and try again."
 
     embeddings:
       configuration:
diff --git a/config/routes.rb b/config/routes.rb
index cad7fc43..68446e4a 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -47,7 +47,7 @@ Discourse::Application.routes.draw do
     get "/ai-personas/:id/files/status", to: "discourse_ai/admin/ai_personas#indexing_status_check"
 
     resources :ai_llms,
-              only: %i[index create show update],
+              only: %i[index create show update destroy],
               path: "ai-llms",
               controller: "discourse_ai/admin/ai_llms" do
       collection { get :test }
diff --git a/lib/configuration/llm_validator.rb b/lib/configuration/llm_validator.rb
index e86f3188..ff0c92c4 100644
--- a/lib/configuration/llm_validator.rb
+++ b/lib/configuration/llm_validator.rb
@@ -28,11 +28,11 @@ module DiscourseAi
             @endpoint = endpoint
             return false
           end
-        end
 
-        if !can_talk_to_model?(val)
-          @unreachable = true
-          return false
+          if !can_talk_to_model?(val)
+            @unreachable = true
+            return false
+          end
         end
 
         true
diff --git a/spec/requests/admin/ai_llms_controller_spec.rb b/spec/requests/admin/ai_llms_controller_spec.rb
index 8bad4b05..736112d0 100644
--- a/spec/requests/admin/ai_llms_controller_spec.rb
+++ b/spec/requests/admin/ai_llms_controller_spec.rb
@@ -112,4 +112,25 @@ RSpec.describe DiscourseAi::Admin::AiLlmsController do
       end
     end
   end
+
+  describe "DELETE #destroy" do
+    fab!(:llm_model)
+
+    it "destroys the requested ai_persona" do
+      expect {
+        delete "/admin/plugins/discourse-ai/ai-llms/#{llm_model.id}.json"
+
+        expect(response).to have_http_status(:no_content)
+      }.to change(LlmModel, :count).by(-1)
+    end
+
+    it "validates the model is not in use" do
+      SiteSetting.ai_helper_model = "custom:#{llm_model.id}"
+
+      delete "/admin/plugins/discourse-ai/ai-llms/#{llm_model.id}.json"
+
+      expect(response.status).to eq(409)
+      expect(llm_model.reload).to eq(llm_model)
+    end
+  end
 end