Merge pull request #832 from OpenTrafficCam/bug/9664-road-user-assignment-highlighting-is-not-correct-when-loading-ottrk-before-otflow

bug/9664-road-user-assignment-highlighting-is-not-correct-when-loading-ottrk-before-otflow

Author: randy-seng

.gitignore

@@ -4,6 +4,9 @@ data/
 tests/data/OTCamera19_FR20_2023-05-24*
 tests/scripts/
 
+# Anthropic Claude
+.claude/rules/shared
+
 # Byte-compiled / optimized / DLL files
 __pycache__/
 *.py[cod]

OTAnalytics/application/application.py

@@ -36,6 +36,9 @@
     EnableFilterTrackByDate,
 )
 from OTAnalytics.application.use_cases.flow_repository import AddFlow
+from OTAnalytics.application.use_cases.flow_repository import (
+    is_flow_name_valid as check_flow_name_valid,
+)
 from OTAnalytics.application.use_cases.flow_statistics import (
     NumberOfTracksAssignedToEachFlow,
 )
@@ -282,7 +285,7 @@ def is_flow_name_valid(self, flow_name: str) -> bool:
         Returns:
             bool: True if a flow with the name already exists, False otherwise.
         """
-        return self._add_flow.is_flow_name_valid(flow_name)
+        return check_flow_name_valid(flow_name, self._datastore.get_all_flows())
 
     def add_flow(self, flow: Flow) -> None:
         self._add_flow(flow)

OTAnalytics/application/use_cases/flow_repository.py

@@ -32,41 +32,41 @@ def __call__(self, flow: Flow) -> None:
         Args:
             flow (Flow): the flow to be added.
         """
-        self.check_flow_already_exists(flow)
+        check_flow_already_exists(flow, self._flow_repository.get_all())
 
         self._flow_repository.add(flow)
 
-    def check_flow_already_exists(self, flow: Flow) -> None:
-        if not self.is_flow_name_valid(flow.name):
-            raise FlowAlreadyExists(
-                f"A flow with the name {flow.name} already exists. "
-                "Choose another name."
-            )
-        if not self.is_flow_id_valid(flow.id):
-            raise FlowIdAlreadyExists(f"A flow with id {flow.id} already exists.")
-
-        if self.flow_with_same_start_end_section_exists(flow):
-            raise FlowAlreadyExists(
-                "Flow with same start and end section already exists."
-            )
-
-    def flow_with_same_start_end_section_exists(self, flow: Flow) -> bool:
-        existing_flows = self._flow_repository.get_all()
-        for existing_flow in existing_flows:
-            if existing_flow.start == flow.start and existing_flow.end == flow.end:
-                return True
-        return False
 
