From 92701058fe9bc9300453c491ff6891578d40163e Mon Sep 17 00:00:00 2001 From: Frank Midigo Date: Wed, 13 Nov 2024 13:52:26 +0300 Subject: [PATCH 1/8] disables workflow edit form when the workflow is deleted --- lib/lightning/workflows.ex | 15 +++++ lib/lightning_web/live/workflow_live/edit.ex | 63 ++++++++++++++++---- 2 files changed, 65 insertions(+), 13 deletions(-) diff --git a/lib/lightning/workflows.ex b/lib/lightning/workflows.ex index b319a89c7d..adbe15629b 100644 --- a/lib/lightning/workflows.ex +++ b/lib/lightning/workflows.ex @@ -57,6 +57,18 @@ defmodule Lightning.Workflows do {:ok, Workflow.t()} | {:error, Ecto.Changeset.t(Workflow.t())} def save_workflow(%Ecto.Changeset{data: %Workflow{}} = changeset) do Multi.new() + |> Multi.run(:validate, fn _repo, _changes -> + if is_nil(changeset.data.deleted_at) do + {:ok, true} + else + {:error, + Ecto.Changeset.add_error( + changeset, + :deleted_at, + "cannot update a deleted workflow" + )} + end + end) |> Multi.insert_or_update(:workflow, changeset) |> then(fn multi -> if changeset.changes == %{} do @@ -77,6 +89,9 @@ defmodule Lightning.Workflows do {:error, :workflow, changeset, _changes} -> {:error, changeset} + {:error, :validate, changeset, _changes} -> + {:error, changeset} + {:error, :snapshot, snapshot_changeset, %{workflow: workflow}} -> Logger.warning(fn -> """ diff --git a/lib/lightning_web/live/workflow_live/edit.ex b/lib/lightning_web/live/workflow_live/edit.ex index 800197f075..0ea3d33c1f 100644 --- a/lib/lightning_web/live/workflow_live/edit.ex +++ b/lib/lightning_web/live/workflow_live/edit.ex @@ -27,7 +27,6 @@ defmodule LightningWeb.WorkflowLive.Edit do alias Lightning.Workflows.Trigger alias Lightning.Workflows.Workflow alias Lightning.WorkOrders - alias LightningWeb.Components.Form alias LightningWeb.WorkflowLive.Helpers alias LightningWeb.WorkflowNewLive.WorkflowParams alias Phoenix.LiveView.JS @@ -146,6 +145,7 @@ defmodule LightningWeb.WorkflowLive.Edit do <.offline_indicator /> <.save_workflow_button + id="top-bar-save-workflow-btn" changeset={@changeset} can_edit_workflow={@can_edit_workflow} snapshot_version_tag={@snapshot_version_tag} @@ -323,6 +323,7 @@ defmodule LightningWeb.WorkflowLive.Edit do /> <.with_changes_indicator changeset={@changeset}> <.save_workflow_button + id="inspector-save-workflow-btn" changeset={@changeset} can_edit_workflow={@can_edit_workflow} snapshot_version_tag={@snapshot_version_tag} @@ -447,7 +448,8 @@ defmodule LightningWeb.WorkflowLive.Edit do <.job_form on_change={&send_form_changed/1} editable={ - @can_edit_workflow && @snapshot_version_tag == "latest" && + is_nil(@workflow.deleted_at) && @can_edit_workflow && + @snapshot_version_tag == "latest" && @has_presence_edit_priority } form={jf} @@ -472,12 +474,14 @@ defmodule LightningWeb.WorkflowLive.Edit do phx-value-id={@selected_job.id} class="focus:ring-red-500 bg-red-600 hover:bg-red-700 disabled:bg-red-300" disabled={ - !@can_edit_workflow or @has_child_edges or @is_first_job or + !is_nil(@workflow.deleted_at) or !@can_edit_workflow or + @has_child_edges or @is_first_job or @snapshot_version_tag != "latest" || !@has_presence_edit_priority } tooltip={ - deletion_tooltip_message( + job_deletion_tooltip_message( + is_struct(@workflow.deleted_at), @can_edit_workflow, @has_child_edges, @is_first_job @@ -524,7 +528,7 @@ defmodule LightningWeb.WorkflowLive.Edit do form={tf} on_change={&send_form_changed/1} disabled={ - !@can_edit_workflow or + !is_nil(@workflow.deleted_at) or !@can_edit_workflow or @snapshot_version_tag != "latest" || !@has_presence_edit_priority } @@ -556,7 +560,7 @@ defmodule LightningWeb.WorkflowLive.Edit do <.edge_form form={ef} disabled={ - !@can_edit_workflow or + !is_nil(@workflow.deleted_at) or !@can_edit_workflow or @snapshot_version_tag != "latest" || !@has_presence_edit_priority } @@ -772,12 +776,16 @@ defmodule LightningWeb.WorkflowLive.Edit do defp processing(_run), do: false - defp deletion_tooltip_message( + defp job_deletion_tooltip_message( + workflow_deleted, can_edit_job, has_child_edges, is_first_job ) do cond do + workflow_deleted -> + "You cannot modify a deleted workflow" + !can_edit_job -> "You are not authorized to delete this step." @@ -1759,10 +1767,13 @@ defmodule LightningWeb.WorkflowLive.Edit do %{ has_presence_edit_priority: has_edit_priority, snapshot_version_tag: version, - can_edit_workflow: can_edit_workflow + can_edit_workflow: can_edit_workflow, + workflow: workflow } = socket.assigns - disabled = !(has_edit_priority && version == "latest" && can_edit_workflow) + disabled = + !(is_nil(workflow.deleted_at) && has_edit_priority && version == "latest" && + can_edit_workflow) push_event(socket, "set-disabled", %{disabled: disabled}) end @@ -1905,6 +1916,9 @@ defmodule LightningWeb.WorkflowLive.Edit do %{manual_run_form: nil} -> true + %{workflow: workflow} -> + !is_nil(workflow.deleted_at) + %{ manual_run_form: manual_run_form, changeset: changeset, @@ -2297,6 +2311,7 @@ defmodule LightningWeb.WorkflowLive.Edit do """ end + attr :id, :string, required: true attr :can_edit_workflow, :boolean, required: true attr :changeset, Ecto.Changeset, required: true attr :snapshot_version_tag, :string, required: true @@ -2314,22 +2329,44 @@ defmodule LightningWeb.WorkflowLive.Edit do assigns |> assign( disabled: - !can_edit_workflow or !changeset.valid? or - snapshot_version_tag != "latest" || !has_presence_priority + !is_nil(changeset.data.deleted_at) or !can_edit_workflow or + !changeset.valid? or + snapshot_version_tag != "latest" or !has_presence_priority ) + |> assign_new(:tooltip, fn -> + cond do + !is_nil(changeset.data.deleted_at) -> + "Workflow has been deleted" + + !can_edit_workflow -> + "You do not have permission to edit this workflow" + + !changeset.valid? -> + "You have some unresolved errors in your workflow" + + snapshot_version_tag != "latest" -> + "You cannot edit an old snapshot of a workflow" + + true -> + nil + end + end) ~H""" - Save - + """ end From 02ac7354a2c3f189e8abe7d9f3fbecacd5f1f5eb Mon Sep 17 00:00:00 2001 From: Frank Midigo Date: Thu, 14 Nov 2024 08:28:56 +0300 Subject: [PATCH 2/8] do not allow retrying of workorders and runs with deleted workflows --- lib/lightning/work_orders.ex | 9 +++++++++ lib/lightning/workflows.ex | 17 +++++++---------- lib/lightning_web/live/workflow_live/edit.ex | 16 ++++++++++++++++ lib/lightning_web/live/workflow_live/helpers.ex | 3 ++- 4 files changed, 34 insertions(+), 11 deletions(-) diff --git a/lib/lightning/work_orders.ex b/lib/lightning/work_orders.ex index 0fc9feab1d..b33c253846 100644 --- a/lib/lightning/work_orders.ex +++ b/lib/lightning/work_orders.ex @@ -310,6 +310,13 @@ defmodule Lightning.WorkOrders do preload: [:job] ) end) + |> Multi.run(:workflow_deleted?, fn _repo, %{run: run} -> + if run.work_order.workflow.deleted_at do + {:error, :workflow_deleted} + else + {:ok, false} + end + end) |> Multi.run(:input_dataclip_id, fn _repo, %{step: %Step{input_dataclip_id: input_dataclip_id}} -> {:ok, input_dataclip_id} @@ -672,6 +679,8 @@ defmodule Lightning.WorkOrders do defp fetch_retriable_workorders(workorder_ids) do workorder_ids |> workorders_with_dataclips_query() + |> join([wo], :inner, wf in assoc(wo, :workflow), as: :workflow) + |> where([workflow: wf], is_nil(wf.deleted_at)) |> Repo.all() end diff --git a/lib/lightning/workflows.ex b/lib/lightning/workflows.ex index adbe15629b..98f9d68e5c 100644 --- a/lib/lightning/workflows.ex +++ b/lib/lightning/workflows.ex @@ -54,19 +54,16 @@ defmodule Lightning.Workflows do def get_workflow(id), do: Repo.get(Workflow, id) @spec save_workflow(Ecto.Changeset.t(Workflow.t()) | map()) :: - {:ok, Workflow.t()} | {:error, Ecto.Changeset.t(Workflow.t())} + {:ok, Workflow.t()} + | {:error, Ecto.Changeset.t(Workflow.t())} + | {:error, :deleted} def save_workflow(%Ecto.Changeset{data: %Workflow{}} = changeset) do Multi.new() |> Multi.run(:validate, fn _repo, _changes -> if is_nil(changeset.data.deleted_at) do {:ok, true} else - {:error, - Ecto.Changeset.add_error( - changeset, - :deleted_at, - "cannot update a deleted workflow" - )} + {:error, :deleted} end end) |> Multi.insert_or_update(:workflow, changeset) @@ -89,9 +86,6 @@ defmodule Lightning.Workflows do {:error, :workflow, changeset, _changes} -> {:error, changeset} - {:error, :validate, changeset, _changes} -> - {:error, changeset} - {:error, :snapshot, snapshot_changeset, %{workflow: workflow}} -> Logger.warning(fn -> """ @@ -101,6 +95,9 @@ defmodule Lightning.Workflows do end) {:error, false} + + {:error, _action, reason, _changes} -> + {:error, reason} end end diff --git a/lib/lightning_web/live/workflow_live/edit.ex b/lib/lightning_web/live/workflow_live/edit.ex index 0ea3d33c1f..b55b9e10ba 100644 --- a/lib/lightning_web/live/workflow_live/edit.ex +++ b/lib/lightning_web/live/workflow_live/edit.ex @@ -1464,6 +1464,14 @@ defmodule LightningWeb.WorkflowLive.Edit do {:error, %{text: message}} -> {:noreply, put_flash(socket, :error, message)} + {:error, :deleted} -> + {:noreply, + put_flash( + socket, + :error, + "Oops! You cannot modify a deleted workflow" + )} + {:error, %Ecto.Changeset{} = changeset} -> {:noreply, socket @@ -1666,6 +1674,14 @@ defmodule LightningWeb.WorkflowLive.Edit do {:error, %{text: message}} -> {:noreply, put_flash(socket, :error, message)} + + {:error, :deleted} -> + {:noreply, + put_flash( + socket, + :error, + "Oops! You cannot modify a deleted workflow" + )} end end diff --git a/lib/lightning_web/live/workflow_live/helpers.ex b/lib/lightning_web/live/workflow_live/helpers.ex index d366a88742..78f2d4c92d 100644 --- a/lib/lightning_web/live/workflow_live/helpers.ex +++ b/lib/lightning_web/live/workflow_live/helpers.ex @@ -25,7 +25,7 @@ defmodule LightningWeb.WorkflowLive.Helpers do @spec save_workflow(Ecto.Changeset.t()) :: {:ok, Workflows.Workflow.t()} - | {:error, Ecto.Changeset.t() | UsageLimiting.message()} + | {:error, Ecto.Changeset.t() | UsageLimiting.message() | :deleted} def save_workflow(changeset) do case WorkflowUsageLimiter.limit_workflow_activation(changeset) do :ok -> @@ -52,6 +52,7 @@ defmodule LightningWeb.WorkflowLive.Helpers do | {:error, Ecto.Changeset.t(Workflows.Workflow.t())} | {:error, Ecto.Changeset.t(WorkOrders.Manual.t())} | {:error, UsageLimiting.message()} + | {:error, :deleted} def run_workflow(workflow_or_changeset, params, opts) do Lightning.Repo.transact(fn -> %{id: project_id} = Keyword.fetch!(opts, :project) From 819435792027ab9cd292737cf55c0792ab7d3361 Mon Sep 17 00:00:00 2001 From: Frank Midigo Date: Thu, 14 Nov 2024 08:46:40 +0300 Subject: [PATCH 3/8] oops --- lib/lightning/work_orders.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/lightning/work_orders.ex b/lib/lightning/work_orders.ex index b33c253846..089fff5430 100644 --- a/lib/lightning/work_orders.ex +++ b/lib/lightning/work_orders.ex @@ -679,7 +679,7 @@ defmodule Lightning.WorkOrders do defp fetch_retriable_workorders(workorder_ids) do workorder_ids |> workorders_with_dataclips_query() - |> join([wo], :inner, wf in assoc(wo, :workflow), as: :workflow) + |> join(:inner, [wo], wf in assoc(wo, :workflow), as: :workflow) |> where([workflow: wf], is_nil(wf.deleted_at)) |> Repo.all() end From 46987616066c801b41c3ca3049ae874d42e98994 Mon Sep 17 00:00:00 2001 From: Frank Midigo Date: Thu, 14 Nov 2024 12:05:50 +0300 Subject: [PATCH 4/8] add unit tests --- lib/lightning/work_orders.ex | 7 +- lib/lightning_web/live/run_live/index.ex | 8 +- lib/lightning_web/live/workflow_live/edit.ex | 170 ++++++++++--------- test/lightning/work_orders_test.exs | 133 ++++++++++++++- test/lightning/workflows_test.exs | 9 + 5 files changed, 243 insertions(+), 84 deletions(-) diff --git a/lib/lightning/work_orders.ex b/lib/lightning/work_orders.ex index 089fff5430..7f2cd8849d 100644 --- a/lib/lightning/work_orders.ex +++ b/lib/lightning/work_orders.ex @@ -283,7 +283,7 @@ defmodule Lightning.WorkOrders do Step.t() | Ecto.UUID.t(), [work_order_option(), ...] ) :: - {:ok, Run.t()} | {:error, Ecto.Changeset.t()} + {:ok, Run.t()} | {:error, Ecto.Changeset.t() | :workflow_deleted} def retry(%Run{id: run_id}, %Step{id: step_id}, opts) do retry(run_id, step_id, opts) end @@ -508,7 +508,10 @@ defmodule Lightning.WorkOrders do retry(run_step.run_id, run_step.step_id, opts) end) - {:ok, Enum.count(results, fn result -> match?({:ok, _}, result) end), 0} + success_count = + Enum.count(results, fn result -> match?({:ok, _}, result) end) + + {:ok, success_count, Enum.count(results) - success_count} end end diff --git a/lib/lightning_web/live/run_live/index.ex b/lib/lightning_web/live/run_live/index.ex index 91c56cb7ef..1db0ace86d 100644 --- a/lib/lightning_web/live/run_live/index.ex +++ b/lib/lightning_web/live/run_live/index.ex @@ -340,6 +340,11 @@ defmodule LightningWeb.RunLive.Index do socket |> put_flash(:error, error_text)} + {:error, :workflow_deleted} -> + {:noreply, + socket + |> put_flash(:error, "Runs for deleted workflows cannot be retried")} + {:error, _changeset} -> {:noreply, socket @@ -362,7 +367,8 @@ defmodule LightningWeb.RunLive.Index do "New run#{if count > 1, do: "s", else: ""} enqueued for #{count} workorder#{if count > 1, do: "s", else: ""}" |> then(fn msg -> if discarded_count > 0 do - msg <> " (#{discarded_count} were discarded due to wiped dataclip)" + msg <> + " (#{discarded_count} were discarded due to wiped dataclip/workflow being deleted)" else msg end diff --git a/lib/lightning_web/live/workflow_live/edit.ex b/lib/lightning_web/live/workflow_live/edit.ex index b55b9e10ba..4dc4ebfea5 100644 --- a/lib/lightning_web/live/workflow_live/edit.ex +++ b/lib/lightning_web/live/workflow_live/edit.ex @@ -1411,20 +1411,9 @@ defmodule LightningWeb.WorkflowLive.Edit do end def handle_event("save", params, socket) do - %{ - project: project, - workflow_params: initial_params, - can_edit_workflow: can_edit_workflow, - snapshot_version_tag: tag, - has_presence_edit_priority: has_presence_edit_priority - } = - socket.assigns + %{project: project, workflow_params: initial_params} = socket.assigns - with true <- can_edit_workflow || :not_authorized, - true <- tag == "latest" || :view_only, - true <- - has_presence_edit_priority || - :presence_low_priority do + with :ok <- check_user_can_save_workflow(socket) do next_params = case params do %{"workflow" => params} -> @@ -1480,27 +1469,6 @@ defmodule LightningWeb.WorkflowLive.Edit do |> put_flash(:error, "Workflow could not be saved") |> push_patches_applied(initial_params)} end - else - :view_only -> - {:noreply, - socket - |> put_flash( - :error, - "Cannot save in snapshot mode, switch to the latest version." - )} - - :presence_low_priority -> - {:noreply, - socket - |> put_flash( - :error, - "Cannot save in view-only mode" - )} - - :not_authorized -> - {:noreply, - socket - |> put_flash(:error, "You are not authorized to perform this action.")} end end @@ -1601,9 +1569,6 @@ defmodule LightningWeb.WorkflowLive.Edit do selected_job: selected_job, current_user: current_user, workflow_params: workflow_params, - can_edit_workflow: can_edit_workflow, - can_run_workflow: can_run_workflow, - snapshot_version_tag: tag, has_presence_edit_priority: has_presence_edit_priority, workflow: workflow, manual_run_form: form @@ -1626,62 +1591,52 @@ defmodule LightningWeb.WorkflowLive.Edit do get_workflow_by_id(workflow.id) end - with true <- (can_run_workflow && can_edit_workflow) || :not_authorized, - true <- tag == "latest" || :view_only, - {:ok, %{workorder: workorder, workflow: workflow}} <- - Helpers.run_workflow( + with :ok <- check_user_can_manual_run_workflow(socket) do + case Helpers.run_workflow( workflow_or_changeset, params, project: project, selected_job: selected_job, created_by: current_user ) do - %{runs: [run]} = workorder + {:ok, %{workorder: workorder, workflow: workflow}} -> + %{runs: [run]} = workorder - Runs.subscribe(run) + Runs.subscribe(run) - snapshot = snapshot_by_version(workflow.id, workflow.lock_version) - - {:noreply, - socket - |> assign_workflow(workflow, snapshot) - |> follow_run(run) - |> push_event("push-hash", %{"hash" => "log"})} - else - {:error, %Ecto.Changeset{data: %WorkOrders.Manual{}} = changeset} -> - {:noreply, - socket - |> assign_manual_run_form(changeset)} + snapshot = snapshot_by_version(workflow.id, workflow.lock_version) - {:error, %Ecto.Changeset{data: %Workflow{}} = changeset} -> - { - :noreply, - socket - |> assign_changeset(changeset) - |> mark_validated() - |> put_flash(:error, "Workflow could not be saved") - } + {:noreply, + socket + |> assign_workflow(workflow, snapshot) + |> follow_run(run) + |> push_event("push-hash", %{"hash" => "log"})} - :not_authorized -> - {:noreply, - socket - |> put_flash(:error, "You are not authorized to perform this action.")} + {:error, %Ecto.Changeset{data: %WorkOrders.Manual{}} = changeset} -> + {:noreply, + socket + |> assign_manual_run_form(changeset)} - :view_only -> - {:noreply, - socket - |> put_flash(:error, "Cannot run in snapshot mode, switch to latest.")} + {:error, %Ecto.Changeset{data: %Workflow{}} = changeset} -> + { + :noreply, + socket + |> assign_changeset(changeset) + |> mark_validated() + |> put_flash(:error, "Workflow could not be saved") + } - {:error, %{text: message}} -> - {:noreply, put_flash(socket, :error, message)} + {:error, %{text: message}} -> + {:noreply, put_flash(socket, :error, message)} - {:error, :deleted} -> - {:noreply, - put_flash( - socket, - :error, - "Oops! You cannot modify a deleted workflow" - )} + {:error, :deleted} -> + {:noreply, + put_flash( + socket, + :error, + "Oops! You cannot modify a deleted workflow" + )} + end end end @@ -1779,6 +1734,61 @@ defmodule LightningWeb.WorkflowLive.Edit do {:noreply, socket} end + defp check_user_can_manual_run_workflow(socket) do + case socket.assigns do + %{ + can_edit_workflow: true, + can_run_workflow: true, + snapshot_version_tag: "latest" + } -> + :ok + + %{can_edit_workflow: false} -> + {:noreply, + socket + |> put_flash(:error, "You are not authorized to perform this action.")} + + %{can_run_workflow: false} -> + {:noreply, + socket + |> put_flash(:error, "You are not authorized to perform this action.")} + + _snapshot_not_latest -> + {:noreply, + socket + |> put_flash(:error, "Cannot run in snapshot mode, switch to latest.")} + end + end + + defp check_user_can_save_workflow(socket) do + case socket.assigns do + %{ + can_edit_workflow: true, + has_presence_edit_priority: true, + snapshot_version_tag: "latest" + } -> + :ok + + %{can_edit_workflow: false} -> + {:noreply, + socket + |> put_flash(:error, "You are not authorized to perform this action.")} + + %{has_presence_edit_priority: false} -> + {:noreply, + socket + |> put_flash( + :error, + "Cannot save in view-only mode" + )} + + _snapshot_not_latest -> + {:noreply, + socket + |> put_flash(:error, "Cannot save in snapshot mode, switch to latest.")} + end + end + defp maybe_disable_canvas(socket) do %{ has_presence_edit_priority: has_edit_priority, diff --git a/test/lightning/work_orders_test.exs b/test/lightning/work_orders_test.exs index 5b9947b473..bb10120cee 100644 --- a/test/lightning/work_orders_test.exs +++ b/test/lightning/work_orders_test.exs @@ -573,6 +573,45 @@ defmodule Lightning.WorkOrdersTest do work_order: %{id: ^workorder_id} } end + + test "returns error in case the workflow is deleted", %{ + workflow: workflow, + snapshot: snapshot, + trigger: trigger, + jobs: [job | _rest] + } do + user = insert(:user) + dataclip = insert(:dataclip) + # create existing complete run + %{runs: [run]} = + insert(:workorder, + workflow: workflow, + snapshot: snapshot, + trigger: trigger, + dataclip: dataclip, + runs: [ + %{ + state: :failed, + dataclip: dataclip, + snapshot: snapshot, + starting_trigger: trigger, + steps: [ + step = insert(:step, job: job, input_dataclip: dataclip) + ] + } + ] + ) + + # delete workflow + workflow + |> Ecto.Changeset.change(%{ + deleted_at: DateTime.utc_now() |> DateTime.truncate(:second) + }) + |> Repo.update!() + + assert {:error, :workflow_deleted} = + WorkOrders.retry(run, step, created_by: user) + end end describe "retry_many/3" do @@ -2036,6 +2075,98 @@ defmodule Lightning.WorkOrdersTest do ) end) end + + test "retrying multiple workorders only retries workorders with workflows that havent been deleted", + %{ + jobs: [job_a | _rest], + snapshot: snapshot, + trigger: trigger, + user: user, + workflow: workflow + } do + workorder_1 = + insert(:workorder, + workflow: workflow, + trigger: trigger, + dataclip: build(:dataclip), + snapshot: snapshot, + runs: [ + %{ + state: :failed, + dataclip: build(:dataclip), + starting_trigger: trigger, + snapshot: snapshot, + steps: [ + insert(:step, + job: job_a, + input_dataclip: build(:dataclip), + output_dataclip: build(:dataclip) + ) + ] + } + ] + ) + + deleted_workflow = + insert(:simple_workflow, + project: workflow.project, + deleted_at: DateTime.utc_now() + ) + + deleted_workflow_job = hd(deleted_workflow.jobs) + + snapshot_2 = + Lightning.Workflows.Snapshot.build(deleted_workflow) |> Repo.insert!() + + workorder_2 = + insert(:workorder, + workflow: deleted_workflow, + trigger: hd(deleted_workflow.triggers), + dataclip: build(:dataclip), + snapshot: snapshot_2, + runs: [ + %{ + state: :failed, + dataclip: build(:dataclip), + starting_trigger: hd(deleted_workflow.triggers), + snapshot: snapshot_2, + steps: [ + insert(:step, + job: deleted_workflow_job, + input_dataclip: build(:dataclip), + output_dataclip: build(:dataclip) + ) + ] + } + ] + ) + + refute Repo.get_by(Lightning.Run, + work_order_id: workorder_1.id, + starting_job_id: job_a.id + ) + + refute Repo.get_by(Lightning.Run, + work_order_id: workorder_2.id, + starting_job_id: deleted_workflow_job.id + ) + + {:ok, 1, 1} = + WorkOrders.retry_many([workorder_2, workorder_1], + created_by: user, + project_id: workflow.project_id + ) + + assert Repo.get_by(Lightning.Run, + work_order_id: workorder_1.id, + starting_job_id: job_a.id + ) + + refute Repo.get_by(Lightning.Run, + work_order_id: workorder_2.id, + starting_job_id: deleted_workflow_job.id + ) + end end describe "retry_many/2 for RunSteps" do @@ -2406,7 +2537,7 @@ defmodule Lightning.WorkOrdersTest do starting_job_id: job_a.id ) - {:ok, 1, 0} = + {:ok, 1, 1} = WorkOrders.retry_many([run_step_2_a, run_step_1_a], created_by: user, project_id: workflow.project_id diff --git a/test/lightning/workflows_test.exs b/test/lightning/workflows_test.exs index c04a03733a..23fcceacf9 100644 --- a/test/lightning/workflows_test.exs +++ b/test/lightning/workflows_test.exs @@ -66,6 +66,15 @@ defmodule Lightning.WorkflowsTest do assert workflow.name == "some-updated-name" end + test "save_workflow/1 for a deleted workflow returns an error" do + workflow = insert(:workflow, deleted_at: DateTime.utc_now()) + update_attrs = %{name: "some-updated-name"} + + assert {:error, :deleted} = + Workflows.change_workflow(workflow, update_attrs) + |> Workflows.save_workflow() + end + test "save_workflow/1 publishes event for updated Kafka triggers" do kafka_configuration = build(:triggers_kafka_configuration) From a09e51a86c7d4a775cbd52b6ab3a3b6133f77d18 Mon Sep 17 00:00:00 2001 From: Frank Midigo Date: Thu, 14 Nov 2024 13:36:51 +0300 Subject: [PATCH 5/8] add UI tests --- lib/lightning/workflows.ex | 4 +- lib/lightning_web/live/workflow_live/edit.ex | 64 ++++++++++--------- .../live/workflow_live/helpers.ex | 2 +- test/lightning/workflows_test.exs | 2 +- .../live/workflow_live/edit_test.exs | 26 ++++++++ .../live/workflow_live/editor_test.exs | 40 ++++++++++++ 6 files changed, 104 insertions(+), 34 deletions(-) diff --git a/lib/lightning/workflows.ex b/lib/lightning/workflows.ex index 98f9d68e5c..761bacaa49 100644 --- a/lib/lightning/workflows.ex +++ b/lib/lightning/workflows.ex @@ -56,14 +56,14 @@ defmodule Lightning.Workflows do @spec save_workflow(Ecto.Changeset.t(Workflow.t()) | map()) :: {:ok, Workflow.t()} | {:error, Ecto.Changeset.t(Workflow.t())} - | {:error, :deleted} + | {:error, :workflow_deleted} def save_workflow(%Ecto.Changeset{data: %Workflow{}} = changeset) do Multi.new() |> Multi.run(:validate, fn _repo, _changes -> if is_nil(changeset.data.deleted_at) do {:ok, true} else - {:error, :deleted} + {:error, :workflow_deleted} end end) |> Multi.insert_or_update(:workflow, changeset) diff --git a/lib/lightning_web/live/workflow_live/edit.ex b/lib/lightning_web/live/workflow_live/edit.ex index 4dc4ebfea5..856f0375cb 100644 --- a/lib/lightning_web/live/workflow_live/edit.ex +++ b/lib/lightning_web/live/workflow_live/edit.ex @@ -1453,7 +1453,7 @@ defmodule LightningWeb.WorkflowLive.Edit do {:error, %{text: message}} -> {:noreply, put_flash(socket, :error, message)} - {:error, :deleted} -> + {:error, :workflow_deleted} -> {:noreply, put_flash( socket, @@ -1542,6 +1542,10 @@ defmodule LightningWeb.WorkflowLive.Edit do {:error, %{text: message}} -> {:noreply, put_flash(socket, :error, message)} + {:error, :workflow_deleted} -> + {:noreply, + put_flash(socket, :error, "Cannot rerun a deleted a workflow")} + {:error, _changeset} -> {:noreply, socket @@ -1629,7 +1633,7 @@ defmodule LightningWeb.WorkflowLive.Edit do {:error, %{text: message}} -> {:noreply, put_flash(socket, :error, message)} - {:error, :deleted} -> + {:error, :workflow_deleted} -> {:noreply, put_flash( socket, @@ -2344,39 +2348,39 @@ defmodule LightningWeb.WorkflowLive.Edit do attr :has_presence_priority, :boolean, required: true defp save_workflow_button(assigns) do - %{ - can_edit_workflow: can_edit_workflow, - changeset: changeset, - snapshot_version_tag: snapshot_version_tag, - has_presence_priority: has_presence_priority - } = assigns + {disabled, tooltip} = + case assigns do + %{ + changeset: %{valid?: true, data: %{deleted_at: nil}}, + can_edit_workflow: true, + snapshot_version_tag: "latest", + has_presence_priority: true + } -> + {false, nil} + + %{changeset: %{data: %{deleted_at: deleted_at}}} + when is_struct(deleted_at) -> + {true, "Workflow has been deleted"} + + %{can_edit_workflow: false} -> + {true, "You do not have permission to edit this workflow"} + + %{changeset: %{valid?: false}} -> + {true, "You have some unresolved errors in your workflow"} + + %{snapshot_version_tag: tag} when tag != "latest" -> + {true, "You cannot edit an old snapshot of a workflow"} + + _other -> + {true, nil} + end assigns = assigns |> assign( - disabled: - !is_nil(changeset.data.deleted_at) or !can_edit_workflow or - !changeset.valid? or - snapshot_version_tag != "latest" or !has_presence_priority + disabled: disabled, + tooltip: tooltip ) - |> assign_new(:tooltip, fn -> - cond do - !is_nil(changeset.data.deleted_at) -> - "Workflow has been deleted" - - !can_edit_workflow -> - "You do not have permission to edit this workflow" - - !changeset.valid? -> - "You have some unresolved errors in your workflow" - - snapshot_version_tag != "latest" -> - "You cannot edit an old snapshot of a workflow" - - true -> - nil - end - end) ~H""" <.button diff --git a/lib/lightning_web/live/workflow_live/helpers.ex b/lib/lightning_web/live/workflow_live/helpers.ex index 78f2d4c92d..63facee5a4 100644 --- a/lib/lightning_web/live/workflow_live/helpers.ex +++ b/lib/lightning_web/live/workflow_live/helpers.ex @@ -52,7 +52,7 @@ defmodule LightningWeb.WorkflowLive.Helpers do | {:error, Ecto.Changeset.t(Workflows.Workflow.t())} | {:error, Ecto.Changeset.t(WorkOrders.Manual.t())} | {:error, UsageLimiting.message()} - | {:error, :deleted} + | {:error, :workflow_deleted} def run_workflow(workflow_or_changeset, params, opts) do Lightning.Repo.transact(fn -> %{id: project_id} = Keyword.fetch!(opts, :project) diff --git a/test/lightning/workflows_test.exs b/test/lightning/workflows_test.exs index 23fcceacf9..3fa7459b33 100644 --- a/test/lightning/workflows_test.exs +++ b/test/lightning/workflows_test.exs @@ -70,7 +70,7 @@ defmodule Lightning.WorkflowsTest do workflow = insert(:workflow, deleted_at: DateTime.utc_now()) update_attrs = %{name: "some-updated-name"} - assert {:error, :deleted} = + assert {:error, :workflow_deleted} = Workflows.change_workflow(workflow, update_attrs) |> Workflows.save_workflow() end diff --git a/test/lightning_web/live/workflow_live/edit_test.exs b/test/lightning_web/live/workflow_live/edit_test.exs index 52fcf1dfee..69da1f0188 100644 --- a/test/lightning_web/live/workflow_live/edit_test.exs +++ b/test/lightning_web/live/workflow_live/edit_test.exs @@ -887,6 +887,32 @@ defmodule LightningWeb.WorkflowLive.EditTest do assert view |> save_is_disabled?() end + test "Save button is disabled when workflow is deleted", %{ + conn: conn, + project: project, + workflow: workflow + } do + workflow + |> Ecto.Changeset.change(%{ + deleted_at: DateTime.utc_now() |> DateTime.truncate(:second) + }) + |> Lightning.Repo.update!() + + {:ok, view, _html} = + live( + conn, + ~p"/projects/#{project.id}/w/#{workflow.id}?#{[v: workflow.lock_version]}" + ) + + assert view |> page_title() =~ workflow.name + + assert view |> save_is_disabled?() + + # try changing the workflow name anyway + assert render_click(view, "save", %{name: "updatename"}) =~ + "Oops! You cannot modify a deleted workflow" + end + test "opens edge Path form and saves the JS expression", %{ conn: conn, project: project, diff --git a/test/lightning_web/live/workflow_live/editor_test.exs b/test/lightning_web/live/workflow_live/editor_test.exs index 5255b32ed6..52c818a673 100644 --- a/test/lightning_web/live/workflow_live/editor_test.exs +++ b/test/lightning_web/live/workflow_live/editor_test.exs @@ -1283,6 +1283,46 @@ defmodule LightningWeb.WorkflowLive.EditorTest do live_children(view) |> Enum.each(&render_async/1) end + test "can't retry when workflow has been deleted", + %{ + conn: conn, + project: project, + workflow: %{jobs: [_job_1, job_2 | _rest]} = workflow, + snapshot: snapshot + } do + {_dataclips, %{runs: [run]} = _workorder} = + rerun_setup(project, workflow, snapshot) + + workflow + |> Ecto.Changeset.change(%{ + deleted_at: DateTime.utc_now() |> DateTime.truncate(:second) + }) + |> Lightning.Repo.update!() + + {:ok, view, _html} = + live( + conn, + ~p"/projects/#{project}/w/#{workflow}?#{[s: job_2.id, a: run.id, m: "expand", v: workflow.lock_version]}" + ) + + live_children(view) |> Enum.each(&render_async/1) + + # user gets no option to rerun + assert has_element?(view, "button[disabled='disabled']", "Retry from here") + + assert has_element?( + view, + "button[disabled='disabled']", + "Create New Work Order" + ) + + # submit event regardless + step = Enum.find(run.steps, fn step -> step.job_id == job_2.id end) + + assert render_click(view, "rerun", %{run_id: run.id, step_id: step.id}) =~ + "Cannot rerun a deleted a workflow" + end + test "followed run with wiped dataclip renders the page correctly", %{ conn: conn, From 71028702739e554fc75acf6ac13a05fcd8ea9ea2 Mon Sep 17 00:00:00 2001 From: Frank Midigo Date: Thu, 14 Nov 2024 14:03:12 +0300 Subject: [PATCH 6/8] update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b36cd59a79..151fabdb6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,8 @@ and this project adheres to - Error when the logger receives a boolean [#2666](https://github.com/OpenFn/lightning/issues/2666) +- Disable save and run actions on deleted workflows + [#2170](https://github.com/OpenFn/lightning/issues/2170) ## [v2.10.1] - 2024-11-13 From 4ff4862768786da1329a7312c4bfdd98a6802d4e Mon Sep 17 00:00:00 2001 From: Frank Midigo Date: Thu, 14 Nov 2024 14:45:23 +0300 Subject: [PATCH 7/8] check that workflow is not deleted before creating a workorder for manual run --- lib/lightning/work_orders.ex | 10 +++++++++- lib/lightning_web/live/workflow_live/helpers.ex | 3 ++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/lightning/work_orders.ex b/lib/lightning/work_orders.ex index 7f2cd8849d..539df6cb51 100644 --- a/lib/lightning/work_orders.ex +++ b/lib/lightning/work_orders.ex @@ -78,7 +78,8 @@ defmodule Lightning.WorkOrders do create_for(job, workflow: workflow, dataclip: dataclip, user: user) """ @spec create_for(Trigger.t() | Job.t(), Multi.t(), [work_order_option()]) :: - {:ok, WorkOrder.t()} | {:error, Ecto.Changeset.t(WorkOrder.t())} + {:ok, WorkOrder.t()} + | {:error, Ecto.Changeset.t(WorkOrder.t()) | :workflow_deleted} def create_for(target, multi \\ Multi.new(), opts) def create_for(%Trigger{} = trigger, multi, opts) do @@ -118,6 +119,13 @@ defmodule Lightning.WorkOrders do def create_for(%Manual{} = manual) do Multi.new() + |> Multi.run(:workflow_deleted?, fn _repo, _changes -> + if manual.workflow.deleted_at do + {:error, :workflow_deleted} + else + {:ok, false} + end + end) |> get_or_insert_dataclip(manual) |> Multi.put(:workflow, manual.workflow) |> get_or_create_snapshot() diff --git a/lib/lightning_web/live/workflow_live/helpers.ex b/lib/lightning_web/live/workflow_live/helpers.ex index 63facee5a4..2150864d73 100644 --- a/lib/lightning_web/live/workflow_live/helpers.ex +++ b/lib/lightning_web/live/workflow_live/helpers.ex @@ -25,7 +25,8 @@ defmodule LightningWeb.WorkflowLive.Helpers do @spec save_workflow(Ecto.Changeset.t()) :: {:ok, Workflows.Workflow.t()} - | {:error, Ecto.Changeset.t() | UsageLimiting.message() | :deleted} + | {:error, + Ecto.Changeset.t() | UsageLimiting.message() | :workflow_deleted} def save_workflow(changeset) do case WorkflowUsageLimiter.limit_workflow_activation(changeset) do :ok -> From 13f16629ae1ed7e278287de06f61944b9f7cb5d9 Mon Sep 17 00:00:00 2001 From: Frank Midigo Date: Fri, 15 Nov 2024 06:35:29 +0300 Subject: [PATCH 8/8] fix failing tests --- lib/lightning_web/live/workflow_live/edit.ex | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/lightning_web/live/workflow_live/edit.ex b/lib/lightning_web/live/workflow_live/edit.ex index 856f0375cb..65a1141423 100644 --- a/lib/lightning_web/live/workflow_live/edit.ex +++ b/lib/lightning_web/live/workflow_live/edit.ex @@ -1789,7 +1789,10 @@ defmodule LightningWeb.WorkflowLive.Edit do _snapshot_not_latest -> {:noreply, socket - |> put_flash(:error, "Cannot save in snapshot mode, switch to latest.")} + |> put_flash( + :error, + "Cannot save in snapshot mode, switch to the latest version." + )} end end @@ -1946,8 +1949,8 @@ defmodule LightningWeb.WorkflowLive.Edit do %{manual_run_form: nil} -> true - %{workflow: workflow} -> - !is_nil(workflow.deleted_at) + %{workflow: %{deleted_at: deleted_at}} when is_struct(deleted_at) -> + true %{ manual_run_form: manual_run_form,