4464 refactor error handling

bc_obps/common/tests/utils/helpers.py

@@ -3,6 +3,7 @@
 from registration.models import User, AppRole
 import uuid
 from django.db import connection
+from reporting.service.report_validation.report_validation_error import Severity
 
 
 def set_db_user_guid_for_tests():
@@ -84,3 +85,22 @@ def test_audit_column_triggers(self):
                 self.assertIn('set_updated_audit_columns', triggers)
             else:
                 pass
+
+
+def assert_error_response(
+    response,
+    status_code: int,
+    message: str,
+    error_key: str = "generic_error",
+) -> None:
+    assert response.status_code == status_code
+
+    response_json = response.json()
+
+    assert response_json["message"] == message
+
+    error = response_json["errors"][0]
+
+    assert error["key"] == error_key
+    assert error["error"]["severity"] == Severity.ERROR.value
+    assert error["error"]["message"] == message

bc_obps/compliance/tests/api/_bccr/_accounts/test_account_id.py

@@ -4,6 +4,7 @@
 from registration.utils import custom_reverse_lazy
 from compliance.dataclass import BCCRAccountResponseDetails
 from common.exceptions import UserError
+from common.tests.utils.helpers import assert_error_response
 
 # Constants
 VALID_ACCOUNT_ID = "123456789012345"
@@ -78,10 +79,12 @@ def test_service_error_handling(self, mock_permission, mock_service):
         # Act
         response = self.client.get(self._get_endpoint_url(VALID_ACCOUNT_ID, COMPLIANCE_REPORT_VERSION_ID))
         # Assert
-        assert response.status_code == 400
-        assert response.json() == {
-            "message": "The system cannot connect to the external application. Please try again later. If the problem persists, contact GHGRegulator@gov.bc.ca for help."
-        }
+        message = "The system cannot connect to the external application. Please try again later. If the problem persists, contact GHGRegulator@gov.bc.ca for help."
+        assert_error_response(
+            response,
+            status_code=400,
+            message=message,
+        )
 
     @patch(BCCR_SERVICE_PATH)
     @patch(VALIDATE_PERMISSION_PATH)
