From d39ef658a2c97c2968402e8627da4dc407f194b9 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Sat, 23 May 2020 15:08:17 -0400 Subject: [PATCH] Made some modifications to the Approval so that it knows exactly what versions of every file are being sent for approval Added the following columns: * date_created - so we know when the file was created * renamed workflow_version to just "version", because everything has a version, this is the version of the request. * workflow_hash - this is just a quick way to see what files and versions are associated with the request, it could be factored out. * study - a quick relationship link to the study, so that this model is easier to use. * workflow - ditto * approval_files - these is a list from a new link table that links an approval to specific files and versions. The RequestApproval is logically sound, but still needs some additional pieces in place to be callable from a BPMN workflow diagram. Altered the file service to pick up on changes to files vs adding new files, so that versions are picked up correctly as users modify their submission - adding new files or replacing existing ones. Deleting files worries me, and I will need to revisit this. The damn base test keeps giving me a headache, so I made changes there to see if clearing and dropping the database each time won't allow the tests to pass more consistently. Lots more tests around the file service to make sure it is versioning user uploaded files correctly. The "Test Request Approval Script" tries to find to assure the correct behavior as this is likely to be called many times repeatedly and with little knowledge of the internal system. So it should just "do the right thing". --- crc/models/approval.py | 25 +++++-- crc/models/file.py | 1 + crc/models/workflow.py | 2 - crc/scripts/request_review.py | 71 ++++++++++++++++++ crc/services/file_service.py | 59 ++++++++++----- tests/base_test.py | 30 ++++++-- tests/test_file_service.py | 76 ++++++++++++++++++++ tests/test_request_approval_script.py | 100 ++++++++++++++++++++++++++ 8 files changed, 337 insertions(+), 27 deletions(-) create mode 100644 crc/scripts/request_review.py create mode 100644 tests/test_file_service.py create mode 100644 tests/test_request_approval_script.py diff --git a/crc/models/approval.py b/crc/models/approval.py index 4860bf59..0a67caa5 100644 --- a/crc/models/approval.py +++ b/crc/models/approval.py @@ -1,8 +1,10 @@ import enum from marshmallow import INCLUDE +from sqlalchemy import func from crc import db, ma +from crc.models.file import FileModel from crc.models.study import StudyModel from crc.models.workflow import WorkflowModel @@ -14,16 +16,32 @@ class ApprovalStatus(enum.Enum): CANCELED = "CANCELED" # The document was replaced with a new version and this review is no longer needed. +class ApprovalFile(db.Model): + id = db.Column(db.Integer, primary_key=True) + file_id = db.Column(db.Integer, db.ForeignKey(FileModel.id), nullable=False) + approval_id = db.Column(db.Integer, db.ForeignKey("approval.id"), nullable=False) + file_version = db.Column(db.Integer, nullable=False) + + approval = db.relationship("ApprovalModel") + file = db.relationship(FileModel) + + class ApprovalModel(db.Model): __tablename__ = 'approval' id = db.Column(db.Integer, primary_key=True) study_id = db.Column(db.Integer, db.ForeignKey(StudyModel.id), nullable=False) - study = db.relationship(StudyModel, backref='approval') workflow_id = db.Column(db.Integer, db.ForeignKey(WorkflowModel.id), nullable=False) - workflow_version = db.Column(db.String) approver_uid = db.Column(db.String) # Not linked to user model, as they may not have logged in yet. status = db.Column(db.String) message = db.Column(db.String) + date_created = db.Column(db.DateTime(timezone=True), default=func.now()) + version = db.Column(db.Integer) # Incremented integer, so 1,2,3 as requests are made. + workflow_hash = db.Column(db.String) # A hash of the workflow at the moment the approval was created. + + study = db.relationship(StudyModel) + workflow = db.relationship(WorkflowModel) + approval_files = db.relationship(ApprovalFile, back_populates="approval") + class Approval(object): @@ -31,9 +49,8 @@ class Approval(object): @classmethod def from_model(cls, model: ApprovalModel): instance = cls() - instance.id = model.id - instance.workflow_version = model.workflow_version + instance.version = model.version instance.approver_uid = model.approver_uid instance.status = model.status instance.study_id = model.study_id diff --git a/crc/models/file.py b/crc/models/file.py index 942b2a80..a96583b8 100644 --- a/crc/models/file.py +++ b/crc/models/file.py @@ -85,6 +85,7 @@ class FileModel(db.Model): latest_version = db.Column(db.Integer, default=0) + class FileModelSchema(SQLAlchemyAutoSchema): class Meta: model = FileModel diff --git a/crc/models/workflow.py b/crc/models/workflow.py index 7d690a4b..9029ac6b 100644 --- a/crc/models/workflow.py +++ b/crc/models/workflow.py @@ -80,5 +80,3 @@ class WorkflowModel(db.Model): total_tasks = db.Column(db.Integer, default=0) completed_tasks = db.Column(db.Integer, default=0) last_updated = db.Column(db.DateTime) - # todo: Add a version that represents the files associated with this workflow - # version = "32" \ No newline at end of file diff --git a/crc/scripts/request_review.py b/crc/scripts/request_review.py new file mode 100644 index 00000000..b08dbdfb --- /dev/null +++ b/crc/scripts/request_review.py @@ -0,0 +1,71 @@ +from datetime import datetime + +from sqlalchemy import desc + +from crc import db +from crc.models.approval import ApprovalModel, ApprovalStatus, ApprovalFile +from crc.scripts.script import Script +from crc.services.file_service import FileService + + +class RequestApproval(Script): + """This still needs to be fully wired up as a Script task callable from the workflow + But the basic logic is here just to get the tests passing and logic sound. """ + + def get_description(self): + return "Creates an approval request on this workflow, by the given approver_uid" + + def add_approval(self, study_id, workflow_id, approver_uid): + """we might have multiple approvals for a workflow, so I would expect this + method to get called many times.""" + + # Find any existing approvals for this workflow and approver. + latest_approval_request = db.session.query(ApprovalModel).\ + filter(ApprovalModel.workflow_id == workflow_id). \ + filter(ApprovalModel.approver_uid == approver_uid). \ + order_by(desc(ApprovalModel.version)).first() + + # Construct as hash of the latest files to see if things have changed since + # the last approval. + latest_files = FileService.get_workflow_files(workflow_id) + current_workflow_hash = self.generate_workflow_hash(latest_files) + + # If an existing approval request exists and no changes were made, do nothing. + # If there is an existing approval request for a previous version of the workflow + # then add a new request, and cancel any waiting/pending requests. + if latest_approval_request: + # We could just compare the ApprovalFile lists here and do away with this hash. + if latest_approval_request.workflow_hash == current_workflow_hash: + return # This approval already exists. + else: + latest_approval_request.status = ApprovalStatus.CANCELED.value + db.session.add(latest_approval_request) + version = latest_approval_request.version + 1 + else: + version = 1 + + model = ApprovalModel(study_id=study_id, workflow_id=workflow_id, + approver_uid=approver_uid, status=ApprovalStatus.WAITING.value, + message="", date_created=datetime.now(), + version=version, workflow_hash=current_workflow_hash) + approval_files = self.create_approval_files(latest_files, model) + db.session.add(model) + db.session.add_all(approval_files) + db.session.commit() + + def create_approval_files(self, files, approval): + """Currently based exclusively on the status of files associated with a workflow.""" + file_approval_models = [] + for file in files: + file_approval_models.append(ApprovalFile(file_id=file.id, + approval=approval, + file_version=file.latest_version)) + return file_approval_models + + def generate_workflow_hash(self, files): + """Currently based exclusively on the status of files associated with a workflow.""" + version_array = [] + for file in files: + version_array.append(str(file.id) + "[" + str(file.latest_version) + "]") + full_version = "-".join(version_array) + return full_version diff --git a/crc/services/file_service.py b/crc/services/file_service.py index 2ac92bdb..ef16fee5 100644 --- a/crc/services/file_service.py +++ b/crc/services/file_service.py @@ -48,14 +48,24 @@ class FileService(object): "When uploading files, the form field id must match a known document in the " "irb_docunents.xslx reference file. This code is not found in that file '%s'" % form_field_key) - file_model = FileModel( - study_id=study_id, - workflow_id=workflow_id, - task_id=task_id, - name=name, - form_field_key=form_field_key, - irb_doc_code=form_field_key - ) + """Assure this is unique to the workflow, task, and document code AND the Name + Because we will allow users to upload multiple files for the same form field + in some cases """ + file_model = session.query(FileModel)\ + .filter(FileModel.workflow_id == workflow_id)\ + .filter(FileModel.task_id == str(task_id))\ + .filter(FileModel.name == name)\ + .filter(FileModel.irb_doc_code == form_field_key).first() + + if not file_model: + file_model = FileModel( + study_id=study_id, + workflow_id=workflow_id, + task_id=task_id, + name=name, + form_field_key=form_field_key, + irb_doc_code=form_field_key + ) return FileService.update_file(file_model, binary_data, content_type) @staticmethod @@ -78,17 +88,31 @@ class FileService(object): @staticmethod def add_task_file(study_id, workflow_id, workflow_spec_id, task_id, name, content_type, binary_data, irb_doc_code=None): - """Create a new file and associate it with an executing task within a workflow.""" - file_model = FileModel( - study_id=study_id, - workflow_id=workflow_id, - workflow_spec_id=workflow_spec_id, - task_id=task_id, - name=name, - irb_doc_code=irb_doc_code - ) + + """Assure this is unique to the workflow, task, and document code. Disregard name.""" + file_model = session.query(FileModel)\ + .filter(FileModel.workflow_id == workflow_id)\ + .filter(FileModel.task_id == str(task_id))\ + .filter(FileModel.irb_doc_code == irb_doc_code).first() + + if not file_model: + """Create a new file and associate it with an executing task within a workflow.""" + file_model = FileModel( + study_id=study_id, + workflow_id=workflow_id, + workflow_spec_id=workflow_spec_id, + task_id=task_id, + name=name, + irb_doc_code=irb_doc_code + ) return FileService.update_file(file_model, binary_data, content_type) + @staticmethod + def get_workflow_files(workflow_id): + """Returns all the file models associated with a running workflow.""" + return session.query(FileModel).filter(FileModel.workflow_id == workflow_id).\ + order_by(FileModel.id).all() + @staticmethod def add_reference_file(name, content_type, binary_data): """Create a file with the given name, but not associated with a spec or workflow. @@ -109,6 +133,7 @@ class FileService(object): return file_extension.lower().strip()[1:] @staticmethod + def update_file(file_model, binary_data, content_type): session.flush() # Assure the database is up-to-date before running this. diff --git a/tests/base_test.py b/tests/base_test.py index 990e72cf..d7a89242 100644 --- a/tests/base_test.py +++ b/tests/base_test.py @@ -85,7 +85,7 @@ class BaseTest(unittest.TestCase): @classmethod def tearDownClass(cls): cls.ctx.pop() - session.remove() + db.drop_all() pass def setUp(self): @@ -151,7 +151,7 @@ class BaseTest(unittest.TestCase): def load_test_spec(dir_name, master_spec=False, category_id=None): """Loads a spec into the database based on a directory in /tests/data""" if session.query(WorkflowSpecModel).filter_by(id=dir_name).count() > 0: - return + return session.query(WorkflowSpecModel).filter_by(id=dir_name).first() filepath = os.path.join(app.root_path, '..', 'tests', 'data', dir_name, "*") return ExampleDataLoader().create_spec(id=dir_name, name=dir_name, filepath=filepath, master_spec=master_spec, category_id=category_id) @@ -205,9 +205,31 @@ class BaseTest(unittest.TestCase): content_type = CONTENT_TYPES[file_extension[1:]] file_service.update_file(file_model, data, content_type) + def create_user(self, uid="dhf8r", email="daniel.h.funk@gmail.com", display_name="Hoopy Frood"): + user = session.query(UserModel).filter(UserModel.uid == uid).first() + if user is None: + user = UserModel(uid=uid, email_address=email, display_name=display_name) + db.session.add(user) + db.session.commit() + return user + + def create_study(self, uid="dhf8r", title="Beer conception in the bipedal software engineer"): + study = session.query(StudyModel).first() + if study is None: + user = self.create_user(uid=uid) + study = StudyModel(title=title, protocol_builder_status=ProtocolBuilderStatus.ACTIVE, + user_uid=user.uid) + db.session.add(study) + db.session.commit() + return study + def create_workflow(self, workflow_name, study=None, category_id=None): - if study == None: - study = session.query(StudyModel).first() + db.session.flush() + workflow = db.session.query(WorkflowSpecModel).filter(WorkflowSpecModel.name == workflow_name).first() + if workflow: + return workflow + if study is None: + study = self.create_study() spec = self.load_test_spec(workflow_name, category_id=category_id) workflow_model = StudyService._create_workflow_model(study, spec) return workflow_model diff --git a/tests/test_file_service.py b/tests/test_file_service.py new file mode 100644 index 00000000..29026117 --- /dev/null +++ b/tests/test_file_service.py @@ -0,0 +1,76 @@ +from tests.base_test import BaseTest +from crc.services.file_service import FileService +from crc.services.workflow_processor import WorkflowProcessor + + +class TestFileService(BaseTest): + """Largely tested via the test_file_api, and time is tight, but adding new tests here.""" + + def test_add_file_from_task_increments_version_and_replaces_on_subsequent_add(self): + self.load_example_data() + self.create_reference_document() + workflow = self.create_workflow('file_upload_form') + processor = WorkflowProcessor(workflow) + task = processor.next_task() + irb_code = "UVACompl_PRCAppr" # The first file referenced in pb required docs. + FileService.add_task_file(study_id=workflow.study_id, workflow_id=workflow.id, + workflow_spec_id=workflow.workflow_spec_id, + task_id=task.id, + name="anything.png", content_type="text", + binary_data=b'1234', irb_doc_code=irb_code) + # Add the file again with different data + FileService.add_task_file(study_id=workflow.study_id, workflow_id=workflow.id, + workflow_spec_id=workflow.workflow_spec_id, + task_id=task.id, + name="anything.png", content_type="text", + binary_data=b'5678', irb_doc_code=irb_code) + + file_models = FileService.get_workflow_files(workflow_id=workflow.id) + self.assertEquals(1, len(file_models)) + self.assertEquals(2, file_models[0].latest_version) + + def test_add_file_from_form_increments_version_and_replaces_on_subsequent_add_with_same_name(self): + self.load_example_data() + self.create_reference_document() + workflow = self.create_workflow('file_upload_form') + processor = WorkflowProcessor(workflow) + task = processor.next_task() + irb_code = "UVACompl_PRCAppr" # The first file referenced in pb required docs. + FileService.add_form_field_file(study_id=workflow.study_id, workflow_id=workflow.id, + task_id=task.id, + form_field_key=irb_code, + name="anything.png", content_type="text", + binary_data=b'1234') + # Add the file again with different data + FileService.add_form_field_file(study_id=workflow.study_id, workflow_id=workflow.id, + form_field_key=irb_code, + task_id=task.id, + name="anything.png", content_type="text", + binary_data=b'5678') + + file_models = FileService.get_workflow_files(workflow_id=workflow.id) + self.assertEquals(1, len(file_models)) + self.assertEquals(2, file_models[0].latest_version) + + def test_add_file_from_form_allows_multiple_files_with_different_names(self): + self.load_example_data() + self.create_reference_document() + workflow = self.create_workflow('file_upload_form') + processor = WorkflowProcessor(workflow) + task = processor.next_task() + irb_code = "UVACompl_PRCAppr" # The first file referenced in pb required docs. + FileService.add_form_field_file(study_id=workflow.study_id, workflow_id=workflow.id, + task_id=task.id, + form_field_key=irb_code, + name="anything.png", content_type="text", + binary_data=b'1234') + # Add the file again with different data + FileService.add_form_field_file(study_id=workflow.study_id, workflow_id=workflow.id, + form_field_key=irb_code, + task_id=task.id, + name="a_different_thing.png", content_type="text", + binary_data=b'5678') + file_models = FileService.get_workflow_files(workflow_id=workflow.id) + self.assertEquals(2, len(file_models)) + self.assertEquals(1, file_models[0].latest_version) + self.assertEquals(1, file_models[1].latest_version) \ No newline at end of file diff --git a/tests/test_request_approval_script.py b/tests/test_request_approval_script.py new file mode 100644 index 00000000..81855c39 --- /dev/null +++ b/tests/test_request_approval_script.py @@ -0,0 +1,100 @@ +from tests.base_test import BaseTest + +from crc.services.file_service import FileService +from crc.services.workflow_processor import WorkflowProcessor + +from crc.scripts.request_review import RequestApproval +from crc import db +from crc.models.approval import ApprovalModel + + +class TestRequestApprovalScript(BaseTest): + + def test_create_approval_record(self): + workflow = self.create_workflow("empty_workflow") + script = RequestApproval() + script.add_approval(study_id=workflow.study_id, workflow_id=workflow.id, approver_uid="dhf8r") + self.assertEquals(1, db.session.query(ApprovalModel).count()) + model = db.session.query(ApprovalModel).first() + self.assertEquals(workflow.study_id, model.study_id) + self.assertEquals(workflow.id, model.workflow_id) + self.assertEquals("dhf8r", model.approver_uid) + self.assertEquals(1, model.version) + self.assertIsNotNone(model.workflow_hash) + + def test_new_requests_dont_add_if_approval_exists_for_current_workflow(self): + workflow = self.create_workflow("empty_workflow") + script = RequestApproval() + script.add_approval(study_id=workflow.study_id, workflow_id=workflow.id, approver_uid="dhf8r") + script.add_approval(study_id=workflow.study_id, workflow_id=workflow.id, approver_uid="dhf8r") + self.assertEquals(1, db.session.query(ApprovalModel).count()) + model = db.session.query(ApprovalModel).first() + self.assertEquals(1, model.version) + + def test_new_approval_requests_after_file_modification_create_new_requests(self): + self.load_example_data() + self.create_reference_document() + workflow = self.create_workflow('empty_workflow') + processor = WorkflowProcessor(workflow) + task = processor.next_task() + + script = RequestApproval() + script.add_approval(study_id=workflow.study_id, workflow_id=workflow.id, approver_uid="dhf8r") + + irb_code_1 = "UVACompl_PRCAppr" # The first file referenced in pb required docs. + FileService.add_task_file(study_id=workflow.study_id, workflow_id=workflow.id, + workflow_spec_id=workflow.workflow_spec_id, + task_id=task.id, + name="anything.png", content_type="text", + binary_data=b'5678', irb_doc_code=irb_code_1) + + script.add_approval(study_id=workflow.study_id, workflow_id=workflow.id, approver_uid="dhf8r") + self.assertEquals(2, db.session.query(ApprovalModel).count()) + models = db.session.query(ApprovalModel).order_by(ApprovalModel.version).all() + self.assertEquals(1, models[0].version) + self.assertEquals(2, models[1].version) + + + def test_generate_workflow_hash_and_version(self): + self.load_example_data() + self.create_reference_document() + workflow = self.create_workflow('empty_workflow') + processor = WorkflowProcessor(workflow) + task = processor.next_task() + irb_code_1 = "UVACompl_PRCAppr" # The first file referenced in pb required docs. + irb_code_2 = "NonUVAIRB_AssuranceForm" # The second file in above. + # Add a task file to the workflow. + FileService.add_task_file(study_id=workflow.study_id, workflow_id=workflow.id, + workflow_spec_id=workflow.workflow_spec_id, + task_id=task.id, + name="anything.png", content_type="text", + binary_data=b'5678', irb_doc_code=irb_code_1) + # Add a two form field files with the same irb_code, but + FileService.add_form_field_file(study_id=workflow.study_id, workflow_id=workflow.id, + task_id=task.id, + form_field_key=irb_code_2, + name="anything.png", content_type="text", + binary_data=b'1234') + FileService.add_form_field_file(study_id=workflow.study_id, workflow_id=workflow.id, + form_field_key=irb_code_2, + task_id=task.id, + name="another_anything.png", content_type="text", + binary_data=b'5678') + + script = RequestApproval() + + # Workflow hash should look be id[1]-id[1]-id[1] + # Sould be three files, each with a version of 1. + # where id is the file id, which we don't know, thus the regex. + latest_files = FileService.get_workflow_files(workflow.id) + self.assertRegexpMatches(script.generate_workflow_hash(latest_files), "\d+\[1\]-\d+\[1\]-\d+\[1\]") + + # Replace last file + # should now be id[1]-id[1]-id[2] + FileService.add_form_field_file(study_id=workflow.study_id, workflow_id=workflow.id, + form_field_key=irb_code_2, + task_id=task.id, + name="another_anything.png", content_type="text", + binary_data=b'9999') + self.assertRegexpMatches(script.generate_workflow_hash(latest_files), "\d+\[1\]-\d+\[1\]-\d+\[2\]") +