diff --git a/lib/radiator/outline.ex b/lib/radiator/outline.ex index 413e9554..a0ef53c0 100644 --- a/lib/radiator/outline.ex +++ b/lib/radiator/outline.ex @@ -21,6 +21,7 @@ defmodule Radiator.Outline do alias Radiator.Outline.Node alias Radiator.Outline.NodeRepoResult alias Radiator.Outline.NodeRepository + alias Radiator.Outline.Validations, as: NodeValidator alias Radiator.Repo require Logger @@ -138,7 +139,12 @@ defmodule Radiator.Outline do node -> parent_node = get_parent_node(node) - case validate_consistency_for_move(node, new_prev_id, new_parent_id, parent_node) do + case NodeValidator.validate_consistency_for_move( + node, + new_prev_id, + new_parent_id, + parent_node + ) do {:error, error} -> {:error, error} @@ -149,39 +155,6 @@ defmodule Radiator.Outline do end end - defp validate_consistency_for_move( - %{prev_id: new_prev_id, parent_id: new_parent_id}, - new_prev_id, - new_parent_id, - _parent_node - ) do - {:error, :noop} - end - - # when prev is nil, every parent is allowed - defp validate_consistency_for_move( - node, - nil, - _new_parent_id, - _parent_node - ) do - {:ok, node} - end - - # 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 - # 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} diff --git a/lib/radiator/outline/dispatch.ex b/lib/radiator/outline/dispatch.ex index 59d5f1f0..711ca622 100644 --- a/lib/radiator/outline/dispatch.ex +++ b/lib/radiator/outline/dispatch.ex @@ -1,8 +1,7 @@ defmodule Radiator.Outline.Dispatch do @moduledoc false - alias Radiator.Outline.Command - alias Radiator.Outline.EventProducer + alias Radiator.Outline.{Command, Event, EventProducer, Validations} def insert_node(attributes, user_id, event_id) do "insert_node" @@ -33,6 +32,13 @@ defmodule Radiator.Outline.Dispatch do end def broadcast(event) do + if Mix.env() == :dev || Mix.env() == :test do + :ok = + event + |> Event.episode_id() + |> Validations.validate_tree_for_episode() + end + Phoenix.PubSub.broadcast(Radiator.PubSub, "events", event) end diff --git a/lib/radiator/outline/event.ex b/lib/radiator/outline/event.ex index f216d57c..948c0d4b 100644 --- a/lib/radiator/outline/event.ex +++ b/lib/radiator/outline/event.ex @@ -10,6 +10,8 @@ defmodule Radiator.Outline.Event do NodeMovedEvent } + alias Radiator.Outline.NodeRepository + def payload(%NodeInsertedEvent{} = event) do %{ node_id: event.node.uuid, @@ -46,4 +48,20 @@ defmodule Radiator.Outline.Event do def event_type(%NodeDeletedEvent{} = _event), do: "NodeDeletedEvent" def event_type(%NodeMovedEvent{} = _event), do: "NodeMovedEvent" + + def episode_id(%NodeInsertedEvent{} = event) do + event.node.episode_id + end + + def episode_id(%NodeContentChangedEvent{} = event) do + NodeRepository.get_node(event.node_id).episode_id + end + + def episode_id(%NodeDeletedEvent{} = event) do + NodeRepository.get_node(event.next_id).episode_id + end + + def episode_id(%NodeMovedEvent{} = event) do + NodeRepository.get_node(event.node_id).episode_id + end end diff --git a/lib/radiator/outline/node.ex b/lib/radiator/outline/node.ex index 0718b7a0..4773ac83 100644 --- a/lib/radiator/outline/node.ex +++ b/lib/radiator/outline/node.ex @@ -15,7 +15,6 @@ defmodule Radiator.Outline.Node do field :parent_id, Ecto.UUID field :prev_id, Ecto.UUID field :level, :integer, virtual: true - field :position, :integer, virtual: true belongs_to :episode, Episode @@ -24,11 +23,8 @@ defmodule Radiator.Outline.Node do @doc """ A changeset for inserting a new node - Work in progress. Since we currently ignore the tree structure, there is - no concept for a root node. - Also questionable wether a node really needs a content from beginning. So probably a root - doesnt have a content - Another issue might be we need to create the uuid upfront and pass it here + A content is not mandatory, + The uuid might be generated upfront """ def insert_changeset(node, attributes) do node diff --git a/lib/radiator/outline/node_repository.ex b/lib/radiator/outline/node_repository.ex index b3ef5bd3..df09983b 100644 --- a/lib/radiator/outline/node_repository.ex +++ b/lib/radiator/outline/node_repository.ex @@ -86,9 +86,9 @@ defmodule Radiator.Outline.NodeRepository do """ def count_nodes_by_episode(episode_id) do - episode_id - |> list_nodes_by_episode() - |> Enum.count() + Node + |> where([p], p.episode_id == ^episode_id) + |> Repo.aggregate(:count) end @doc """ diff --git a/lib/radiator/outline/validations.ex b/lib/radiator/outline/validations.ex new file mode 100644 index 00000000..4f90a3e4 --- /dev/null +++ b/lib/radiator/outline/validations.ex @@ -0,0 +1,104 @@ +defmodule Radiator.Outline.Validations do + @moduledoc """ + Collection of consistency validations for the outline tree. + """ + alias Radiator.Outline + alias Radiator.Outline.Node + alias Radiator.Outline.NodeRepository + + def validate_consistency_for_move( + %{prev_id: new_prev_id, parent_id: new_parent_id}, + new_prev_id, + new_parent_id, + _parent_node + ) do + {:error, :noop} + end + + # when prev is nil, every parent is allowed + def validate_consistency_for_move( + node, + nil, + _new_parent_id, + _parent_node + ) do + {:ok, node} + end + + # when prev is not nil, parent and prev must be consistent + def 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 + + @doc """ + Validates a tree for an episode. + Returns :ok if the tree is valid + """ + def validate_tree_for_episode(episode_id) do + {:ok, tree_nodes} = Outline.get_node_tree(episode_id) + + if Enum.count(tree_nodes) == NodeRepository.count_nodes_by_episode(episode_id) do + validate_tree_nodes(tree_nodes) + else + {:error, :node_count_not_consistent} + end + end + + # iterate through the levels of the tree + # every level has 1 node with prev_id nil + # all other nodes in level have prev_id set and are connected to the previous node + # should be used in dev and test only + # might crash if the tree is not consistent + defp validate_tree_nodes(tree_nodes) do + tree_nodes + |> Enum.group_by(& &1.parent_id) + |> Enum.map(fn {_level, nodes} -> + validate_sub_tree(nodes) + end) + |> Enum.reject(&(&1 == :ok)) + |> first_error() + end + + defp first_error([]), do: :ok + defp first_error([err | _]), do: err + + defp validate_sub_tree(nodes) do + # get the node with prev_id nil + first_node = Enum.find(nodes, &(&1.prev_id == nil)) + # get the rest of the nodes + rest_nodes = Enum.reject(nodes, &(&1.prev_id == nil)) + + if Enum.count(rest_nodes) + 1 != Enum.count(nodes) do + {:error, :prev_id_not_consistent} + else + validate_prev_node(first_node, rest_nodes) + end + end + + def validate_prev_node(node, rest_nodes, searched_nodes \\ []) + + def validate_prev_node( + %Node{uuid: id}, + [%Node{prev_id: id} = node | rest_nodes], + searched_nodes + ) do + validate_prev_node(node, rest_nodes ++ searched_nodes, []) + end + + def validate_prev_node(%Node{}, [], []), do: :ok + + def validate_prev_node(%Node{} = prev_node, [node | rest_nodes], search_nodes) do + validate_prev_node(prev_node, rest_nodes, search_nodes ++ [node]) + end + + def validate_prev_node(%Node{}, [], _search_nodes), do: {:error, :prev_id_not_consistent} +end diff --git a/lib/radiator_web/live/episode_live/index.ex b/lib/radiator_web/live/episode_live/index.ex index a70363c0..66ff235d 100644 --- a/lib/radiator_web/live/episode_live/index.ex +++ b/lib/radiator_web/live/episode_live/index.ex @@ -11,10 +11,8 @@ defmodule RadiatorWeb.EpisodeLive.Index do } alias Radiator.Outline.NodeRepository - # alias Radiator.EventStore alias Radiator.Podcast alias Radiator.Podcast.Episode - alias RadiatorWeb.OutlineComponents @impl true diff --git a/test/radiator/outline/validations_test.exs b/test/radiator/outline/validations_test.exs new file mode 100644 index 00000000..b6e86592 --- /dev/null +++ b/test/radiator/outline/validations_test.exs @@ -0,0 +1,97 @@ +defmodule Radiator.Outline.ValidationsTest do + @moduledoc false + use Radiator.DataCase + + alias Radiator.Outline.Node + alias Radiator.Outline.NodeRepository + alias Radiator.Outline.Validations + + import Ecto.Query, warn: false + + describe "validate_tree_for_episode/1" do + setup :complex_node_fixture + + test "validates a tree", %{ + node_1: %Node{episode_id: episode_id} + } do + assert :ok = Validations.validate_tree_for_episode(episode_id) + end + + test "a level might have different subtrees", %{ + node_1: %Node{episode_id: episode_id} = node_1 + } do + {:ok, %Node{} = _nested_node} = + %{ + episode_id: episode_id, + parent_id: node_1.uuid, + prev_id: nil, + content: "child of node 1" + } + |> NodeRepository.create_node() + + assert :ok = Validations.validate_tree_for_episode(episode_id) + end + + test "when two nodes share the same prev_id the tree is invalid", %{ + node_2: %Node{episode_id: episode_id} = node_2 + } do + {:ok, %Node{} = _node_invalid} = + %{ + episode_id: episode_id, + parent_id: node_2.parent_id, + prev_id: node_2.prev_id + } + |> NodeRepository.create_node() + + assert {:error, :prev_id_not_consistent} = + Validations.validate_tree_for_episode(episode_id) + end + + test "when a nodes has a non connected prev_id the tree is invalid", %{ + node_2: %Node{episode_id: episode_id} = node_2 + } do + {:ok, %Node{} = _node_invalid} = + %{ + episode_id: episode_id, + parent_id: node_2.parent_id, + prev_id: node_2.prev_id + } + |> NodeRepository.create_node() + + assert {:error, :prev_id_not_consistent} = + Validations.validate_tree_for_episode(episode_id) + end + + test "when a parent has two childs with prev_id nil the tree is invalid", %{ + nested_node_1: %Node{episode_id: episode_id, parent_id: parent_id} + } do + {:ok, %Node{} = _node_invalid} = + %{ + episode_id: episode_id, + parent_id: parent_id, + prev_id: nil, + content: "invalid node" + } + |> NodeRepository.create_node() + + assert {:error, :prev_id_not_consistent} = + Validations.validate_tree_for_episode(episode_id) + end + + test "a tree with a node where parent and prev are not consistent is invalid", %{ + parent_node: %Node{episode_id: episode_id} = parent_node, + nested_node_2: nested_node_2 + } do + {:ok, %Node{} = _node_invalid} = + %{ + episode_id: episode_id, + parent_id: parent_node.uuid, + prev_id: nested_node_2.uuid + } + |> NodeRepository.create_node() + + result = Validations.validate_tree_for_episode(episode_id) + assert {:error, :prev_id_not_consistent} = result + end + end +end diff --git a/test/radiator/outline_test.exs b/test/radiator/outline_test.exs index ffe0643e..3602d132 100644 --- a/test/radiator/outline_test.exs +++ b/test/radiator/outline_test.exs @@ -920,133 +920,4 @@ defmodule Radiator.OutlineTest do node = Enum.filter(tree, fn n -> n.uuid == node.uuid end) |> List.first() assert node.level == level end - - defp simple_node_fixture(_) do - episode = PodcastFixtures.episode_fixture() - - node_1 = - node_fixture( - episode_id: episode.id, - parent_id: nil, - prev_id: nil, - content: "node_1" - ) - - node_2 = - node_fixture( - episode_id: episode.id, - parent_id: nil, - prev_id: node_1.uuid, - content: "node_2" - ) - - assert node_2.prev_id == node_1.uuid - assert node_1.prev_id == nil - assert node_1.parent_id == nil - assert node_2.parent_id == nil - - %{ - node_1: node_1, - node_2: node_2 - } - end - - defp complex_node_fixture(_) do - episode = PodcastFixtures.episode_fixture() - - parent_node = - node_fixture( - episode_id: episode.id, - parent_id: nil, - prev_id: nil, - content: "root of all evil" - ) - - node_1 = - node_fixture( - episode_id: episode.id, - parent_id: parent_node.uuid, - prev_id: nil, - content: "node_1" - ) - - node_2 = - node_fixture( - episode_id: episode.id, - parent_id: parent_node.uuid, - prev_id: node_1.uuid, - content: "node_2" - ) - - node_3 = - node_fixture( - episode_id: episode.id, - parent_id: parent_node.uuid, - prev_id: node_2.uuid, - content: "node_3" - ) - - node_4 = - node_fixture( - episode_id: episode.id, - parent_id: parent_node.uuid, - prev_id: node_3.uuid, - content: "node_4" - ) - - node_5 = - node_fixture( - episode_id: episode.id, - parent_id: parent_node.uuid, - prev_id: node_4.uuid, - content: "node_5" - ) - - node_6 = - node_fixture( - episode_id: episode.id, - parent_id: parent_node.uuid, - prev_id: node_5.uuid, - content: "node_6" - ) - - nested_node_1 = - node_fixture( - episode_id: episode.id, - parent_id: node_3.uuid, - prev_id: nil, - content: "nested_node_1" - ) - - nested_node_2 = - node_fixture( - episode_id: episode.id, - parent_id: node_3.uuid, - prev_id: nested_node_1.uuid, - content: "nested_node_2" - ) - - assert node_5.prev_id == node_4.uuid - assert node_4.prev_id == node_3.uuid - assert node_3.prev_id == node_2.uuid - assert node_2.prev_id == node_1.uuid - assert node_1.prev_id == nil - - 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 - - %{ - 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, - parent_node: parent_node - } - end end diff --git a/test/support/data_case.ex b/test/support/data_case.ex index 38036b68..a7e9ee3d 100644 --- a/test/support/data_case.ex +++ b/test/support/data_case.ex @@ -16,6 +16,9 @@ defmodule Radiator.DataCase do use ExUnit.CaseTemplate alias Ecto.Adapters.SQL.Sandbox + alias Radiator.PodcastFixtures + + import Radiator.OutlineFixtures using do quote do @@ -56,4 +59,133 @@ defmodule Radiator.DataCase do end) end) end + + def simple_node_fixture(_) do + episode = PodcastFixtures.episode_fixture() + + node_1 = + node_fixture( + episode_id: episode.id, + parent_id: nil, + prev_id: nil, + content: "node_1" + ) + + node_2 = + node_fixture( + episode_id: episode.id, + parent_id: nil, + prev_id: node_1.uuid, + content: "node_2" + ) + + assert node_2.prev_id == node_1.uuid + assert node_1.prev_id == nil + assert node_1.parent_id == nil + assert node_2.parent_id == nil + + %{ + node_1: node_1, + node_2: node_2 + } + end + + def complex_node_fixture(_) do + episode = PodcastFixtures.episode_fixture() + + parent_node = + node_fixture( + episode_id: episode.id, + parent_id: nil, + prev_id: nil, + content: "root of all evil" + ) + + node_1 = + node_fixture( + episode_id: episode.id, + parent_id: parent_node.uuid, + prev_id: nil, + content: "node_1" + ) + + node_2 = + node_fixture( + episode_id: episode.id, + parent_id: parent_node.uuid, + prev_id: node_1.uuid, + content: "node_2" + ) + + node_3 = + node_fixture( + episode_id: episode.id, + parent_id: parent_node.uuid, + prev_id: node_2.uuid, + content: "node_3" + ) + + node_4 = + node_fixture( + episode_id: episode.id, + parent_id: parent_node.uuid, + prev_id: node_3.uuid, + content: "node_4" + ) + + node_5 = + node_fixture( + episode_id: episode.id, + parent_id: parent_node.uuid, + prev_id: node_4.uuid, + content: "node_5" + ) + + node_6 = + node_fixture( + episode_id: episode.id, + parent_id: parent_node.uuid, + prev_id: node_5.uuid, + content: "node_6" + ) + + nested_node_1 = + node_fixture( + episode_id: episode.id, + parent_id: node_3.uuid, + prev_id: nil, + content: "nested_node_1" + ) + + nested_node_2 = + node_fixture( + episode_id: episode.id, + parent_id: node_3.uuid, + prev_id: nested_node_1.uuid, + content: "nested_node_2" + ) + + assert node_5.prev_id == node_4.uuid + assert node_4.prev_id == node_3.uuid + assert node_3.prev_id == node_2.uuid + assert node_2.prev_id == node_1.uuid + assert node_1.prev_id == nil + + 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 + + %{ + 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, + parent_node: parent_node + } + end end