@@ -117,8 +120,9 @@ def test_account_type_validation_error_wrong_type(self, mock_permission, mock_se
         response = self.client.get(self._get_endpoint_url(VALID_ACCOUNT_ID, COMPLIANCE_REPORT_VERSION_ID))
 
         # Assert
-        assert response.status_code == 400
-        assert (
-            "Account exists but does not match the required account type. Expected account type ID: 11, found: 14"
-            in response.json().get("message")
+        message = "Account exists but does not match the required account type. Expected account type ID: 11, found: 14"
+        assert_error_response(
+            response,
+            status_code=400,
+            message=message,
         )

bc_obps/compliance/tests/api/_bccr/_compliance_report_versions/_compliance_report_version_id/test_applied_compliance_units.py

@@ -3,6 +3,7 @@
 from django.test import SimpleTestCase, override_settings, Client
 from compliance.service.bc_carbon_registry.exceptions import BCCarbonRegistryError
 from registration.utils import custom_reverse_lazy
+from common.tests.utils.helpers import assert_error_response
 
 VALIDATE_PERMISSION_PATH = "common.permissions.validate_all"
 SERVICE_PATH = "compliance.service.bc_carbon_registry.apply_compliance_units_service.ApplyComplianceUnitsService.get_applied_compliance_units_data"
@@ -89,8 +90,10 @@ def test_service_error_handling(self, mock_permission, mock_service):
         # Act
         response = self.client.get(self._get_endpoint_url())
         # Assert
-        assert response.status_code == 400
-        assert response.json() == {
-            "message": "The system cannot connect to the external application. Please try again later. If the problem persists, contact GHGRegulator@gov.bc.ca for help."
-        }
+        message = "The system cannot connect to the external application. Please try again later. If the problem persists, contact GHGRegulator@gov.bc.ca for help."
+        assert_error_response(
+            response,
+            status_code=400,
+            message=message,
+        )
         mock_service.assert_called_once_with(compliance_report_version_id=self.compliance_report_version_id)

bc_obps/compliance/tests/api/_compliance_report_versions/_compliance_report_version_id/test_obligation.py

@@ -5,6 +5,7 @@
 from registration.utils import custom_reverse_lazy
 from compliance.dataclass import ObligationData
 from compliance.models import ElicensingPayment
+from common.tests.utils.helpers import assert_error_response
 
 VALIDATE_VERSION_OWNERSHIP_PATH = "compliance.api.permissions._validate_version_ownership_in_url"
 
@@ -171,8 +172,7 @@ def test_invalid_compliance_report_version_id(self, _):
         )
 
         # Assert
-        assert response.status_code == 404
-        assert response.json() == {'message': 'Not Found'}
+        assert_error_response(response, 404, "Not Found")
 
     @patch(
         "compliance.service.compliance_dashboard_service.ComplianceDashboardService.get_compliance_obligation_payments_by_compliance_report_version_id"
@@ -202,8 +202,7 @@ def test_service_raises_does_not_exist_exception(self, _, mock_get_obligation_da
         )
 
         # Assert
-        assert response.status_code == 404
-        assert response.json() == {'message': 'Not Found'}
+        assert_error_response(response, 404, "Not Found")
 
     @patch(VALIDATE_VERSION_OWNERSHIP_PATH, return_value=True)
     def test_access_to_other_operators_obligation(self, _):
@@ -226,5 +225,4 @@ def test_access_to_other_operators_obligation(self, _):
         )
 
         # Assert
-        assert response.status_code == 404
-        assert response.json() == {'message': 'Not Found'}
+        assert_error_response(response, 404, "Not Found")

bc_obps/compliance/tests/api/_compliance_report_versions/_compliance_report_version_id/test_operation.py

@@ -3,6 +3,7 @@
 from registration.tests.utils.helpers import CommonTestSetup, TestUtils
 from registration.utils import custom_reverse_lazy
 from compliance.tests.utils.compliance_test_helper import ComplianceTestHelper
+from common.tests.utils.helpers import assert_error_response
 
 VALIDATE_VERSION_OWNERSHIP_PATH = "compliance.api.permissions._validate_version_ownership_in_url"
 
@@ -51,5 +52,4 @@ def test_invalid_compliance_report_version_id(self, _):
             self._get_endpoint_url(invalid_compliance_report_version_id),
         )
         # Assert
-        assert response.status_code == 404
-        assert response.json() == {'message': 'Not Found'}
+        assert_error_response(response, 404, "Not Found")

bc_obps/compliance/tests/api/_compliance_report_versions/_compliance_report_version_id/test_penalty.py

@@ -7,6 +7,7 @@
 from registration.tests.utils.helpers import CommonTestSetup, TestUtils
 from registration.utils import custom_reverse_lazy
 from compliance.models import ElicensingPayment
+from common.tests.utils.helpers import assert_error_response
 
 
 class TestPenaltyByComplianceReportVersionEndpoint(CommonTestSetup):
@@ -71,8 +72,7 @@ def test_invalid_compliance_report_version_id(self, _mock_get_summary, _):
             ),
         )
 
-        assert response.status_code == 404
-        assert response.json() == {"message": "Not Found"}
+        assert_error_response(response, 404, "Not Found")
 
 
 class TestLateSubmissionPenaltyByComplianceReportVersionEndpoint(CommonTestSetup):
@@ -136,5 +136,4 @@ def test_invalid_compliance_report_version_id(self, _mock_get_summary, _):
             ),
         )
 
-        assert response.status_code == 404
-        assert response.json() == {"message": "Not Found"}
+        assert_error_response(response, 404, "Not Found")

bc_obps/registration/schema/generic.py

@@ -1,6 +1,18 @@
+from typing import Optional
 from ninja import Schema
 
 
+class ErrorDetail(Schema):
+    severity: str
+    message: str
+
+
+class ErrorItem(Schema):
+    key: str
+    error: ErrorDetail
+
+
 # Generic schemas
 class Message(Schema):
     message: str
