refactor: address PR review feedback

chore: cleanup

chore: cleanup

chore: cleanup

chore: cleanup

Author: shon-button

bc_obps/common/migrations/0131_update_dashboard_data.py

@@ -0,0 +1,19 @@
+from django.db import migrations
+
+from common.utils import reset_dashboard_data
+
+
+def reset_dashboard_data_fn(_, schema_editor):
+    reset_dashboard_data()
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("common", "0131_V5_15_1"),
+    ]
+
+    operations = [
+        migrations.RunPython(
+            code=reset_dashboard_data_fn,
+        ),
+    ]

bc_obps/common/migrations/0132_update_dashboard_data.py

@@ -7,7 +7,7 @@
 
 class TestOperationsPreviousReportableEndpoint(CommonTestSetup):
     @patch("service.operation_service.OperationService.list_previous_reportable_operations")
-    def test_list_previous_reportable_operations(
+    def test_calls_and_returns_the_service_method_output(
         self,
         mock_list_previous_reportable_operations,
     ):
@@ -40,11 +40,15 @@ def test_list_previous_reportable_operations(
 
         assert response.status_code == 200
         assert len(response.json()) == 1
-        assert response.json()[0].get("operation_name") == operation.name
-        assert response.json()[0].get("operation_id") == str(operation.id)
-        assert response.json()[0].get("reporting_year") == 2023
-        assert response.json()[0].get("registration_purposes") == [
-            Operation.Purposes.REPORTING_OPERATION,
+        assert response.json() == [
+            {
+                "operation_id": str(operation.id),
+                "operation_name": operation.name,
+                "reporting_year": 2023,
+                "registration_purposes": [
+                    str(Operation.Purposes.REPORTING_OPERATION),
+                ],
+            }
         ]
 
         mock_list_previous_reportable_operations.assert_called_once()

bc_obps/registration/tests/endpoints/_operations/test_reportable.py

@@ -258,4 +258,6 @@ def test_create_report_for_reporting_year(
 
         # Verify that the selected registration purpose was passed to the service
         _, kwargs = mock_create_report_for_reporting_year.call_args
+        assert kwargs["data"].operation_id == operation.id
+        assert kwargs["data"].reporting_year == 2023
         assert kwargs["data"].registration_purpose == Operation.Purposes.REPORTING_OPERATION

bc_obps/reporting/tests/api/test_reports.py

@@ -67,22 +67,21 @@ def get_operation_designated_operator_for_reporting_year(
     def get_operation_designated_operators_for_reporting_years(
         cls,
         operation_ids: set[UUID],
-        reporting_years: set[int],
+        min_year: int,
+        max_year: int,
     ) -> dict[tuple[UUID, int], OperationDesignatedOperatorTimelinePlus]:
         """
-        Bulk version of get_operation_designated_operator_for_reporting_year
+        Bulk version of get_operation_designated_operator_for_reporting_year.
 
-        It is intended for scenarios where many operation/reporting year
-        combinations need to be evaluated, such as determining the list of reportable operations
+        Returns designated operator timelines for every operation/year combination
+        between min_year and max_year, inclusive.
 
-        Returns a lookup keyed by (operation_id, reporting_year)
+        Returns a lookup keyed by (operation_id, reporting_year).
         """
-
-        if not operation_ids or not reporting_years:
+        if not operation_ids:
             return {}
 
-        min_year = min(reporting_years)
-        max_year = max(reporting_years)
+        reporting_years = range(min_year, max_year + 1)
 
         timelines = (
             OperationDesignatedOperatorTimeline.objects.select_related(
@@ -100,24 +99,24 @@ def get_operation_designated_operators_for_reporting_years(
         lookup: dict[tuple[UUID, int], OperationDesignatedOperatorTimelinePlus] = {}
 
         for timeline in timelines:
-            start_date = timeline.start_date
-            end_date = timeline.end_date
-
-            if start_date is None:
+            if timeline.start_date is None:
                 continue
 
+            start_year = timeline.start_date.year
+            end_year = timeline.end_date.year if timeline.end_date else None
+
             for reporting_year in reporting_years:
-                if start_date.year > reporting_year:
+                if start_year > reporting_year:
                     continue
 
-                if end_date is not None and end_date.year <= reporting_year:
+                if end_year is not None and end_year <= reporting_year:
                     continue
 
                 lookup[(timeline.operation_id, reporting_year)] = OperationDesignatedOperatorTimelinePlus(
                     operation=timeline.operation,
                     operator=timeline.operator,
-                    start_date=start_date,
-                    end_date=end_date,
+                    start_date=timeline.start_date,
+                    end_date=timeline.end_date,
                 )
 
         return lookup

bc_obps/service/operation_designated_operator_timeline_service.py

@@ -816,27 +816,16 @@ def _get_registration_purposes_for_operation_type(
         raise ValueError(f"Unsupported operation type: {operation_type}")
 
     @classmethod
-    def _add_reportable_operation(
+    def _build_previous_reportable_operation_row(
         cls,
-        reportable_operations: list[dict],
-        added_operation_years: set[tuple[UUID, int]],
         operation: Operation,
         reporting_year: ReportingYear,
         is_current_registered_fallback: bool,
-    ) -> None:
-        """
-        Adds a reportable operation/reporting year combination to the result set
-        and tracks it to prevent duplicates.
-        """
-        operation_year = (operation.id, reporting_year.reporting_year)
-
-        reportable_operations.append(
-            {
-                **cls._build_reportable_operation_row(operation, reporting_year),
-                "is_current_registered_fallback": is_current_registered_fallback,
-            }
-        )
-        added_operation_years.add(operation_year)
+    ) -> dict:
+        return {
+            **cls._build_reportable_operation_row(operation, reporting_year),
+            "is_current_registered_fallback": is_current_registered_fallback,
+        }
 
     @classmethod
     def _build_reportable_operation_row(
@@ -909,7 +898,8 @@ def list_previous_reportable_operations(
         designations_lookup = (
             OperationDesignatedOperatorTimelineService.get_operation_designated_operators_for_reporting_years(
                 operation_ids=all_operation_ids,
-                reporting_years=year_values,
+                min_year=min(year_values),
+                max_year=max(year_values),
             )
         )
 
@@ -923,7 +913,6 @@ def list_previous_reportable_operations(
                     reporting_year.reporting_year,
                 )
 
-                # Skip combinations that have already have a report
                 if not cls._is_reportable_operation_year(
                     operation_year,
                     added_operation_years,
@@ -933,18 +922,21 @@ def list_previous_reportable_operations(
 
                 designated_operator_timeline = designations_lookup.get(operation_year)
 
-                # Include operations designated to the user's operator for the reporting year
-                if cls._is_designated_to_user_operator(
+                if not cls._is_designated_to_user_operator(
                     designated_operator_timeline,
                     user_operator,
                 ):
-                    cls._add_reportable_operation(
-                        reportable_operations,
-                        added_operation_years,
+                    continue
+
+                reportable_operations = [
+                    *reportable_operations,
+                    cls._build_previous_reportable_operation_row(
                         timeline.operation,
                         reporting_year,
                         False,
-                    )
+                    ),
+                ]
+            added_operation_years = {*added_operation_years, operation_year}
 
         for operation in current_registered_operations:
             for reporting_year in reporting_years:
@@ -953,21 +945,21 @@ def list_previous_reportable_operations(
                     reporting_year.reporting_year,
                 )
 
-                # Skip combinations that have already have a report
                 if not cls._is_reportable_operation_year(
                     operation_year,
                     added_operation_years,
                     existing_reports,
                 ):
                     continue
 
-                # Fall back to the current registered operator when no historical designation applies
-                cls._add_reportable_operation(
-                    reportable_operations,
-                    added_operation_years,
-                    operation,
-                    reporting_year,
-                    True,
-                )
+                reportable_operations = [
+                    *reportable_operations,
+                    cls._build_previous_reportable_operation_row(
+                        operation,
+                        reporting_year,
+                        True,
+                    ),
+                ]
+            added_operation_years = {*added_operation_years, operation_year}
 
         return reportable_operations

bc_obps/service/operation_service.py

@@ -1,7 +1,10 @@
 import pytest
 from django.utils import timezone
 from model_bakery import baker
-from service.operation_designated_operator_timeline_service import OperationDesignatedOperatorTimelineService
+from service.operation_designated_operator_timeline_service import (
+    OperationDesignatedOperatorTimelinePlus,
+    OperationDesignatedOperatorTimelineService,
+)
 
 pytestmark = pytest.mark.django_db
 
@@ -94,59 +97,67 @@ def test_get_operation_designated_operator_for_reporting_year():
         assert result_none is None
 
     @staticmethod
-    def test_get_operation_designated_operators_for_reporting_years():
-        operation = baker.make_recipe('registration.tests.utils.operation')
-        operator1 = baker.make_recipe('registration.tests.utils.operator')
-        operator2 = baker.make_recipe('registration.tests.utils.operator')
+    def test_get_operation_designated_operators_for_reporting_years_returns_matching_timelines():
+        operation = baker.make_recipe("registration.tests.utils.operation")
+        operator1 = baker.make_recipe("registration.tests.utils.operator")
+        operator2 = baker.make_recipe("registration.tests.utils.operator")
 
         timeline1 = baker.make_recipe(
-            'registration.tests.utils.operation_designated_operator_timeline',
+            "registration.tests.utils.operation_designated_operator_timeline",
             operation=operation,
             operator=operator1,
-            start_date=timezone.datetime(2024, 6, 1).date(),
-            end_date=timezone.datetime(2025, 5, 31).date(),
+            start_date=timezone.make_aware(timezone.datetime(2024, 6, 1)),
+            end_date=timezone.make_aware(timezone.datetime(2025, 5, 31)),
         )
         timeline2 = baker.make_recipe(
-            'registration.tests.utils.operation_designated_operator_timeline',
+            "registration.tests.utils.operation_designated_operator_timeline",
             operation=operation,
             operator=operator2,
-            start_date=timezone.datetime(2025, 5, 31).date(),
+            start_date=timezone.make_aware(timezone.datetime(2025, 5, 31)),
             end_date=None,
         )
 
         result = OperationDesignatedOperatorTimelineService.get_operation_designated_operators_for_reporting_years(
             operation_ids={operation.id},
-            reporting_years={2023, 2024, 2025},
+            min_year=2023,
+            max_year=2025,
         )
 
+        assert len(result) == 2
         assert (operation.id, 2023) not in result
 
-        result_2024 = result[(operation.id, 2024)]
-        assert result_2024.operation == timeline1.operation
-        assert result_2024.operator == timeline1.operator
-        assert result_2024.start_date == timeline1.start_date
-        assert result_2024.end_date == timeline1.end_date
-        assert result_2024.has_been_transferred is True
-
-        result_2025 = result[(operation.id, 2025)]
-        assert result_2025.operation == timeline2.operation
-        assert result_2025.operator == timeline2.operator
-        assert result_2025.start_date == timeline2.start_date
-        assert result_2025.end_date == timeline2.end_date
-        assert result_2025.has_been_transferred is False
-
-        empty_result = (
-            OperationDesignatedOperatorTimelineService.get_operation_designated_operators_for_reporting_years(
-                operation_ids=set(),
-                reporting_years={2024, 2025},
-            )
-        )
-        assert empty_result == {}
-
-        empty_result = (
-            OperationDesignatedOperatorTimelineService.get_operation_designated_operators_for_reporting_years(
-                operation_ids={operation.id},
-                reporting_years=set(),
-            )
-        )
-        assert empty_result == {}
+        assert result[(operation.id, 2024)] == OperationDesignatedOperatorTimelinePlus(
+            operation=timeline1.operation,
+            operator=timeline1.operator,
+            start_date=timeline1.start_date,
+            end_date=timeline1.end_date,
+        )
+
+        assert result[(operation.id, 2025)] == OperationDesignatedOperatorTimelinePlus(
+            operation=timeline2.operation,
+            operator=timeline2.operator,
+            start_date=timeline2.start_date,
+            end_date=timeline2.end_date,
+        )
+
+    @staticmethod
+    def test_get_operation_designated_operators_for_reporting_years_returns_empty_when_operation_ids_empty():
+        result = OperationDesignatedOperatorTimelineService.get_operation_designated_operators_for_reporting_years(
+            operation_ids=set(),
+            min_year=2024,
+            max_year=2025,
+        )
+
+        assert result == {}
+
+    @staticmethod
+    def test_get_operation_designated_operators_for_reporting_years_returns_empty_when_year_range_is_empty():
+        operation = baker.make_recipe("registration.tests.utils.operation")
+
+        result = OperationDesignatedOperatorTimelineService.get_operation_designated_operators_for_reporting_years(
+            operation_ids={operation.id},
+            min_year=2025,
+            max_year=2024,
+        )
+
+        assert result == {}