-
-
Notifications
You must be signed in to change notification settings - Fork 782
Bug fix in modified action delete API #5351
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
Changes from all commits
5194f38
3900f03
65b4827
11920b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -258,12 +258,22 @@ def delete(self, ref_or_id, requester_user): | |
| entry_point=entry_point, | ||
| metadata_file=metadata_file, | ||
| ) | ||
|
|
||
| except PermissionError as e: | ||
| LOG.error("No permission to delete resource files from disk.") | ||
| action_db.id = None | ||
| Action.add_or_update(action_db) | ||
| abort(http_client.FORBIDDEN, six.text_type(e)) | ||
| return | ||
|
|
||
| except Exception as e: | ||
| LOG.error( | ||
| "Exception encountered during deleting resource files from disk." | ||
| "Exception encountered during deleting resource files from disk. " | ||
| "Exception was %s", | ||
| e, | ||
| ) | ||
| action_db.id = None | ||
| Action.add_or_update(action_db) | ||
| abort(http_client.INTERNAL_SERVER_ERROR, six.text_type(e)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, if the deletion is related to permission error, can you change the error code to 403 Forbidden here? Otherwise for any other error, return 500 is fine. |
||
| return | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -626,6 +626,58 @@ def test_delete(self): | |
| del_resp = self.__do_delete(self.__get_action_id(post_resp)) | ||
| self.assertEqual(del_resp.status_int, 204) | ||
|
|
||
| @mock.patch.object( | ||
| action_validator, "validate_action", mock.MagicMock(return_value=True) | ||
| ) | ||
| def test_delete_permission_error_and_action_reregistered_to_database(self): | ||
| post_resp = self.__do_post(ACTION_1) | ||
|
|
||
| with mock.patch( | ||
| "st2api.controllers.v1.actions.delete_action_files_from_pack" | ||
| ) as mock_remove_files: | ||
| msg = "No permission to delete action files from disk" | ||
| mock_remove_files.side_effect = PermissionError(msg) | ||
| del_resp = self.__do_delete( | ||
| self.__get_action_id(post_resp), expect_errors=True | ||
| ) | ||
| self.assertEqual(del_resp.status_int, 403) | ||
| self.assertEqual(del_resp.json["faultstring"], msg) | ||
|
|
||
| # retrieving reregistered action | ||
| get_resp = self.__do_get_actions_by_url_parameter("name", ACTION_1["name"]) | ||
| expected_uid = post_resp.json["uid"] | ||
| actual_uid = get_resp.json[0]["uid"] | ||
| self.assertEqual(actual_uid, expected_uid) | ||
| action_id = get_resp.json[0]["id"] | ||
| del_resp = self.__do_delete(action_id) | ||
| self.assertEqual(del_resp.status_int, 204) | ||
|
|
||
| @mock.patch.object( | ||
| action_validator, "validate_action", mock.MagicMock(return_value=True) | ||
| ) | ||
| def test_delete_exception_and_action_reregistered_to_database(self): | ||
| post_resp = self.__do_post(ACTION_1) | ||
|
|
||
| with mock.patch( | ||
| "st2api.controllers.v1.actions.delete_action_files_from_pack" | ||
| ) as mock_remove_files: | ||
| msg = "Exception encountered during removing files from disk" | ||
| mock_remove_files.side_effect = Exception(msg) | ||
| del_resp = self.__do_delete( | ||
| self.__get_action_id(post_resp), expect_errors=True | ||
| ) | ||
| self.assertEqual(del_resp.status_int, 500) | ||
| self.assertEqual(del_resp.json["faultstring"], msg) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a check to make sure that ACTION_1 is added back to the database? |
||
|
|
||
| # retrieving reregistered action | ||
| get_resp = self.__do_get_actions_by_url_parameter("name", ACTION_1["name"]) | ||
| expected_uid = post_resp.json["uid"] | ||
| actual_uid = get_resp.json[0]["uid"] | ||
| self.assertEqual(actual_uid, expected_uid) | ||
| action_id = get_resp.json[0]["id"] | ||
| del_resp = self.__do_delete(action_id) | ||
| self.assertEqual(del_resp.status_int, 204) | ||
|
|
||
| @mock.patch.object( | ||
| action_validator, "validate_action", mock.MagicMock(return_value=True) | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you modify the unit tests to cover this change?