+    errors: Optional[list[ErrorItem]] = None

bc_obps/registration/tests/endpoints/test_user_operators.py

@@ -7,6 +7,7 @@
 from registration.tests.utils.bakers import operator_baker
 from registration.tests.utils.helpers import CommonTestSetup, TestUtils
 from registration.utils import custom_reverse_lazy
+from common.tests.utils.helpers import assert_error_response
 
 
 class TestCreateUserOperator(CommonTestSetup):
@@ -72,19 +73,23 @@ def test_duplicates_not_allowed(self):
             "legal_name": operator.legal_name,
         }
         post_response = self._post_with_auth(payload_with_duplicate)
-
-        assert post_response.status_code == status_code
-        assert post_response.json() == {"message": message}
-
+        assert_error_response(
+            post_response,
+            status_code=status_code,
+            message=message,
+        )
         # duplicate CRA business number
         message = "Cra Business Number: Operator with this Cra business number already exists."
         payload_with_duplicate = {
             **self.payload,
             "cra_business_number": operator.cra_business_number,
         }
         post_response = self._post_with_auth(payload_with_duplicate)
-        assert post_response.status_code == status_code
-        assert post_response.json() == {"message": message}
+        assert_error_response(
+            post_response,
+            status_code=status_code,
+            message=message,
+        )
 
         # duplicate Bc Corporate Registry Number
         message = "Bc Corporate Registry Number: Operator with this Bc corporate registry number already exists."
@@ -93,8 +98,11 @@ def test_duplicates_not_allowed(self):
             "bc_corporate_registry_number": operator.bc_corporate_registry_number,
         }
         post_response = self._post_with_auth(payload_with_duplicate)
