diff --git a/Pipfile b/Pipfile index 17497132..6a5e31dd 100644 --- a/Pipfile +++ b/Pipfile @@ -38,6 +38,7 @@ xlrd = "*" ldap3 = "*" gunicorn = "*" werkzeug = "*" +sentry-sdk = {extras = ["flask"],version = "==0.14.4"} [requires] python_version = "3.7" diff --git a/Pipfile.lock b/Pipfile.lock index ce620efc..aca63a57 100644 --- a/Pipfile.lock +++ b/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "979f996148ee181e3e0af2a3777aa1d00d0fd5d943d49df65963e694b8a88871" + "sha256": "54d9d51360f54762138a3acc7696badd1d711e7b1dde9e2d82aa706e40c17102" }, "pipfile-spec": 6, "requires": { @@ -32,10 +32,10 @@ }, "amqp": { "hashes": [ - "sha256:6e649ca13a7df3faacdc8bbb280aa9a6602d22fd9d545336077e573a1f4ff3b8", - "sha256:77f1aef9410698d20eaeac5b73a87817365f457a507d82edf292e12cbb83b08d" + "sha256:24dbaff8ce4f30566bb88976b398e8c4e77637171af3af6f1b9650f48890e60b", + "sha256:bb68f8d2bced8f93ccfd07d96c689b716b3227720add971be980accfc2952139" ], - "version": "==2.5.2" + "version": "==2.6.0" }, "aniso8601": { "hashes": [ @@ -96,12 +96,18 @@ ], "version": "==3.6.3.0" }, + "blinker": { + "hashes": [ + "sha256:471aee25f3992bd325afa3772f1063dbdbbca947a041b8b89466dc00d606f8b6" + ], + "version": "==1.4" + }, "celery": { "hashes": [ - "sha256:108a0bf9018a871620936c33a3ee9f6336a89f8ef0a0f567a9001f4aa361415f", - "sha256:5b4b37e276033fe47575107a2775469f0b721646a08c96ec2c61531e4fe45f2a" + "sha256:5147662e23dc6bc39c17a2cbc9a148debe08ecfb128b0eded14a0d9c81fc5742", + "sha256:df2937b7536a2a9b18024776a3a46fd281721813636c03a5177fa02fe66078f6" ], - "version": "==4.4.2" + "version": "==4.4.3" }, "certifi": { "hashes": [ @@ -381,10 +387,10 @@ }, "kombu": { "hashes": [ - "sha256:2d1cda774126a044d91a7ff5fa6d09edf99f46924ab332a810760fe6740e9b76", - "sha256:598e7e749d6ab54f646b74b2d2df67755dee13894f73ab02a2a9feb8870c7cb2" + "sha256:ab0afaa5388dd2979cbc439d3623b86a4f7a58d41f621096bef7767c37bc2505", + "sha256:aece08f48706743aaa1b9d607fee300559481eafcc5ee56451aa0ef867a3be07" ], - "version": "==4.6.8" + "version": "==4.6.9" }, "ldap3": { "hashes": [ @@ -704,6 +710,17 @@ "index": "pypi", "version": "==2.23.0" }, + "sentry-sdk": { + "extras": [ + "flask" + ], + "hashes": [ + "sha256:0e5e947d0f7a969314aa23669a94a9712be5a688ff069ff7b9fc36c66adc160c", + "sha256:799a8bf76b012e3030a881be00e97bc0b922ce35dde699c6537122b751d80e2c" + ], + "index": "pypi", + "version": "==0.14.4" + }, "six": { "hashes": [ "sha256:30639c035cdb23534cd4aa2dd52c3bf48f06e5f4a941509c8bafd8ce11080259", @@ -838,10 +855,10 @@ }, "waitress": { "hashes": [ - "sha256:045b3efc3d97c93362173ab1dfc159b52cfa22b46c3334ffc805dbdbf0e4309e", - "sha256:77ff3f3226931a1d7d8624c5371de07c8e90c7e5d80c5cc660d72659aaf23f38" + "sha256:1bb436508a7487ac6cb097ae7a7fe5413aefca610550baf58f0940e51ecfb261", + "sha256:3d633e78149eb83b60a07dfabb35579c29aac2d24bb803c18b26fb2ab1a584db" ], - "version": "==1.4.3" + "version": "==1.4.4" }, "webob": { "hashes": [ @@ -966,10 +983,10 @@ }, "wcwidth": { "hashes": [ - "sha256:cafe2186b3c009a04067022ce1dcd79cb38d8d65ee4f4791b8888d6599d1bbe1", - "sha256:ee73862862a156bf77ff92b09034fc4825dd3af9cf81bc5b360668d425f3c5f1" + "sha256:3de2e41158cb650b91f9654cbf9a3e053cee0719c9df4ddc11e4b568669e9829", + "sha256:b651b6b081476420e4e9ae61239ac4c1b49d0c5ace42b2e81dc2ff49ed50c566" ], - "version": "==0.1.9" + "version": "==0.2.2" }, "zipp": { "hashes": [ diff --git a/config/default.py b/config/default.py index e368b32d..289c506f 100644 --- a/config/default.py +++ b/config/default.py @@ -13,6 +13,9 @@ DEVELOPMENT = environ.get('DEVELOPMENT', default="true") == "true" TESTING = environ.get('TESTING', default="false") == "true" PRODUCTION = (environ.get('PRODUCTION', default="false") == "true") or (not DEVELOPMENT and not TESTING) +# Sentry flag +ENABLE_SENTRY = environ.get('ENABLE_SENTRY', default="false") == "true" + # Add trailing slash to base path APPLICATION_ROOT = re.sub(r'//', '/', '/%s/' % environ.get('APPLICATION_ROOT', default="/").strip('/')) diff --git a/crc/__init__.py b/crc/__init__.py index fe510daf..e705321b 100644 --- a/crc/__init__.py +++ b/crc/__init__.py @@ -1,11 +1,13 @@ import logging import os +import sentry_sdk import connexion from flask_cors import CORS from flask_marshmallow import Marshmallow from flask_migrate import Migrate from flask_sqlalchemy import SQLAlchemy +from sentry_sdk.integrations.flask import FlaskIntegration logging.basicConfig(level=logging.INFO) @@ -40,6 +42,12 @@ connexion_app.add_api('api.yml', base_path='/v1.0') origins_re = [r"^https?:\/\/%s(.*)" % o.replace('.', '\.') for o in app.config['CORS_ALLOW_ORIGINS']] cors = CORS(connexion_app.app, origins=origins_re) +if app.config['ENABLE_SENTRY']: + sentry_sdk.init( + dsn="https://25342ca4e2d443c6a5c49707d68e9f40@o401361.ingest.sentry.io/5260915", + integrations=[FlaskIntegration()] + ) + print('=== USING THESE CONFIG SETTINGS: ===') print('DB_HOST = ', ) print('CORS_ALLOW_ORIGINS = ', app.config['CORS_ALLOW_ORIGINS']) diff --git a/crc/api.yml b/crc/api.yml index 758169b7..f89f7a92 100644 --- a/crc/api.yml +++ b/crc/api.yml @@ -808,12 +808,12 @@ paths: $ref: "#/components/schemas/Script" /approval: parameters: - - name: approver_uid + - name: everything in: query required: false - description: Restrict results to a given approver uid, maybe we restrict the use of this at somepoint. + description: If set to true, returns all the approvals known to the system. schema: - type: string + type: boolean get: operationId: crc.api.approval.get_approvals summary: Provides a list of workflows approvals diff --git a/crc/api/approval.py b/crc/api/approval.py index 32238cf0..5a4299e2 100644 --- a/crc/api/approval.py +++ b/crc/api/approval.py @@ -1,3 +1,5 @@ +from flask import g + from crc import app, db, session from crc.api.common import ApiError, ApiErrorSchema @@ -5,11 +7,11 @@ from crc.models.approval import Approval, ApprovalModel, ApprovalSchema from crc.services.approval_service import ApprovalService -def get_approvals(approver_uid=None): - if not approver_uid: +def get_approvals(everything=False): + if everything: db_approvals = ApprovalService.get_all_approvals() else: - db_approvals = ApprovalService.get_approvals_per_user(approver_uid) + db_approvals = ApprovalService.get_approvals_per_user(g.user.uid) approvals = [Approval.from_model(approval_model) for approval_model in db_approvals] results = ApprovalSchema(many=True).dump(approvals) @@ -32,6 +34,9 @@ def update_approval(approval_id, body): raise ApiError('unknown_approval', 'The approval "' + str(approval_id) + '" is not recognized.') approval: Approval = ApprovalSchema().load(body) + if approval_model.approver_uid != g.user.uid: + raise ApiError("not_your_approval", "You may not modify this approval. It belongs to another user.") + approval.update_model(approval_model) session.commit() diff --git a/crc/api/user.py b/crc/api/user.py index afa2e894..b9315f89 100644 --- a/crc/api/user.py +++ b/crc/api/user.py @@ -1,8 +1,5 @@ -import json - -import connexion import flask -from flask import redirect, g, request +from flask import g, request from crc import app, db from crc.api.common import ApiError diff --git a/crc/models/approval.py b/crc/models/approval.py index 1f7eed38..3a95c680 100644 --- a/crc/models/approval.py +++ b/crc/models/approval.py @@ -68,24 +68,33 @@ class Approval(object): if model.study: instance.title = model.study.title - principal_investigator_id = model.study.primary_investigator_id instance.approver = {} try: ldap_service = LdapService() - user_info = ldap_service.user_info(principal_investigator_id) - except (ApiError, LDAPSocketOpenError) as exception: - user_info = None - instance.approver['display_name'] = 'Primary Investigator details' - instance.approver['department'] = 'currently not available' - - if user_info: - # TODO: Rename approver to primary investigator + user_info = ldap_service.user_info(model.approver_uid) instance.approver['uid'] = model.approver_uid instance.approver['display_name'] = user_info.display_name instance.approver['title'] = user_info.title instance.approver['department'] = user_info.department + except (ApiError, LDAPSocketOpenError) as exception: + user_info = None + instance.approver['display_name'] = 'Unknown' + instance.approver['department'] = 'currently not available' + + instance.primary_investigator = {} + try: + ldap_service = LdapService() + user_info = ldap_service.user_info(model.study.primary_investigator_id) + instance.primary_investigator['uid'] = model.approver_uid + instance.primary_investigator['display_name'] = user_info.display_name + instance.primary_investigator['title'] = user_info.title + instance.primary_investigator['department'] = user_info.department + except (ApiError, LDAPSocketOpenError) as exception: + user_info = None + instance.primary_investigator['display_name'] = 'Primary Investigator details' + instance.primary_investigator['department'] = 'currently not available' + - # TODO: Organize it properly, move it to services doc_dictionary = FileService.get_reference_data(FileService.DOCUMENT_LIST, 'code', ['id']) instance.associated_files = [] @@ -98,12 +107,13 @@ class Approval(object): associated_file['id'] = approval_file.file_data.file_model.id if extra_info: irb_doc_code = approval_file.file_data.file_model.irb_doc_code - associated_file['name'] = '_'.join((irb_doc_code, approval_file.file_data.file_model.name)) + associated_file['name'] = '_'.join((extra_info['category1'], + approval_file.file_data.file_model.name)) associated_file['description'] = extra_info['description'] else: associated_file['name'] = approval_file.file_data.file_model.name associated_file['description'] = 'No description available' - associated_file['name'] = '(' + principal_investigator_id + ')' + associated_file['name'] + associated_file['name'] = '(' + model.study.primary_investigator_id + ')' + associated_file['name'] associated_file['content_type'] = approval_file.file_data.file_model.content_type instance.associated_files.append(associated_file) @@ -118,7 +128,8 @@ class ApprovalSchema(ma.Schema): class Meta: model = Approval fields = ["id", "study_id", "workflow_id", "version", "title", - "version", "status", "message", "approver", "associated_files"] + "status", "message", "approver", "primary_investigator", + "associated_files", "date_created"] unknown = INCLUDE @marshmallow.post_load diff --git a/crc/scripts/request_approval.py b/crc/scripts/request_approval.py index 1df1a670..1e5d2c6c 100644 --- a/crc/scripts/request_approval.py +++ b/crc/scripts/request_approval.py @@ -26,7 +26,8 @@ RequestApproval approver1 "dhf8r" ApprovalService.add_approval(study_id, workflow_id, args) elif isinstance(uids, list): for id in uids: - ApprovalService.add_approval(study_id, workflow_id, id) + if id: ## Assure it's not empty or null + ApprovalService.add_approval(study_id, workflow_id, id) def get_uids(self, task, args): if len(args) < 1: diff --git a/package-lock.json b/package-lock.json new file mode 100644 index 00000000..48e341a0 --- /dev/null +++ b/package-lock.json @@ -0,0 +1,3 @@ +{ + "lockfileVersion": 1 +} diff --git a/tests/test_approvals_api.py b/tests/test_approvals_api.py index b9b6d226..89eb4ab2 100644 --- a/tests/test_approvals_api.py +++ b/tests/test_approvals_api.py @@ -5,35 +5,6 @@ from crc import app, db, session from crc.models.approval import ApprovalModel, ApprovalSchema, ApprovalStatus -APPROVAL_PAYLOAD = { - 'id': None, - 'approver': { - 'uid': 'bgb22', - 'display_name': 'Billy Bob (bgb22)', - 'title': 'E42:He\'s a hoopy frood', - 'department': 'E0:EN-Eng Study of Parallel Universes' - }, - 'title': 'El Study', - 'status': 'DECLINED', - 'version': 1, - 'message': 'Incorrect documents', - 'associated_files': [ - { - 'id': 42, - 'name': 'File 1', - 'content_type': 'document' - }, - { - 'id': 43, - 'name': 'File 2', - 'content_type': 'document' - } - ], - 'workflow_id': 1, - 'study_id': 1 -} - - class TestApprovals(BaseTest): def setUp(self): """Initial setup shared by all TestApprovals tests""" @@ -64,7 +35,7 @@ class TestApprovals(BaseTest): def test_list_approvals_per_approver(self): """Only approvals associated with approver should be returned""" approver_uid = self.approval_2.approver_uid - rv = self.app.get(f'/v1.0/approval?approver_uid={approver_uid}', headers=self.logged_in_headers()) + rv = self.app.get(f'/v1.0/approval', headers=self.logged_in_headers()) self.assert_success(rv) response = json.loads(rv.get_data(as_text=True)) @@ -82,7 +53,7 @@ class TestApprovals(BaseTest): def test_list_approvals_per_admin(self): """All approvals will be returned""" - rv = self.app.get('/v1.0/approval', headers=self.logged_in_headers()) + rv = self.app.get('/v1.0/approval?everything=true', headers=self.logged_in_headers()) self.assert_success(rv) response = json.loads(rv.get_data(as_text=True)) @@ -90,24 +61,67 @@ class TestApprovals(BaseTest): # Returned approvals should match what's in the db approvals_count = ApprovalModel.query.count() response_count = len(response) - self.assertEqual(approvals_count, response_count) + self.assertEqual(2, response_count) - def test_update_approval(self): - """Approval status will be updated""" - approval_id = self.approval.id - data = dict(APPROVAL_PAYLOAD) - data['id'] = approval_id + rv = self.app.get('/v1.0/approval', headers=self.logged_in_headers()) + self.assert_success(rv) + response = json.loads(rv.get_data(as_text=True)) + response_count = len(response) + self.assertEqual(1, response_count) - self.assertEqual(self.approval.status, ApprovalStatus.PENDING.value) + def test_update_approval_fails_if_not_the_approver(self): + approval = session.query(ApprovalModel).filter_by(approver_uid='arc93').first() + data = {'id': approval.id, + "approver_uid": "dhf8r", + 'message': "Approved. I like the cut of your jib.", + 'status': ApprovalStatus.APPROVED.value} - rv = self.app.put(f'/v1.0/approval/{approval_id}', + self.assertEqual(approval.status, ApprovalStatus.PENDING.value) + + rv = self.app.put(f'/v1.0/approval/{approval.id}', content_type="application/json", - headers=self.logged_in_headers(), + headers=self.logged_in_headers(), # As dhf8r + data=json.dumps(data)) + self.assert_failure(rv) + + def test_accept_approval(self): + approval = session.query(ApprovalModel).filter_by(approver_uid='dhf8r').first() + data = {'id': approval.id, + "approver_uid": "dhf8r", + 'message': "Approved. I like the cut of your jib.", + 'status': ApprovalStatus.APPROVED.value} + + self.assertEqual(approval.status, ApprovalStatus.PENDING.value) + + rv = self.app.put(f'/v1.0/approval/{approval.id}', + content_type="application/json", + headers=self.logged_in_headers(), # As dhf8r data=json.dumps(data)) self.assert_success(rv) - session.refresh(self.approval) + session.refresh(approval) # Updated record should now have the data sent to the endpoint - self.assertEqual(self.approval.message, data['message']) - self.assertEqual(self.approval.status, ApprovalStatus.DECLINED.value) + self.assertEqual(approval.message, data['message']) + self.assertEqual(approval.status, ApprovalStatus.APPROVED.value) + + def test_decline_approval(self): + approval = session.query(ApprovalModel).filter_by(approver_uid='dhf8r').first() + data = {'id': approval.id, + "approver_uid": "dhf8r", + 'message': "Approved. I find the cut of your jib lacking.", + 'status': ApprovalStatus.DECLINED.value} + + self.assertEqual(approval.status, ApprovalStatus.PENDING.value) + + rv = self.app.put(f'/v1.0/approval/{approval.id}', + content_type="application/json", + headers=self.logged_in_headers(), # As dhf8r + data=json.dumps(data)) + self.assert_success(rv) + + session.refresh(approval) + + # Updated record should now have the data sent to the endpoint + self.assertEqual(approval.message, data['message']) + self.assertEqual(approval.status, ApprovalStatus.DECLINED.value) diff --git a/tests/test_request_approval_script.py b/tests/test_request_approval_script.py index 2f4ab49e..8cd56807 100644 --- a/tests/test_request_approval_script.py +++ b/tests/test_request_approval_script.py @@ -1,6 +1,6 @@ -from crc.services.file_service import FileService from tests.base_test import BaseTest +from crc.services.file_service import FileService from crc.scripts.request_approval import RequestApproval from crc.services.workflow_processor import WorkflowProcessor from crc.api.common import ApiError @@ -26,6 +26,22 @@ class TestRequestApprovalScript(BaseTest): script.do_task(task, workflow.study_id, workflow.id, "study.approval1", "study.approval2") self.assertEquals(2, db.session.query(ApprovalModel).count()) + def test_do_task_with_blank_second_approver(self): + self.load_example_data() + self.create_reference_document() + workflow = self.create_workflow('empty_workflow') + processor = WorkflowProcessor(workflow) + task = processor.next_task() + task.data = {"study": {"approval1": "dhf8r", 'approval2':''}} + FileService.add_workflow_file(workflow_id=workflow.id, + irb_doc_code="UVACompl_PRCAppr", + name="anything.png", content_type="text", + binary_data=b'1234') + script = RequestApproval() + script.do_task(task, workflow.study_id, workflow.id, "study.approval1", "study.approval2") + self.assertEquals(1, db.session.query(ApprovalModel).count()) + + def test_do_task_with_incorrect_argument(self): """This script should raise an error if it can't figure out the approvers.""" self.load_example_data() diff --git a/tests/test_tasks_api.py b/tests/test_tasks_api.py index 67a644ef..37849867 100644 --- a/tests/test_tasks_api.py +++ b/tests/test_tasks_api.py @@ -10,7 +10,6 @@ from crc.models.api_models import WorkflowApiSchema, MultiInstanceType, TaskSche from crc.models.file import FileModelSchema from crc.models.stats import TaskEventModel from crc.models.workflow import WorkflowStatus -from crc.services.protocol_builder import ProtocolBuilderService from crc.services.workflow_service import WorkflowService