Merge pull request #552 from podlove/task/more-consistence

Task/more consistence

lib/radiator/outline.ex

@@ -126,26 +126,30 @@ defmodule Radiator.Outline do
     {:error, :circle_link}
   end
 
+  def move_node(_node_id, other_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
     case NodeRepository.get_node(node_id) do
       nil ->
         {:error, :not_found}
 
       node ->
-        prev_node = get_prev_node(node)
         parent_node = get_parent_node(node)
 
-        case validate_consistency(node, new_prev_id, new_parent_id, parent_node) do
+        case validate_consistency_for_move(node, new_prev_id, new_parent_id, parent_node) do
           {:error, error} ->
             {:error, error}
 
           {:ok, node} ->
+            prev_node = get_prev_node(node)
             do_move_node(node, new_prev_id, new_parent_id, prev_node, parent_node)
         end
     end
   end
 
-  defp validate_consistency(
+  defp validate_consistency_for_move(
          %{prev_id: new_prev_id, parent_id: new_parent_id},
          new_prev_id,
          new_parent_id,
@@ -154,40 +158,27 @@ defmodule Radiator.Outline do
     {:error, :noop}
   end
 
-  defp validate_consistency(
-         %{parent_id: new_parent_id} = node,
-         _new_prev_id,
-         new_parent_id,
+  # when prev is nil, every parent is allowed
+  defp validate_consistency_for_move(
+         node,
+         nil,
+         _new_parent_id,
          _parent_node
        ) do
     {:ok, node}
   end
 
-  defp validate_consistency(node, new_prev_id, new_parent_id, parent_node) do
-    node
-    |> get_all_children()
-    |> Enum.map(& &1.uuid)
-    |> Enum.member?(new_parent_id)
-    |> case do
-      true ->
-        {:error, :circle_link}
-
-      #  parent and previous must be consistent, prev must be direct child of parent
-      false ->
-        validate_prev_child_of_parent(new_prev_id, parent_node, node)
-    end
-  end
-
-  defp validate_prev_child_of_parent(nil, _parent_node, node), do: {:ok, node}
-
-  defp validate_prev_child_of_parent(new_prev_id, parent_node, node) do
-    parent_node
-    |> get_all_siblings()
-    |> Enum.map(& &1.uuid)
-    |> Enum.member?(new_prev_id)
-    |> case do
-      true -> {:ok, node}
-      false -> {:error, :parent_and_prev_not_consistent}
+  # when prev is not nil, parent and prev must be consistent
+  defp validate_consistency_for_move(
+         node,
+         new_prev_id,
+         new_parent_id,
+         _parent_node
+       ) do
+    if NodeRepository.get_node(new_prev_id).parent_id == new_parent_id do
+      {:ok, node}
+    else
+      {:error, :parent_and_prev_not_consistent}
     end
   end
 
@@ -351,7 +342,11 @@ defmodule Radiator.Outline do
   end
 
   @doc """
-  TODO + test
+  get all children of a node. there is no limit of levels.
+  It basically calls `get_all_siblings` recursively and flattens the result.
+  ## Examples
+        iex> get_all_children(%Node{})
+        [%Node{}, %Node{}]
   """
   def get_all_children(node) do
     siblings = node |> get_all_siblings()

test/radiator/outline_test.exs

@@ -44,19 +44,61 @@ defmodule Radiator.OutlineTest do
   describe "get_all_siblings/1" do
     setup :complex_node_fixture
 
-    test "returns all child nodes", %{
+    test "returns all direct child nodes", %{
       node_3: node_3,
       nested_node_1: nested_node_1,
       nested_node_2: nested_node_2
     } do
       assert Outline.get_all_siblings(node_3) == [nested_node_1, nested_node_2]
     end
 
+    test "does not return child nodes deeper then 1 level", %{
+      parent_node: parent_node,
+      node_1: node_1,
+      nested_node_1: nested_node_1,
+      nested_node_2: nested_node_2
+    } do
+      assert parent_node |> Outline.get_all_siblings() |> Enum.member?(node_1)
+      refute parent_node |> Outline.get_all_siblings() |> Enum.member?(nested_node_1)
+      refute parent_node |> Outline.get_all_siblings() |> Enum.member?(nested_node_2)
+    end
+
     test "returns an empty list if there are no child nodes", %{node_1: node_1} do
       assert Outline.get_all_siblings(node_1) == []
     end
   end
 
+  describe "get_all_children/1" do
+    setup :complex_node_fixture
+
+    test "returns all child nodes", %{
+      parent_node: parent_node,
+      node_1: node_1,
+      node_2: node_2,
+      node_3: node_3,
+      node_4: node_4,
+      node_5: node_5,
+      node_6: node_6,
+      nested_node_1: nested_node_1,
+      nested_node_2: nested_node_2
+    } do
+      assert Outline.get_all_children(parent_node) == [
+               node_1,
+               node_2,
+               node_3,
+               node_4,
+               node_5,
+               node_6,
+               nested_node_1,
+               nested_node_2
+             ]
+    end
+
+    test "returns an empty list if there are no child nodes", %{nested_node_1: nested_node_1} do
+      assert Outline.get_all_children(nested_node_1) == []
+    end
+  end
+
   describe "insert_node/1" do
     setup :complex_node_fixture
 
@@ -345,6 +387,14 @@ defmodule Radiator.OutlineTest do
       assert node_1.parent_id == nil
       assert node_2.parent_id == node_1.uuid
     end
+
+    test "error when parent and prev are the same", %{
+      node_1: node_1,
+      node_2: node_2
+    } do
+      {:error, :parent_and_prev_not_consistent} =
+        Outline.move_node(node_2.uuid, node_1.uuid, node_1.uuid)
+    end
   end
 
   describe "move_node/3" do
@@ -524,6 +574,56 @@ defmodule Radiator.OutlineTest do
       assert node_result.old_prev_id == nil
       assert node_result.old_next_id == node_2.uuid
     end
+
+    test "error when prev id is the same as node id", %{
+      node_1: node_1
+    } do
+      {:error, :self_link} =
+        Outline.move_node(node_1.uuid, node_1.uuid, node_1.parent_id)
+    end
+
+    test "error when parent id is the same as node id", %{
+      node_1: node_1,
+      node_5: node_5
+    } do
+      {:error, :circle_link} =
+        Outline.move_node(node_1.uuid, node_5.uuid, node_1.uuid)
+    end
+
+    test "error when parent is a child of new node", %{
+      node_1: node_1,
+      node_5: node_5,
+      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)
+    end
+
+    test "error when parent is on same level of new node", %{
+      node_1: node_1,
+      node_5: node_5,
+      node_3: node_3
+    } do
+      {:error, :parent_and_prev_not_consistent} =
+        Outline.move_node(node_1.uuid, node_5.uuid, node_3.uuid)
+    end
+
+    test "error when new prev is not a direct child of new parent", %{
+      parent_node: parent_node,
+      node_1: node_1,
+      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)
+    end
+
+    test "error when new new parent is nil but prev is not on level 1", %{
+      node_1: node_1,
+      nested_node_1: nested_node_1
+    } do
+      {:error, :parent_and_prev_not_consistent} =
+        Outline.move_node(node_1.uuid, nested_node_1.uuid, nil)
+    end
   end
 
   describe "remove_node/1" do