-        assert post_response.status_code == status_code
-        assert post_response.json() == {"message": message}
+        assert_error_response(
+            post_response,
+            status_code=status_code,
+            message=message,
+        )
 
     def test_payload_required_fields(self):
         """

bc_obps/reporting/api_v2/__init__.py

@@ -5,4 +5,4 @@
 from .forms.report_emission_allocation_data import get_emission_allocation_form_data
 from ._reports._report_id.history import get_report_history
 from .forms.report_compliance_summary_data import get_compliance_summary_form_data
-from .forms.report_validation_data import get_report_validation_data
+from .validation.report_validation_data import get_report_validation_data

bc_obps/reporting/api_v2/response_builder.py

@@ -1,4 +1,4 @@
-from typing import Iterable, Self, override
+from typing import Any, Iterable, Self, override
 
 from django.http import HttpRequest
 from ninja.pagination import PageNumberPagination
@@ -21,6 +21,14 @@ def payload(self, payload: dict) -> Self:
         self.response["payload"] = payload
         return self
 
+    def errors(self, errors: Iterable[Any] | None) -> Self:
+        if errors is None:
+            self.response["errors"] = None
+        else:
+            self.response["errors"] = [error.serialize() if hasattr(error, "serialize") else error for error in errors]
+
+        return self
+
     def build(self) -> dict:
         return self.response
 

bc_obps/reporting/api_v2/forms/report_validation_data.py → bc_obps/reporting/api_v2/validation/report_validation_data.py

@@ -1,26 +1,23 @@
 from typing import Any, Literal, Tuple
 
 from django.http import HttpRequest
-
 from reporting.api.permissions import approved_industry_user_report_version_composite_auth
-from reporting.api_v2.forms.form_response_builder import FormResponseBuilder
-from reporting.api_v2.forms.form_schema import ReportingFormSchema
 from reporting.constants import EMISSIONS_REPORT_TAGS
 from reporting.schema.generic import Message
-from reporting.schema.report_validation_data import ReportValidationPayloadSchema
 from reporting.service.report_validation.report_validation_service import (
     ReportValidationService,
 )
 from service.error_service.custom_codes_4xx import custom_codes_4xx
 from reporting.service.report_validation.report_validation_tags import ValidationTags
+from reporting.schema.report_validation_data import ValidationErrorsSchemaOut
 
 from ..router import router
 
 
 @router.get(
-    "report-version/{version_id}/forms/validation-data",
+    "report-version/{version_id}/validation/validation-data",
     response={
-        200: ReportingFormSchema[ReportValidationPayloadSchema],
+        200: ValidationErrorsSchemaOut,
         custom_codes_4xx: Message,
     },
     tags=EMISSIONS_REPORT_TAGS,
@@ -33,11 +30,13 @@ def get_report_validation_data(
     version_id: int,
 ) -> Tuple[Literal[200], dict[str, Any]]:
     errors = ReportValidationService.validate_report_version(
-        version_id=version_id, tag=ValidationTags.REPORT_VALIDATION
+        version_id=version_id,
+        tag=ValidationTags.REPORT_VALIDATION,
     )
-    payload_errors: list[dict[str, Any]] = []
 
-    for key, error in errors.items():
+    payload: list[dict[str, Any]] = []
+
+    for error in errors.values():
         error_data: dict[str, Any] = {
             "severity": error.severity.value,
             "message": error.message,
@@ -49,15 +48,11 @@ def get_report_validation_data(
                 by_alias=False,
             )
 
-        payload_errors.append(
+        payload.append(
             {
                 "key": getattr(error.key, "value", error.key),
                 "error": error_data,
             }
         )
 
-    payload: dict[str, Any] = {"errors": payload_errors}
-
-    response = FormResponseBuilder(version_id).operation_data().payload(payload).build()
-
-    return 200, response
+    return 200, {"errors": payload}

bc_obps/reporting/schema/report_validation_data.py

@@ -6,7 +6,6 @@ class ErrorContextSchema(Schema):
     report_version_id: int
     facility_id: UUID | None = None
     facility_name: str | None = None
-
     activity_id: int | None = None
     activity_name: str | None = None
     source_type_id: int | None = None
@@ -15,27 +14,25 @@ class ErrorContextSchema(Schema):
     gas_type_name: str | None = None
     methodology_name: str | None = None
     reporting_field: str | None = None
-
     emission_category_id: int | None = None
     emission_category_name: str | None = None
     section: str | None = None
     section_title: str | None = None
     missing_fields: list[str] | None = None
-
     expected_range: str | None = None
     user_input: str | None = None
 
 
-class ReportValidationErrorDetailSchema(Schema):
+class ValidationErrorDetailSchema(Schema):
     severity: str
     message: str
     context: ErrorContextSchema | None = None
 
 
-class ReportValidationItemSchema(Schema):
+class ValidationErrorSchema(Schema):
     key: str
-    error: ReportValidationErrorDetailSchema
+    error: ValidationErrorDetailSchema
 
 
-class ReportValidationPayloadSchema(Schema):
-    errors: list[ReportValidationItemSchema]
+class ValidationErrorsSchemaOut(Schema):
+    errors: list[ValidationErrorSchema]

bc_obps/reporting/tests/api/test_facility_report_api.py

@@ -7,6 +7,7 @@
 
 from reporting.schema.facility_report import FacilityReportListInSchema
 from reporting.tests.utils.report_access_validation import assert_report_version_ownership_is_validated
+from common.tests.utils.helpers import assert_error_response
 
 
 class TestFacilityReportEndpoints(CommonTestSetup):
@@ -21,8 +22,7 @@ def test_error_if_no_facility_report_exists(self):
         endpoint_under_test = f'/api/reporting/report-version/{facility_report.report_version.id}/facility-report/00000000-0000-0000-0000-000000000000'
         response = TestUtils.mock_get_with_auth_role(self, 'cas_admin', endpoint_under_test)
 
-        assert response.status_code == 404
-        assert response.json()["message"] == "Not Found"
+        assert_error_response(response, 404, "Not Found")
 
     def test_error_if_no_invalid_facility_id(self):
         facility_report = baker.make_recipe('reporting.tests.utils.facility_report')