From c393a53d36f8b342cf9ade43af40b0d09ca0056f Mon Sep 17 00:00:00 2001 From: Mohamed Abdel Wedoud Date: Tue, 18 Jun 2024 13:55:11 +0200 Subject: [PATCH] feat(delete-apis): update following code review --- antarest/core/exceptions.py | 32 ++++++++------ antarest/study/service.py | 42 +++++++------------ antarest/study/web/study_data_blueprint.py | 2 +- .../study_data_blueprint/test_thermal.py | 19 ++------- tests/integration/test_integration.py | 20 +++------ 5 files changed, 46 insertions(+), 69 deletions(-) diff --git a/antarest/core/exceptions.py b/antarest/core/exceptions.py index 07b0f9a0f7..2d9ea03f58 100644 --- a/antarest/core/exceptions.py +++ b/antarest/core/exceptions.py @@ -4,6 +4,8 @@ from fastapi.exceptions import HTTPException +MAX_BINDING_CONSTRAINTS_TO_DISPLAY = 10 + class ShouldNotHappenException(Exception): pass @@ -326,10 +328,12 @@ def __init__(self, is_variant: bool) -> None: class AreaDeletionNotAllowed(HTTPException): - def __init__(self, uuid: str, message: t.Optional[str] = None) -> None: - msg = f"Area {uuid} is not allowed to be deleted" - if message: - msg += f"\n{message}" + def __init__(self, uuid: str, binding_constraints_ids: t.List[str]) -> None: + first_bcs_ids = "" + for i, bc in enumerate(binding_constraints_ids[:MAX_BINDING_CONSTRAINTS_TO_DISPLAY]): + first_bcs_ids += f"{i+1}- {bc}\n" + message = "Area is referenced in the following binding constraints:\n" + first_bcs_ids + msg = f"Area {uuid} is not allowed to be deleted\n{message}" super().__init__( HTTPStatus.FORBIDDEN, msg, @@ -337,10 +341,12 @@ def __init__(self, uuid: str, message: t.Optional[str] = None) -> None: class LinkDeletionNotAllowed(HTTPException): - def __init__(self, uuid: str, message: t.Optional[str] = None) -> None: - msg = f"Link {uuid} is not allowed to be deleted" - if message: - msg += f"\n{message}" + def __init__(self, uuid: str, binding_constraints_ids: t.List[str]) -> None: + first_bcs_ids = "" + for i, bc in enumerate(binding_constraints_ids[:MAX_BINDING_CONSTRAINTS_TO_DISPLAY]): + first_bcs_ids += f"{i+1}- {bc}\n" + message = "Link is referenced in the following binding constraints:\n" + first_bcs_ids + msg = f"Link {uuid} is not allowed to be deleted\n{message}" super().__init__( HTTPStatus.FORBIDDEN, msg, @@ -348,10 +354,12 @@ def __init__(self, uuid: str, message: t.Optional[str] = None) -> None: class ClusterDeletionNotAllowed(HTTPException): - def __init__(self, uuid: str, message: t.Optional[str] = None) -> None: - msg = f"Cluster {uuid} is not allowed to be deleted" - if message: - msg += f"\n{message}" + def __init__(self, uuid: str, binding_constraints_ids: t.List[str]) -> None: + first_bcs_ids = "" + for i, bc in enumerate(binding_constraints_ids[:MAX_BINDING_CONSTRAINTS_TO_DISPLAY]): + first_bcs_ids += f"{i+1}- {bc}\n" + message = "Cluster is referenced in the following binding constraints:\n" + first_bcs_ids + msg = f"Cluster {uuid} is not allowed to be deleted\n{message}" super().__init__( HTTPStatus.FORBIDDEN, msg, diff --git a/antarest/study/service.py b/antarest/study/service.py index abee5392bd..4ab20a4723 100644 --- a/antarest/study/service.py +++ b/antarest/study/service.py @@ -140,7 +140,6 @@ logger = logging.getLogger(__name__) MAX_MISSING_STUDY_TIMEOUT = 2 # days -MAX_BINDING_CONSTRAINTS_TO_DISPLAY = 10 def get_disk_usage(path: t.Union[str, Path]) -> int: @@ -1864,14 +1863,9 @@ def delete_area(self, uuid: str, area_id: str, params: RequestParameters) -> Non self._assert_study_unarchived(study) referencing_binding_constraints = self.binding_constraint_manager.get_binding_constraints( study, ConstraintFilters(area_name=area_id) - )[:MAX_BINDING_CONSTRAINTS_TO_DISPLAY] + ) if referencing_binding_constraints: - first_bcs_ids = "" - for i, bc in enumerate(referencing_binding_constraints): - first_bcs_ids += f"{i+1}- {bc.id}\n" - raise AreaDeletionNotAllowed( - area_id, "Area is referenced in the following binding constraints:\n" + first_bcs_ids - ) + raise AreaDeletionNotAllowed(area_id, [bc.id for bc in referencing_binding_constraints]) self.areas.delete_area(study, area_id) self.event_bus.push( Event( @@ -1894,14 +1888,9 @@ def delete_link( link_id = LinkTerm(area1=area_from, area2=area_to).generate_id() referencing_binding_constraints = self.binding_constraint_manager.get_binding_constraints( study, ConstraintFilters(link_id=link_id) - )[:MAX_BINDING_CONSTRAINTS_TO_DISPLAY] + ) if referencing_binding_constraints: - first_bcs_ids = "" - for i, bc in enumerate(referencing_binding_constraints): - first_bcs_ids += f"{i+1}- {bc.id}\n" - raise LinkDeletionNotAllowed( - link_id, "Link is referenced in the following binding constraints:\n" + first_bcs_ids - ) + raise LinkDeletionNotAllowed(link_id, [bc.id for bc in referencing_binding_constraints]) self.links.delete_link(study, area_from, area_to) self.event_bus.push( Event( @@ -2544,27 +2533,26 @@ def get_matrix_with_index_and_header( return df_matrix - def assert_no_cluster_referenced_in_bcs(self, study: Study, area_id: str, cluster_ids: t.Sequence[str]) -> None: + def asserts_no_thermal_in_binding_constraints( + self, study: Study, area_id: str, cluster_ids: t.Sequence[str] + ) -> None: """ - Check that no cluster is referenced in a binding constraint otherwise raise an ClusterDeletionNotAllowed Exception + Check that no cluster is referenced in a binding constraint otherwise raise an ClusterDeletionNotAllowed + Exception Args: - study: input study + study: input study for which an update is to be committed area_id: area ID to be checked - cluster_ids: cluster IDs to be checked + cluster_ids: IDs of the thermal clusters to be checked - Returns: + Returns: None + Raises: ClusterDeletionNotAllowed if a cluster is referenced in a binding constraint """ for cluster_id in cluster_ids: referencing_binding_constraints = self.binding_constraint_manager.get_binding_constraints( study, ConstraintFilters(cluster_id=f"{area_id}.{cluster_id}") - )[:MAX_BINDING_CONSTRAINTS_TO_DISPLAY] + ) if referencing_binding_constraints: - first_bcs_ids = "" - for i, bc in enumerate(referencing_binding_constraints): - first_bcs_ids += f"{i+1}- {bc.id}\n" - raise ClusterDeletionNotAllowed( - cluster_id, "Cluster is referenced in the following binding constraints:\n" + first_bcs_ids - ) + raise ClusterDeletionNotAllowed(cluster_id, [bc.id for bc in referencing_binding_constraints]) diff --git a/antarest/study/web/study_data_blueprint.py b/antarest/study/web/study_data_blueprint.py index ab785c3a16..43d71400d5 100644 --- a/antarest/study/web/study_data_blueprint.py +++ b/antarest/study/web/study_data_blueprint.py @@ -2185,7 +2185,7 @@ def delete_thermal_clusters( ) request_params = RequestParameters(user=current_user) study = study_service.check_study_access(uuid, StudyPermissionType.WRITE, request_params) - study_service.assert_no_cluster_referenced_in_bcs(study, area_id, cluster_ids) + study_service.asserts_no_thermal_in_binding_constraints(study, area_id, cluster_ids) study_service.thermal_manager.delete_clusters(study, area_id, cluster_ids) @bp.get( diff --git a/tests/integration/study_data_blueprint/test_thermal.py b/tests/integration/study_data_blueprint/test_thermal.py index b093bc3e28..921e1ebab0 100644 --- a/tests/integration/study_data_blueprint/test_thermal.py +++ b/tests/integration/study_data_blueprint/test_thermal.py @@ -654,12 +654,9 @@ def test_lifecycle( json=[fr_gas_conventional_id], ) assert res.status_code == 403, res.json() - assert res.json() == { - "description": "Cluster FR_Gas conventional is not allowed to be deleted\n" - "Cluster is referenced in the following binding constraints:" - "\n1- binding constraint\n", - "exception": "ClusterDeletionNotAllowed", - } + description = res.json()["description"] + assert all([elm in description for elm in [fr_gas_conventional, "binding constraint"]]) + assert res.json()["exception"] == "ClusterDeletionNotAllowed" # delete the binding constraint res = client.delete( @@ -1051,21 +1048,13 @@ def test_variant_lifecycle(self, client: TestClient, user_access_token: str, var "remove_cluster", ] - def test_thermal_cluster_deletion(self, client: TestClient, user_access_token: str) -> None: + def test_thermal_cluster_deletion(self, client: TestClient, user_access_token: str, study_id: str) -> None: """ Test that creating a thermal cluster with invalid properties raises a validation error. """ client.headers = {"Authorization": f"Bearer {user_access_token}"} - # Create a new study - res = client.post( - f"/v1/studies", - params={"name": "My Study"}, - ) - assert res.status_code in {200, 201}, res.json() - study_id = res.json() - # Create an area "area_1" in the study res = client.post( f"/v1/studies/{study_id}/areas", diff --git a/tests/integration/test_integration.py b/tests/integration/test_integration.py index 2da1c792c3..d7e2f69864 100644 --- a/tests/integration/test_integration.py +++ b/tests/integration/test_integration.py @@ -1431,10 +1431,8 @@ def test_area_management(client: TestClient, admin_access_token: str) -> None: result = client.delete(f"/v1/studies/{study_id}/areas/area%201") assert result.status_code == 403, res.json() # verify the error message - assert ( - result.json()["description"] == "Area area 1 is not allowed to be deleted\nArea is referenced " - "in the following binding constraints:\n1- binding constraint 1\n" - ) + description = result.json()["description"] + assert all([elm in description for elm in ["area 1", "binding constraint 1"]]) # check the exception assert result.json()["exception"] == "AreaDeletionNotAllowed" @@ -1717,7 +1715,7 @@ def test_copy(client: TestClient, admin_access_token: str, study_id: str) -> Non assert res["public_mode"] == "READ" -def test_links_deletion(client: TestClient, user_access_token: str) -> None: +def test_links_deletion(client: TestClient, user_access_token: str, study_id: str) -> None: """ Test the deletion of links between areas. """ @@ -1725,9 +1723,6 @@ def test_links_deletion(client: TestClient, user_access_token: str) -> None: # set client headers to user access token client.headers = {"Authorization": f"Bearer {user_access_token}"} - # create a study - study_id = client.post("/v1/studies?name=test").json() - # Create an area "area_1" in the study res = client.post( f"/v1/studies/{study_id}/areas", @@ -1783,12 +1778,9 @@ def test_links_deletion(client: TestClient, user_access_token: str) -> None: # try to delete the link before deleting the binding constraint res = client.delete(f"/v1/studies/{study_id}/links/area_1/area_2") assert res.status_code == 403, res.json() - assert res.json() == { - "description": "Link area_1%area_2 is not allowed to be deleted\n" - "Link is referenced in the following binding constraints:\n" - "1- bc_1\n", - "exception": "LinkDeletionNotAllowed", - } + description = res.json()["description"] + assert all([elm in description for elm in ["area_1%area_2", "bc_1"]]) + assert res.json()["exception"] == "LinkDeletionNotAllowed" # delete the binding constraint res = client.delete(f"/v1/studies/{study_id}/bindingconstraints/bc_1")