Better handling of assistant import (#591)

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

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

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

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

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