Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat:added new entrant info page #2439

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

ayeshmcg
Copy link
Contributor

@ayeshmcg ayeshmcg commented Nov 1, 2024

Card: bcgov/cas-reporting#351
Changes:

  • Added New Entrant Information: New page for collecting New Entrant Information data.

  • Create CheckboxGroup Widget

  • Modified Checkbox widget and DateWidget

  • Schema and UI Schema: Defined schemas to validate and structure the new data.

APIs/Services:

  • Get Report New Entrant Data
  • Save New Entrant Report Data
  • Get Report New Entrant Service

Model :

  • Report New Entrant
  • Report New Entrant Production

@ayeshmcg ayeshmcg force-pushed the feat/351-New-Entrant-Information branch from c90b749 to a72c7cd Compare November 8, 2024 02:35
@ayeshmcg ayeshmcg marked this pull request as ready for review November 8, 2024 02:35
Copy link
Contributor

@pbastia pbastia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work! This form is quite involved.
Let me know if you have any questions about the comments I left, happy to pair on it next week

We're also missing tests for the backend services and API endpoints

bc_obps/reporting/api/production_data.py Show resolved Hide resolved
response={200: int, custom_codes_4xx: Message},
tags=["Emissions Report"],
description="Saves the data for the new entrant report",
# auth=authorize("approved_industry_user"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auth

dateOfFirstShipment,
dateOfNewEntrantPeriod,
}: AdditionalReportingDataProps) {
const [formData, setFormData] = useState(initialFormData || {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the interface already guarantees that

Suggested change
const [formData, setFormData] = useState(initialFormData || {});
const [formData, setFormData] = useState(initialFormData);

Comment on lines +65 to +75
const handleSubmit = async () => {
const endpoint = `reporting/report-version/${versionId}/new-entrant-data`;
const method = "POST";

const response = await actionHandler(endpoint, method, endpoint, {
body: JSON.stringify(formData),
});
if (response) {
router.push(saveAndContinueUrl);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should take the formData from the submit argument, not from the state, since it has a guarantee of being up to date, whereas the state could be behind.
Then we don't need to keep the formData in the state at all!

Suggested change
const handleSubmit = async () => {
const endpoint = `reporting/report-version/${versionId}/new-entrant-data`;
const method = "POST";
const response = await actionHandler(endpoint, method, endpoint, {
body: JSON.stringify(formData),
});
if (response) {
router.push(saveAndContinueUrl);
}
};
const handleSubmit = async (data: any) => {
const endpoint = `reporting/report-version/${versionId}/new-entrant-data`;
const method = "POST";
const response = await actionHandler(endpoint, method, endpoint, {
body: JSON.stringify(data),
});
if (response) {
router.push(saveAndContinueUrl);
}
};

Comment on lines +4 to +12
let response = await actionHandler(
`reporting/report-version/${version_id}/new-entrant-data`,
"GET",
`reporting/report-version/${version_id}/new-entrant-data`,
);

if (response && !response.error) {
return response;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actionHandler already implements error handling, with sentry and everything. We don't need to do it here, whatever response is returned is guaranteed to be good :)

Suggested change
let response = await actionHandler(
`reporting/report-version/${version_id}/new-entrant-data`,
"GET",
`reporting/report-version/${version_id}/new-entrant-data`,
);
if (response && !response.error) {
return response;
}
return actionHandler(
`reporting/report-version/${version_id}/new-entrant-data`,
"GET",
`reporting/report-version/${version_id}/new-entrant-data`,
);

Comment on lines +80 to +94
...selectedProducts?.reduce(
(acc, product) => ({
...acc,
[`${product.id}`]: {
type: "object",
title: `Product: ${product.name}`,
properties: {
production_amount: {
type: "number",
title: "Production after new entrant period began",
default: product.production_amount,
},
},
},
}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works! I think in other places we've used an array instead. It might make the API easier to implement

Comment on lines +221 to +231
...selectedProducts?.reduce((acc, product) => {
acc[product.id] = {
"ui:FieldTemplate": ({ id, children }) => (
<ProductionDataTitleWidget id={id} value={product.name}>
{children}
</ProductionDataTitleWidget>
),
production_amount: {
"ui:FieldTemplate": InlineFieldTemplate,
},
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we use an array, this will look a lot simpler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants