From 637cea970b57615a686650a2fc597d60d79979b2 Mon Sep 17 00:00:00 2001 From: Milo Thurston Date: Fri, 21 Jun 2024 15:31:28 +0100 Subject: [PATCH 1/6] Fixed issue /ptmd/.../queries/users.py:147 #128 --- ptmd/api/queries/users.py | 4 +++- tests/test_api/test_queries/test_users.py | 10 ++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/ptmd/api/queries/users.py b/ptmd/api/queries/users.py index 16d7b37..badc38c 100644 --- a/ptmd/api/queries/users.py +++ b/ptmd/api/queries/users.py @@ -11,6 +11,7 @@ from sqlalchemy.exc import IntegrityError from jsonschema import Draft4Validator as Validator +from ptmd.logger import LOGGER from ptmd.config import session from ptmd.const import CREATE_USER_SCHEMA_PATH from ptmd.database import login_user, get_token, User, Token, Organisation @@ -144,7 +145,8 @@ def enable_account(token: str) -> tuple[Response, int]: user.set_role('enabled') return jsonify(msg="Account enabled. An email has been to an admin to validate your account."), 200 except Exception as e: - return jsonify(msg=str(e)), 400 + LOGGER.error("Account not enabled: %s", e) + return jsonify(msg='Failed to enable your account. Please contact an admin for assistance.'), 400 @check_role(role='admin') diff --git a/tests/test_api/test_queries/test_users.py b/tests/test_api/test_queries/test_users.py index 463aa7c..0f32aff 100644 --- a/tests/test_api/test_queries/test_users.py +++ b/tests/test_api/test_queries/test_users.py @@ -254,13 +254,19 @@ def test_enable_account_errors(self, mock_session_commit, mock_session_delete, response = client.get('/api/users/enable/2', headers={'Authorization': f'Bearer {123}', **HEADERS}) mock_session_commit.assert_called_once() mock_session_delete.assert_called_once_with(mocked_token.query.filter().first.return_value) - self.assertEqual(response.json, {'msg': 'Token expired'}) + self.assertEqual(response.json, + { + 'msg': 'Failed to enable your account. Please contact an admin for assistance.' + }) self.assertEqual(response.status_code, 400) mocked_token.query.filter().first.return_value = None with app.test_client() as client: response = client.get('/api/users/enable/2', headers={'Authorization': f'Bearer {123}', **HEADERS}) - self.assertEqual(response.json, {'msg': 'Invalid token'}) + self.assertEqual(response.json, + { + 'msg': 'Failed to enable your account. Please contact an admin for assistance.' + }) self.assertEqual(response.status_code, 400) @patch('ptmd.api.queries.users.User') From 8a94e9086e1d93f23968d38c33024f5a6ad85bf3 Mon Sep 17 00:00:00 2001 From: Milo Thurston Date: Fri, 21 Jun 2024 15:51:55 +0100 Subject: [PATCH 2/6] Fixed issues in shipment.py #128 --- ptmd/api/queries/files/shipment.py | 10 +++++++--- .../test_api/test_queries/test_files/test_shipment.py | 4 ++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/ptmd/api/queries/files/shipment.py b/ptmd/api/queries/files/shipment.py index 414c551..dbabdb7 100644 --- a/ptmd/api/queries/files/shipment.py +++ b/ptmd/api/queries/files/shipment.py @@ -4,6 +4,7 @@ from flask import jsonify, Response, request +from ptmd.logger import LOGGER from ptmd.database import File, get_shipped_file from ptmd.database.queries.users import email_admins_file_shipped from ptmd.api.queries.utils import check_role @@ -34,7 +35,8 @@ def ship_data(file_id: int) -> tuple[Response, int]: except PermissionError: return jsonify({'message': f'File {file_id} could not be locked but has been sent anyway'}), 200 except ValueError as e: - return jsonify({'message': str(e)}), 400 + LOGGER.error("Value error: %s", str(e)) + return jsonify({'message': 'File is not in the correct state.'}), 400 except Exception: session.rollback() return jsonify({'message': 'An unknown error occurred. Sorry.'}), 500 @@ -58,9 +60,11 @@ def receive_data(file_id: int) -> tuple[Response, int]: except BatchError as e: return e.serialize() except PermissionError as e: - return jsonify({'message': str(e)}), 403 + LOGGER.error("User is not an admin: %s", str(e)) + return jsonify({'message': 'You do not have permission to perform this action.'}), 403 except ValueError as e: - return jsonify({'message': str(e)}), 400 + LOGGER.error("File not in correct state: %s", str(e)) + return jsonify({'message': 'File is not in the correct state.'}), 400 def validate_batch(file_id: int, new_batch: str | None = None) -> File: diff --git a/tests/test_api/test_queries/test_files/test_shipment.py b/tests/test_api/test_queries/test_files/test_shipment.py index 84b298c..95b887c 100644 --- a/tests/test_api/test_queries/test_files/test_shipment.py +++ b/tests/test_api/test_queries/test_files/test_shipment.py @@ -47,7 +47,7 @@ def ship_samples(self, at): mock_user().role = 'admin' with app.test_client() as client: response = client.post('/api/files/1/ship', headers=HEADERS, json={}) - self.assertEqual(response.json, {'message': 'A value error.'}) + self.assertEqual(response.json, {'message': 'File is not in the correct state.'}) self.assertEqual(response.status_code, 400) @patch('ptmd.api.queries.files.shipment.validate_batch') @@ -143,7 +143,7 @@ def shipment_was_received(self, at): mock_user().role = 'admin' with app.test_client() as client: response = client.post('/api/files/1/receive', headers=HEADERS, json={}) - self.assertEqual(response.json, {'message': 'A value error.'}) + self.assertEqual(response.json, {'message': 'File is not in the correct state.'}) self.assertEqual(response.status_code, 400) @patch('ptmd.api.queries.files.shipment.validate_batch') From cafd65366bcb933e7fce56f84b76c379ad67fcd1 Mon Sep 17 00:00:00 2001 From: Milo Thurston Date: Mon, 24 Jun 2024 14:19:26 +0100 Subject: [PATCH 3/6] Needs a new test for register.py and better exceptions for create.py --- ptmd/api/queries/files/delete.py | 4 +- ptmd/api/queries/files/isa.py | 7 ++- ptmd/api/queries/files/register.py | 8 ++- ptmd/api/queries/files/shipment.py | 6 +- ptmd/api/queries/users.py | 2 +- .../test_queries/test_files/test_isa.py | 2 +- .../test_queries/test_files/test_register.py | 57 +++++++++++++++++++ 7 files changed, 76 insertions(+), 10 deletions(-) diff --git a/ptmd/api/queries/files/delete.py b/ptmd/api/queries/files/delete.py index 936d2e4..03934fe 100644 --- a/ptmd/api/queries/files/delete.py +++ b/ptmd/api/queries/files/delete.py @@ -3,6 +3,7 @@ from flask import jsonify, Response +from ptmd.logger import LOGGER from ptmd.database.models import File from ptmd.api.queries.utils import check_role @@ -23,4 +24,5 @@ def delete_file(file_id: int) -> tuple[Response, int]: file.remove() return jsonify({"message": f"File {file_id} was successfully deleted."}), 200 except PermissionError as e: - return jsonify({"message": str(e)}), 403 + LOGGER.error("Permission Error: %s" % (str(e))) + return jsonify({"message": 'You do not have permission to perform this action.'}), 403 diff --git a/ptmd/api/queries/files/isa.py b/ptmd/api/queries/files/isa.py index bde1fc9..c3a53c2 100644 --- a/ptmd/api/queries/files/isa.py +++ b/ptmd/api/queries/files/isa.py @@ -3,6 +3,7 @@ from flask import jsonify, Response +from ptmd.logger import LOGGER from ptmd.lib.isa import convert_file_to_isa from ptmd.api.queries.utils import check_role @@ -17,7 +18,9 @@ def convert_to_isa(file_id: int) -> tuple[Response, int]: try: response: dict = convert_file_to_isa(file_id)[0] except ValueError as e: - return jsonify({'message': str(e)}), 400 + LOGGER.error("ValueError: %s" % (str(e))) + return jsonify({'message': 'File conversion failed.'}), 400 except FileNotFoundError as e: - return jsonify({'message': str(e)}), 404 + LOGGER.error("File not found: %s" % (str(e))) + return jsonify({'message': 'File not found.'}), 404 return jsonify(response), 200 diff --git a/ptmd/api/queries/files/register.py b/ptmd/api/queries/files/register.py index c360f52..1b8ca4c 100644 --- a/ptmd/api/queries/files/register.py +++ b/ptmd/api/queries/files/register.py @@ -8,6 +8,7 @@ from flask import jsonify, Response, request from flask_jwt_extended import get_current_user +from ptmd.logger import LOGGER from ptmd.config import session from ptmd.database import Organisation, File, get_shipped_file from ptmd.lib import BatchUpdater, GoogleDriveConnector @@ -60,7 +61,7 @@ def register_gdrive_file() -> tuple[Response, int]: file_data: dict[str, str] | None = connector.upload_file(organisation.gdrive_id, filepath, filename) remove(filepath) if not file_data: - raise ValueError(f"File '{file_id}' could not be uploaded.") + raise ValueError(f"File '{file_id}' could not be uploaded.") # TODO: Test this file: File = File(gdrive_id=file_data['id'], name=filename, @@ -71,8 +72,11 @@ def register_gdrive_file() -> tuple[Response, int]: session.commit() msg: str = f'file {file_id} was successfully created with internal id {file.file_id}' return jsonify({"message": msg, "file": dict(file)}), 201 - except Exception as e: + except ValueError as e: return jsonify({"message": str(e)}), 400 + except Exception as e: + LOGGER.error("Registration error: %s" % (str(e))) + return jsonify({"message": 'An unexpected error occurred.'}), 500 def change_batch(new_batch: str, species: str, filepath: str, filename: str) -> tuple[Response, int] | str: diff --git a/ptmd/api/queries/files/shipment.py b/ptmd/api/queries/files/shipment.py index dbabdb7..982c320 100644 --- a/ptmd/api/queries/files/shipment.py +++ b/ptmd/api/queries/files/shipment.py @@ -35,7 +35,7 @@ def ship_data(file_id: int) -> tuple[Response, int]: except PermissionError: return jsonify({'message': f'File {file_id} could not be locked but has been sent anyway'}), 200 except ValueError as e: - LOGGER.error("Value error: %s", str(e)) + LOGGER.error("Value error: %s" % (str(e))) return jsonify({'message': 'File is not in the correct state.'}), 400 except Exception: session.rollback() @@ -60,10 +60,10 @@ def receive_data(file_id: int) -> tuple[Response, int]: except BatchError as e: return e.serialize() except PermissionError as e: - LOGGER.error("User is not an admin: %s", str(e)) + LOGGER.error("User is not an admin: %s" % (str(e))) return jsonify({'message': 'You do not have permission to perform this action.'}), 403 except ValueError as e: - LOGGER.error("File not in correct state: %s", str(e)) + LOGGER.error("File not in correct state: %s" % (str(e))) return jsonify({'message': 'File is not in the correct state.'}), 400 diff --git a/ptmd/api/queries/users.py b/ptmd/api/queries/users.py index badc38c..0661562 100644 --- a/ptmd/api/queries/users.py +++ b/ptmd/api/queries/users.py @@ -145,7 +145,7 @@ def enable_account(token: str) -> tuple[Response, int]: user.set_role('enabled') return jsonify(msg="Account enabled. An email has been to an admin to validate your account."), 200 except Exception as e: - LOGGER.error("Account not enabled: %s", e) + LOGGER.error("Account not enabled: %s" % (str(e))) return jsonify(msg='Failed to enable your account. Please contact an admin for assistance.'), 400 diff --git a/tests/test_api/test_queries/test_files/test_isa.py b/tests/test_api/test_queries/test_files/test_isa.py index fcaebd3..1bb8b16 100644 --- a/tests/test_api/test_queries/test_files/test_isa.py +++ b/tests/test_api/test_queries/test_files/test_isa.py @@ -29,7 +29,7 @@ def test_convert_error_400(self, mock_jwt_verify_flask, mock_jwt_verify_utils, m mock_convert.side_effect = ValueError('A 400 error.') with app.test_client() as client: response = client.get('/api/files/1/isa', headers=HEADERS) - self.assertEqual(response.json, {'message': 'A 400 error.'}) + self.assertEqual(response.json, {'message': 'File conversion failed.'}) self.assertEqual(response.status_code, 400) def test_convert_success(self, mock_jwt_verify_flask, mock_jwt_verify_utils, mock_user): diff --git a/tests/test_api/test_queries/test_files/test_register.py b/tests/test_api/test_queries/test_files/test_register.py index 4604317..8ee51d0 100644 --- a/tests/test_api/test_queries/test_files/test_register.py +++ b/tests/test_api/test_queries/test_files/test_register.py @@ -150,6 +150,30 @@ def test_register_file_database_errors(self, mock_gdrive, mock_get_user, self.assertEqual(response.json, {'message': "File '123' does not exist."}) self.assertEqual(response.status_code, 400) + + # TODO: Fix this test, which should check line 65 in register.py + ''' + @patch('ptmd.api.queries.files.register.get_current_user') + @patch('ptmd.api.queries.utils.verify_jwt_in_request') + @patch('flask_jwt_extended.view_decorators.verify_jwt_in_request') + @patch('ptmd.api.queries.utils.get_current_user') + @patch('ptmd.api.queries.files.register.GoogleDriveConnector', return_value=MockGoogleDrive()) + def test_register_no_file_data(self, mock_gdrive, mock_get_user, + mock_jwt_in_request, mock_verify_jwt, mock_get_current_user): + mock_get_user().role = 'admin' + mock_get_current_user().id = 1 + mock_gdrive.upload_file = ['banana', 'banana'] + with app.test_client() as test_client: + response = test_client.post('api/files/register', data=json_dumps({ + 'file_id': '123', + 'batch': 'AA', + 'organism': 'human', + 'partner': 'UOB' + }), headers=HEADERS) + self.assertEqual(response.json, {'message': "File '123' could not be uploaded."}) + self.assertEqual(response.status_code, 400) + ''' + @patch('ptmd.api.queries.files.register.get_current_user') @patch('ptmd.api.queries.utils.verify_jwt_in_request') @patch('flask_jwt_extended.view_decorators.verify_jwt_in_request') @@ -266,3 +290,36 @@ def test_change_batch_error_412_success(self, mock_shipped_file, mock_batch_upda mock_rm.assert_not_called() mock_batch_updated.assert_called_with(batch='AA', filepath='filepath') self.assertEqual(response, 'filenameAA') + + + @patch('ptmd.api.queries.files.register.session') + @patch('ptmd.api.queries.files.register.GoogleDriveConnector', return_value=MockGoogleDriveError()) + @patch('flask_jwt_extended.view_decorators.verify_jwt_in_request') + @patch('ptmd.api.queries.files.register.File') + @patch('ptmd.api.queries.utils.verify_jwt_in_request') + @patch('ptmd.api.queries.utils.get_current_user') + @patch('ptmd.api.queries.files.register.Organisation') + @patch('ptmd.api.queries.files.register.get_current_user') + @patch('ptmd.api.queries.files.register.extract_data_from_spreadsheet') + @patch('ptmd.api.queries.files.register.remove') + @patch('ptmd.api.queries.files.register.get_shipped_file') + def test_register_file_error_upload(self, mocked_shipped_file, mock_rm, mock_data, mock_user, mock_organisation, + mock_get_user, mock_jwt_in_request, mock_file, + mock_verify_jwt, mock_gdrive, mock_session): + mock_get_user().role = 'admin' + mock_file.return_value.file_id = '123' + mocked_shipped_file.return_value = None + + mock_organisation.query.filter().first.return_value.gdrive_id = '123' + mock_organisation.query.filter().first.return_value.name = 'test' + mock_user().id = 1 + mocked_shipped_file.side_effect = Exception() + + with app.test_client() as client: + with patch('ptmd.api.queries.files.register.jsonify') as mock_jsonify: + external_file = {'file_id': '123', 'batch': 'AA', 'organism': 'human', 'partner': 'partner'} + response = client.post('/api/files/register', + headers={'Authorization': f'Bearer {123}', **HEADERS}, + data=json_dumps(external_file)) + mock_jsonify.assert_called_once_with({'message': 'An unexpected error occurred.'}) + self.assertEqual(response.status_code, 500) From 940488fbf497c8177bb355cb61b50c0f94d2bf80 Mon Sep 17 00:00:00 2001 From: Milo Thurston Date: Mon, 24 Jun 2024 14:27:51 +0100 Subject: [PATCH 4/6] Removed unused test. --- .../test_queries/test_files/test_register.py | 23 ------------------- 1 file changed, 23 deletions(-) diff --git a/tests/test_api/test_queries/test_files/test_register.py b/tests/test_api/test_queries/test_files/test_register.py index 8ee51d0..523fb32 100644 --- a/tests/test_api/test_queries/test_files/test_register.py +++ b/tests/test_api/test_queries/test_files/test_register.py @@ -151,29 +151,6 @@ def test_register_file_database_errors(self, mock_gdrive, mock_get_user, self.assertEqual(response.status_code, 400) - # TODO: Fix this test, which should check line 65 in register.py - ''' - @patch('ptmd.api.queries.files.register.get_current_user') - @patch('ptmd.api.queries.utils.verify_jwt_in_request') - @patch('flask_jwt_extended.view_decorators.verify_jwt_in_request') - @patch('ptmd.api.queries.utils.get_current_user') - @patch('ptmd.api.queries.files.register.GoogleDriveConnector', return_value=MockGoogleDrive()) - def test_register_no_file_data(self, mock_gdrive, mock_get_user, - mock_jwt_in_request, mock_verify_jwt, mock_get_current_user): - mock_get_user().role = 'admin' - mock_get_current_user().id = 1 - mock_gdrive.upload_file = ['banana', 'banana'] - with app.test_client() as test_client: - response = test_client.post('api/files/register', data=json_dumps({ - 'file_id': '123', - 'batch': 'AA', - 'organism': 'human', - 'partner': 'UOB' - }), headers=HEADERS) - self.assertEqual(response.json, {'message': "File '123' could not be uploaded."}) - self.assertEqual(response.status_code, 400) - ''' - @patch('ptmd.api.queries.files.register.get_current_user') @patch('ptmd.api.queries.utils.verify_jwt_in_request') @patch('flask_jwt_extended.view_decorators.verify_jwt_in_request') From 553e98f57a7dc66571f910a8acd3dce49a82ea0b Mon Sep 17 00:00:00 2001 From: Milo Thurston Date: Mon, 24 Jun 2024 14:33:17 +0100 Subject: [PATCH 5/6] Renamed test. #128 --- tests/test_api/test_queries/test_files/test_register.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_api/test_queries/test_files/test_register.py b/tests/test_api/test_queries/test_files/test_register.py index 523fb32..04f7fb3 100644 --- a/tests/test_api/test_queries/test_files/test_register.py +++ b/tests/test_api/test_queries/test_files/test_register.py @@ -280,7 +280,7 @@ def test_change_batch_error_412_success(self, mock_shipped_file, mock_batch_upda @patch('ptmd.api.queries.files.register.extract_data_from_spreadsheet') @patch('ptmd.api.queries.files.register.remove') @patch('ptmd.api.queries.files.register.get_shipped_file') - def test_register_file_error_upload(self, mocked_shipped_file, mock_rm, mock_data, mock_user, mock_organisation, + def test_register_file_exception_upload(self, mocked_shipped_file, mock_rm, mock_data, mock_user, mock_organisation, mock_get_user, mock_jwt_in_request, mock_file, mock_verify_jwt, mock_gdrive, mock_session): mock_get_user().role = 'admin' From 48c37d2af6bc46286f4686d421b36d7481e7997f Mon Sep 17 00:00:00 2001 From: Milo Thurston Date: Mon, 24 Jun 2024 14:47:42 +0100 Subject: [PATCH 6/6] Linting. --- tests/test_api/test_queries/test_files/test_register.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/test_api/test_queries/test_files/test_register.py b/tests/test_api/test_queries/test_files/test_register.py index 04f7fb3..70c5565 100644 --- a/tests/test_api/test_queries/test_files/test_register.py +++ b/tests/test_api/test_queries/test_files/test_register.py @@ -150,7 +150,6 @@ def test_register_file_database_errors(self, mock_gdrive, mock_get_user, self.assertEqual(response.json, {'message': "File '123' does not exist."}) self.assertEqual(response.status_code, 400) - @patch('ptmd.api.queries.files.register.get_current_user') @patch('ptmd.api.queries.utils.verify_jwt_in_request') @patch('flask_jwt_extended.view_decorators.verify_jwt_in_request') @@ -268,7 +267,6 @@ def test_change_batch_error_412_success(self, mock_shipped_file, mock_batch_upda mock_batch_updated.assert_called_with(batch='AA', filepath='filepath') self.assertEqual(response, 'filenameAA') - @patch('ptmd.api.queries.files.register.session') @patch('ptmd.api.queries.files.register.GoogleDriveConnector', return_value=MockGoogleDriveError()) @patch('flask_jwt_extended.view_decorators.verify_jwt_in_request') @@ -281,8 +279,8 @@ def test_change_batch_error_412_success(self, mock_shipped_file, mock_batch_upda @patch('ptmd.api.queries.files.register.remove') @patch('ptmd.api.queries.files.register.get_shipped_file') def test_register_file_exception_upload(self, mocked_shipped_file, mock_rm, mock_data, mock_user, mock_organisation, - mock_get_user, mock_jwt_in_request, mock_file, - mock_verify_jwt, mock_gdrive, mock_session): + mock_get_user, mock_jwt_in_request, mock_file, + mock_verify_jwt, mock_gdrive, mock_session): mock_get_user().role = 'admin' mock_file.return_value.file_id = '123' mocked_shipped_file.return_value = None