-    def is_flow_name_valid(self, flow_name: str) -> bool:
-        if not flow_name:
-            return False
-        return all(
-            stored_flow.name != flow_name
-            for stored_flow in self._flow_repository.get_all()
+def check_flow_already_exists(given: Flow, existing_flows: Iterable[Flow]) -> None:
+    if not is_flow_name_valid(given.name, existing_flows):
+        raise FlowAlreadyExists(
+            f"A flow with the name {given.name} already exists. " "Choose another name."
         )
+    existing_flow_ids = {flow.id for flow in existing_flows}
+    if not is_flow_id_valid(given.id, existing_flow_ids):
+        raise FlowIdAlreadyExists(f"A flow with id {given.id} already exists.")
+
+    if flow_with_same_start_end_section_exists(given, existing_flows):
+        raise FlowAlreadyExists("Flow with same start and end section already exists.")
+
+
+def flow_with_same_start_end_section_exists(
+    given: Flow, existing_flows: Iterable[Flow]
+) -> bool:
+    for existing_flow in existing_flows:
+        if existing_flow.start == given.start and existing_flow.end == given.end:
+            return True
+    return False
+
+
+def is_flow_name_valid(flow_name: str, existing_flows: Iterable[Flow]) -> bool:
+    if not flow_name:
+        return False
+    return all(stored_flow.name != flow_name for stored_flow in existing_flows)
+
 
-    def is_flow_id_valid(self, flow_id: FlowId) -> bool:
-        return not (flow_id in self._flow_repository.get_flow_ids())
+def is_flow_id_valid(given: FlowId, existing_ids: Iterable[FlowId]) -> bool:
+    return given not in existing_ids
 
 
 class ClearAllFlows:
@@ -93,9 +93,24 @@ def get(self) -> list[Flow]:
 
 
 class AddAllFlows:
-    def __init__(self, add_flow: AddFlow) -> None:
-        self._add_flow = add_flow
+    def __init__(self, flow_repository: FlowRepository) -> None:
+        self._flow_repository = flow_repository
 
     def add(self, flows: Iterable[Flow]) -> None:
-        for flow in flows:
-            self._add_flow(flow)
+        flow_list = list(flows)
+
+        if not flow_list:
+            return
+
+        if not flows_are_unique(flow_list):
+            raise FlowAlreadyExists("Flows to be added are not unique.")
+
+        existing_flows = self._flow_repository.get_all()
+        for flow in flow_list:
+            check_flow_already_exists(flow, existing_flows)
+
+        self._flow_repository.add_all(flow_list)
+
+
+def flows_are_unique(flows: list[Flow]) -> bool:
+    return len(flows) == len(set(flows))

OTAnalytics/application/use_cases/load_otflow.py

@@ -6,7 +6,7 @@
 from OTAnalytics.application.state import ConfigurationFile
 from OTAnalytics.application.use_cases.event_repository import ClearAllEvents
 from OTAnalytics.application.use_cases.flow_repository import (
-    AddFlow,
+    AddAllFlows,
     ClearAllFlows,
     FlowAlreadyExists,
 )
@@ -34,7 +34,7 @@ class LoadOtflow:
         clear_all_events (ClearAllEvents): use case to clear event repository.
         flow_parser (FlowParser): to parse sections and flows from file.
         add_section (AddSection): use case to add sections to section repository.
-        add_flow (AddFlow): use case to add flows to flow repository.
+        add_all_flows (AddAllFlows): use case to batch-add flows to flow repository.
     """
 
     def __init__(
@@ -44,15 +44,15 @@ def __init__(
         clear_all_events: ClearAllEvents,
         flow_parser: FlowParser,
         add_section: AddSection,
-        add_flow: AddFlow,
+        add_all_flows: AddAllFlows,
         deserialize: Deserializer,
     ) -> None:
         self._clear_all_sections = clear_all_sections
         self._clear_all_flows = clear_all_flows
         self._clear_all_events = clear_all_events
         self._flow_parser = flow_parser
         self._add_section = add_section
-        self._add_flow = add_flow
+        self._add_all_flows = add_all_flows
         self._deserialize = deserialize
         self._subject = Subject[ConfigurationFile]()
 
@@ -87,8 +87,7 @@ def _add_sections(self, sections: Iterable[Section]) -> None:
             self._add_section(section)
 
     def _add_flows(self, flows: Iterable[Flow]) -> None:
-        for flow in flows:
-            self._add_flow(flow)
+        self._add_all_flows.add(flows)
 
     def register(self, observer: OBSERVER[ConfigurationFile]) -> None:
         self._subject.register(observer)

OTAnalytics/plugin_ui/base_application.py

@@ -376,7 +376,7 @@ def load_otconfig(self) -> LoadOtconfig:
             self.project_updater,
             AddAllVideos(self.video_repository),
             AddAllSections(self.add_section),
-            AddAllFlows(self.add_flow),
+            self.add_all_flows,
             self.load_track_files,
             self.add_new_remark,
             parse_json,
@@ -491,6 +491,10 @@ def clear_all_flows(self) -> ClearAllFlows:
     def add_flow(self) -> AddFlow:
         return AddFlow(self.flow_repository)
 
+    @cached_property
+    def add_all_flows(self) -> AddAllFlows:
+        return AddAllFlows(self.flow_repository)
+
     @cached_property
     def clear_all_sections(self) -> ClearAllSections:
         return ClearAllSections(self.section_repository)
@@ -802,7 +806,7 @@ def load_otflow(self) -> LoadOtflow:
             self.clear_all_events,
             self.flow_parser,
             self.add_section,
-            self.add_flow,
+            self.add_all_flows,
             parse_json,
         )
 

tests/unit/OTAnalytics/application/use_cases/test_flow_repository.py

@@ -57,7 +57,6 @@ def test_add_flow_with_different_names(
         assert flow_repository.add.call_args_list == [
             call(second_flow),
         ]
-        flow_repository.get_flow_ids.assert_called_once()
 
     def test_add_flow_with_same_names(
         self, flow_repository: Mock, first_flow: Mock
@@ -113,12 +112,65 @@ def test_get(self) -> None:
 
 class TestAddAllFlows:
     def test_add(self, first_flow: Flow, second_flow: Flow) -> None:
-        add_flow = Mock()
-        add_all_flows = AddAllFlows(add_flow)
+        flow_repository = Mock(spec=FlowRepository)
+        flow_repository.get_all.return_value = []
+        add_all_flows = AddAllFlows(flow_repository)
 
         add_all_flows.add([first_flow, second_flow])
 
-        assert add_flow.call_args_list == [
-            call(first_flow),
-            call(second_flow),
-        ]
+        flow_repository.add_all.assert_called_once_with([first_flow, second_flow])
+
+    def test_add_empty_list_does_nothing(self) -> None:
+        flow_repository = Mock(spec=FlowRepository)
+        add_all_flows = AddAllFlows(flow_repository)
+
+        add_all_flows.add([])
+
+        flow_repository.get_all.assert_not_called()
+        flow_repository.add_all.assert_not_called()
+
+    def test_add_raises_when_flows_not_unique(self, first_flow: Flow) -> None:
+        flow_repository = Mock(spec=FlowRepository)
+        add_all_flows = AddAllFlows(flow_repository)
+
+        with pytest.raises(FlowAlreadyExists):
+            add_all_flows.add([first_flow, first_flow])
+
+        flow_repository.add_all.assert_not_called()
+
+    def test_add_raises_when_flow_already_exists_in_repository(
+        self, first_flow: Flow, second_flow: Flow
+    ) -> None:
+        flow_repository = Mock(spec=FlowRepository)
+        flow_repository.get_all.return_value = [first_flow]
+        add_all_flows = AddAllFlows(flow_repository)
+
+        with pytest.raises(FlowAlreadyExists):
+            add_all_flows.add([second_flow, first_flow])
+
+        flow_repository.add_all.assert_not_called()
+
+    def test_add_raises_when_flows_added_are_not_unique(
+        self, first_flow: Flow, second_flow: Flow
+    ) -> None:
+        flow_repository = Mock(spec=FlowRepository)
+        flow_repository.get_all.return_value = [first_flow]
+        add_all_flows = AddAllFlows(flow_repository)
+
+        with pytest.raises(FlowAlreadyExists):
+            add_all_flows.add([first_flow, first_flow])
+
+        flow_repository.add_all.assert_not_called()
+
+    def test_add_able_to_handle_exhaustable_iterables(
+        self, first_flow: Flow, second_flow: Flow
+    ) -> None:
+        flow_repository = Mock(spec=FlowRepository)
+        flow_repository.get_all.return_value = []
+        add_all_flows = AddAllFlows(flow_repository)
+
+        exhaustable_iterable = (_flow for _flow in [first_flow, second_flow])
+
+        add_all_flows.add(exhaustable_iterable)
+
+        flow_repository.add_all.assert_called_once_with([first_flow, second_flow])

tests/unit/OTAnalytics/application/use_cases/test_load_otflow.py

@@ -8,7 +8,7 @@
 from OTAnalytics.application.state import ConfigurationFile
 from OTAnalytics.application.use_cases.event_repository import ClearAllEvents
 from OTAnalytics.application.use_cases.flow_repository import (
-    AddFlow,
+    AddAllFlows,
     ClearAllFlows,
     FlowAlreadyExists,
 )
@@ -31,7 +31,7 @@ class MockDependencies(TypedDict):
     clear_all_events: Mock
     flow_parser: Mock
     add_section: Mock
-    add_flow: Mock
+    add_all_flows: Mock
     deserialize: Mock
 
 
@@ -63,15 +63,15 @@ def mock_deps(
         )
 
         add_section = Mock(spec=AddSection)
-        add_flow = Mock(spec=AddFlow)
+        add_all_flows = Mock(spec=AddAllFlows)
         deserializer = Mock()
         return {
             "clear_all_sections": clear_all_sections,
             "clear_all_flows": clear_all_flows,
             "clear_all_events": clear_all_events,
             "flow_parser": flow_parser,
             "add_section": add_section,
-            "add_flow": add_flow,
+            "add_all_flows": add_all_flows,
             "deserialize": deserializer,
         }
 
@@ -96,7 +96,7 @@ def test_load_flow_file(
             call(mock_first_section),
             call(mock_second_section),
         ]
-        assert mock_deps["add_flow"].call_args_list == [call(mock_flow)]
+        mock_deps["add_all_flows"].add.assert_called_once_with([mock_flow])
         mock_deps["clear_all_events"].assert_called_once()
         mock_deps["clear_all_flows"].assert_called_once()
         mock_deps["clear_all_sections"].assert_called_once()
@@ -117,7 +117,7 @@ def test_load_flow_file_invalid_section_file(
             load_flow_file(otflow_file)
 
         mock_deps["add_section"].assert_called_once_with(mock_first_section)
-        mock_deps["add_flow"].assert_not_called()
+        mock_deps["add_all_flows"].add.assert_not_called()
         assert mock_deps["clear_all_sections"].call_count == 2
         assert mock_deps["clear_all_flows"].call_count == 2
         assert mock_deps["clear_all_events"].call_count == 2
@@ -130,7 +130,7 @@ def test_load_flow_file_invalid_flow_file(
         mock_second_section: Mock,
         mock_flow: Mock,
     ) -> None:
-        mock_deps["add_flow"].side_effect = FlowAlreadyExists
+        mock_deps["add_all_flows"].add.side_effect = FlowAlreadyExists
         load_flow_file = LoadOtflow(**mock_deps)
         observer = Mock()
         load_flow_file.register(observer)
@@ -143,7 +143,7 @@ def test_load_flow_file_invalid_flow_file(
             call(mock_first_section),
             call(mock_second_section),
         ]
-        mock_deps["add_flow"].assert_called_once_with(mock_flow)
+        mock_deps["add_all_flows"].add.assert_called_once_with([mock_flow])
         assert mock_deps["clear_all_sections"].call_count == 2
         assert mock_deps["clear_all_flows"].call_count == 2
         assert mock_deps["clear_all_events"].call_count == 2