LCFS-1207: Adding new columns to Other Use #1269

areyeslo · 2024-11-21T01:14:17Z

Add new columns to the Fuels for Other Use AG grid table in the compliance report and Report Details:

Determining Carbon Intensity
Fuel Code
CI of Fuel

github-actions · 2024-11-21T01:23:42Z

Backend Test Results

400 tests ±0 400 ✅ ±0 1m 33s ⏱️ +6s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 4c6d8af. ± Comparison against base commit 3d6f8e0.

♻️ This comment has been updated with latest results.

github-actions · 2024-11-21T01:23:48Z

Frontend Test Results

1 files ±0 114 suites ±0 38s ⏱️ +3s
367 tests ±0 347 ✅ ±0 20 💤 ±0 0 ❌ ±0
369 runs ±0 349 ✅ ±0 20 💤 ±0 0 ❌ ±0

Results for commit 4c6d8af. ± Comparison against base commit 3d6f8e0.

♻️ This comment has been updated with latest results.

dhaselhan

Tests needs to be fixed

areyeslo · 2024-11-21T18:55:06Z

Tests passing :)

dhaselhan · 2024-11-22T00:45:53Z

backend/lcfs/tests/other_uses/test_other_uses_view.py

@@ -136,9 +136,27 @@ async def test_save_other_uses_row_create(
    ) as mock_validate_organization_access:
        set_mock_user(fastapi_app, [RoleEnum.SUPPLIER])
        url = fastapi_app.url_path_for("save_other_uses_row")
-        payload = create_mock_schema({}).model_dump()
+        payload = {


Why are you moving away from the create_mock_schema helper?

dhaselhan · 2024-11-22T00:54:56Z

frontend/src/assets/locales/en/otherUses.json

-    "quantitySupplied": "Quantity supplied",
-    "addressForService": "Address for service",
-    "legalName": "Legal name",
+    "complianceReportId": "Compliance Report ID",


We don't use title case normally, was this part of the requirements?

dhaselhan · 2024-11-22T00:55:40Z

frontend/src/views/OtherUses/AddEditOtherUses.jsx

+          params.colDef.field
+        )
+      ) {
+        let ciOfFuel = 0


Put this in a helper function so I can better understand what its trying to do

Considering the following object:

{ "fuelTypeId": 1, "fuelType": "Biodiesel", "fossilDerived": null, "provision1Id": null, "provision2Id": null, "defaultCarbonIntensity": 100.21, "fuelCodes": [ { "fuelCodeId": 2, "fuelCode": "BCLCF124.4", "carbonIntensity": 3.62 }, { "fuelCodeId": 3, "fuelCode": "BCLCF125.4", "carbonIntensity": -2.14 }, { "fuelCodeId": 4, "fuelCode": "BCLCF138.5", "carbonIntensity": 4.26 }, { "fuelCodeId": 6, "fuelCode": "BCLCF251.2", "carbonIntensity": 0.35 }, { "fuelCodeId": 8, "fuelCode": "BCLCF274.2", "carbonIntensity": -15.03 }, { "fuelCodeId": 15, "fuelCode": "BCLCF362.1", "carbonIntensity": -1 } ], "provisionOfTheAct": [ { "provisionOfTheActId": 2, "name": "Fuel code - section 19 (b) (i)" }, { "provisionOfTheActId": 3, "name": "Default carbon intensity - section 19 (b) (ii)" } ], "units": "L" }

We are just checking if provision_of_the_act is 'Fuel code - section 19 (b) (i)', If it is the case, then we find the fuel_code associated to the fuel_type selected and provide the value to ciOfFuel. Otherwise, just take the value from defaultCarbonIntensity in fuel_type. which in this case is 100.21.

dhaselhan · 2024-11-22T00:56:00Z

frontend/src/views/OtherUses/AddEditOtherUses.jsx

@@ -107,6 +107,38 @@ export const AddEditOtherUses = () => {
    }
  }

