From ec65eb2c52fe40da4cb54621dd5a49c5f82dde8b Mon Sep 17 00:00:00 2001 From: Hamed Valiollahi Bayeki Date: Tue, 17 Dec 2024 13:17:56 -0800 Subject: [PATCH 1/6] fix: ensure fuel code assignment and prevent field clearing on prefix change --- .../components/BCDataGrid/BCGridEditor.jsx | 155 ++++++++++++------ .../FuelCodes/AddFuelCode/AddEditFuelCode.jsx | 128 ++++++++++----- .../views/FuelCodes/AddFuelCode/_schema.jsx | 19 +-- 3 files changed, 193 insertions(+), 109 deletions(-) diff --git a/frontend/src/components/BCDataGrid/BCGridEditor.jsx b/frontend/src/components/BCDataGrid/BCGridEditor.jsx index cdc02be64..03c9b4649 100644 --- a/frontend/src/components/BCDataGrid/BCGridEditor.jsx +++ b/frontend/src/components/BCDataGrid/BCGridEditor.jsx @@ -27,6 +27,7 @@ import { BCAlert2 } from '@/components/BCAlert' * @property {React.Ref} gridRef * @property {Function} handlePaste * @property {Function} onAction + * @property {Function} onAddRows * * @param {BCGridEditorProps & GridOptions} props * @returns {JSX.Element} @@ -44,6 +45,7 @@ export const BCGridEditor = ({ saveButtonProps = { enabled: false }, + onAddRows, ...props }) => { const localRef = useRef(null) @@ -59,32 +61,36 @@ export const BCGridEditor = ({ if (!firstEditableColumnRef.current) { const columns = ref.current.api.getAllDisplayedColumns() - firstEditableColumnRef.current = columns.find(col => - col.colDef.editable !== false && - !['action', 'checkbox'].includes(col.colDef.field) + firstEditableColumnRef.current = columns.find( + (col) => + col.colDef.editable !== false && + !['action', 'checkbox'].includes(col.colDef.field) ) } return firstEditableColumnRef.current }, []) // Helper function to start editing first editable cell in a row - const startEditingFirstEditableCell = useCallback((rowIndex) => { - if (!ref.current?.api) return + const startEditingFirstEditableCell = useCallback( + (rowIndex) => { + if (!ref.current?.api) return - // Ensure we have the first editable column - const firstEditableColumn = findFirstEditableColumn() - if (!firstEditableColumn) return + // Ensure we have the first editable column + const firstEditableColumn = findFirstEditableColumn() + if (!firstEditableColumn) return - // Use setTimeout to ensure the grid is ready - setTimeout(() => { - ref.current.api.ensureIndexVisible(rowIndex) - ref.current.api.setFocusedCell(rowIndex, firstEditableColumn.getColId()) - ref.current.api.startEditingCell({ - rowIndex, - colKey: firstEditableColumn.getColId() - }) - }, 100) - }, [findFirstEditableColumn]) + // Use setTimeout to ensure the grid is ready + setTimeout(() => { + ref.current.api.ensureIndexVisible(rowIndex) + ref.current.api.setFocusedCell(rowIndex, firstEditableColumn.getColId()) + ref.current.api.startEditingCell({ + rowIndex, + colKey: firstEditableColumn.getColId() + }) + }, 100) + }, + [findFirstEditableColumn] + ) const handleExcelPaste = useCallback( (params) => { @@ -175,11 +181,18 @@ export const BCGridEditor = ({ params.event.target.dataset.action && onAction ) { - const transaction = await onAction(params.event.target.dataset.action, params) - // Focus and edit the first editable column of the duplicated row - if (transaction?.add.length > 0) { - const duplicatedRowNode = transaction.add[0] - startEditingFirstEditableCell(duplicatedRowNode.rowIndex) + const action = params.event.target.dataset.action + const transaction = await onAction(action, params) + + // Apply the transaction if it exists + if (transaction?.add?.length > 0) { + const res = ref.current.api.applyTransaction(transaction) + + // Focus and edit the first editable column of the added rows + if (res.add && res.add.length > 0) { + const firstNewRow = res.add[0] + startEditingFirstEditableCell(firstNewRow.rowIndex) + } } } } @@ -192,28 +205,45 @@ export const BCGridEditor = ({ setAnchorEl(null) } - const handleAddRows = useCallback((numRows) => { - let newRows = [] - if (props.onAddRows) { - newRows = props.onAddRows(numRows) - } else { - newRows = Array(numRows) - .fill() - .map(() => ({ id: uuid() })) - } + const handleAddRowsInternal = useCallback( + async (numRows) => { + let newRows = [] - // Add the new rows - ref.current.api.applyTransaction({ - add: newRows, - addIndex: ref.current.api.getDisplayedRowCount() - }) + if (onAction) { + try { + for (let i = 0; i < numRows; i++) { + const transaction = await onAction('add') + if (transaction?.add?.length > 0) { + newRows = [...newRows, ...transaction.add] + } + } + } catch (error) { + console.error('Error during onAction add:', error) + } + } - // Focus and start editing the first new row - const firstNewRowIndex = ref.current.api.getDisplayedRowCount() - numRows - startEditingFirstEditableCell(firstNewRowIndex) + // Default logic if onAction doesn't return rows + if (newRows.length === 0) { + newRows = Array(numRows) + .fill() + .map(() => ({ id: uuid() })) + } - setAnchorEl(null) - }, [props.onAddRows, startEditingFirstEditableCell]) + // Apply the new rows to the grid + const result = ref.current.api.applyTransaction({ + add: newRows, + addIndex: ref.current.api.getDisplayedRowCount() + }) + + // Focus the first editable cell in the first new row + if (result.add && result.add.length > 0) { + startEditingFirstEditableCell(result.add[0].rowIndex) + } + + setAnchorEl(null) + }, + [onAction, startEditingFirstEditableCell] + ) const isGridValid = () => { let isValid = true @@ -238,24 +268,25 @@ export const BCGridEditor = ({ setShowCloseModal(true) } const hasRequiredHeaderComponent = useCallback(() => { - const columnDefs = ref.current?.api?.getColumnDefs() || []; + const columnDefs = ref.current?.api?.getColumnDefs() || [] // Check if any column has `headerComponent` matching "RequiredHeader" - return columnDefs.some( - colDef => colDef.headerComponent?.name === 'RequiredHeader' - ) || columnDefs.some(colDef => !!colDef.headerComponent) + return ( + columnDefs.some( + (colDef) => colDef.headerComponent?.name === 'RequiredHeader' + ) || columnDefs.some((colDef) => !!colDef.headerComponent) + ) }, [ref]) - return ( - {hasRequiredHeaderComponent() && + {hasRequiredHeaderComponent() && ( - } + )} ) } - onClick={addMultiRow ? handleAddRowsClick : () => handleAddRows(1)} + onClick={ + addMultiRow ? handleAddRowsClick : () => handleAddRowsInternal(1) + } > Add row @@ -301,9 +334,15 @@ export const BCGridEditor = ({ } }} > - handleAddRows(1)}>1 row - handleAddRows(5)}>5 rows - handleAddRows(10)}>10 rows + handleAddRowsInternal(1)}> + 1 row + + handleAddRowsInternal(5)}> + 5 rows + + handleAddRowsInternal(10)}> + 10 rows + )} @@ -345,8 +384,16 @@ BCGridEditor.propTypes = { alertRef: PropTypes.shape({ current: PropTypes.any }), handlePaste: PropTypes.func, onAction: PropTypes.func, + onAddRows: PropTypes.func, onRowEditingStopped: PropTypes.func, onCellValueChanged: PropTypes.func, showAddRowsButton: PropTypes.bool, - onAddRows: PropTypes.func + addMultiRow: PropTypes.bool, + saveButtonProps: PropTypes.shape({ + enabled: PropTypes.bool, + text: PropTypes.string, + onSave: PropTypes.func, + confirmText: PropTypes.string, + confirmLabel: PropTypes.string + }) } diff --git a/frontend/src/views/FuelCodes/AddFuelCode/AddEditFuelCode.jsx b/frontend/src/views/FuelCodes/AddFuelCode/AddEditFuelCode.jsx index c06a73a7b..ece671487 100644 --- a/frontend/src/views/FuelCodes/AddFuelCode/AddEditFuelCode.jsx +++ b/frontend/src/views/FuelCodes/AddFuelCode/AddEditFuelCode.jsx @@ -38,9 +38,15 @@ const AddEditFuelCodeBase = () => { const [columnDefs, setColumnDefs] = useState([]) const [isGridReady, setGridReady] = useState(false) const [modalData, setModalData] = useState(null) + const [initialized, setInitialized] = useState(false) const { hasRoles } = useCurrentUser() - const { data: optionsData, isLoading, isFetched } = useFuelCodeOptions() + const { + data: optionsData, + isLoading, + isFetched, + refetch: refetchOptions + } = useFuelCodeOptions() const { mutateAsync: updateFuelCode } = useUpdateFuelCode(fuelCodeID) const { mutateAsync: createFuelCode } = useCreateFuelCode() const { mutateAsync: deleteFuelCode } = useDeleteFuelCode() @@ -51,6 +57,35 @@ const AddEditFuelCodeBase = () => { refetch } = useGetFuelCode(fuelCodeID) + useEffect(() => { + // Only initialize rowData once when all data is available and the grid is ready + if (!initialized && isFetched && !isLoadingExistingCode && isGridReady) { + if (existingFuelCode) { + setRowData([existingFuelCode]) + } else { + const defaultPrefix = optionsData?.fuelCodePrefixes?.find( + (item) => item.prefix === 'BCLCF' + ) + setRowData([ + { + id: uuid(), + prefixId: defaultPrefix?.fuelCodePrefixId || 1, + prefix: defaultPrefix?.prefix || 'BCLCF', + fuelSuffix: defaultPrefix?.nextFuelCode + } + ]) + } + setInitialized(true) + } + }, [ + initialized, + isFetched, + isLoadingExistingCode, + isGridReady, + existingFuelCode, + optionsData + ]) + useEffect(() => { if (optionsData) { const updatedColumnDefs = fuelCodeColDefs( @@ -62,23 +97,7 @@ const AddEditFuelCodeBase = () => { ) setColumnDefs(updatedColumnDefs) } - }, [errors, optionsData, existingFuelCode]) - - useEffect(() => { - if (existingFuelCode) { - setRowData([existingFuelCode]) - } else { - setRowData([ - { - id: uuid(), - prefixId: 1, - fuelSuffix: optionsData?.fuelCodePrefixes?.find( - (item) => item.prefix === 'BCLCF' - ).nextFuelCode - } - ]) - } - }, [optionsData, existingFuelCode, isGridReady]) + }, [errors, optionsData, existingFuelCode, hasRoles]) const onGridReady = (params) => { setGridReady(true) @@ -100,7 +119,7 @@ const AddEditFuelCodeBase = () => { if (params.colDef.field === 'prefix') { updatedData.fuelSuffix = optionsData?.fuelCodePrefixes?.find( (item) => item.prefix === params.newValue - ).nextFuelCode + )?.nextFuelCode } params.api.applyTransaction({ update: [updatedData] }) @@ -213,8 +232,8 @@ const AddEditFuelCodeBase = () => { } catch (error) { setErrors({ [params.node.data.id]: - error.response.data?.errors && - error.response.data?.errors[0]?.fields + error.response?.data?.errors && + error.response.data.errors[0]?.fields }) updatedData = { @@ -244,7 +263,7 @@ const AddEditFuelCodeBase = () => { params.node.updateData(updatedData) }, - [updateFuelCode, t] + [updateFuelCode, t, createFuelCode] ) const handlePaste = useCallback( @@ -311,8 +330,16 @@ const AddEditFuelCodeBase = () => { ) const duplicateFuelCode = async (params) => { - const rowData = { - ...params.data, + const originalData = params.data + const originalPrefix = originalData.prefix || 'BCLCF' + + const updatedOptions = await refetchOptions() + const selectedPrefix = updatedOptions.data.fuelCodePrefixes?.find( + (p) => p.prefix === originalPrefix + ) + + const newRow = { + ...originalData, id: uuid(), fuelCodeId: null, modified: true, @@ -320,10 +347,18 @@ const AddEditFuelCodeBase = () => { validationStatus: 'error', validationMsg: 'Fill in the missing fields' } + + if (selectedPrefix) { + newRow.prefixId = selectedPrefix.fuelCodePrefixId + newRow.prefix = selectedPrefix.prefix + newRow.fuelSuffix = selectedPrefix.nextFuelCode + } + if (params.api) { - if (params.data.fuelCodeId) { + if (originalData.fuelCodeId) { try { - const response = await updateFuelCode(rowData) + // If the original was a saved row, create a new code in the backend + const response = await createFuelCode(newRow) const updatedData = { ...response.data, id: uuid(), @@ -331,23 +366,13 @@ const AddEditFuelCodeBase = () => { isValid: false, validationStatus: 'error' } - params.api.applyTransaction({ - add: [updatedData], - addIndex: params.node?.rowIndex + 1 - }) - params.api.refreshCells() - alertRef.current?.triggerAlert({ - message: 'Row duplicated successfully.', - severity: 'success' - }) + return { add: [updatedData] } } catch (error) { handleError(error, `Error duplicating row: ${error.message}`) } } else { - params.api.applyTransaction({ - add: [rowData], - addIndex: params.node?.rowIndex + 1 - }) + // If the original row wasn’t saved, just return the transaction + return { add: [newRow] } } } } @@ -359,12 +384,31 @@ const AddEditFuelCodeBase = () => { const onAction = useCallback( async (action, params) => { if (action === 'duplicate') { - await duplicateFuelCode(params) + return await duplicateFuelCode(params) } else if (action === 'delete') { await openDeleteModal(params.data.fuelCodeId, params) + } else if (action === 'add') { + // Refetch options to get updated nextFuelCode + const updatedOptions = await refetchOptions() + const defaultPrefix = updatedOptions.data.fuelCodePrefixes.find( + (item) => item.prefix === 'BCLCF' + ) + + const newRow = { + id: uuid(), + prefixId: defaultPrefix.fuelCodePrefixId, + prefix: defaultPrefix.prefix, + fuelSuffix: defaultPrefix.nextFuelCode, + modified: true, + validationStatus: 'error', + validationMsg: 'Fill in missing fields' + } + + // Return a transaction (no resetting rowData) + return { add: [newRow] } } }, - [updateFuelCode, deleteFuelCode] + [duplicateFuelCode, refetchOptions] ) if (isLoading || isLoadingExistingCode) { @@ -390,7 +434,7 @@ const AddEditFuelCodeBase = () => { columnDefs={columnDefs} defaultColDef={defaultColDef} onGridReady={onGridReady} - rowData={rowData} + rowData={rowData} // Only set once, do not update again onCellValueChanged={onCellValueChanged} onCellEditingStopped={onCellEditingStopped} onAction={onAction} diff --git a/frontend/src/views/FuelCodes/AddFuelCode/_schema.jsx b/frontend/src/views/FuelCodes/AddFuelCode/_schema.jsx index 66d033af6..8e335ba2e 100644 --- a/frontend/src/views/FuelCodes/AddFuelCode/_schema.jsx +++ b/frontend/src/views/FuelCodes/AddFuelCode/_schema.jsx @@ -97,20 +97,13 @@ export const fuelCodeColDefs = (optionsData, errors, isCreate, canEdit) => [ const selectedPrefix = optionsData?.fuelCodePrefixes?.find( (obj) => obj.prefix === params.newValue ) - params.data.fuelCodePrefixId = selectedPrefix.fuelCodePrefixId + if (selectedPrefix) { + params.data.prefixId = selectedPrefix.fuelCodePrefixId + params.data.fuelCodePrefixId = selectedPrefix.fuelCodePrefixId + params.data.fuelCodePrefix = selectedPrefix.fuelCodePrefix - params.data.fuelSuffix = optionsData?.fuelCodePrefixes?.find( - (obj) => obj.prefix === params.newValue - )?.nextFuelCode - params.data.company = undefined - params.data.fuel = undefined - params.data.feedstock = undefined - params.data.feedstockLocation = undefined - params.data.feedstockFuelTransportMode = [] - params.data.finishedFuelTransportMode = [] - params.data.formerCompany = undefined - params.data.contactName = undefined - params.data.contactEmail = undefined + params.data.fuelSuffix = selectedPrefix.nextFuelCode + } } return true }, From ab89b0b3a51abba1a936052b48654005efcbb35b Mon Sep 17 00:00:00 2001 From: Daniel Haselhan Date: Tue, 17 Dec 2024 13:24:30 -0800 Subject: [PATCH 2/6] 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) From cef77fd16dd050a12647a5abbd27b1e8ef96f6d9 Mon Sep 17 00:00:00 2001 From: Hamed Valiollahi Bayeki Date: Tue, 17 Dec 2024 14:37:17 -0800 Subject: [PATCH 3/6] fix: relocate 'Comments to the Director' widget to correct position --- .../views/Transfers/AddEditViewTransfer.jsx | 19 ++++++++++++++++++- .../Transfers/components/TransferView.jsx | 12 ++---------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/frontend/src/views/Transfers/AddEditViewTransfer.jsx b/frontend/src/views/Transfers/AddEditViewTransfer.jsx index 2968b16a8..4dec7f977 100644 --- a/frontend/src/views/Transfers/AddEditViewTransfer.jsx +++ b/frontend/src/views/Transfers/AddEditViewTransfer.jsx @@ -7,7 +7,7 @@ import { useNavigate, useParams } from 'react-router-dom' -import { roles } from '@/constants/roles' +import { roles, govRoles } from '@/constants/roles' import { ROUTES } from '@/constants/routes' import { TRANSACTIONS } from '@/constants/routes/routes' import { TRANSFER_STATUSES } from '@/constants/statuses' @@ -47,6 +47,7 @@ import { buttonClusterConfigFn } from './buttonConfigs' import { CategoryCheckbox } from './components/CategoryCheckbox' import { Recommendation } from './components/Recommendation' import SigningAuthority from './components/SigningAuthority' +import InternalComments from '@/components/InternalComments' export const AddEditViewTransfer = () => { const queryClient = useQueryClient() @@ -444,6 +445,22 @@ export const AddEditViewTransfer = () => { )} + {/* Internal Comments */} + {!editorMode && ( + <> + + {transferId && ( + + + + )} + + + )} + {/* Signing Authority Confirmation show it to FromOrg user when in draft and ToOrg when in Sent status */} {(!currentStatus || (currentStatus === TRANSFER_STATUSES.DRAFT && diff --git a/frontend/src/views/Transfers/components/TransferView.jsx b/frontend/src/views/Transfers/components/TransferView.jsx index cd837d0e8..b44013d8f 100644 --- a/frontend/src/views/Transfers/components/TransferView.jsx +++ b/frontend/src/views/Transfers/components/TransferView.jsx @@ -1,7 +1,6 @@ import BCBox from '@/components/BCBox' -import InternalComments from '@/components/InternalComments' -import { Role } from '@/components/Role' -import { roles, govRoles } from '@/constants/roles' + +import { roles } from '@/constants/roles' import { TRANSFER_STATUSES, getAllTerminalTransferStatuses @@ -89,13 +88,6 @@ export const TransferView = ({ transferId, editorMode, transferData }) => { /> )} - {/* Internal Comments */} - - {transferId && ( - - )} - - {/* List of attachments */} {/* */} From c33d1b5d9012a4d62a14fb52cdad4caf02ff10df Mon Sep 17 00:00:00 2001 From: Hamed Valiollahi Bayeki Date: Tue, 17 Dec 2024 15:04:18 -0800 Subject: [PATCH 4/6] refactor: remove unnecessary React fragment wrapper --- .../views/Transfers/AddEditViewTransfer.jsx | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/frontend/src/views/Transfers/AddEditViewTransfer.jsx b/frontend/src/views/Transfers/AddEditViewTransfer.jsx index 4dec7f977..4dfb34c09 100644 --- a/frontend/src/views/Transfers/AddEditViewTransfer.jsx +++ b/frontend/src/views/Transfers/AddEditViewTransfer.jsx @@ -447,18 +447,16 @@ export const AddEditViewTransfer = () => { {/* Internal Comments */} {!editorMode && ( - <> - - {transferId && ( - - - - )} - - + + {transferId && ( + + + + )} + )} {/* Signing Authority Confirmation show it to FromOrg user when in draft and ToOrg when in Sent status */} From a458e4c2525814ff4d3587bc51cf801d80b86999 Mon Sep 17 00:00:00 2001 From: Kevin Hashimoto Date: Tue, 17 Dec 2024 13:44:45 -0800 Subject: [PATCH 5/6] feat: toast on 400+ errors --- frontend/package-lock.json | 39 ++++++++++++++++++++++++++ frontend/package.json | 1 + frontend/src/main.jsx | 7 +++-- frontend/src/services/useApiService.js | 20 +++++++++++++ 4 files changed, 65 insertions(+), 2 deletions(-) diff --git a/frontend/package-lock.json b/frontend/package-lock.json index fdce412f9..3fd41e4ec 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -40,6 +40,7 @@ "material-ui-popup-state": "^5.0.10", "moment": "^2.30.1", "mui-daterange-picker-plus": "^1.0.4", + "notistack": "^3.0.1", "papaparse": "^5.4.1", "pretty-bytes": "^6.1.1", "quill": "^2.0.2", @@ -13052,6 +13053,14 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/goober": { + "version": "2.1.16", + "resolved": "https://registry.npmjs.org/goober/-/goober-2.1.16.tgz", + "integrity": "sha512-erjk19y1U33+XAMe1VTvIONHYoSqE4iS7BYUZfHaqeohLmnC0FdxEh7rQU+6MZ4OajItzjZFSRtVANrQwNq6/g==", + "peerDependencies": { + "csstype": "^3.0.10" + } + }, "node_modules/gopd": { "version": "1.0.1", "resolved": "https://registry.npmjs.org/gopd/-/gopd-1.0.1.tgz", @@ -16449,6 +16458,36 @@ "node": ">=0.10.0" } }, + "node_modules/notistack": { + "version": "3.0.1", + "resolved": "https://registry.npmjs.org/notistack/-/notistack-3.0.1.tgz", + "integrity": "sha512-ntVZXXgSQH5WYfyU+3HfcXuKaapzAJ8fBLQ/G618rn3yvSzEbnOB8ZSOwhX+dAORy/lw+GC2N061JA0+gYWTVA==", + "license": "MIT", + "dependencies": { + "clsx": "^1.1.0", + "goober": "^2.0.33" + }, + "engines": { + "node": ">=12.0.0", + "npm": ">=6.0.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/notistack" + }, + "peerDependencies": { + "react": "^16.8.0 || ^17.0.0 || ^18.0.0", + "react-dom": "^16.8.0 || ^17.0.0 || ^18.0.0" + } + }, + "node_modules/notistack/node_modules/clsx": { + "version": "1.2.1", + "resolved": "https://registry.npmjs.org/clsx/-/clsx-1.2.1.tgz", + "integrity": "sha512-EcR6r5a8bj6pu3ycsa/E/cKVGuTgZJZdsyUYHOksG/UHIiKfjxzRxYJpyVBwYaQeOvghal9fcc4PidlgzugAQg==", + "engines": { + "node": ">=6" + } + }, "node_modules/npm-run-path": { "version": "4.0.1", "resolved": "https://registry.npmjs.org/npm-run-path/-/npm-run-path-4.0.1.tgz", diff --git a/frontend/package.json b/frontend/package.json index 6e8783097..d6bec5c8f 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -62,6 +62,7 @@ "material-ui-popup-state": "^5.0.10", "moment": "^2.30.1", "mui-daterange-picker-plus": "^1.0.4", + "notistack": "^3.0.1", "papaparse": "^5.4.1", "pretty-bytes": "^6.1.1", "quill": "^2.0.2", diff --git a/frontend/src/main.jsx b/frontend/src/main.jsx index acb541131..509ac3e8e 100644 --- a/frontend/src/main.jsx +++ b/frontend/src/main.jsx @@ -8,6 +8,7 @@ import { KeycloakProvider } from '@/components/KeycloakProvider' import { getKeycloak } from '@/utils/keycloak' import { LocalizationProvider } from '@mui/x-date-pickers' import { AdapterDateFns } from '@mui/x-date-pickers/AdapterDateFnsV3' +import { SnackbarProvider } from 'notistack' const queryClient = new QueryClient() const keycloak = getKeycloak() @@ -20,8 +21,10 @@ if (root) { - - + + + + diff --git a/frontend/src/services/useApiService.js b/frontend/src/services/useApiService.js index a8525e2ae..31a34bb40 100644 --- a/frontend/src/services/useApiService.js +++ b/frontend/src/services/useApiService.js @@ -2,9 +2,11 @@ import { useMemo } from 'react' import axios from 'axios' import { useKeycloak } from '@react-keycloak/web' import { CONFIG } from '@/constants/config' +import { useSnackbar } from 'notistack' export const useApiService = (opts = {}) => { const { keycloak } = useKeycloak() + const { enqueueSnackbar, closeSnackbar } = useSnackbar() // useMemo to memoize the apiService instance const apiService = useMemo(() => { @@ -25,6 +27,24 @@ export const useApiService = (opts = {}) => { } ) + // Add response interceptor + instance.interceptors.response.use( + (response) => response, + (error) => { + if (error.response?.status >= 400) { + console.error( + 'API Error:', + error.response.status, + error.response.data + ) + enqueueSnackbar(`${error.response.status} error`, { + autoHideDuration: 5000, + variant: 'error' + }) + } + } + ) + // Download method instance.download = async (url, params = {}) => { try { From 1ee64051a53d7f7eefe8db7a98bd469ecf7a2d7b Mon Sep 17 00:00:00 2001 From: Kevin Hashimoto Date: Tue, 17 Dec 2024 13:48:07 -0800 Subject: [PATCH 6/6] chore: remove unused methods --- frontend/src/services/useApiService.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/services/useApiService.js b/frontend/src/services/useApiService.js index 31a34bb40..7b4fdca4b 100644 --- a/frontend/src/services/useApiService.js +++ b/frontend/src/services/useApiService.js @@ -6,7 +6,7 @@ import { useSnackbar } from 'notistack' export const useApiService = (opts = {}) => { const { keycloak } = useKeycloak() - const { enqueueSnackbar, closeSnackbar } = useSnackbar() + const { enqueueSnackbar } = useSnackbar() // useMemo to memoize the apiService instance const apiService = useMemo(() => {