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

1879 edit operation #2253

Merged
merged 24 commits into from
Sep 25, 2024
Merged

1879 edit operation #2253

merged 24 commits into from
Sep 25, 2024

Conversation

marcellmueller
Copy link
Contributor

@marcellmueller marcellmueller commented Sep 20, 2024

#1879
To test this make sure to reset the database since it includes updated fixtures:

  • cd bc_obps && make reset_db && make run
  • Navigate to the Operations dashboard in the Administration module
  • Select any Operation that has View Operation in the action column and click View Operation (made an issue to fix the Start Registration and Continue Registration URLs)
  • Click Edit button at the bottom of the form
  • Make any necessary changes (file upload fields will definitely be required)
  • Click Submit
  • The form should save. You can refresh the page and the updates will remain.

Other things to test:

  • Click View Details for Operation 14 which has a purpose of Electricity Import Operation
  • The Regulated product names field does not appear under Registration Purpose
  • Return to Operations DataGrid, click View Details for Operation 14 which has a purpose of Potential Reporting Operation
  • The Regulated product names field does not appear under Registration Purpose
  • The Multiple Operators section works

@marcellmueller marcellmueller force-pushed the 1879-edit-operation branch 3 times, most recently from 35e2059 to 87d046c Compare September 23, 2024 21:08
@marcellmueller marcellmueller marked this pull request as ready for review September 23, 2024 22:10
Comment on lines +35 to +45
@router.get(
"/v2/operations/{uuid:operation_id}/with-documents",
response={200: OperationOutWithDocuments, codes_4xx: Message},
tags=OPERATION_TAGS,
description="""Retrieves the details of a specific operation by its ID along with it's documents""",
exclude_none=True,
auth=authorize("approved_authorized_roles"),
)
@handle_http_errors()
def get_operation_with_documents(request: HttpRequest, operation_id: UUID) -> Tuple[Literal[200], Operation]:
return 200, OperationService.get_if_authorized(get_current_user_guid(request), operation_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new API route, though I was split on if I should add a query param like documents and return the documents if it was true. Can we have conditional Django Ninja schemas? Is a separate route fine?

@marcellmueller marcellmueller force-pushed the 1879-edit-operation branch 2 times, most recently from 167ab97 to c6bc0f9 Compare September 24, 2024 16:03
@shon-button
Copy link
Contributor

shon-button commented Sep 24, 2024

#1879

To test this make sure to reset the database since it includes updated fixtures:

  • cd bc_obps && make reset_db && make run
  • Navigate to the Operations dashboard in the Administration module
  • Select any Operation that has View Operation in the action column and click View Operation (made an issue to fix the Start Registration and Continue Registration URLs)
  • Click Edit button at the bottom of the form
  • Make any necessary changes (file upload fields will definitely be required)
  • Click Submit
  • The form should save. You can refresh the page and the updates will remain.

Other things to test:

  • Click View Details for Operation 14 which has a purpose of Electricity Import Operation
  • The Regulated product names field does not appear under Registration Purpose
  • Return to Operations DataGrid, click View Details for Operation 14 which has a purpose of Potential Reporting Operation
  • The Regulated product names field does not appear under Registration Purpose

Issue Edited fields do not reflect entered values after page refresh

Test Steps

  • Edited\completed fields: Process Flow Diagram*; Boundary Map*; Equipment List*;
  • Clicked Submit
  • Refreshed page
    image

@marcellmueller marcellmueller force-pushed the 1879-edit-operation branch 2 times, most recently from fd00682 to 3d61bf8 Compare September 25, 2024 14:45
@shon-button
Copy link
Contributor

shon-button commented Sep 25, 2024

@marcellmueller

Re #2287
FYI: Error is also seen on client.
image

image

@shon-button
Copy link
Contributor

@marcellmueller

Q: are the uploaded files supposed to have download links?

Thanks

@marcellmueller
Copy link
Contributor Author

marcellmueller commented Sep 25, 2024

@shon-button

There is a bug with the Read only file widget, it's not unique to this issue:
#2287

@marcellmueller
Copy link
Contributor Author

@marcellmueller

Q: are the uploaded files supposed to have download links?

Thanks

49782dd

Added the file preview prop! Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcellmueller

FYC, :)

Perhaps the backend service should verify that regulated_products should only be set for specific registration purposes (e.g., ELECTRICITY_IMPORT_OPERATION and POTENTIAL_REPORTING_OPERATION)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked Brianna about this and she said not to worry about it seeing as all of the purpose conditionals are changing re: the business area.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment here to remind us to potentially add that conditional in the future after the create registration purpose service gets redone. Thanks for the heads up, this was a good issue to raise 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcellmueller

nit: perhaps def test_industry_users_can_only_get_their_own_operations_with_documents(self) should be two separate tests:

def test_industry_user_can_get_owned_operation_with_documents(self):
def test_industry_user_cannot_get_unowned_operation_with_documents(self):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't spend much time in here and was just following the patterns set in the get_operation test above this. However I can update this if you think it's a good use of my time.

@marcellmueller marcellmueller merged commit 9f01977 into develop Sep 25, 2024
43 checks passed
@marcellmueller marcellmueller deleted the 1879-edit-operation branch September 25, 2024 19:54
@marcellmueller marcellmueller restored the 1879-edit-operation branch September 25, 2024 19:54
@marcellmueller marcellmueller deleted the 1879-edit-operation branch September 25, 2024 19:54
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