diff --git a/CHANGELOG.md b/CHANGELOG.md index f32967f59b..0468b8d26b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ and this project adheres to ### Fixed +- Fix saving workflow with same name as deleted workflow + [#4165](https://github.com/OpenFn/lightning/pull/4165) - Fix validation error states not changing after undo (Ctrl+Z) on Workflow Settings [#4182](https://github.com/OpenFn/lightning/issues/4182)) diff --git a/assets/js/collaborative-editor/types/workflow.ts b/assets/js/collaborative-editor/types/workflow.ts index e8c794a079..cace5d8c13 100644 --- a/assets/js/collaborative-editor/types/workflow.ts +++ b/assets/js/collaborative-editor/types/workflow.ts @@ -17,7 +17,7 @@ import type { Trigger as TriggerType } from './trigger'; /** * Zod schema for workflow validation * - * Mirrors backend validation from lib/lightning/workflows/workflow.ex:81-89 + * Mirrors backend validation from lib/lightning/workflows/workflow.ex:81-103 */ export const WorkflowSchema = z.object({ id: z.string().uuid(), diff --git a/lib/lightning/workflows.ex b/lib/lightning/workflows.ex index cbbfcc93d3..1999c4e036 100644 --- a/lib/lightning/workflows.ex +++ b/lib/lightning/workflows.ex @@ -462,12 +462,16 @@ defmodule Lightning.Workflows do where: t.workflow_id == ^workflow.id ) + new_name = resolve_name_for_pending_deletion(workflow) + Multi.new() |> Multi.update( :workflow, - Workflow.request_deletion_changeset(workflow, %{ + workflow + |> Workflow.request_deletion_changeset(%{ "deleted_at" => DateTime.utc_now() }) + |> Ecto.Changeset.put_change(:name, new_name) ) |> Multi.insert(:audit, Audit.marked_for_deletion(workflow.id, actor)) |> Multi.update_all( @@ -486,6 +490,33 @@ defmodule Lightning.Workflows do end) end + defp resolve_name_for_pending_deletion(%Workflow{ + name: name, + project_id: project_id + }) do + base_name = "#{name}_del" + + existing_names = + from(w in Workflow, + where: + w.project_id == ^project_id and + (w.name == ^base_name or like(w.name, ^"#{base_name}%")), + select: w.name + ) + |> Repo.all() + |> MapSet.new() + + find_available_name(base_name, existing_names) + end + + defp find_available_name(base_name, existing_names, n \\ 0) do + candidate = if n == 0, do: base_name, else: "#{base_name}#{n}" + + if MapSet.member?(existing_names, candidate), + do: find_available_name(base_name, existing_names, n + 1), + else: candidate + end + defp notify_of_affected_kafka_triggers(%{triggers: triggers}) do triggers |> Enum.filter(&(&1.type == :kafka)) diff --git a/lib/lightning/workflows/workflow.ex b/lib/lightning/workflows/workflow.ex index 9493f005cb..1bc3cc02c4 100644 --- a/lib/lightning/workflows/workflow.ex +++ b/lib/lightning/workflows/workflow.ex @@ -84,7 +84,8 @@ defmodule Lightning.Workflows.Workflow do |> validate_number(:concurrency, greater_than_or_equal_to: 1) |> validate_required([:name]) |> unique_constraint([:name, :project_id], - message: "a workflow with this name already exists in this project." + message: + "A workflow with this name already exists (possibly pending deletion) in this project." ) end diff --git a/test/lightning/workflows/workflow_test.exs b/test/lightning/workflows/workflow_test.exs index 652bfd5026..efccdec483 100644 --- a/test/lightning/workflows/workflow_test.exs +++ b/test/lightning/workflows/workflow_test.exs @@ -79,7 +79,7 @@ defmodule Lightning.Workflows.WorkflowTest do |> Workflow.changeset(%{name: "w1", project_id: p1.id}) |> Repo.insert() - assert "a workflow with this name already exists in this project." in errors_on( + assert "A workflow with this name already exists (possibly pending deletion) in this project." in errors_on( cs ).name diff --git a/test/lightning/workflows_test.exs b/test/lightning/workflows_test.exs index edd6d80755..aae13233fc 100644 --- a/test/lightning/workflows_test.exs +++ b/test/lightning/workflows_test.exs @@ -52,7 +52,7 @@ defmodule Lightning.WorkflowsTest do assert %{ name: [ - "a workflow with this name already exists in this project." + "A workflow with this name already exists (possibly pending deletion) in this project." ] } = errors_on(changeset) @@ -833,6 +833,88 @@ defmodule Lightning.WorkflowsTest do assert_received %KafkaTriggerUpdated{trigger_id: ^kafka_trigger_2_id} assert_received %KafkaTriggerUpdated{trigger_id: ^kafka_trigger_1_id} end + + test "mark_for_deletion/3 renames workflow with _del suffix" do + # Use a separate project to avoid pollution from setup + project = insert(:project) + user = insert(:user) + w1 = insert(:workflow, project: project, name: "Test Workflow") + + assert {:ok, %{workflow: workflow}} = Workflows.mark_for_deletion(w1, user) + assert workflow.name == "Test Workflow_del" + + # Test incrementing when deleting another workflow with the same name + w2 = insert(:workflow, project: project, name: "Test Workflow") + + assert {:ok, %{workflow: workflow2}} = + Workflows.mark_for_deletion(w2, user) + + assert workflow2.name == "Test Workflow_del1" + + # Test incrementing again + w3 = insert(:workflow, project: project, name: "Test Workflow") + + assert {:ok, %{workflow: workflow3}} = + Workflows.mark_for_deletion(w3, user) + + assert workflow3.name == "Test Workflow_del2" + end + + test "mark_for_deletion/3 does not conflict with active workflows ending in _del" do + # An active workflow named "water_gap_analysis_del" (where _del is Delaware) + # should not interfere with deleting "water_gap_analysis" + project = insert(:project) + user = insert(:user) + + # Active workflow with name ending in _del (not deleted) + _delaware_workflow = + insert(:workflow, project: project, name: "water_gap_analysis_del") + + # Workflow to be deleted + w1 = insert(:workflow, project: project, name: "water_gap_analysis") + + # Should still get _del suffix since the existing _del workflow is not deleted + assert {:ok, %{workflow: deleted}} = Workflows.mark_for_deletion(w1, user) + assert deleted.name == "water_gap_analysis_del1" + + assert {:ok, %{workflow: deleted}} = Workflows.mark_for_deletion(w1, user) + assert deleted.name == "water_gap_analysis_del2" + end + + test "allows reusing workflow name after marking for deletion, then validates error when using deleted workflow name" do + project = insert(:project) + user = insert(:user) + w1 = insert(:workflow, project: project, name: "My Workflow") + + # Mark workflow for deletion + assert {:ok, %{workflow: deleted_workflow}} = + Workflows.mark_for_deletion(w1, user) + + assert deleted_workflow.name == "My Workflow_del" + + # Should be able to create a new workflow with the original name + assert {:ok, new_workflow} = + Workflows.save_workflow( + %{name: "My Workflow", project_id: project.id}, + user + ) + + assert new_workflow.name == "My Workflow" + assert new_workflow.deleted_at == nil + + # Should NOT be able to create another workflow with the deleted workflow's name + assert {:error, changeset} = + Workflows.save_workflow( + %{name: "My Workflow_del", project_id: project.id}, + user + ) + + assert errors_on(changeset) == %{ + name: [ + "A workflow with this name already exists (possibly pending deletion) in this project." + ] + } + end end describe "get_workflows_for/2" do diff --git a/test/lightning_web/live/workflow_live/edit_test.exs b/test/lightning_web/live/workflow_live/edit_test.exs index efbaafaeea..b0ff7b0717 100644 --- a/test/lightning_web/live/workflow_live/edit_test.exs +++ b/test/lightning_web/live/workflow_live/edit_test.exs @@ -1229,7 +1229,9 @@ defmodule LightningWeb.WorkflowLive.EditTest do }) |> render_submit() - assert html =~ "a workflow with this name already exists in this project." + assert html =~ + "A workflow with this name already exists (possibly pending deletion) in this project." + assert html =~ "Workflow could not be saved" assert view