From 727274ae33ac21481024764fa6176c057c5037ee Mon Sep 17 00:00:00 2001 From: Carlos Lopez Date: Mon, 25 May 2020 15:40:24 -0600 Subject: [PATCH 1/4] Using full approval payload to update record --- crc/api/approval.py | 20 +++++++++----------- crc/models/approval.py | 14 ++++++++++++++ tests/test_approvals_api.py | 37 ++++++++++++++++++++++++++++++++----- 3 files changed, 55 insertions(+), 16 deletions(-) diff --git a/crc/api/approval.py b/crc/api/approval.py index 5d1feea6..78e7a050 100644 --- a/crc/api/approval.py +++ b/crc/api/approval.py @@ -1,3 +1,5 @@ +from crc import app, db, session + from crc.api.common import ApiError, ApiErrorSchema from crc.models.approval import Approval, ApprovalModel, ApprovalSchema from crc.services.approval_service import ApprovalService @@ -13,18 +15,14 @@ def update_approval(approval_id, body): if approval_id is None: raise ApiError('unknown_approval', 'Please provide a valid Approval ID.') - approver_uid = body.get('approver_uid') - status = body.get('status') - - if approver_uid is None: - raise ApiError('bad_formed_approval', 'Please provide a valid Approver UID') - if status is None: - raise ApiError('bad_formed_approval', 'Please provide a valid status for approval update') - - db_approval = ApprovalService.update_approval(approval_id, approver_uid, status) - if db_approval is None: + approval_model = session.query(ApprovalModel).get(approval_id) + if approval_model is None: raise ApiError('unknown_approval', 'The approval "' + str(approval_id) + '" is not recognized.') - approval = Approval.from_model(db_approval) + approval: Approval = ApprovalSchema().load(body) + approval.update_model(approval_model) + session.add(approval_model) + session.commit() + result = ApprovalSchema().dump(approval) return result diff --git a/crc/models/approval.py b/crc/models/approval.py index 37262e54..9fd9a627 100644 --- a/crc/models/approval.py +++ b/crc/models/approval.py @@ -1,4 +1,5 @@ import enum +import marshmallow from marshmallow import INCLUDE from sqlalchemy import func @@ -45,8 +46,12 @@ class ApprovalModel(db.Model): class Approval(object): + def __init__(self, **kwargs): + self.__dict__.update(kwargs) + @classmethod def from_model(cls, model: ApprovalModel): + # TODO: Reduce the code by iterating over model's dict keys instance = cls() instance.id = model.id instance.study_id = model.study_id @@ -79,6 +84,11 @@ class Approval(object): return instance + def update_model(self, study_model: StudyModel): + for k,v in self.__dict__.items(): + if not k.startswith('_'): + study_model.__dict__[k] = v + class ApprovalSchema(ma.Schema): class Meta: @@ -87,6 +97,10 @@ class ApprovalSchema(ma.Schema): "version", "status", "approver", "associated_files"] unknown = INCLUDE + @marshmallow.post_load + def make_approval(self, data, **kwargs): + """Loads the basic approval data for updates to the database""" + return Approval(**data) # Carlos: Here is the data structure I was trying to imagine. # If I were to continue down my current traing of thought, I'd create diff --git a/tests/test_approvals_api.py b/tests/test_approvals_api.py index b0453732..e582aa91 100644 --- a/tests/test_approvals_api.py +++ b/tests/test_approvals_api.py @@ -5,6 +5,34 @@ from crc import app, db, session from crc.models.approval import ApprovalModel +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, + '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""" @@ -34,13 +62,12 @@ class TestApprovals(BaseTest): def test_update_approval(self): """Approval status will be updated""" - body = { - 'approver_uid': 'rvr98', - 'status': 'DECLINED' - } approval_id = self.approval.id + data = dict(APPROVAL_PAYLOAD) + data['id'] = approval_id + data = json.dumps(data) rv = self.app.put(f'/v1.0/approval/{approval_id}', content_type="application/json", headers=self.logged_in_headers(), - data=json.dumps(body)) + data=data) self.assert_success(rv) From 1231b963d08f7d2b7dc44aa23f8101d0cd9be7eb Mon Sep 17 00:00:00 2001 From: Carlos Lopez Date: Mon, 25 May 2020 17:30:16 -0600 Subject: [PATCH 2/4] Enabling ldap lookup --- crc/models/approval.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/crc/models/approval.py b/crc/models/approval.py index 9fd9a627..ce5b367e 100644 --- a/crc/models/approval.py +++ b/crc/models/approval.py @@ -5,9 +5,11 @@ from marshmallow import INCLUDE from sqlalchemy import func from crc import db, ma +from crc.api.common import ApiError from crc.models.file import FileModel from crc.models.study import StudyModel from crc.models.workflow import WorkflowModel +from crc.services.ldap_service import LdapService class ApprovalStatus(enum.Enum): @@ -68,11 +70,18 @@ class Approval(object): instance.title = model.study.title # TODO: Use ldap lookup - instance.approver = {} - instance.approver['uid'] = 'bgb22' - instance.approver['display_name'] = 'Billy Bob (bgb22)' - instance.approver['title'] = 'E42:He\'s a hoopy frood' - instance.approver['department'] = 'E0:EN-Eng Study of Parallel Universes' + ldap_service = LdapService() + try: + user_info = ldap_service.user_info(model.approver_uid) + except ApiError: + user_info = None + + if user_info: + instance.approver = {} + 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 instance.associated_files = [] for approval_file in model.approval_files: From 72b59deeaf4686933ff4fd578e80547a0b6e94ab Mon Sep 17 00:00:00 2001 From: Carlos Lopez Date: Tue, 26 May 2020 10:21:36 -0600 Subject: [PATCH 3/4] Completing tests --- crc/api/approval.py | 5 ++++- crc/models/approval.py | 1 - tests/test_approvals_api.py | 39 +++++++++++++++++++++++++++++++++---- 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/crc/api/approval.py b/crc/api/approval.py index 78e7a050..c51634e4 100644 --- a/crc/api/approval.py +++ b/crc/api/approval.py @@ -6,7 +6,10 @@ from crc.services.approval_service import ApprovalService def get_approvals(approver_uid = None): - db_approvals = ApprovalService.get_all_approvals() + if not approver_uid: + db_approvals = ApprovalService.get_all_approvals() + else: + db_approvals = ApprovalService.get_approvals_per_user(approver_uid) approvals = [Approval.from_model(approval_model) for approval_model in db_approvals] results = ApprovalSchema(many=True).dump(approvals) return results diff --git a/crc/models/approval.py b/crc/models/approval.py index ce5b367e..d5fc334d 100644 --- a/crc/models/approval.py +++ b/crc/models/approval.py @@ -69,7 +69,6 @@ class Approval(object): if model.study: instance.title = model.study.title - # TODO: Use ldap lookup ldap_service = LdapService() try: user_info = ldap_service.user_info(model.approver_uid) diff --git a/tests/test_approvals_api.py b/tests/test_approvals_api.py index e582aa91..8cc14d14 100644 --- a/tests/test_approvals_api.py +++ b/tests/test_approvals_api.py @@ -2,7 +2,7 @@ import json from tests.base_test import BaseTest from crc import app, db, session -from crc.models.approval import ApprovalModel +from crc.models.approval import ApprovalModel, ApprovalSchema, ApprovalStatus APPROVAL_PAYLOAD = { @@ -43,23 +43,54 @@ class TestApprovals(BaseTest): self.approval = ApprovalModel( study=self.study, workflow=self.workflow, - approver_uid='bgb22', - status='WAITING', # TODO: Use enumerate options + approver_uid='arc93', + status=ApprovalStatus.WAITING.value, version=1 ) session.add(self.approval) + + self.approval_2 = ApprovalModel( + study=self.study, + workflow=self.workflow, + approver_uid='dhf8r', + status=ApprovalStatus.WAITING.value, + version=1 + ) + session.add(self.approval_2) + session.commit() def test_list_approvals_per_approver(self): """Only approvals associated with approver should be returned""" - rv = self.app.get('/v1.0/approval', headers=self.logged_in_headers()) + approver_uid = self.approval_2.approver_uid + rv = self.app.get(f'/v1.0/approval?approver_uid={approver_uid}', headers=self.logged_in_headers()) self.assert_success(rv) + response = json.loads(rv.get_data(as_text=True)) + + # Stored approvals are 2 + approvals_count = ApprovalModel.query.count() + self.assertEqual(approvals_count, 2) + + # but Dan's approvals should be only 1 + self.assertEqual(len(response), 1) + + # Confirm approver UID matches returned payload + approval = ApprovalSchema().load(response[0]) + self.assertEqual(approval.approver['uid'], approver_uid) + def test_list_approvals_per_admin(self): """All approvals will be returned""" 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)) + + # Returned approvals should match what's in the db + approvals_count = ApprovalModel.query.count() + response_count = len(response) + self.assertEqual(approvals_count, response_count) + def test_update_approval(self): """Approval status will be updated""" approval_id = self.approval.id From 7ed9411c7476f28e41128a7f5beab09e3049c375 Mon Sep 17 00:00:00 2001 From: Carlos Lopez Date: Tue, 26 May 2020 11:29:24 -0600 Subject: [PATCH 4/4] Handling unavailability of ldap connection --- crc/models/approval.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crc/models/approval.py b/crc/models/approval.py index d5fc334d..90fa14e2 100644 --- a/crc/models/approval.py +++ b/crc/models/approval.py @@ -4,6 +4,8 @@ import marshmallow from marshmallow import INCLUDE from sqlalchemy import func +from ldap3.core.exceptions import LDAPSocketOpenError + from crc import db, ma from crc.api.common import ApiError from crc.models.file import FileModel @@ -69,10 +71,10 @@ class Approval(object): if model.study: instance.title = model.study.title - ldap_service = LdapService() try: + ldap_service = LdapService() user_info = ldap_service.user_info(model.approver_uid) - except ApiError: + except (ApiError, LDAPSocketOpenError) as exception: user_info = None if user_info: