From ab89b0b3a51abba1a936052b48654005efcbb35b Mon Sep 17 00:00:00 2001
From: Daniel Haselhan <daniel.haselhan@quartech.com>
Date: Tue, 17 Dec 2024 13:24:30 -0800
Subject: [PATCH] feat: Add validation rule for other_fuel_type

* Validate that if the fuel type is unrecognized we require other to be populated
* Add new tests
* Fix broken tests in develop
---
 .../compliance_report/test_summary_service.py |  6 +-
 .../test_fuel_supplies_validation.py          | 90 +++++++++++++++++--
 .../internal_comment/test_internal_comment.py |  8 ++
 backend/lcfs/tests/user/test_user_views.py    |  2 +-
 .../lcfs/web/api/fuel_supply/validation.py    | 23 ++++-
 backend/lcfs/web/api/fuel_supply/views.py     |  1 +
 .../lcfs/web/api/internal_comment/services.py |  5 +-
 backend/lcfs/web/core/decorators.py           |  3 +
 8 files changed, 122 insertions(+), 16 deletions(-)

diff --git a/backend/lcfs/tests/compliance_report/test_summary_service.py b/backend/lcfs/tests/compliance_report/test_summary_service.py
index efc5f51ce..30cf918ef 100644
--- a/backend/lcfs/tests/compliance_report/test_summary_service.py
+++ b/backend/lcfs/tests/compliance_report/test_summary_service.py
@@ -810,8 +810,7 @@ async def test_calculate_fuel_quantities_renewable(
 ):
     # Create a mock repository
     mock_repo.aggregate_fuel_supplies.return_value = {"gasoline": 200.0}
-    mock_repo.aggregate_other_uses.return_value = {"diesel": 75.0}
-    mock_repo.aggregate_allocation_agreements.return_value = {"jet-fuel": 25.0}
+    mock_repo.aggregate_other_uses.return_value = {"diesel": 75.0, "jet-fuel": 25.0}
 
     # Define test inputs
     compliance_report_id = 2
