From 71291e8bc559532be8e4b0384f897bbb5d9cde7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20W=C3=B6ginger?= Date: Fri, 6 Sep 2024 21:25:43 +0200 Subject: [PATCH] Optional parent node for move node (#561) * parent id is optional for move node in order to achieve this we need a different signature, use a keyword list instead of fixed parameters * rename prev_node_id, parent_node_id => prev_id, parent_id --------- Co-authored-by: sorax --- lib/radiator/outline.ex | 46 +++++-- lib/radiator/outline/command.ex | 6 +- lib/radiator/outline/command_processor.ex | 2 +- lib/radiator/outline/dispatch.ex | 7 +- lib/radiator_web/live/outline_component.ex | 4 +- test/radiator/outline_test.exs | 149 ++++++++++++++++++--- 6 files changed, 171 insertions(+), 43 deletions(-) diff --git a/lib/radiator/outline.ex b/lib/radiator/outline.ex index ee478e38..cd701deb 100644 --- a/lib/radiator/outline.ex +++ b/lib/radiator/outline.ex @@ -82,23 +82,23 @@ defmodule Radiator.Outline do # if no previous node is given, the new node will be inserted as the first child of the parent node def insert_node(attrs) do Repo.transaction(fn -> - prev_node_id = attrs["prev_id"] - parent_node_id = attrs["parent_id"] + prev_id = attrs["prev_id"] + parent_id = attrs["parent_id"] episode_id = attrs["episode_id"] # find Node which has been previously connected to prev_node next_node = Node |> where(episode_id: ^episode_id) - |> where_prev_node_equals(prev_node_id) - |> where_parent_node_equals(parent_node_id) + |> where_prev_node_equals(prev_id) + |> where_parent_node_equals(parent_id) |> Repo.one() - with prev_node <- NodeRepository.get_node_if(prev_node_id), - parent_node <- find_parent_node(prev_node, parent_node_id), + with prev_node <- NodeRepository.get_node_if(prev_id), + parent_node <- find_parent_node(prev_node, parent_id), true <- parent_and_prev_consistent?(parent_node, prev_node), true <- episode_valid?(episode_id, parent_node, prev_node), {:ok, node} <- NodeRepository.create_node(set_parent_id_if(attrs, parent_node)), - {:ok, _node_to_move} <- move_node_if(next_node, parent_node_id, node.uuid) do + {:ok, _node_to_move} <- move_node_if(next_node, parent_id, node.uuid) do %NodeRepoResult{node: node, next_id: get_node_id(next_node)} else false -> @@ -138,19 +138,19 @@ defmodule Radiator.Outline do iex> move_node(node_id, new_prev_id, new_parent_id) {:ok, %Node{}} """ - def move_node(node_id, node_id, _new_parent_id) do + def move_node(node_id, prev_id: node_id, parent_id: _new_parent_id) do {:error, :self_link} end - def move_node(node_id, _new_prev_id, node_id) do + def move_node(node_id, prev_id: _new_prev_id, parent_id: node_id) do {:error, :circle_link} end - def move_node(_node_id, other_id, other_id) when not is_nil(other_id) do + def move_node(_node_id, prev_id: other_id, parent_id: other_id) when not is_nil(other_id) do {:error, :parent_and_prev_not_consistent} end - def move_node(node_id, new_prev_id, new_parent_id) do + def move_node(node_id, prev_id: new_prev_id, parent_id: new_parent_id) do case NodeRepository.get_node(node_id) do nil -> {:error, :not_found} @@ -174,6 +174,19 @@ defmodule Radiator.Outline do end end + def move_node(node_id, parent_id: parent_id, prev_id: new_prev_id), + do: move_node(node_id, prev_id: new_prev_id, parent_id: parent_id) + + def move_node(node_id, prev_id: new_prev_id) do + parent_id = + new_prev_id + |> NodeRepository.get_node_if() + |> get_parent_node + |> get_node_id + + move_node(node_id, prev_id: new_prev_id, parent_id: parent_id) + end + # low level function to move a node defp do_move_node(node, new_prev_id, new_parent_id, prev_node, parent_node) do node_repo_result = %NodeRepoResult{node: node} @@ -286,6 +299,7 @@ defmodule Radiator.Outline do %Node{uuid: 42} """ + def get_prev_node(nil), do: nil def get_prev_node(node) when is_nil(node.prev_id), do: nil def get_prev_node(node) do @@ -305,7 +319,11 @@ defmodule Radiator.Outline do iex> get_parent_node(%Node{parent_id: 42}) %Node{uuid: 42} + iex> get_parent_node(nil) + nil + """ + def get_parent_node(nil), do: nil def get_parent_node(node) when is_nil(node.parent_id), do: nil def get_parent_node(node) do @@ -437,11 +455,11 @@ defmodule Radiator.Outline do defp move_node_if(nil, _parent_node_id, _prev_node_id), do: {:ok, nil} - defp move_node_if(node, parent_node_id, prev_node_id) do + defp move_node_if(node, parent_id, prev_id) do node |> Node.move_node_changeset(%{ - parent_id: parent_node_id, - prev_id: prev_node_id + parent_id: parent_id, + prev_id: prev_id }) |> Repo.update() end diff --git a/lib/radiator/outline/command.ex b/lib/radiator/outline/command.ex index fa333c04..db461e1a 100644 --- a/lib/radiator/outline/command.ex +++ b/lib/radiator/outline/command.ex @@ -33,13 +33,13 @@ defmodule Radiator.Outline.Command do } end - def build("move_node", node_id, parent_node_id, prev_node_id, user_id, event_id) do + def build("move_node", node_id, parent_id, prev_id, user_id, event_id) do %MoveNodeCommand{ event_id: event_id, user_id: user_id, node_id: node_id, - parent_id: parent_node_id, - prev_id: prev_node_id + parent_id: parent_id, + prev_id: prev_id } end end diff --git a/lib/radiator/outline/command_processor.ex b/lib/radiator/outline/command_processor.ex index f8fddfdc..62a8fad4 100644 --- a/lib/radiator/outline/command_processor.ex +++ b/lib/radiator/outline/command_processor.ex @@ -64,7 +64,7 @@ defmodule Radiator.Outline.CommandProcessor do } = command ) do node_id - |> Outline.move_node(prev_id, parent_id) + |> Outline.move_node(prev_id: prev_id, parent_id: parent_id) |> handle_move_node_result(command) end diff --git a/lib/radiator/outline/dispatch.ex b/lib/radiator/outline/dispatch.ex index 6657f6cf..455fa8e9 100644 --- a/lib/radiator/outline/dispatch.ex +++ b/lib/radiator/outline/dispatch.ex @@ -15,9 +15,12 @@ defmodule Radiator.Outline.Dispatch do |> EventProducer.enqueue() end - def move_node(node_id, parent_node_id, prev_node_id, user_id, event_id) do + def move_node(node_id, user_id, event_id, + parent_id: parent_id, + prev_id: prev_id + ) do "move_node" - |> Command.build(node_id, parent_node_id, prev_node_id, user_id, event_id) + |> Command.build(node_id, parent_id, prev_id, user_id, event_id) |> EventProducer.enqueue() end diff --git a/lib/radiator_web/live/outline_component.ex b/lib/radiator_web/live/outline_component.ex index 66e69fef..a34677eb 100644 --- a/lib/radiator_web/live/outline_component.ex +++ b/lib/radiator_web/live/outline_component.ex @@ -217,7 +217,7 @@ defmodule RadiatorWeb.OutlineComponent do } user_id = socket.assigns.user_id - Dispatch.move_node(uuid, prev_id, nil, user_id, generate_event_id(socket.id)) + Dispatch.move_node(uuid, user_id, generate_event_id(socket.id), prev_id: prev_id) socket |> stream_insert(:nodes, to_change_form(node, %{})) @@ -232,7 +232,7 @@ defmodule RadiatorWeb.OutlineComponent do } user_id = socket.assigns.user_id - Dispatch.move_node(uuid, nil, parent_id, user_id, generate_event_id(socket.id)) + Dispatch.move_node(uuid, user_id, generate_event_id(socket.id), parent_id: parent_id) socket |> stream_insert(:nodes, to_change_form(node, %{})) diff --git a/test/radiator/outline_test.exs b/test/radiator/outline_test.exs index 548bf6b6..06e496ff 100644 --- a/test/radiator/outline_test.exs +++ b/test/radiator/outline_test.exs @@ -335,7 +335,7 @@ defmodule Radiator.OutlineTest do node_1: node_1, node_2: node_2 } do - {:ok, _} = Outline.move_node(node_2.uuid, nil, node_2.parent_id) + {:ok, _} = Outline.move_node(node_2.uuid, prev_id: nil, parent_id: node_2.parent_id) # reload nodes node_1 = Repo.reload!(node_1) @@ -351,7 +351,21 @@ defmodule Radiator.OutlineTest do node_1: node_1, node_2: node_2 } do - {:ok, _} = Outline.move_node(node_1.uuid, node_2.uuid, node_1.parent_id) + {:ok, _} = Outline.move_node(node_1.uuid, prev_id: node_2.uuid, parent_id: node_1.parent_id) + + # reload nodes + node_1 = Repo.reload!(node_1) + node_2 = Repo.reload!(node_2) + + assert node_1.prev_id == node_2.uuid + assert node_2.prev_id == nil + end + + test "if prev_id has been given the parent_id can be omitted", %{ + node_1: node_1, + node_2: node_2 + } do + {:ok, _} = Outline.move_node(node_1.uuid, prev_id: node_2.uuid) # reload nodes node_1 = Repo.reload!(node_1) @@ -365,7 +379,8 @@ defmodule Radiator.OutlineTest do node_1: node_1, node_2: node_2 } do - {:error, :noop} = Outline.move_node(node_2.uuid, node_1.uuid, node_2.parent_id) + {:error, :noop} = + Outline.move_node(node_2.uuid, prev_id: node_1.uuid, parent_id: node_2.parent_id) # reload nodes node_1 = Repo.reload!(node_1) @@ -379,7 +394,7 @@ defmodule Radiator.OutlineTest do node_1: node_1, node_2: node_2 } do - {:error, :noop} = Outline.move_node(node_1.uuid, nil, node_2.parent_id) + {:error, :noop} = Outline.move_node(node_1.uuid, prev_id: nil, parent_id: node_2.parent_id) # reload nodes node_1 = Repo.reload!(node_1) @@ -393,7 +408,7 @@ defmodule Radiator.OutlineTest do node_1: node_1, node_2: node_2 } do - {:ok, _} = Outline.move_node(node_2.uuid, nil, node_1.uuid) + {:ok, _} = Outline.move_node(node_2.uuid, prev_id: nil, parent_id: node_1.uuid) # reload nodes node_1 = Repo.reload!(node_1) @@ -411,7 +426,7 @@ defmodule Radiator.OutlineTest do node_2: node_2 } do {:error, :parent_and_prev_not_consistent} = - Outline.move_node(node_2.uuid, node_1.uuid, node_1.uuid) + Outline.move_node(node_2.uuid, prev_id: node_1.uuid, parent_id: node_1.uuid) end end @@ -426,7 +441,28 @@ defmodule Radiator.OutlineTest do node_4: node_4, node_5: node_5 } do - {:ok, _} = Outline.move_node(node_4.uuid, node_2.uuid, node_4.parent_id) + {:ok, _} = Outline.move_node(node_4.uuid, prev_id: node_2.uuid, parent_id: node_4.parent_id) + + # reload nodes + node_5 = Repo.reload!(node_5) + node_4 = Repo.reload!(node_4) + node_3 = Repo.reload!(node_3) + node_2 = Repo.reload!(node_2) + + assert node_4.prev_id == node_2.uuid + assert node_3.prev_id == node_4.uuid + assert node_5.prev_id == node_3.uuid + end + + # before 1 2 3 4 5 + # after 1 2 4 3 5 + test "move node 4 within list to node 2, but ommitting the parent_id", %{ + node_2: node_2, + node_3: node_3, + node_4: node_4, + node_5: node_5 + } do + {:ok, _} = Outline.move_node(node_4.uuid, prev_id: node_2.uuid) # reload nodes node_5 = Repo.reload!(node_5) @@ -448,7 +484,7 @@ defmodule Radiator.OutlineTest do node_4: node_4, node_5: node_5 } do - {:ok, _} = Outline.move_node(node_4.uuid, nil, node_4.parent_id) + {:ok, _} = Outline.move_node(node_4.uuid, prev_id: nil, parent_id: node_4.parent_id) # reload nodes node_5 = Repo.reload!(node_5) @@ -473,7 +509,31 @@ defmodule Radiator.OutlineTest do node_4: node_4, node_5: node_5 } do - {:ok, _} = Outline.move_node(node_2.uuid, node_5.uuid, node_2.parent_id) + {:ok, _} = Outline.move_node(node_2.uuid, prev_id: node_5.uuid, parent_id: node_2.parent_id) + + # reload nodes + node_5 = Repo.reload!(node_5) + node_4 = Repo.reload!(node_4) + node_3 = Repo.reload!(node_3) + node_2 = Repo.reload!(node_2) + node_1 = Repo.reload!(node_1) + + assert node_3.prev_id == node_1.uuid + assert node_4.prev_id == node_3.uuid + assert node_5.prev_id == node_4.uuid + assert node_2.prev_id == node_5.uuid + end + + # before 1 2 3 4 5 + # after 1 3 4 5 2 + test "move node 2 to the end of the list but ommitting the parent_id", %{ + node_1: node_1, + node_2: node_2, + node_3: node_3, + node_4: node_4, + node_5: node_5 + } do + {:ok, _} = Outline.move_node(node_2.uuid, prev_id: node_5.uuid) # reload nodes node_5 = Repo.reload!(node_5) @@ -497,7 +557,31 @@ defmodule Radiator.OutlineTest do node_4: node_4, node_5: node_5 } do - {:ok, _} = Outline.move_node(node_1.uuid, node_5.uuid, node_2.parent_id) + {:ok, _} = Outline.move_node(node_1.uuid, prev_id: node_5.uuid, parent_id: node_2.parent_id) + + # reload nodes + node_5 = Repo.reload!(node_5) + node_4 = Repo.reload!(node_4) + node_3 = Repo.reload!(node_3) + node_2 = Repo.reload!(node_2) + node_1 = Repo.reload!(node_1) + + assert node_1.prev_id == node_5.uuid + assert node_5.prev_id == node_4.uuid + assert node_3.prev_id == node_2.uuid + assert node_2.prev_id == nil + end + + # before 1 2 3 4 5 + # after 2 3 4 5 1 + test "move first node to the end of the list and ommitting the parent_id", %{ + node_1: node_1, + node_2: node_2, + node_3: node_3, + node_4: node_4, + node_5: node_5 + } do + {:ok, _} = Outline.move_node(node_1.uuid, prev_id: node_5.uuid) # reload nodes node_5 = Repo.reload!(node_5) @@ -521,7 +605,7 @@ defmodule Radiator.OutlineTest do node_4: node_4, node_5: node_5 } do - {:ok, _} = Outline.move_node(node_5.uuid, nil, node_2.parent_id) + {:ok, _} = Outline.move_node(node_5.uuid, prev_id: nil, parent_id: node_2.parent_id) # reload nodes node_5 = Repo.reload!(node_5) @@ -543,7 +627,7 @@ defmodule Radiator.OutlineTest do nested_node_1: nested_node_1, nested_node_2: nested_node_2 } do - {:ok, _} = Outline.move_node(nested_node_1.uuid, nil, node_2.uuid) + {:ok, _} = Outline.move_node(nested_node_1.uuid, prev_id: nil, parent_id: node_2.uuid) # reload nodes nested_node_1 = Repo.reload!(nested_node_1) @@ -561,7 +645,30 @@ defmodule Radiator.OutlineTest do nested_node_1: nested_node_1, nested_node_2: nested_node_2 } do - {:ok, _} = Outline.move_node(node_3.uuid, nil, nil) + {:ok, _} = Outline.move_node(node_3.uuid, prev_id: nil, parent_id: nil) + + # reload nodes + parent_node = Repo.reload!(parent_node) + node_3 = Repo.reload!(node_3) + nested_node_1 = Repo.reload!(nested_node_1) + nested_node_2 = Repo.reload!(nested_node_2) + + assert node_3.prev_id == nil + assert node_3.parent_id == nil + assert parent_node.prev_id == node_3.uuid + assert nested_node_1.parent_id == node_3.uuid + assert nested_node_2.parent_id == node_3.uuid + assert nested_node_1.prev_id == nil + assert nested_node_2.prev_id == nested_node_1.uuid + end + + test "move node with child elements to top also works without parent_id", %{ + parent_node: parent_node, + node_3: node_3, + nested_node_1: nested_node_1, + nested_node_2: nested_node_2 + } do + {:ok, _} = Outline.move_node(node_3.uuid, prev_id: nil) # reload nodes parent_node = Repo.reload!(parent_node) @@ -586,7 +693,7 @@ defmodule Radiator.OutlineTest do node_5: node_5 } do {:ok, %NodeRepoResult{} = node_result} = - Outline.move_node(node_1.uuid, node_5.uuid, node_2.parent_id) + Outline.move_node(node_1.uuid, prev_id: node_5.uuid, parent_id: node_2.parent_id) assert node_result.node.uuid == node_1.uuid assert node_result.old_prev_id == nil @@ -597,7 +704,7 @@ defmodule Radiator.OutlineTest do node_1: node_1 } do {:error, :self_link} = - Outline.move_node(node_1.uuid, node_1.uuid, node_1.parent_id) + Outline.move_node(node_1.uuid, prev_id: node_1.uuid, parent_id: node_1.parent_id) end test "error when parent id is the same as node id", %{ @@ -605,7 +712,7 @@ defmodule Radiator.OutlineTest do node_5: node_5 } do {:error, :circle_link} = - Outline.move_node(node_1.uuid, node_5.uuid, node_1.uuid) + Outline.move_node(node_1.uuid, prev_id: node_5.uuid, parent_id: node_1.uuid) end test "error when parent is a child of new node", %{ @@ -614,7 +721,7 @@ defmodule Radiator.OutlineTest do nested_node_1: nested_node_1 } do {:error, :parent_and_prev_not_consistent} = - Outline.move_node(node_1.uuid, node_5.uuid, nested_node_1.uuid) + Outline.move_node(node_1.uuid, prev_id: node_5.uuid, parent_id: nested_node_1.uuid) end test "error when parent is on same level of new node", %{ @@ -623,7 +730,7 @@ defmodule Radiator.OutlineTest do node_3: node_3 } do {:error, :parent_and_prev_not_consistent} = - Outline.move_node(node_1.uuid, node_5.uuid, node_3.uuid) + Outline.move_node(node_1.uuid, prev_id: node_5.uuid, parent_id: node_3.uuid) end test "error when new prev is not a direct child of new parent", %{ @@ -632,7 +739,7 @@ defmodule Radiator.OutlineTest do nested_node_1: nested_node_1 } do {:error, :parent_and_prev_not_consistent} = - Outline.move_node(node_1.uuid, nested_node_1.uuid, parent_node.uuid) + Outline.move_node(node_1.uuid, prev_id: nested_node_1.uuid, parent_id: parent_node.uuid) end test "error when new new parent is nil but prev is not on level 1", %{ @@ -640,7 +747,7 @@ defmodule Radiator.OutlineTest do nested_node_1: nested_node_1 } do {:error, :parent_and_prev_not_consistent} = - Outline.move_node(node_1.uuid, nested_node_1.uuid, nil) + Outline.move_node(node_1.uuid, prev_id: nested_node_1.uuid, parent_id: nil) end end @@ -927,7 +1034,7 @@ defmodule Radiator.OutlineTest do ) {:ok, %NodeRepoResult{} = _result} = - Outline.move_node(node_3.uuid, node_2.uuid, node_1.uuid) + Outline.move_node(node_3.uuid, prev_id: node_2.uuid, parent_id: node_1.uuid) assert node_1 |> Outline.order_child_nodes() |> Enum.map(& &1.content) == ["node_2", "node_3"]