Bugfix issue when nodes were not sorted when saved in a flow (jsocol#151)

* Bugfix issue when nodes were not sorted when saved in a flow

* Upgrade requirements

Author: Wellan89

.pre-commit-config.yaml

@@ -67,7 +67,7 @@ repos:
         additional_dependencies: [types-PyYAML, types-requests]
 
 -   repo: https://github.com/pycqa/pylint
-    rev: v2.14.0-b1
+    rev: v2.14.0
     hooks:
         - id: pylint
           args:

requirements-dev.txt

@@ -1,15 +1,15 @@
 # File generated automatically. Do not update it manually.
 # Update files in the requirements folder instead.
-alembic==1.7.7
+alembic==1.8.0
 anyio==3.6.1
 apispec==5.2.2
 apispec-webframeworks==0.5.2
 asgiref==3.5.2
 astroid==2.11.5
 attrs==21.4.0
 black==22.3.0
-boto3==1.23.10
-botocore==1.26.10
+boto3==1.24.0
+botocore==1.27.0
 callee==0.3.1
 certifi==2022.5.18.1
 cffi==1.15.0
@@ -25,7 +25,7 @@ distlib==0.3.4
 factory-boy==3.2.1
 Faker==13.12.0
 fastapi==0.78.0
-filelock==3.7.0
+filelock==3.7.1
 Flask==2.1.2
 freezegun==1.2.1
 funcy==1.17
@@ -62,7 +62,7 @@ psycopg2-binary==2.9.3
 py==1.11.0
 pycparser==2.21
 pydantic==1.9.1
-pylint==2.13.9
+pylint==2.14.0
 pyparsing==3.0.9
 pytest==7.1.2
 pytest-cases==3.6.13
@@ -78,16 +78,17 @@ pytz==2022.1
 PyYAML==6.0
 requests==2.27.1
 responses==0.21.0
-s3transfer==0.5.2
+s3transfer==0.6.0
 sentry-sdk==1.5.12
 six==1.16.0
 sniffio==1.2.0
-SQLAlchemy==1.4.36
+SQLAlchemy==1.4.37
 sqlalchemy-repr==0.0.2
 starlette==0.19.1
 -e git+ssh://git@github.com/Destygo/pystatsd.git@c755b95da1f4692c49b8191f9ea0ef92f4c13c2c#egg=statsd
 toml==0.10.2
 tomli==2.0.1
+tomlkit==0.11.0
 typeguard==2.13.3
 typing-inspect==0.7.1
 typing_extensions==4.2.0

requirements.txt

@@ -1,10 +1,10 @@
 # File generated automatically. Do not update it manually.
 # Update files in the requirements folder instead.
-alembic==1.7.7
+alembic==1.8.0
 anyio==3.6.1
 asgiref==3.5.2
-boto3==1.23.10
-botocore==1.26.10
+boto3==1.24.0
+botocore==1.27.0
 certifi==2022.5.18.1
 charset-normalizer==2.0.12
 click==8.1.3
@@ -34,11 +34,11 @@ python-dotenv==0.20.0
 python-magic==0.4.26
 python-multipart==0.0.5
 requests==2.27.1
-s3transfer==0.5.2
+s3transfer==0.6.0
 sentry-sdk==1.5.12
 six==1.16.0
 sniffio==1.2.0
-SQLAlchemy==1.4.36
+SQLAlchemy==1.4.37
 sqlalchemy-repr==0.0.2
 starlette==0.19.1
 -e git+ssh://git@github.com/Destygo/pystatsd.git@c755b95da1f4692c49b8191f9ea0ef92f4c13c2c#egg=statsd

tests/adapters/flow_validation/test_flow_validation_success.py

@@ -1,13 +1,48 @@
 # pylint: disable=redefined-outer-name
+from random import Random
+from uuid import UUID
+
+import pytest
+import pytest_cases
+
 from tests.adapters.flow_validation import helpers
-from tests.fixtures.valid_flow_json import *  # noqa: F401, F403, pylint: disable=wildcard-import,unused-wildcard-import
+from tests.fixtures.valid_flow_json import *  # noqa: F403, pylint: disable=wildcard-import,unused-wildcard-import
 from use_case_executor.adapters.flow_validation.flow import Flow
 from use_case_executor.domain.flow.edge_data.edge_type import EdgeType
 from use_case_executor.domain.flow.node_data.answer_data.answer_data import ButtonType
 
 
-def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
-    *,
+def _shuffled(objects: list, random_instance: Random) -> list:
+    objects = list(objects)
+    random_instance.shuffle(objects)
+    return objects
+
+
+@pytest.fixture
+def shuffled_reference_flow(reference_flow: dict) -> dict:
+    random_instance = Random(123)
+    return {
+        **reference_flow,
+        "nodes": _shuffled(reference_flow["nodes"], random_instance=random_instance),
+        "edges": _shuffled(reference_flow["edges"], random_instance=random_instance),
+    }
+
+
+@pytest.fixture
+def inverted_reference_flow(reference_flow: dict) -> dict:
+    return {
+        **reference_flow,
+        "nodes": reference_flow["nodes"][::-1],
+        "edges": reference_flow["edges"][::-1],
+    }
+
+
+@pytest_cases.parametrize(
+    "flow_dict",
+    [reference_flow, shuffled_reference_flow, inverted_reference_flow],  # noqa: F405
+)
+@pytest.mark.parametrize("convert_into_domain_object", [False, True])
+def test_working_flow_is_loaded(  # pylint: disable=too-many-locals,too-many-arguments
     reference_flow,
     start_node_uuid,
     button_uuid,
@@ -33,16 +68,20 @@ def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
     edge_to_second_user_input_uuid,
     edge_to_create_ticket_uuid,
     edge_to_empty_node_uuid,
+    flow_dict,
+    convert_into_domain_object,
 ):
-    flow = Flow.load_flow(reference_flow)
+    flow = Flow.load_flow(flow_dict)
+    if convert_into_domain_object:
+        flow = Flow.from_domain_object(domain_flow=flow.to_domain_object())
+
     assert flow.schema_version == "v0.2"
     assert flow.start_node_uuid == start_node_uuid
 
-    nodes = flow.nodes
-    assert len(nodes) == 10
-
+    assert len(flow.nodes) == 10
+    nodes_per_uuid = {node.uuid: node for node in flow.nodes}
     helpers.check_answer_node(
-        node=nodes[0],
+        node=nodes_per_uuid[UUID(reference_flow["nodes"][0]["uuid"])],
         node_uuid=start_node_uuid,
         bubbles=reference_flow["nodes"][0]["data"]["bubbles"],
         buttons=[
@@ -66,17 +105,17 @@ def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
         is_deflection=True,
     )
     helpers.check_go_to_flow_node(
-        node=nodes[1],
+        node=nodes_per_uuid[UUID(reference_flow["nodes"][1]["uuid"])],
         node_uuid=go_to_flow_uuid,
         target_use_case_uuid=target_use_case_uuid,
     )
     helpers.check_go_to_advanced_node(
-        node=nodes[2],
+        node=nodes_per_uuid[UUID(reference_flow["nodes"][2]["uuid"])],
         node_uuid=go_to_advanced_uuid,
         target_intent_record_id=target_intent_record_id,
     )
     helpers.check_answer_node(
-        node=nodes[3],
+        node=nodes_per_uuid[UUID(reference_flow["nodes"][3]["uuid"])],
         node_uuid=second_answer_node_uuid,
         bubbles=reference_flow["nodes"][3]["data"]["bubbles"],
         buttons=[],
@@ -86,23 +125,23 @@ def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
         is_deflection=False,
     )
     helpers.check_agent_handover_node(
-        node=nodes[4],
+        node=nodes_per_uuid[UUID(reference_flow["nodes"][4]["uuid"])],
         node_uuid=agent_handover_uuid,
         agent_channel_uuid=agent_handover_channel_uuid,
         handover_success_message_translations={"en_US": "Handover success text"},
     )
     helpers.check_user_input_node(
-        node=nodes[5],
+        node=nodes_per_uuid[UUID(reference_flow["nodes"][5]["uuid"])],
         node_uuid=user_input_uuid,
         node_data_dict=reference_flow["nodes"][5]["data"],
     )
     helpers.check_user_input_node(
-        node=nodes[6],
+        node=nodes_per_uuid[UUID(reference_flow["nodes"][6]["uuid"])],
         node_uuid=second_user_input_uuid,
         node_data_dict=reference_flow["nodes"][6]["data"],
     )
     helpers.check_answer_node(
-        node=nodes[7],
+        node=nodes_per_uuid[UUID(reference_flow["nodes"][7]["uuid"])],
         node_uuid=third_answer_node_uuid,
         bubbles=reference_flow["nodes"][7]["data"]["bubbles"],
         buttons=[],
@@ -111,16 +150,20 @@ def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
         has_go_back_quick_reply=True,
         is_deflection=False,
     )
-    helpers.check_empty_node(node=nodes[8], node_uuid=empty_node_uuid)
+    helpers.check_empty_node(
+        node=nodes_per_uuid[UUID(reference_flow["nodes"][8]["uuid"])],
+        node_uuid=empty_node_uuid,
+    )
     helpers.check_create_ticket_node(
-        node=nodes[9],
+        node=nodes_per_uuid[UUID(reference_flow["nodes"][9]["uuid"])],
         node_uuid=create_ticket_node_uuid,
         node_data_dict=reference_flow["nodes"][9]["data"],
     )
 
     assert len(flow.edges) == 9
+    edges_per_uuid = {edge.data.uuid: edge for edge in flow.edges}
     helpers.check_quick_reply_edge(
-        edge=flow.edges[0],
+        edge=edges_per_uuid[UUID(reference_flow["edges"][0]["data"]["uuid"])],
         source=start_node_uuid,
         target=go_to_flow_uuid,
         data={
@@ -133,7 +176,7 @@ def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
         },
     )
     helpers.check_quick_reply_edge(
-        edge=flow.edges[1],
+        edge=edges_per_uuid[UUID(reference_flow["edges"][1]["data"]["uuid"])],
         source=start_node_uuid,
         target=go_to_advanced_uuid,
         data={
@@ -146,7 +189,7 @@ def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
         },
     )
     helpers.check_quick_reply_edge(
-        edge=flow.edges[2],
+        edge=edges_per_uuid[UUID(reference_flow["edges"][2]["data"]["uuid"])],
         source=start_node_uuid,
         target=second_answer_node_uuid,
         data={
@@ -159,7 +202,7 @@ def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
         },
     )
     helpers.check_quick_reply_edge(
-        edge=flow.edges[3],
+        edge=edges_per_uuid[UUID(reference_flow["edges"][3]["data"]["uuid"])],
         source=start_node_uuid,
         target=agent_handover_uuid,
         data={
@@ -170,7 +213,7 @@ def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
         },
     )
     helpers.check_quick_reply_edge(
-        edge=flow.edges[4],
+        edge=edges_per_uuid[UUID(reference_flow["edges"][4]["data"]["uuid"])],
         source=start_node_uuid,
         target=user_input_uuid,
         data={
@@ -181,7 +224,7 @@ def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
         },
     )
     helpers.check_quick_reply_edge(
-        edge=flow.edges[5],
+        edge=edges_per_uuid[UUID(reference_flow["edges"][5]["data"]["uuid"])],
         source=start_node_uuid,
         target=second_user_input_uuid,
         data={
@@ -194,7 +237,7 @@ def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
         },
     )
     helpers.check_quick_reply_edge(
-        edge=flow.edges[6],
+        edge=edges_per_uuid[UUID(reference_flow["edges"][6]["data"]["uuid"])],
         source=start_node_uuid,
         target=empty_node_uuid,
         data={
@@ -205,7 +248,7 @@ def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
         },
     )
     helpers.check_quick_reply_edge(
-        edge=flow.edges[7],
+        edge=edges_per_uuid[UUID(reference_flow["edges"][7]["data"]["uuid"])],
         source=start_node_uuid,
         target=create_ticket_node_uuid,
         data={
@@ -216,7 +259,7 @@ def test_working_flow_is_loaded(  # pylint: disable=too-many-locals
         },
     )
     helpers.check_direct_link_edge(
-        flow.edges[8],
+        edge=edges_per_uuid[UUID(reference_flow["edges"][8]["data"]["uuid"])],
         source=user_input_uuid,
         target=third_answer_node_uuid,
         data={"type": EdgeType.direct_link, "uuid": edge_to_third_answer_uuid},

use_case_executor/adapters/flow_validation/flow.py

@@ -3,7 +3,6 @@
 from typing import Any
 from uuid import UUID
 
-import funcy
 import marshmallow_dataclass
 import more_itertools
 from marshmallow import Schema
@@ -222,27 +221,19 @@ def to_domain_object(self) -> DomainFlow:
             start_node_type=start_node.type,
             start_node_data=start_node.data.to_domain_object(),
         )
-
-        # As each node except the start node must have exactly one parent,
-        # then iterating on all nodes and creating their parent edge for each of them
-        # is enough to create all nodes and all edges in the domain flow.
-        incoming_edges_per_node_uuid: dict[UUID, list[Edge]] = funcy.group_by(
-            lambda edge: edge.target, self.edges
-        )
         for node in self.nodes:
-            if node.uuid == start_node.uuid:
-                continue
-
-            incoming_edges = incoming_edges_per_node_uuid.get(node.uuid, [])
-            incoming_edge: Edge = more_itertools.one(incoming_edges)
-            domain_flow.add_node(
-                parent_node=domain_flow.get_node(uuid=incoming_edge.source),
-                node_uuid=node.uuid,
-                node_type=node.type,
-                node_data=node.data.to_domain_object(),
-                incoming_edge_data=incoming_edge.data.to_domain_object(),
+            if node.uuid != start_node.uuid:
+                domain_flow.add_orphan_node(
+                    node_uuid=node.uuid,
+                    node_type=node.type,
+                    node_data=node.data.to_domain_object(),
+                )
+        for edge in self.edges:
+            domain_flow.assign_parent_to_orphan_node(
+                node=domain_flow.get_node(uuid=edge.target),
+                parent_node=domain_flow.get_node(uuid=edge.source),
+                incoming_edge_data=edge.data.to_domain_object(),
             )
-
         return domain_flow
 
     @classmethod

use_case_executor/domain/flow/flow.py

@@ -57,6 +57,32 @@ def add_node(
         parent_node.add_edge(data=incoming_edge_data, target_node=node)
         return node
 
+    def add_orphan_node(
+        self,
+        node_uuid: UUID,
+        node_type: NodeType,
+        node_data: NodeData,
+    ) -> Node:
+        assert node_uuid not in self._node_per_uuid
+        node = Node(
+            uuid=node_uuid,
+            type=node_type,
+            data=node_data,
+            parent_node_uuid=None,
+            outgoing_edges=[],
+        )
+        self._node_per_uuid[node_uuid] = node
+        return node
+
+    def assign_parent_to_orphan_node(
+        self, node: Node, parent_node: Node, incoming_edge_data: EdgeData
+    ) -> None:
+        assert node.uuid in self._node_per_uuid
+        assert parent_node.uuid in self._node_per_uuid
+        assert node.parent_node_uuid is None
+        node.parent_node_uuid = parent_node.uuid
+        parent_node.add_edge(data=incoming_edge_data, target_node=node)
+
     def get_nodes(self) -> list[Node]:
         return list(self._node_per_uuid.values())