@@ -830,7 +829,4 @@ async def test_calculate_fuel_quantities_renewable(
     mock_repo.aggregate_other_uses.assert_awaited_once_with(
         compliance_report_id, fossil_derived
     )
-    mock_repo.aggregate_allocation_agreements.assert_awaited_once_with(
-        compliance_report_id
-    )
     assert result == {"gasoline": 200.0, "diesel": 75.0, "jet-fuel": 25.0}
diff --git a/backend/lcfs/tests/fuel_supply/test_fuel_supplies_validation.py b/backend/lcfs/tests/fuel_supply/test_fuel_supplies_validation.py
index 747416563..eefa25c91 100644
--- a/backend/lcfs/tests/fuel_supply/test_fuel_supplies_validation.py
+++ b/backend/lcfs/tests/fuel_supply/test_fuel_supplies_validation.py
@@ -1,26 +1,30 @@
 from unittest.mock import MagicMock, AsyncMock
-
 import pytest
-from fastapi import Request
+from fastapi.exceptions import RequestValidationError
 
 from lcfs.web.api.fuel_supply.repo import FuelSupplyRepository
+from lcfs.web.api.fuel_code.repo import FuelCodeRepository
 from lcfs.web.api.fuel_supply.schema import FuelSupplyCreateUpdateSchema
 from lcfs.web.api.fuel_supply.validation import FuelSupplyValidation
 
 
 @pytest.fixture
 def fuel_supply_validation():
+    # Mock repositories
     mock_fs_repo = MagicMock(spec=FuelSupplyRepository)
-    request = MagicMock(spec=Request)
+    mock_fc_repo = MagicMock(spec=FuelCodeRepository)
+
+    # Create the validation instance with mocked repositories
     validation = FuelSupplyValidation(
-        request=request, fs_repo=mock_fs_repo
+        fs_repo=mock_fs_repo,
+        fc_repo=mock_fc_repo,
     )
-    return validation, mock_fs_repo
+    return validation, mock_fs_repo, mock_fc_repo
 
 
 @pytest.mark.anyio
 async def test_check_duplicate(fuel_supply_validation):
-    validation, mock_fs_repo = fuel_supply_validation
+    validation, mock_fs_repo, _ = fuel_supply_validation
     fuel_supply_data = FuelSupplyCreateUpdateSchema(
         compliance_report_id=1,
         fuel_type_id=1,
@@ -29,9 +33,83 @@ async def test_check_duplicate(fuel_supply_validation):
         quantity=2000,
         units="L",
     )
+
     mock_fs_repo.check_duplicate = AsyncMock(return_value=True)
 
     result = await validation.check_duplicate(fuel_supply_data)
 
     assert result is True
     mock_fs_repo.check_duplicate.assert_awaited_once_with(fuel_supply_data)
+
+
+@pytest.mark.anyio
+async def test_validate_other_recognized_type(fuel_supply_validation):
+    validation, _, mock_fc_repo = fuel_supply_validation
+    # Mock a recognized fuel type (unrecognized = False)
+    mock_fc_repo.get_fuel_type_by_id = AsyncMock(
+        return_value=MagicMock(unrecognized=False)
+    )
+
+    fuel_supply_data = FuelSupplyCreateUpdateSchema(
+        compliance_report_id=1,
+        fuel_type_id=1,  # Some recognized type ID
+        fuel_category_id=1,
+        provision_of_the_act_id=1,
+        quantity=2000,
+        units="L",
+    )
+
+    # Should not raise any error as fuel_type_other is not needed for recognized type
+    await validation.validate_other(fuel_supply_data)
+
+
+@pytest.mark.anyio
+async def test_validate_other_unrecognized_type_with_other(fuel_supply_validation):
+    validation, _, mock_fc_repo = fuel_supply_validation
+    # Mock an unrecognized fuel type
+    mock_fc_repo.get_fuel_type_by_id = AsyncMock(
+        return_value=MagicMock(unrecognized=True)
+    )
+
+    # Provide fuel_type_other since it's unrecognized
+    fuel_supply_data = FuelSupplyCreateUpdateSchema(
+        compliance_report_id=1,
+        fuel_type_id=99,  # Assume 99 is unrecognized "Other" type
+        fuel_category_id=1,
+        provision_of_the_act_id=1,
+        quantity=2000,
+        units="L",
+        fuel_type_other="Some other fuel",
+    )
+
+    # Should not raise an error since fuel_type_other is provided
+    await validation.validate_other(fuel_supply_data)
+
+
+@pytest.mark.anyio
+async def test_validate_other_unrecognized_type_missing_other(fuel_supply_validation):
+    validation, _, mock_fc_repo = fuel_supply_validation
+    # Mock an unrecognized fuel type
+    mock_fc_repo.get_fuel_type_by_id = AsyncMock(
+        return_value=MagicMock(unrecognized=True)
+    )
+
+    # Missing fuel_type_other
+    fuel_supply_data = FuelSupplyCreateUpdateSchema(
+        compliance_report_id=1,
+        fuel_type_id=99,  # Assume 99 is unrecognized "Other" type
+        fuel_category_id=1,
+        provision_of_the_act_id=1,
+        quantity=2000,
+        units="L",
+    )
+
+    # Should raise RequestValidationError since fuel_type_other is required
+    with pytest.raises(RequestValidationError) as exc:
+        await validation.validate_other(fuel_supply_data)
+
+    # Assert that the error message is as expected
+    errors = exc.value.errors()
+    assert len(errors) == 1
+    assert errors[0]["loc"] == ("fuelTypeOther",)
+    assert "required when using Other" in errors[0]["msg"]
diff --git a/backend/lcfs/tests/internal_comment/test_internal_comment.py b/backend/lcfs/tests/internal_comment/test_internal_comment.py
index dfae4ca5a..046327621 100644
--- a/backend/lcfs/tests/internal_comment/test_internal_comment.py
+++ b/backend/lcfs/tests/internal_comment/test_internal_comment.py
@@ -4,6 +4,7 @@
 from httpx import AsyncClient
 from datetime import datetime
 
+from lcfs.db.models import UserProfile
 from lcfs.db.models.transfer.Transfer import Transfer, TransferRecommendationEnum
 from lcfs.db.models.initiative_agreement.InitiativeAgreement import InitiativeAgreement
 from lcfs.db.models.admin_adjustment.AdminAdjustment import AdminAdjustment
@@ -334,6 +335,13 @@ async def test_get_internal_comments_multiple_comments(
     )
     await add_models([transfer])
 
+    user = UserProfile(
+        keycloak_username="IDIRUSER",
+        first_name="Test",
+        last_name="User",
+    )
+    await add_models([user])
+
     comments = []
     for i in range(3):
         internal_comment = InternalComment(
diff --git a/backend/lcfs/tests/user/test_user_views.py b/backend/lcfs/tests/user/test_user_views.py
index 1c68b120d..24c9e303f 100644
--- a/backend/lcfs/tests/user/test_user_views.py
+++ b/backend/lcfs/tests/user/test_user_views.py
@@ -125,7 +125,7 @@ async def test_get_user_activities_as_manage_users_same_org(
     add_models,
 ):
     # Mock the current user as a user with MANAGE_USERS
-    set_mock_user(fastapi_app, [RoleEnum.MANAGE_USERS])
+    set_mock_user(fastapi_app, [RoleEnum.ADMINISTRATOR, RoleEnum.MANAGE_USERS])
 
     # Assuming target user with user_profile_id=3 exists and is in organization_id=1
     target_user_id = 1
diff --git a/backend/lcfs/web/api/fuel_supply/validation.py b/backend/lcfs/web/api/fuel_supply/validation.py
index bdc2ba2b4..dc065e06a 100644
--- a/backend/lcfs/web/api/fuel_supply/validation.py
+++ b/backend/lcfs/web/api/fuel_supply/validation.py
@@ -1,5 +1,7 @@
-from fastapi import Depends, Request
+from fastapi import Depends
+from fastapi.exceptions import RequestValidationError
 
+from lcfs.web.api.fuel_code.repo import FuelCodeRepository
 from lcfs.web.api.fuel_supply.repo import FuelSupplyRepository
 from lcfs.web.api.fuel_supply.schema import FuelSupplyCreateUpdateSchema
 
@@ -7,11 +9,26 @@
 class FuelSupplyValidation:
     def __init__(
         self,
-        request: Request = None,
         fs_repo: FuelSupplyRepository = Depends(FuelSupplyRepository),
+        fc_repo: FuelCodeRepository = Depends(FuelCodeRepository),
     ):
         self.fs_repo = fs_repo
-        self.request = request
+        self.fc_repo = fc_repo
 
     async def check_duplicate(self, fuel_supply: FuelSupplyCreateUpdateSchema):
         return await self.fs_repo.check_duplicate(fuel_supply)
+
+    async def validate_other(self, fuel_supply: FuelSupplyCreateUpdateSchema):
+        fuel_type = await self.fc_repo.get_fuel_type_by_id(fuel_supply.fuel_type_id)
+
+        if fuel_type.unrecognized:
+            if not fuel_supply.fuel_type_other:
+                raise RequestValidationError(
+                    [
+                        {
+                            "loc": ("fuelTypeOther",),
+                            "msg": "required when using Other",
+                            "type": "value_error",
+                        }
+                    ]
+                )
diff --git a/backend/lcfs/web/api/fuel_supply/views.py b/backend/lcfs/web/api/fuel_supply/views.py
index 52c1b8b82..3f5674408 100644
--- a/backend/lcfs/web/api/fuel_supply/views.py
+++ b/backend/lcfs/web/api/fuel_supply/views.py
@@ -95,6 +95,7 @@ async def save_fuel_supply_row(
         return await action_service.delete_fuel_supply(request_data, current_user_type)
     else:
         duplicate_id = await fs_validate.check_duplicate(request_data)
+        await fs_validate.validate_other(request_data)
         if duplicate_id is not None:
             duplicate_response = format_duplicate_error(duplicate_id)
             return duplicate_response
diff --git a/backend/lcfs/web/api/internal_comment/services.py b/backend/lcfs/web/api/internal_comment/services.py
index 965d03e8e..c051f03cb 100644
--- a/backend/lcfs/web/api/internal_comment/services.py
+++ b/backend/lcfs/web/api/internal_comment/services.py
@@ -73,7 +73,10 @@ async def get_internal_comments(
             List[InternalCommentResponseSchema]: A list of internal comments as data transfer objects.
         """
         comments = await self.repo.get_internal_comments(entity_type, entity_id)
-        return [InternalCommentResponseSchema.from_orm(comment) for comment in comments]
+        return [
+            InternalCommentResponseSchema.model_validate(comment)
+            for comment in comments
+        ]
 
     @service_handler
     async def get_internal_comment_by_id(
diff --git a/backend/lcfs/web/core/decorators.py b/backend/lcfs/web/core/decorators.py
index e67d9afca..0ccfc9a3b 100644
--- a/backend/lcfs/web/core/decorators.py
+++ b/backend/lcfs/web/core/decorators.py
@@ -9,6 +9,7 @@
 import os
 
 from fastapi import HTTPException, Request
+from fastapi.exceptions import RequestValidationError
 
 from lcfs.services.clamav.client import VirusScanException
 from lcfs.web.exception.exceptions import (
@@ -191,6 +192,8 @@ async def wrapper(request: Request, *args, **kwargs):
                     status_code=422,
                     detail="Viruses detected in file, please upload another",
                 )
+            except RequestValidationError as e:
+                raise e
             except Exception as e:
                 context = extract_context()
                 log_unhandled_exception(logger, e, context, "view", func=func)