From 083d60f048de5c5b68e83035fd50a0756c24bba2 Mon Sep 17 00:00:00 2001 From: Keith Schacht Date: Sun, 12 Jan 2025 21:09:19 -0600 Subject: [PATCH] Better handling of assistant import (#591) --- app/models/assistant/export.rb | 2 +- app/models/assistant/slug.rb | 11 ++- .../settings/assistants_controller_test.rb | 18 +++++ test/models/assistant/export_test.rb | 71 +++++++++++++++++++ test/models/assistant/slug_test.rb | 45 +++++++++++- 5 files changed, 140 insertions(+), 7 deletions(-) diff --git a/app/models/assistant/export.rb b/app/models/assistant/export.rb index f0290cc1a..6cd077513 100644 --- a/app/models/assistant/export.rb +++ b/app/models/assistant/export.rb @@ -43,7 +43,7 @@ def import_from_file(path: Rails.root.join(DEFAULT_ASSISTANT_FILE), users: User. assistant = assistant.with_indifferent_access users.each do |user| asst = user.assistants.find_or_create_by(slug: assistant["slug"]) - asst.assign_attributes(assistant.except("slug")) + asst.assign_attributes(assistant.except("slug")) if asst.deleted_at.nil? asst.save! end end diff --git a/app/models/assistant/slug.rb b/app/models/assistant/slug.rb index a3a605ec2..fa21a0861 100644 --- a/app/models/assistant/slug.rb +++ b/app/models/assistant/slug.rb @@ -3,13 +3,18 @@ module Assistant::Slug included do before_validation :set_default_slug - before_save :clear_slug_on_delete + before_validation :clear_conflicting_deleted_assistant_slug + validates :slug, uniqueness: { scope: :user_id, message: "has already been taken" } end private - def clear_slug_on_delete - self.slug = nil if deleted_at_changed? && deleted_at_was.nil? && deleted_at.present? + def clear_conflicting_deleted_assistant_slug + return if slug.blank? + return if !slug_changed? + + conflicting_assistant = user.assistants_including_deleted.where.not(deleted_at: nil).find_by(slug: slug) + conflicting_assistant&.update_column(:slug, nil) if conflicting_assistant != self end def set_default_slug diff --git a/test/controllers/settings/assistants_controller_test.rb b/test/controllers/settings/assistants_controller_test.rb index dd2735fe5..2db8a531b 100644 --- a/test/controllers/settings/assistants_controller_test.rb +++ b/test/controllers/settings/assistants_controller_test.rb @@ -24,6 +24,24 @@ class Settings::AssistantsControllerTest < ActionDispatch::IntegrationTest assert_equal params, Assistant.last.slice(:name, :description, :instructions, :language_model_id) end + test "should show error when creating assistant with duplicate slug" do + existing_assistant = assistants(:samantha) + params = { + name: "New Assistant", + slug: existing_assistant.slug, + description: "A new description", + instructions: "New instructions", + language_model_id: language_models(:gpt_4o).id + } + + assert_no_difference("Assistant.count") do + post settings_assistants_url, params: { assistant: params } + end + + assert_response :unprocessable_entity + assert_contains_text "main", "Slug has already been taken" + end + test "should get edit" do get edit_settings_assistant_url(@assistant) assert_response :success diff --git a/test/models/assistant/export_test.rb b/test/models/assistant/export_test.rb index 60c8537f1..255864a90 100644 --- a/test/models/assistant/export_test.rb +++ b/test/models/assistant/export_test.rb @@ -33,6 +33,77 @@ class Assistant::ExportTest < ActiveSupport::TestCase assert_equal assistants.first.keys.sort, %w[name description instructions slug language_model_api_name].sort end + test "import_from_file updates existing undeleted assistant with matching slug" do + user = users(:keith) + existing_assistant = user.assistants.not_deleted.first + new_name = "Updated Name" + assistants = [{ + name: new_name, + slug: existing_assistant.slug, + description: "new description", + instructions: "new instructions", + language_model_api_name: language_models(:gpt_4o).api_name + }] + storage = { "assistants" => assistants } + path = Rails.root.join("tmp/update_existing.yml") + File.write(path, storage.to_yaml) + + assert_no_difference "Assistant.count" do + Assistant.import_from_file(path:, users: [user]) + end + existing_assistant.reload + assert_equal new_name, existing_assistant.name + end + + test "import_from_file skips deleted assistant with matching slug" do + user = users(:keith) + deleted_assistant = user.assistants.first + original_name = deleted_assistant.name + deleted_assistant.deleted! + + assistants = [{ + name: "New Assistant", + slug: deleted_assistant.slug, + description: "new description", + instructions: "new instructions", + language_model_api_name: language_models(:gpt_4o).api_name + }] + storage = { "assistants" => assistants } + path = Rails.root.join("tmp/skip_deleted.yml") + File.write(path, storage.to_yaml) + + assert_no_difference "Assistant.count" do + Assistant.import_from_file(path:, users: [user]) + end + deleted_assistant.reload + assert_equal original_name, deleted_assistant.name + assert deleted_assistant.deleted? + assert_nil user.assistants.not_deleted.find_by(slug: deleted_assistant.slug) + end + + test "import_from_file creates new assistant when no matching slug exists" do + user = users(:keith) + new_slug = "completely-new-slug" + + assistants = [{ + name: "Brand New Assistant", + slug: new_slug, + description: "new description", + instructions: "new instructions", + language_model_api_name: language_models(:gpt_4o).api_name + }] + storage = { "assistants" => assistants } + path = Rails.root.join("tmp/new_assistant.yml") + File.write(path, storage.to_yaml) + + assert_difference "Assistant.count", 1 do + Assistant.import_from_file(path:, users: [user]) + end + new_assistant = user.assistants.find_by(slug: new_slug) + assert_not_nil new_assistant + assert_equal "Brand New Assistant", new_assistant.name + end + test "import_from_file with only new models" do user = users(:keith) user.assistants.destroy_all diff --git a/test/models/assistant/slug_test.rb b/test/models/assistant/slug_test.rb index 554b6885c..4ec7df815 100644 --- a/test/models/assistant/slug_test.rb +++ b/test/models/assistant/slug_test.rb @@ -32,21 +32,42 @@ class Assistant::SlugTest < ActiveSupport::TestCase end end - test "clears slug when assistant is deleted" do + test "keeps slug when assistant is deleted" do assistant = assistants(:samantha) original_slug = assistant.slug assert_not_nil original_slug assistant.deleted! - assert_nil assistant.reload.slug + assert_equal original_slug, assistant.reload.slug + end - # Create a new assistant with the same name + test "clears slug of deleted assistant when new assistant takes its slug" do + assistant = assistants(:samantha) + original_slug = assistant.slug + assistant.deleted! + assert_equal original_slug, assistant.reload.slug + + # Create a new assistant with the same slug new_assistant = assistant.user.assistants.create!( name: assistant.name, + slug: original_slug, language_model: assistant.language_model, tools: assistant.tools ) assert_equal original_slug, new_assistant.slug + assert_nil assistant.reload.slug + end + + test "clears slug of deleted assistant when existing assistant changes to its slug" do + deleted_assistant = assistants(:samantha) + original_slug = deleted_assistant.slug + deleted_assistant.deleted! + + existing_assistant = assistants(:keith_gpt4) + existing_assistant.update!(slug: original_slug) + + assert_equal original_slug, existing_assistant.reload.slug + assert_nil deleted_assistant.reload.slug end test "does not clear slug when other attributes change" do @@ -56,4 +77,22 @@ class Assistant::SlugTest < ActiveSupport::TestCase assistant.update!(name: "New Name") assert_equal original_slug, assistant.reload.slug end + + test "fails to create a new assistant when slug collides with an existing assistant" do + existing = assistants(:samantha) + new_assistant = existing.user.assistants.new( + name: "Different Name", + slug: existing.slug, + language_model: existing.language_model + ) + assert_not new_assistant.valid? + assert_includes new_assistant.errors[:slug], "has already been taken" + end + + test "fails to update an assistant's slug when it collides with an existing assistant" do + existing = assistants(:samantha) + other = assistants(:keith_gpt4) + assert_not other.update(slug: existing.slug) + assert_includes other.errors[:slug], "has already been taken" + end end