Ticketing PR reviews (jsocol#125)

tests/adapters/test_message_router_ticket_creator.py

@@ -1,4 +1,7 @@
 import pytest
+from requests import Timeout
+from requests.exceptions import ConnectionError  # pylint: disable=redefined-builtin
+from requests.exceptions import RequestException
 
 from tests import common
 from use_case_executor import config
@@ -7,7 +10,9 @@
 )
 from use_case_executor.domain.errors import ImpossibleTicketCreation
 from use_case_executor.domain.flow.node_data.create_ticket_data import SystemField
-from use_case_executor.domain.message_processing.ticket_creator import SystemFieldValue
+from use_case_executor.domain.message_processing.system_field_value import (
+    SystemFieldValue,
+)
 
 
 def test_creates_ticket_and_returns_response(responses):
@@ -17,16 +22,14 @@ def test_creates_ticket_and_returns_response(responses):
         json={"ticket": {"id": 123}},
     )
 
-    result = MessageRouterTicketCreator().create_ticket(
+    MessageRouterTicketCreator().create_ticket(
         system_field_values=[
             SystemFieldValue(
                 system_field=SystemField.ticket_description, value="a value"
             )
         ]
     )
 
-    assert result == {"ticket": {"id": 123}}
-
     common.assert_responses_call(
         call=responses.calls[0],
         method="POST",
@@ -39,11 +42,20 @@ def test_creates_ticket_and_returns_response(responses):
     )
 
 
-def test_raises_if_error(responses):
+@pytest.mark.parametrize(
+    "message_router_mock_params",
+    [
+        {"status": 400},
+        {"body": ConnectionError()},
+        {"body": Timeout()},
+        {"body": RequestException()},
+    ],
+)
+def test_raises_if_error(responses, message_router_mock_params):
     responses.add(
         responses.POST,
         url=f"{config.MESSAGE_ROUTER_URL}/create_ticket",
-        status=400,
+        **message_router_mock_params,
     )
 
     with pytest.raises(ImpossibleTicketCreation):

tests/domain/execute_flow_from_position/test_answer_node_bubbles_and_plain_text_pieces.py

@@ -94,7 +94,7 @@ def test_raises_if_no_text_in_language(
 
     with pytest.raises(
         InvalidFlowConfiguration,
-        match="Empty text for answer in language another_language",
+        match="Empty text for bubble in language another_language",
     ):
         facade.execute_flow_from_position(
             flow=flow,

tests/domain/execute_flow_from_position/test_create_ticket_node.py

@@ -5,10 +5,10 @@
 
 from tests import common
 from use_case_executor.domain import facade
+from use_case_executor.domain.errors import InvalidFlowConfiguration
 from use_case_executor.domain.flow.edge_data.quick_reply_data import QuickReplyData
-from use_case_executor.domain.flow.node_data.bubble import (
-    PlainTextPieceData,
-)
+from use_case_executor.domain.flow.node_data.bubble import Bubble
+from use_case_executor.domain.flow.node_data.bubble import PlainTextPieceData
 from use_case_executor.domain.flow.node_data.bubble import TextPiece
 from use_case_executor.domain.flow.node_data.bubble import (
     TextPieceDataType,
@@ -30,7 +30,9 @@
 from use_case_executor.domain.flow_execution.position import StartPosition
 from use_case_executor.domain.flow_execution.position import SubstepPosition
 from use_case_executor.domain.flow_execution.substeps.substep_names import SubstepName
-from use_case_executor.domain.message_processing.ticket_creator import SystemFieldValue
+from use_case_executor.domain.message_processing.system_field_value import (
+    SystemFieldValue,
+)
 
 
 @pytest.fixture
@@ -141,7 +143,114 @@ def test_creates_ticket_and_returns_success_bubbles(
     )
 
 
-# TODO: discuss with @vincent: the other cases (unknown variable, wrong language, etc) are already tested in
-#       the answer node tests. This seems to mean we should mock out the "render text piece" and "render bubble"
-#       here, and maybe have dedicated tests for them rather than test them in the answer. Or maybe even put them in
-#       the facade ? I'm not sure here, we could also just duplicate the tests but it feels strange
+def test_variables_without_value_are_replaced_by_empty_strings(
+    mocker,
+    mock_repository,
+    mock_ticket_creator,
+    flow,
+    create_ticket_node_uuid,
+    execution_environment,
+):
+    ticket_creation_data: CreateTicketData = flow.get_node(
+        uuid=create_ticket_node_uuid
+    ).data
+    ticket_creation_data.ticket_creation_success_bubbles = [
+        Bubble(
+            delay_seconds=0,
+            templated_text_translations={
+                execution_environment.language: [
+                    TextPiece(
+                        type=TextPieceDataType.variable,
+                        data=VariableTextPieceData(variable_uuid=uuid.uuid4()),
+                    ),
+                    TextPiece(
+                        type=TextPieceDataType.plain_text,
+                        data=PlainTextPieceData(text="text"),
+                    ),
+                ]
+            },
+        )
+    ]
+
+    mock_ticket_creator.set_create_ticket_mock(mocker=mocker, return_value={})
+    mock_repository.set_query_variable_value_return_value(
+        mocker=mocker, return_value=None
+    )
+
+    flow_execution_result = facade.execute_flow_from_position(
+        flow=flow,
+        position=NodePosition(
+            use_case_uuid=execution_environment.use_case_uuid,
+            node_uuid=create_ticket_node_uuid,
+        ),
+        execution_environment=execution_environment,
+    )
+
+    mock_ticket_creator.assert_create_ticket_called_once_with(
+        system_field_values=[
+            SystemFieldValue(system_field=SystemField.client_email, value="Email")
+        ]
+    )
+
+    assert flow_execution_result == FlowExecutionResult(
+        answer=Answer(
+            text="text",
+            buttons=[],
+            tracked_links=[],
+            quick_replies=[],
+            with_csat=False,
+            agent_channel_redirection_uuid=None,
+        ),
+        next_position=StartPosition(),
+        last_executed_node_uuid=create_ticket_node_uuid,
+    )
+
+
+def test_returns_fallback_if_no_success_message_in_language(
+    mocker,
+    mock_repository,
+    mock_ticket_creator,
+    flow,
+    create_ticket_node_uuid,
+    execution_environment,
+):
+    ticket_creation_data: CreateTicketData = flow.get_node(
+        uuid=create_ticket_node_uuid
+    ).data
+    ticket_creation_data.ticket_creation_success_bubbles = [
+        Bubble(
+            delay_seconds=0,
+            templated_text_translations={
+                "another_language": [
+                    TextPiece(
+                        type=TextPieceDataType.plain_text,
+                        data=PlainTextPieceData(text="text"),
+                    )
+                ]
+            },
+        )
+    ]
+
+    mock_ticket_creator.set_create_ticket_mock(mocker=mocker, return_value={})
+    mock_repository.set_query_variable_value_return_value(
+        mocker=mocker, return_value=None
+    )
+
+    with pytest.raises(
+        InvalidFlowConfiguration,
+        match=f"Empty text for bubble in language {execution_environment.language}",
+    ):
+        facade.execute_flow_from_position(
+            flow=flow,
+            position=NodePosition(
+                use_case_uuid=execution_environment.use_case_uuid,
+                node_uuid=create_ticket_node_uuid,
+            ),
+            execution_environment=execution_environment,
+        )
+
+    mock_ticket_creator.assert_create_ticket_called_once_with(
+        system_field_values=[
+            SystemFieldValue(system_field=SystemField.client_email, value="Email")
+        ]
+    )

tests/domain/execute_use_case_and_send_answer/test_variables_validation.py

@@ -1,8 +1,8 @@
 # pylint: disable=redefined-outer-name
+import contextlib
 import dataclasses
 import datetime as dt
 import uuid
-from contextlib import contextmanager
 
 import pytest
 
@@ -30,11 +30,6 @@
 from use_case_executor.domain.use_case.use_case_version import UseCaseVersion
 
 
-@contextmanager
-def does_not_raise():
-    yield
-
-
 @pytest.fixture
 def variable_uuid():
     return uuid.uuid4()
@@ -57,7 +52,7 @@ def flow(content_language):
                 match="The flow to be executed uses variables that are not defined on this instance",
             ),
         ),
-        (True, does_not_raise()),
+        (True, contextlib.nullcontext()),
     ],
 )
 def test_unknown_variable_in_user_input_node_raises_invalid_flow_configuration_error(
@@ -142,7 +137,7 @@ def test_unknown_variable_in_user_input_node_raises_invalid_flow_configuration_e
                 match="The flow to be executed uses variables that are not defined on this instance",
             ),
         ),
