From 5ea765783bfcde70c9d7a256077b4510f4573456 Mon Sep 17 00:00:00 2001 From: Midigo Frank <39288959+midigofrank@users.noreply.github.com> Date: Thu, 6 Mar 2025 12:40:14 +0300 Subject: [PATCH] Do not include `v` param in workflow links when latest (#2984) * Do not include param in workflow links when it is latest * do not add v param after saving workflow * fix credo warnings --- CHANGELOG.md | 3 + lib/lightning_web/live/run_live/components.ex | 15 +- .../live/run_live/run_viewer_live.ex | 4 + lib/lightning_web/live/run_live/show.ex | 1 + .../workflow_live/dashboard_components.ex | 4 +- lib/lightning_web/live/workflow_live/edit.ex | 39 ++++-- .../live/run_live/components_test.exs | 128 ++++++++++++++++++ .../live/workflow_live/edit_test.exs | 13 +- .../live/workflow_live/index_test.exs | 6 +- 9 files changed, 180 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2af5cb64ce..3e937c4f17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ and this project adheres to ### Changed +- Do not include `v` param in workflow links when it is latest + [#2941](https://github.com/OpenFn/lightning/issues/2941) + ### Fixed ## [v2.10.16] - 2025-02-28 diff --git a/lib/lightning_web/live/run_live/components.ex b/lib/lightning_web/live/run_live/components.ex index 4f902d1e7e..78d3af5b08 100644 --- a/lib/lightning_web/live/run_live/components.ex +++ b/lib/lightning_web/live/run_live/components.ex @@ -132,6 +132,7 @@ defmodule LightningWeb.RunLive.Components do """ end + attr :workflow_version, :integer, required: true attr :step, Lightning.Invocation.Step, required: true attr :is_clone, :boolean, default: false attr :run_id, :string @@ -192,7 +193,7 @@ defmodule LightningWeb.RunLive.Components do <.link class="pl-1" patch={ - ~p"/projects/#{@project_id}/w/#{@step.snapshot.workflow_id}?#{%{v: @step.snapshot.lock_version, a: @run_id, m: "expand", s: @job.id}}" <> "#log" + ~p"/projects/#{@project_id}/w/#{@step.snapshot.workflow_id}?#{maybe_add_snapshot_version(%{a: @run_id, m: "expand", s: @job.id}, @step.snapshot.lock_version, @workflow_version)}" <> "#log" } > <.icon @@ -226,6 +227,14 @@ defmodule LightningWeb.RunLive.Components do """ end + defp maybe_add_snapshot_version(params, snapshot_version, workflow_version) do + if snapshot_version != workflow_version do + Map.merge(params, %{v: snapshot_version}) + else + params + end + end + def loading_filler(assigns) do ~H""" <.detail_list class="animate-pulse"> @@ -380,8 +389,8 @@ defmodule LightningWeb.RunLive.Components do aria-label="Inspect this step" class="cursor-pointer" navigate={ - ~p"/projects/#{@project_id}/w/#{@step.snapshot.workflow_id}" - <> "?v=#{@step.snapshot.lock_version}&a=#{@run.id}&m=expand&s=#{@job.id}#log" + ~p"/projects/#{@project_id}/w/#{@step.snapshot.workflow_id}?#{maybe_add_snapshot_version(%{a: @run.id, m: "expand", s: @job.id}, @step.snapshot.lock_version, @workflow_version)}" + <> "#log" } > <.icon diff --git a/lib/lightning_web/live/run_live/run_viewer_live.ex b/lib/lightning_web/live/run_live/run_viewer_live.ex index 88f3a31004..4ab0fa0724 100644 --- a/lib/lightning_web/live/run_live/run_viewer_live.ex +++ b/lib/lightning_web/live/run_live/run_viewer_live.ex @@ -97,6 +97,7 @@ defmodule LightningWeb.RunLive.RunViewerLive do > <.step_item step={step} + workflow_version={@workflow.lock_version} run_id={run.id} job_id={@job_id} is_clone={ @@ -121,6 +122,7 @@ defmodule LightningWeb.RunLive.RunViewerLive do > <.step_item step={step} + workflow_version={@workflow.lock_version} run_id={run.id} job_id={@job_id} is_clone={ @@ -165,6 +167,7 @@ defmodule LightningWeb.RunLive.RunViewerLive do > <.step_item step={step} + workflow_version={@workflow.lock_version} run_id={run.id} job_id={@job_id} is_clone={ @@ -213,6 +216,7 @@ defmodule LightningWeb.RunLive.RunViewerLive do > <.step_item step={step} + workflow_version={@workflow.lock_version} run_id={run.id} job_id={@job_id} is_clone={ diff --git a/lib/lightning_web/live/run_live/show.ex b/lib/lightning_web/live/run_live/show.ex index c3530814ac..e96ee477e1 100644 --- a/lib/lightning_web/live/run_live/show.ex +++ b/lib/lightning_web/live/run_live/show.ex @@ -165,6 +165,7 @@ defmodule LightningWeb.RunLive.Show do <.link patch={"?step=#{step.id}"}> <.step_item step={step} + workflow_version={@workflow.lock_version} is_clone={ DateTime.compare(step.inserted_at, run.inserted_at) == :lt } diff --git a/lib/lightning_web/live/workflow_live/dashboard_components.ex b/lib/lightning_web/live/workflow_live/dashboard_components.ex index bf79db1035..db24fb01cb 100644 --- a/lib/lightning_web/live/workflow_live/dashboard_components.ex +++ b/lib/lightning_web/live/workflow_live/dashboard_components.ex @@ -193,9 +193,7 @@ defmodule LightningWeb.WorkflowLive.DashboardComponents do
<.link id={"workflow-card-#{@workflow.id}"} - navigate={ - ~p"/projects/#{@project.id}/w/#{@workflow.id}?v=#{@workflow.lock_version}" - } + navigate={~p"/projects/#{@project.id}/w/#{@workflow.id}"} role="button" >
diff --git a/lib/lightning_web/live/workflow_live/edit.ex b/lib/lightning_web/live/workflow_live/edit.ex index ca9b1c171c..603fe5e2a7 100644 --- a/lib/lightning_web/live/workflow_live/edit.ex +++ b/lib/lightning_web/live/workflow_live/edit.ex @@ -479,6 +479,7 @@ defmodule LightningWeb.WorkflowLive.Edit do <.expand_job_editor base_url={@base_url} snapshot_lock_version={@snapshot && @snapshot.lock_version} + snapshot_version_tag={@snapshot_version_tag} job={@selected_job} selected_run={@selected_run} form={@workflow_form} @@ -802,10 +803,19 @@ defmodule LightningWeb.WorkflowLive.Edit do query_string = query_params - |> Enum.reject(fn {_k, v} -> is_nil(v) end) + |> Enum.reject(fn {k, v} -> + is_nil(v) or (k == "v" and assigns.snapshot_version_tag == "latest") + end) |> URI.encode_query() + |> then(fn query -> + if byte_size(query) > 0 do + "?" <> query + else + query + end + end) - "#{assigns[:base_url]}?#{query_string}" + "#{assigns[:base_url]}#{query_string}" end defp display_switcher(snapshot, workflow) do @@ -956,7 +966,7 @@ defmodule LightningWeb.WorkflowLive.Edit do ~H""" <.link id={"open-inspector-#{@job.id}"} - patch={"#{@base_url}?s=#{@job.id}&m=expand" <> (if @snapshot_lock_version, do: "&v=#{@snapshot_lock_version}", else: "") <> (if @selected_run, do: "&a=#{@selected_run}", else: "")} + patch={"#{@base_url}?s=#{@job.id}&m=expand" <> (if @snapshot_lock_version && @snapshot_version_tag != "latest", do: "&v=#{@snapshot_lock_version}", else: "") <> (if @selected_run, do: "&a=#{@selected_run}", else: "")} class={@button_classes} > <.icon name="hero-code-bracket" class="w-4 h-4 text-grey-400" /> @@ -1277,12 +1287,9 @@ defmodule LightningWeb.WorkflowLive.Edit do patches = WorkflowParams.to_patches(prev_params, next_params) - lock_version = Ecto.Changeset.get_field(next_changeset, :lock_version) - query_params = socket.assigns.query_params - |> Map.reject(fn {_k, v} -> is_nil(v) end) - |> Map.put("v", lock_version) + |> Map.reject(fn {k, v} -> is_nil(v) or k == "v" end) url = ~p"/projects/#{project.id}/w/#{workflow.id}?#{query_params}" @@ -1317,13 +1324,9 @@ defmodule LightningWeb.WorkflowLive.Edit do patches = WorkflowParams.to_patches(prev_params, next_params) - lock_version = - Ecto.Changeset.get_field(next_changeset, :lock_version) - query_params = socket.assigns.query_params - |> Map.reject(fn {_k, v} -> is_nil(v) end) - |> Map.put("v", lock_version) + |> Map.reject(fn {k, v} -> is_nil(v) or k == "v" end) url = ~p"/projects/#{project.id}/w/#{workflow.id}?#{query_params}" @@ -1532,8 +1535,8 @@ defmodule LightningWeb.WorkflowLive.Edit do query_params = socket.assigns.query_params - |> Map.put("v", workflow.lock_version) |> Map.reject(fn {_key, value} -> is_nil(value) end) + |> Map.drop(["v"]) flash_msg = "Workflow saved successfully." <> @@ -2439,8 +2442,14 @@ defmodule LightningWeb.WorkflowLive.Edit do params = query_params |> Map.put("a", run.id) - |> Map.put("v", version) - |> Enum.reject(fn {_k, v} -> is_nil(v) end) + |> Map.reject(fn {_k, v} -> is_nil(v) end) + |> then(fn params -> + if workflow.lock_version == version do + Map.drop(params, ["v"]) + else + Map.put(params, "v", version) + end + end) socket |> push_patch(to: ~p"/projects/#{project}/w/#{workflow}?#{params}") diff --git a/test/lightning_web/live/run_live/components_test.exs b/test/lightning_web/live/run_live/components_test.exs index ffe59d351a..1fddcad9cd 100644 --- a/test/lightning_web/live/run_live/components_test.exs +++ b/test/lightning_web/live/run_live/components_test.exs @@ -116,6 +116,46 @@ defmodule LightningWeb.RunLive.ComponentsTest do assert has_run_step_link?(html, workflow.project, run, first_step) + # snapshot version is not included in the link when it is latest + assert workflow.lock_version == snapshot.lock_version + + assert html + |> Floki.find( + ~s{a[href='#{~p"/projects/#{workflow.project}/w/#{workflow}?#{%{a: run.id, m: "expand", s: job_1.id}}"}#log']} + ) + |> Enum.any?() + + refute html + |> Floki.find( + ~s{a[href='#{~p"/projects/#{workflow.project}/w/#{workflow}?#{%{a: run.id, m: "expand", s: job_1.id, v: snapshot.lock_version}}"}#log']} + ) + |> Enum.any?() + + # if the snapshot version is not latest, the version is included + + html = + render_component(&Components.step_list_item/1, + step: first_step, + run: run, + workflow_version: workflow.lock_version + 1, + project_id: project_id, + can_run_workflow: true, + can_edit_data_retention: true + ) + |> Floki.parse_fragment!() + + refute html + |> Floki.find( + ~s{a[href='#{~p"/projects/#{workflow.project}/w/#{workflow}?#{%{a: run.id, m: "expand", s: job_1.id}}"}#log']} + ) + |> Enum.any?() + + assert html + |> Floki.find( + ~s{a[href='#{~p"/projects/#{workflow.project}/w/#{workflow}?#{%{a: run.id, m: "expand", s: job_1.id, v: snapshot.lock_version}}"}#log']} + ) + |> Enum.any?() + html = render_component(&Components.step_list_item/1, step: second_step, @@ -213,6 +253,94 @@ defmodule LightningWeb.RunLive.ComponentsTest do refute html =~ "This step was originally executed in a previous run" end + describe "step_item/1" do + test "renders link to inspector correctly" do + %{triggers: [trigger], jobs: [job_1 | _rest]} = + workflow = insert(:complex_workflow) + + {:ok, snapshot} = Workflows.Snapshot.create(workflow) + + dataclip = insert(:dataclip) + output_dataclip = insert(:dataclip) + + %{runs: [run]} = + insert(:workorder, + workflow: workflow, + trigger: trigger, + dataclip: dataclip, + snapshot: snapshot, + runs: [ + %{ + state: :failed, + dataclip: dataclip, + starting_trigger: trigger, + snapshot: snapshot, + steps: [ + insert(:step, + job: job_1, + snapshot: snapshot, + input_dataclip: dataclip, + output_dataclip: output_dataclip, + finished_at: build(:timestamp), + exit_reason: "fail" + ) + ] + } + ] + ) + + [first_step] = run.steps + + project_id = workflow.project_id + + # snapshot version is not included in the link when it is latest + html = + render_component(&Components.step_item/1, + step: first_step, + run_id: run.id, + workflow_version: workflow.lock_version, + project_id: project_id + ) + |> Floki.parse_fragment!() + + assert workflow.lock_version == snapshot.lock_version + + assert html + |> Floki.find( + ~s{a[href='#{~p"/projects/#{workflow.project}/w/#{workflow}?#{%{a: run.id, m: "expand", s: job_1.id}}"}#log']} + ) + |> Enum.any?() + + refute html + |> Floki.find( + ~s{a[href='#{~p"/projects/#{workflow.project}/w/#{workflow}?#{%{a: run.id, m: "expand", s: job_1.id, v: snapshot.lock_version}}"}#log']} + ) + |> Enum.any?() + + # snapshot version is included in the link when it is outdated + html = + render_component(&Components.step_item/1, + step: first_step, + run_id: run.id, + workflow_version: workflow.lock_version + 1, + project_id: project_id + ) + |> Floki.parse_fragment!() + + refute html + |> Floki.find( + ~s{a[href='#{~p"/projects/#{workflow.project}/w/#{workflow}?#{%{a: run.id, m: "expand", s: job_1.id}}"}#log']} + ) + |> Enum.any?() + + assert html + |> Floki.find( + ~s{a[href='#{~p"/projects/#{workflow.project}/w/#{workflow}?#{%{a: run.id, m: "expand", s: job_1.id, v: snapshot.lock_version}}"}#log']} + ) + |> Enum.any?() + end + end + test "no rerun button is displayed when user can't rerun a job" do %{triggers: [trigger], jobs: [job | _rest]} = workflow = insert(:simple_workflow) diff --git a/test/lightning_web/live/workflow_live/edit_test.exs b/test/lightning_web/live/workflow_live/edit_test.exs index 50c0b09d60..8e1ed77e66 100644 --- a/test/lightning_web/live/workflow_live/edit_test.exs +++ b/test/lightning_web/live/workflow_live/edit_test.exs @@ -226,7 +226,6 @@ defmodule LightningWeb.WorkflowLive.EditTest do click_save(view) assert %{id: workflow_id} = - workflow = Lightning.Repo.one( from w in Workflow, where: @@ -235,7 +234,7 @@ defmodule LightningWeb.WorkflowLive.EditTest do assert_patched( view, - ~p"/projects/#{project.id}/w/#{workflow_id}?#{[m: "expand", s: job.id, v: workflow.lock_version]}" + ~p"/projects/#{project.id}/w/#{workflow_id}?#{[m: "expand", s: job.id]}" ) assert render(view) =~ "Workflow saved" @@ -1961,11 +1960,9 @@ defmodule LightningWeb.WorkflowLive.EditTest do # submit workflow form view |> form("#workflow-form") |> render_submit() - workflow = Lightning.Repo.reload(workflow) - assert_patched( view, - ~p"/projects/#{project}/w/#{workflow}?#{[m: "expand", s: job_1.id, v: workflow.lock_version]}" + ~p"/projects/#{project}/w/#{workflow}?#{[m: "expand", s: job_1.id]}" ) # manual run form still has the body @@ -2209,7 +2206,7 @@ defmodule LightningWeb.WorkflowLive.EditTest do assert_patched( view, - ~p"/projects/#{project.id}/w/#{workflow.id}?#{[m: "expand", s: job.id, v: workflow.lock_version]}" + ~p"/projects/#{project.id}/w/#{workflow.id}?#{[m: "expand", s: job.id]}" ) assert render(view) =~ "Workflow saved and synced to GitHub successfully." @@ -2276,11 +2273,9 @@ defmodule LightningWeb.WorkflowLive.EditTest do |> form("#workflow-form") |> render_submit(%{"github-sync" => %{"commit_message" => "some message"}}) - workflow = Repo.reload!(workflow) - assert_patched( view, - ~p"/projects/#{project.id}/w/#{workflow.id}?#{[s: job_2.id, v: workflow.lock_version]}" + ~p"/projects/#{project.id}/w/#{workflow.id}?#{[s: job_2.id]}" ) assert render(view) =~ diff --git a/test/lightning_web/live/workflow_live/index_test.exs b/test/lightning_web/live/workflow_live/index_test.exs index 61bbd55269..cf4a8eed8b 100644 --- a/test/lightning_web/live/workflow_live/index_test.exs +++ b/test/lightning_web/live/workflow_live/index_test.exs @@ -168,19 +168,19 @@ defmodule LightningWeb.WorkflowLive.IndexTest do # Workflow links assert view |> has_link?( - ~p"/projects/#{project.id}/w/#{workflow1.id}?v=#{workflow1.lock_version}", + ~p"/projects/#{project.id}/w/#{workflow1.id}", "One" ) assert view |> has_link?( - ~p"/projects/#{project.id}/w/#{workflow2.id}?v=#{workflow2.lock_version}", + ~p"/projects/#{project.id}/w/#{workflow2.id}", "Two" ) assert view |> has_link?( - ~p"/projects/#{project.id}/w/#{new_workflow.id}?v=#{new_workflow.lock_version}", + ~p"/projects/#{project.id}/w/#{new_workflow.id}", new_workflow.name )