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

324 update operation review #2174

Merged
merged 10 commits into from
Sep 24, 2024
Merged

324 update operation review #2174

merged 10 commits into from
Sep 24, 2024

Conversation

ayeshmcg
Copy link
Contributor

@ayeshmcg ayeshmcg commented Sep 9, 2024

Modified Operation Review Information page.

Changes include:

  • Adding new component that includes Multistep form with task list.
  • Modified Operation Review Page
  • Modified Schema
  • Added a blank page - Person Responsible
  • Modified APIs

To test:

  • Go to page /reporting/reports after logging in with BCeId
  • Select any operation
  • It wil open operation reveiw information page and it the version doesnt exist it will show no version Id.
  • If there are any previously added activities or products, it will pre populated otherwise will show empty.

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.

Nice work! And good thinking about refactoring the page component to house the tasklist, the navigation and the form.

A few things:

  • Missing tests for the MultiStepFormWithTaskList / MultiStepTaskListButton
  • The MultiStepTaskListButton component isn't used anywhere, was it supposed to be in the MultiStepFormWithTaskList?
  • I've left a lot of comments about error handling: I think that in general, we can leave it up to Django/React/Sentry to handle the "something went wrong on the network, in the database, or unexpected behaviour" errors. Surrounding code with a try/catch block in random places makes it difficult to get an accurate stack trace and error message once the code is in production.
    The only errors we should catch are the ones part of the program flow, that we know will happen for certain cases.

bc_obps/service/product_service.py Outdated Show resolved Hide resolved
bc_obps/service/report_service.py Outdated Show resolved Hide resolved
bc_obps/service/reporting_year_service.py Outdated Show resolved Hide resolved
bc_obps/service/activity_service.py Outdated Show resolved Hide resolved
bciers/apps/reporting/src/data/jsonSchema/operations.ts Outdated Show resolved Hide resolved
@ayeshmcg ayeshmcg force-pushed the 324-update-operation-review branch 4 times, most recently from 3402dc7 to c013b05 Compare September 12, 2024 21:18
@ayeshmcg ayeshmcg force-pushed the 324-update-operation-review branch 2 times, most recently from 028feff to 63017c2 Compare September 23, 2024 15:42
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.

Nice!! There's one last piece that needs to be ironed out, between facility_id and report_facility_id. We can pair on that if you'd like.

I'll preemptively approve it

bc_obps/service/report_service.py Outdated Show resolved Hide resolved
bc_obps/service/report_service.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dleard dleard left a comment

Choose a reason for hiding this comment

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

Just some leftover code to remove. The facility_report api/service stuff has been split into its own separate files to separate the concerns from report_service functionality

bc_obps/service/report_service.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dleard dleard left a comment

Choose a reason for hiding this comment

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

👍

@dleard dleard merged commit b021dab into develop Sep 24, 2024
43 of 52 checks passed
@dleard dleard deleted the 324-update-operation-review branch September 24, 2024 21:31
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.

3 participants