+  const onCellValueChanged = useCallback(
+    async (params) => {
+      if (params.colDef.field === 'provisionOfTheAct') {


If this only applies to a single value, it should be in the valueSetter of that column in _schema

Did you see this comment?

dhaselhan · 2024-11-22T00:57:04Z

frontend/src/views/OtherUses/OtherUsesSummary.jsx

+      headerName: t('otherUses:otherUsesColLabels.ciOfFuel'),
+      field: 'ciOfFuel',
+      floatingFilter: false,
+      valueFormatter: (params) => {


Can you use decimalFormatter instead?

frontend/src/views/OtherUses/_schema.jsx

dhaselhan · 2024-11-25T21:48:21Z

frontend/src/views/OtherUses/_schema.jsx

@@ -92,7 +98,7 @@ export const otherUsesColDefs = (optionsData, errors) => [
  },
  {
    field: 'fuelCategory',
-    headerName: i18n.t('otherUses:otherUsesColLabels.fuelCategory'),
+	  headerName: i18n.t('otherUses:otherUsesColLabels.fuelCategory'),


dhaselhan · 2024-11-25T21:48:28Z

frontend/src/views/OtherUses/_schema.jsx

@@ -119,7 +219,8 @@ export const otherUsesColDefs = (optionsData, errors) => [
      min: 0,
      showStepperButtons: false
    },
-    cellStyle: (params) => StandardCellErrors(params, errors)
+    cellStyle: (params) => StandardCellErrors(params, errors),
+	minWidth: 200


dhaselhan · 2024-11-25T21:49:45Z

frontend/src/views/OtherUses/_schema.jsx

  },
  {
    field: 'rationale',
    flex: 1,
    headerName: i18n.t('otherUses:otherUsesColLabels.otherExpectedUse'),
    cellEditor: 'agTextCellEditor',
    cellDataType: 'text',
-    editable: (params) => params.data.expectedUse === 'Other'
+    editable: (params) => params.data.expectedUse === 'Other',
+	minWidth: 300


dhaselhan · 2024-11-25T21:49:48Z

frontend/src/views/OtherUses/_schema.jsx

@@ -155,15 +291,17 @@ export const otherUsesColDefs = (optionsData, errors) => [
    suppressKeyboardEvent,
    cellRenderer: (params) =>
      params.value || <Typography variant="body4">Select</Typography>,
-    cellStyle: (params) => StandardCellErrors(params, errors)
+    cellStyle: (params) => StandardCellErrors(params, errors),
+	minWidth: 200


dhaselhan · 2024-11-25T21:50:15Z

frontend/src/views/OtherUses/AddEditOtherUses.jsx

@@ -107,6 +107,38 @@ export const AddEditOtherUses = () => {
    }
  }

+  const onCellValueChanged = useCallback(
+    async (params) => {
+      if (params.colDef.field === 'provisionOfTheAct') {


Did you see this comment?

dhaselhan · 2024-11-25T21:51:07Z

frontend/src/views/OtherUses/AddEditOtherUses.jsx

@@ -47,6 +47,20 @@ export const AddEditOtherUses = () => {
    }
  }, [location.state])

+  // If otherUses data is available, set the rowData
+  useEffect(() => {


This looks like a duplicate of the code below. Why do we need this since its being set in onGridReady?

This is just for updates. Once the record is saved for first time, there is a race condition that otherUses is not loaded, so I have to wait until the data is available.

dhaselhan · 2024-11-25T21:52:17Z

backend/lcfs/tests/other_uses/conftest.py

@@ -29,21 +43,25 @@ def create_mock_entity(overrides: dict):


 def create_mock_schema(overrides: dict):
-    mock_schema = OtherUsesCreateSchema(


Why are you moving away from a typed object?

dhaselhan · 2024-11-25T21:52:41Z

backend/lcfs/tests/other_uses/test_other_uses_repo.py

    mock_compliance_report_uuid = "mock_group_uuid"

    # Mock the first db.execute call for fetching compliance report group UUID
    mock_first_execute = MagicMock()
    mock_first_execute.scalar.return_value = mock_compliance_report_uuid

+    # Create a realistic mock other_use object
+    mock_other_use = MagicMock()


Why are you not using create_mock_entity?

dhaselhan · 2024-11-25T21:53:55Z

frontend/src/views/OtherUses/_schema.jsx

+    valueFormatter: (params) => parseFloat(params.value).toFixed(2),
+    cellStyle: (params) => StandardCellErrors(params, errors),
+    editable: false,
+    valueGetter: (params) => {


If this is being properly set on the backend, why do we need such a complex getter?

because there are dependencies based on the selection of the user. This is related to the comment above:
Considering the following object:

{ "fuelTypeId": 1, "fuelType": "Biodiesel", "fossilDerived": null, "provision1Id": null, "provision2Id": null, "defaultCarbonIntensity": 100.21, "fuelCodes": [ { "fuelCodeId": 2, "fuelCode": "BCLCF124.4", "carbonIntensity": 3.62 }, { "fuelCodeId": 3, "fuelCode": "BCLCF125.4", "carbonIntensity": -2.14 }, { "fuelCodeId": 4, "fuelCode": "BCLCF138.5", "carbonIntensity": 4.26 }, { "fuelCodeId": 6, "fuelCode": "BCLCF251.2", "carbonIntensity": 0.35 }, { "fuelCodeId": 8, "fuelCode": "BCLCF274.2", "carbonIntensity": -15.03 }, { "fuelCodeId": 15, "fuelCode": "BCLCF362.1", "carbonIntensity": -1 } ], "provisionOfTheAct": [ { "provisionOfTheActId": 2, "name": "Fuel code - section 19 (b) (i)" }, { "provisionOfTheActId": 3, "name": "Default carbon intensity - section 19 (b) (ii)" } ], "units": "L" }

We are just checking if provision_of_the_act is 'Fuel code - section 19 (b) (i)', If it is the case, then we find the fuel_code associated to the fuel_type selected and provide the value to ciOfFuel. Otherwise, just take the value from defaultCarbonIntensity in fuel_type. which in this case is 100.21.

areyeslo force-pushed the LCFS-1207-AddColumnsToOtherUse_v2 branch from d78f715 to 714de3f Compare November 21, 2024 16:57

dhaselhan requested changes Nov 21, 2024

View reviewed changes

areyeslo force-pushed the LCFS-1207-AddColumnsToOtherUse_v2 branch 4 times, most recently from 85c1f6e to 22d783f Compare November 21, 2024 18:44

areyeslo requested a review from dhaselhan November 21, 2024 18:55

areyeslo force-pushed the LCFS-1207-AddColumnsToOtherUse_v2 branch from 22d783f to 1d5a193 Compare November 21, 2024 19:43

dhaselhan requested changes Nov 22, 2024

View reviewed changes

areyeslo force-pushed the LCFS-1207-AddColumnsToOtherUse_v2 branch 2 times, most recently from dac6903 to 7708949 Compare November 22, 2024 21:03

areyeslo requested a review from dhaselhan November 22, 2024 21:19

dhaselhan requested changes Nov 25, 2024

View reviewed changes

areyeslo force-pushed the LCFS-1207-AddColumnsToOtherUse_v2 branch from a07288a to d7abb36 Compare November 26, 2024 01:13

areyeslo added 3 commits November 25, 2024 18:13

solving conflicts

7b3ae3a

Update pytests for other_uses.

6994d17

code review

7a45ff6

areyeslo force-pushed the LCFS-1207-AddColumnsToOtherUse_v2 branch from d7abb36 to 0fb1b0a Compare November 26, 2024 01:13

areyeslo added 2 commits November 26, 2024 09:23

Changes due to code review

e222d3b

Fix for migration

49d186c

areyeslo force-pushed the LCFS-1207-AddColumnsToOtherUse_v2 branch from 0fb1b0a to 49d186c Compare November 26, 2024 16:33

Merge branch 'release-0.2.0' into LCFS-1207-AddColumnsToOtherUse_v2

4c6d8af

areyeslo requested a review from dhaselhan November 26, 2024 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LCFS-1207: Adding new columns to Other Use #1269

LCFS-1207: Adding new columns to Other Use #1269

areyeslo commented Nov 21, 2024

github-actions bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

dhaselhan left a comment

areyeslo commented Nov 21, 2024

dhaselhan Nov 22, 2024

areyeslo Nov 22, 2024

dhaselhan Nov 22, 2024

areyeslo Nov 22, 2024

dhaselhan Nov 22, 2024

areyeslo Nov 22, 2024

dhaselhan Nov 22, 2024

dhaselhan Nov 25, 2024

dhaselhan Nov 22, 2024

areyeslo Nov 22, 2024

dhaselhan Nov 25, 2024

dhaselhan Nov 25, 2024

dhaselhan Nov 25, 2024

dhaselhan Nov 25, 2024

dhaselhan Nov 25, 2024

dhaselhan Nov 25, 2024

areyeslo Nov 26, 2024

dhaselhan Nov 25, 2024 •

edited

Loading

dhaselhan Nov 25, 2024

dhaselhan Nov 25, 2024

areyeslo Nov 26, 2024

		@@ -29,21 +43,25 @@ def create_mock_entity(overrides: dict):


		def create_mock_schema(overrides: dict):
		mock_schema = OtherUsesCreateSchema(

LCFS-1207: Adding new columns to Other Use #1269

Are you sure you want to change the base?

LCFS-1207: Adding new columns to Other Use #1269

Conversation

areyeslo commented Nov 21, 2024

github-actions bot commented Nov 21, 2024 • edited Loading

Backend Test Results

github-actions bot commented Nov 21, 2024 • edited Loading

Frontend Test Results

dhaselhan left a comment

Choose a reason for hiding this comment

areyeslo commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaselhan Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

dhaselhan Nov 25, 2024 •

edited

Loading