-        (True, does_not_raise()),
+        (True, contextlib.nullcontext()),
     ],
 )
 def test_unknown_variable_in_variable_text_piece_data_raises_invalid_flow_configuration_error(
@@ -224,7 +219,7 @@ def test_unknown_variable_in_variable_text_piece_data_raises_invalid_flow_config
                 match="The flow to be executed uses variables that are not defined on this instance",
             ),
         ),
-        (True, does_not_raise()),
+        (True, contextlib.nullcontext()),
     ],
 )
 def test_unknown_variable_in_variable_text_piece_data_in_create_ticket_raises_invalid_flow_configuration_error(
@@ -323,7 +318,7 @@ def test_unknown_variable_in_variable_text_piece_data_in_create_ticket_raises_in
                 match="The flow to be executed uses variables that are not defined on this instance",
             ),
         ),
-        (True, does_not_raise()),
+        (True, contextlib.nullcontext()),
     ],
 )
 def test_unknown_variable_in_variable_text_piece_data_in_create_ticket_success_raises_invalid_flow_configuration_error(

tests/domain/mocks/mock_ticket_creator.py

@@ -1,11 +1,13 @@
 from uuid import UUID
 
-from use_case_executor.domain.message_processing.ticket_creator import SystemFieldValue
+from use_case_executor.domain.message_processing.system_field_value import (
+    SystemFieldValue,
+)
 from use_case_executor.domain.message_processing.ticket_creator import TicketCreator
 
 
 class MockTicketCreator(TicketCreator):
-    def create_ticket(self, system_field_values: list[SystemFieldValue]) -> dict:
+    def create_ticket(self, system_field_values: list[SystemFieldValue]) -> None:
         raise AssertionError("This test method should never be called")
 
     def set_create_ticket_mock(self, mocker, return_value: bool) -> None:

tests/worker/flow_execution/test_answer_node_text.py

@@ -105,4 +105,4 @@ def test_flow_returns_fallback_message_when_answer_text_is_empty(
     )
 
     with caplog.at_level(logging.ERROR):
-        assert f"Empty text for answer in language {content_language}" in caplog.text
+        assert f"Empty text for bubble in language {content_language}" in caplog.text

use_case_executor/adapters/flow_validation/node.py

@@ -90,10 +90,10 @@ def convert_data_from_domain_object(
         for domain_data_type, data_type in [
             (DomainAgentHandoverData, AgentHandoverData),
             (DomainAnswerData, AnswerData),
+            (DomainCreateTicketData, CreateTicketData),
             (DomainEmptyData, EmptyData),
             (DomainGoToUseCaseData, GoToUseCaseData),
             (DomainUserInputData, UserInputData),
-            (DomainCreateTicketData, CreateTicketData),
         ]:
             if isinstance(domain_node_data, domain_data_type):
                 assert data is None

use_case_executor/adapters/flow_validation/node_data/create_ticket_data.py

@@ -33,7 +33,6 @@ def to_domain_object(self) -> DomainSystemFieldValueTemplate:
     def from_domain_object(
         cls, domain_system_field_value_template: DomainSystemFieldValueTemplate
     ) -> "SystemFieldValueTemplate":
-
         return cls(
             system_field=domain_system_field_value_template.system_field,
             value_template=[
@@ -63,7 +62,6 @@ def to_domain_object(self) -> DomainCreateTicketData:
     def from_domain_object(
         cls, domain_node_data: DomainCreateTicketData
     ) -> "CreateTicketData":
-
         return cls(
             system_field_value_templates=[
                 SystemFieldValueTemplate.from_domain_object(field)

use_case_executor/adapters/message_router_ticket_creator.py

@@ -1,14 +1,16 @@
-from requests import HTTPError
+from requests import RequestException
 
 from use_case_executor import config
 from use_case_executor.domain.errors import ImpossibleTicketCreation
-from use_case_executor.domain.message_processing.ticket_creator import SystemFieldValue
+from use_case_executor.domain.message_processing.system_field_value import (
+    SystemFieldValue,
+)
 from use_case_executor.domain.message_processing.ticket_creator import TicketCreator
 from use_case_executor.monitoring import monitored_requests
 
 
 class MessageRouterTicketCreator(TicketCreator):
-    def create_ticket(self, system_field_values: list[SystemFieldValue]) -> dict:
+    def create_ticket(self, system_field_values: list[SystemFieldValue]) -> None:
         try:
             response = monitored_requests.post(
                 url=f"{config.MESSAGE_ROUTER_URL}/create_ticket",
@@ -24,6 +26,5 @@ def create_ticket(self, system_field_values: list[SystemFieldValue]) -> dict:
                 },
             )
             monitored_requests.raise_for_status_with_logging(response)
-        except HTTPError as exc:
+        except RequestException as exc:
             raise ImpossibleTicketCreation("Could not create ticket") from exc
-        return response.json()

use_case_executor/api/namespaces/use_cases/clone_helpers.py

@@ -89,7 +89,7 @@ def create_new_use_case_and_version(
 
 def _create_new_variables_for_flow(
     instance_id: int, flow: DomainFlow, session: Session
-):
+) -> None:
     flow_variable_uuids = variables_flow_visitor.collect_all_variable_uuids_in_flow(
         flow=flow
     )

use_case_executor/domain/flow/node_data/node_type.py

@@ -5,7 +5,7 @@ class NodeType(Enum):
     # The name of the variable is the reference for deserialization
     agent_handover = "agent_handover"
     answer = "answer"
+    create_ticket = "create_ticket"
     empty = "empty"
     go_to_use_case = "go_to_use_case"
     user_input = "user_input"
-    create_ticket = "create_ticket"

use_case_executor/domain/flow_execution/node_executors/node_executor_factory.py

@@ -28,10 +28,10 @@
 _NODE_EXECUTOR_TYPE_PER_NODE_TYPE: dict[NodeType, Type[NodeExecutor]] = {
     NodeType.agent_handover: AgentHandoverNodeExecutor,
     NodeType.answer: AnswerNodeExecutor,
+    NodeType.create_ticket: CreateTicketExecutor,
     NodeType.empty: EmptyNodeExecutor,
     NodeType.go_to_use_case: GoToUseCaseNodeExecutor,
     NodeType.user_input: UserInputNodeExecutor,
-    NodeType.create_ticket: CreateTicketExecutor,
 }
 assert set(_NODE_EXECUTOR_TYPE_PER_NODE_TYPE.keys()) == set(NodeType)