From c91e81e35624fc11798a16240e227d70ea7e9981 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 27 Sep 2021 17:15:53 -0400 Subject: [PATCH 01/21] Clean up file data. Always keep at least 1 version. Do not delete if in lookup or dependency tables --- crc/services/file_service.py | 46 ++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/crc/services/file_service.py b/crc/services/file_service.py index 4e9ae6d9..5e0e4bb7 100644 --- a/crc/services/file_service.py +++ b/crc/services/file_service.py @@ -545,3 +545,49 @@ class FileService(object): dmn_file = prefix + etree.tostring(root) return dmn_file + + @staticmethod + def cleanup_file_data(copies_to_keep=1): + if isinstance(copies_to_keep, int) and copies_to_keep > 0: + + deleted_models = [] + saved_models = [] + current_models = [] + + session.flush() + + workflow_spec_models = session.query(WorkflowSpecModel).all() + + for wf_spec_model in workflow_spec_models: + file_models = session.query(FileModel)\ + .filter(FileModel.workflow_spec_id == wf_spec_model.id)\ + .all() + # current_model = file_models[0] + for file_model in file_models: + file_data_models = session.query(FileDataModel)\ + .filter(FileDataModel.file_model_id == file_model.id)\ + .order_by(desc(FileDataModel.date_created))\ + .all() + current_models.append(file_data_models[:copies_to_keep]) + for fd_model in file_data_models[copies_to_keep:]: + dependencies = session.query(WorkflowSpecDependencyFile)\ + .filter(WorkflowSpecDependencyFile.file_data_id == fd_model.id)\ + .all() + if len(dependencies) > 0: + saved_models.append(fd_model) + continue + lookups = session.query(LookupFileModel)\ + .filter(LookupFileModel.file_data_model_id == fd_model.id)\ + .all() + if len(lookups) > 0: + saved_models.append(fd_model) + continue + deleted_models.append(fd_model) + # session.delete(fd_model) + + session.commit() + return current_models, saved_models, deleted_models + + else: + raise ApiError(code='bad_keep', + message='You must keep at least 1 version') From 0ee377dda3c9618d8f31240a14cde064672000b7 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 27 Sep 2021 17:16:21 -0400 Subject: [PATCH 02/21] Test for file data cleanup --- tests/files/test_file_data_cleanup.py | 145 ++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 tests/files/test_file_data_cleanup.py diff --git a/tests/files/test_file_data_cleanup.py b/tests/files/test_file_data_cleanup.py new file mode 100644 index 00000000..74a9f9a0 --- /dev/null +++ b/tests/files/test_file_data_cleanup.py @@ -0,0 +1,145 @@ +from tests.base_test import BaseTest + +from crc import session +from crc.models.file import FileModel, FileDataModel, LookupFileModel +from crc.models.workflow import WorkflowSpecModel, WorkflowSpecDependencyFile +from crc.services.file_service import FileService + +from sqlalchemy import desc + +import io +import json + + +class TestFileDataCleanup(BaseTest): + + xml_str_one = b""" + + + + + + + + + + + + """ + + xml_str_two = b""" + + + + Flow_1v0s5ht + + + # Hello + Flow_1v0s5ht + Flow_12k5ua1 + + + + Flow_12k5ua1 + + + + + + + + + + + + + + + + + + + + + + + + + + """ + + def test_file_data_cleanup(self): + """Update a file twice. Make sure we clean up the correct files""" + + self.load_example_data() + workflow = self.create_workflow('empty_workflow') + file_data_model_count = session.query(FileDataModel).count() + + # Use for comparison after cleanup + replaced_models = [] + + # Get workflow spec + workflow_spec_model = session.query(WorkflowSpecModel).first() + + # Add lookup file + data = {'file': (io.BytesIO(b'asdf'), 'lookup_1.xlsx')} + rv = self.app.post('/v1.0/file?workflow_spec_id=%s' % workflow_spec_model.id, data=data, follow_redirects=True, + content_type='multipart/form-data', headers=self.logged_in_headers()) + self.assert_success(rv) + file_json = json.loads(rv.get_data(as_text=True)) + file_id = file_json['id'] + lookup_model = LookupFileModel(file_data_model_id=file_id, + workflow_spec_id=workflow_spec_model.id) + session.add(lookup_model) + session.commit() + + + # Grab first file model + file_model = session.query(FileModel)\ + .filter(FileModel.workflow_spec_id == workflow_spec_model.id)\ + .first() + + # Grab the file data model we want to replace + old_file_data_model = session.query(FileDataModel)\ + .filter(FileDataModel.file_model_id == file_model.id)\ + .order_by(desc(FileDataModel.date_created))\ + .first() + + # Update first time + replaced_models.append(old_file_data_model) + data = {'file': (io.BytesIO(self.xml_str_one), 'test_bpmn_1.bpmn')} + rv = self.app.put('/v1.0/file/%i/data' % file_model.id, data=data, follow_redirects=True, + content_type='multipart/form-data', headers=self.logged_in_headers()) + self.assert_success(rv) + file_json = json.loads(rv.get_data(as_text=True)) + + # Grab the new file data model we want to replace + old_file_data_model = session.query(FileDataModel)\ + .filter(FileDataModel.file_model_id == file_model.id)\ + .order_by(desc(FileDataModel.date_created))\ + .first() + + # Update second time + replaced_models.append(old_file_data_model) + data = {'file': (io.BytesIO(self.xml_str_two), 'test_bpmn_1.bpmn')} + rv = self.app.put('/v1.0/file/%i/data' % file_json['id'], data=data, follow_redirects=True, + content_type='multipart/form-data', headers=self.logged_in_headers()) + self.assert_success(rv) + file_json = json.loads(rv.get_data(as_text=True)) + file_id = file_json['id'] + + # Add file to dependencies + wf_spec_depend_model = WorkflowSpecDependencyFile(file_data_id=file_id, + workflow_id=workflow.id) + session.add(wf_spec_depend_model) + session.commit() + + # Run the cleanup files process + current_models, saved_models, deleted_models = FileService.cleanup_file_data() + + # assert correct versions are removed + new_count = session.query(FileDataModel).count() + self.assertEqual(set(deleted_models), set(replaced_models)) + self.assertEqual(file_data_model_count, new_count) + + print('test_file_data_cleanup') From 29798f1ba64c795e0ac4d12fb94572aa1b7c9d30 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 28 Sep 2021 10:16:47 -0400 Subject: [PATCH 03/21] turned it on --- crc/services/file_service.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crc/services/file_service.py b/crc/services/file_service.py index 5e0e4bb7..aea6ba61 100644 --- a/crc/services/file_service.py +++ b/crc/services/file_service.py @@ -562,7 +562,7 @@ class FileService(object): file_models = session.query(FileModel)\ .filter(FileModel.workflow_spec_id == wf_spec_model.id)\ .all() - # current_model = file_models[0] + for file_model in file_models: file_data_models = session.query(FileDataModel)\ .filter(FileDataModel.file_model_id == file_model.id)\ @@ -583,7 +583,7 @@ class FileService(object): saved_models.append(fd_model) continue deleted_models.append(fd_model) - # session.delete(fd_model) + session.delete(fd_model) session.commit() return current_models, saved_models, deleted_models From 767a90faba12ef1ba3327e85ff8005d48bf7532e Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 28 Sep 2021 10:20:45 -0400 Subject: [PATCH 04/21] Removed debug print statement --- tests/files/test_file_data_cleanup.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/files/test_file_data_cleanup.py b/tests/files/test_file_data_cleanup.py index 74a9f9a0..76df91eb 100644 --- a/tests/files/test_file_data_cleanup.py +++ b/tests/files/test_file_data_cleanup.py @@ -93,7 +93,6 @@ class TestFileDataCleanup(BaseTest): session.add(lookup_model) session.commit() - # Grab first file model file_model = session.query(FileModel)\ .filter(FileModel.workflow_spec_id == workflow_spec_model.id)\ @@ -141,5 +140,3 @@ class TestFileDataCleanup(BaseTest): new_count = session.query(FileDataModel).count() self.assertEqual(set(deleted_models), set(replaced_models)) self.assertEqual(file_data_model_count, new_count) - - print('test_file_data_cleanup') From 89b8be075585f330a5806e5239574cff399a400f Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 28 Sep 2021 10:25:13 -0400 Subject: [PATCH 05/21] Take saved files into account during assertion --- tests/files/test_file_data_cleanup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/files/test_file_data_cleanup.py b/tests/files/test_file_data_cleanup.py index 76df91eb..5222339c 100644 --- a/tests/files/test_file_data_cleanup.py +++ b/tests/files/test_file_data_cleanup.py @@ -138,5 +138,5 @@ class TestFileDataCleanup(BaseTest): # assert correct versions are removed new_count = session.query(FileDataModel).count() - self.assertEqual(set(deleted_models), set(replaced_models)) + self.assertEqual(set(deleted_models).union(set(saved_models)), set(replaced_models)) self.assertEqual(file_data_model_count, new_count) From cc0b7853e3d3ee80f00e5f1e2ea67d869de10091 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 28 Sep 2021 13:12:16 -0400 Subject: [PATCH 06/21] reordered the setup and tests --- tests/files/test_file_data_cleanup.py | 93 ++++++++++++++------------- 1 file changed, 50 insertions(+), 43 deletions(-) diff --git a/tests/files/test_file_data_cleanup.py b/tests/files/test_file_data_cleanup.py index 5222339c..96110d09 100644 --- a/tests/files/test_file_data_cleanup.py +++ b/tests/files/test_file_data_cleanup.py @@ -78,8 +78,44 @@ class TestFileDataCleanup(BaseTest): # Use for comparison after cleanup replaced_models = [] - # Get workflow spec - workflow_spec_model = session.query(WorkflowSpecModel).first() + # Get `empty_workflow` workflow spec + workflow_spec_model = session.query(WorkflowSpecModel)\ + .filter(WorkflowSpecModel.name == 'empty_workflow')\ + .first() + + # Get file model for empty_workflow spec + file_model = session.query(FileModel)\ + .filter(FileModel.workflow_spec_id == workflow_spec_model.id)\ + .first() + + # Grab the file data model for empty_workflow file_model + original_file_data_model = session.query(FileDataModel)\ + .filter(FileDataModel.file_model_id == file_model.id)\ + .order_by(desc(FileDataModel.date_created))\ + .first() + + # Add file to dependencies + # It should not get deleted + wf_spec_depend_model = WorkflowSpecDependencyFile(file_data_id=original_file_data_model.id, + workflow_id=workflow.id) + session.add(wf_spec_depend_model) + session.commit() + + # Update first time + replaced_models.append(original_file_data_model) + data = {'file': (io.BytesIO(self.xml_str_one), file_model.name)} + rv = self.app.put('/v1.0/file/%i/data' % file_model.id, data=data, follow_redirects=True, + content_type='multipart/form-data', headers=self.logged_in_headers()) + self.assert_success(rv) + file_json_first = json.loads(rv.get_data(as_text=True)) + + # Update second time + # replaced_models.append(old_file_data_model) + data = {'file': (io.BytesIO(self.xml_str_two), file_model.name)} + rv = self.app.put('/v1.0/file/%i/data' % file_model.id, data=data, follow_redirects=True, + content_type='multipart/form-data', headers=self.logged_in_headers()) + self.assert_success(rv) + file_json_second = json.loads(rv.get_data(as_text=True)) # Add lookup file data = {'file': (io.BytesIO(b'asdf'), 'lookup_1.xlsx')} @@ -87,56 +123,27 @@ class TestFileDataCleanup(BaseTest): content_type='multipart/form-data', headers=self.logged_in_headers()) self.assert_success(rv) file_json = json.loads(rv.get_data(as_text=True)) - file_id = file_json['id'] - lookup_model = LookupFileModel(file_data_model_id=file_id, + lookup_file_id = file_json['id'] + lookup_data_model = session.query(FileDataModel).filter(FileDataModel.file_model_id == lookup_file_id).first() + lookup_model = LookupFileModel(file_data_model_id=lookup_data_model.id, workflow_spec_id=workflow_spec_model.id) session.add(lookup_model) session.commit() - # Grab first file model - file_model = session.query(FileModel)\ - .filter(FileModel.workflow_spec_id == workflow_spec_model.id)\ - .first() - - # Grab the file data model we want to replace - old_file_data_model = session.query(FileDataModel)\ - .filter(FileDataModel.file_model_id == file_model.id)\ - .order_by(desc(FileDataModel.date_created))\ - .first() - - # Update first time - replaced_models.append(old_file_data_model) - data = {'file': (io.BytesIO(self.xml_str_one), 'test_bpmn_1.bpmn')} - rv = self.app.put('/v1.0/file/%i/data' % file_model.id, data=data, follow_redirects=True, + # Update lookup file + data = {'file': (io.BytesIO(b'1234'), 'lookup_1.xlsx')} + rv = self.app.put('/v1.0/file/%i/data' % lookup_file_id, data=data, follow_redirects=True, content_type='multipart/form-data', headers=self.logged_in_headers()) self.assert_success(rv) - file_json = json.loads(rv.get_data(as_text=True)) - - # Grab the new file data model we want to replace - old_file_data_model = session.query(FileDataModel)\ - .filter(FileDataModel.file_model_id == file_model.id)\ - .order_by(desc(FileDataModel.date_created))\ - .first() - - # Update second time - replaced_models.append(old_file_data_model) - data = {'file': (io.BytesIO(self.xml_str_two), 'test_bpmn_1.bpmn')} - rv = self.app.put('/v1.0/file/%i/data' % file_json['id'], data=data, follow_redirects=True, - content_type='multipart/form-data', headers=self.logged_in_headers()) - self.assert_success(rv) - file_json = json.loads(rv.get_data(as_text=True)) - file_id = file_json['id'] - - # Add file to dependencies - wf_spec_depend_model = WorkflowSpecDependencyFile(file_data_id=file_id, - workflow_id=workflow.id) - session.add(wf_spec_depend_model) - session.commit() # Run the cleanup files process current_models, saved_models, deleted_models = FileService.cleanup_file_data() # assert correct versions are removed new_count = session.query(FileDataModel).count() - self.assertEqual(set(deleted_models).union(set(saved_models)), set(replaced_models)) - self.assertEqual(file_data_model_count, new_count) + self.assertEqual(8, new_count) + self.assertEqual(4, len(current_models)) + self.assertEqual(2, len(saved_models)) + self.assertEqual(1, len(deleted_models)) + + print('test_file_data_cleanup') From f44b9836ceb95c229d9832879fdaacbbd81fd609 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Thu, 30 Sep 2021 14:11:33 -0400 Subject: [PATCH 07/21] Add file cleanup to the scheduler --- crc/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crc/__init__.py b/crc/__init__.py index 31fb8ceb..e18d09e7 100644 --- a/crc/__init__.py +++ b/crc/__init__.py @@ -46,6 +46,7 @@ ma = Marshmallow(app) from crc import models from crc import api from crc.api import admin +from crc.services.file_service import FileService from crc.services.workflow_service import WorkflowService connexion_app.add_api('api.yml', base_path='/v1.0') @@ -56,6 +57,7 @@ def process_waiting_tasks(): WorkflowService.do_waiting() scheduler.add_job(process_waiting_tasks,'interval',minutes=1) +scheduler.add_job(FileService.cleanup_file_data, 'interval', minutes=1440) # once a day scheduler.start() From e72cf27fe3c06c68cc0597c6eb95c0bf8dec31fd Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 13 Oct 2021 11:41:44 -0400 Subject: [PATCH 08/21] Return email model from email script. --- crc/scripts/email.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crc/scripts/email.py b/crc/scripts/email.py index bca72827..8ab14f45 100644 --- a/crc/scripts/email.py +++ b/crc/scripts/email.py @@ -3,6 +3,7 @@ import traceback from crc import app, session from crc.api.common import ApiError +from crc.models.email import EmailModelSchema from crc.models.file import FileModel, CONTENT_TYPES from crc.models.workflow import WorkflowModel from crc.services.document_service import DocumentService @@ -92,7 +93,7 @@ email(subject="My Subject", recipients="user@example.com", attachments=['Study_A print(repr(traceback.format_exception(exc_type, exc_value, exc_traceback))) raise e - return email_model.id + return EmailModelSchema().dump(email_model) def get_email_addresses(self, users, study_id): emails = [] From dcb06f3753e983a27480c25c1ce461534c208a48 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 13 Oct 2021 11:42:01 -0400 Subject: [PATCH 09/21] Schema for returning email model --- crc/models/email.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crc/models/email.py b/crc/models/email.py index a549cfdb..cf2bb543 100644 --- a/crc/models/email.py +++ b/crc/models/email.py @@ -23,9 +23,8 @@ class EmailModel(db.Model): class EmailModelSchema(ma.Schema): - # TODO: clean this us. Do we need load_instance and unknown? + class Meta: model = EmailModel - load_instance = True - additional = ["id", "subject", "sender", "recipients", "timestamp"] - unknown = INCLUDE + fields = ["id", "subject", "sender", "recipients", "cc", "bcc", "content", "content_html", + "study_id", "timestamp", "workflow_spec_id"] From f1dbda7f95590a672f5b8fc833a1744f7caef69a Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 13 Oct 2021 11:46:57 -0400 Subject: [PATCH 10/21] Fix timestamp field to use (UTC) timezone --- .../ba6df7e560a1_modify_email_timestamp.py | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 migrations/versions/ba6df7e560a1_modify_email_timestamp.py diff --git a/migrations/versions/ba6df7e560a1_modify_email_timestamp.py b/migrations/versions/ba6df7e560a1_modify_email_timestamp.py new file mode 100644 index 00000000..58f870de --- /dev/null +++ b/migrations/versions/ba6df7e560a1_modify_email_timestamp.py @@ -0,0 +1,24 @@ +"""modify email timestamp + +Revision ID: ba6df7e560a1 +Revises: 6d8ceb1c18cb +Create Date: 2021-10-13 10:54:23.894034 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'ba6df7e560a1' +down_revision = '6d8ceb1c18cb' +branch_labels = None +depends_on = None + + +def upgrade(): + op.execute("alter table email alter column timestamp type timestamp with time zone") + + +def downgrade(): + op.execute("alter table email alter column timestamp type timestamp without time zone") From a1a06f06caebc40dea4f208bb3030d25b263160e Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 13 Oct 2021 11:47:40 -0400 Subject: [PATCH 11/21] We now get an email model back --- tests/data/email/email.bpmn | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/data/email/email.bpmn b/tests/data/email/email.bpmn index 40489409..389ba06d 100644 --- a/tests/data/email/email.bpmn +++ b/tests/data/email/email.bpmn @@ -19,15 +19,12 @@ Email content to be delivered to {{ ApprvlApprvr1 }} --- **Test Some Formatting** - _UVA Tracking Number:_ {{ 321 }} - - + _UVA Tracking Number:_ {{ 321 }} Flow_08n2npe Flow_1xlrgne subject="Camunda Email Subject" recipients=[ApprvlApprvr1,PIComputingID] -email_id = email(subject=subject,recipients=recipients) - +email_model = email(subject=subject,recipients=recipients) From bd6a2f30058fd159c32a9ea7d3217c373300c0e8 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 13 Oct 2021 11:48:27 -0400 Subject: [PATCH 12/21] Check the model we get back from the email script Make sure timestamp is UTC --- tests/emails/test_email_script.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/emails/test_email_script.py b/tests/emails/test_email_script.py index 0b1ec499..b84e8bbd 100644 --- a/tests/emails/test_email_script.py +++ b/tests/emails/test_email_script.py @@ -1,6 +1,7 @@ from tests.base_test import BaseTest from crc import mail from crc.models.email import EmailModel +import datetime class TestEmailScript(BaseTest): @@ -35,5 +36,16 @@ class TestEmailScript(BaseTest): # Correct From field self.assertEqual('uvacrconnect@virginia.edu', outbox[0].sender) - db_emails = EmailModel.query.count() - self.assertEqual(db_emails, 1) + # Make sure we log the email + # Grab model for comparison below + db_emails = EmailModel.query.all() + self.assertEqual(len(db_emails), 1) + + # Check whether we get a good email model back from the script + self.assertIn('email_model', workflow_api.next_task.data) + self.assertEqual(db_emails[0].recipients, workflow_api.next_task.data['email_model']['recipients']) + self.assertEqual(db_emails[0].sender, workflow_api.next_task.data['email_model']['sender']) + self.assertEqual(db_emails[0].subject, workflow_api.next_task.data['email_model']['subject']) + + # Make sure timestamp is UTC + self.assertEqual(db_emails[0].timestamp.tzinfo, datetime.timezone.utc) From 3ae00d190dfe3dc9abf338e498694b7876f8eb53 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 13 Oct 2021 12:03:48 -0400 Subject: [PATCH 13/21] Raise error if we have a problem sending email. This should be processed by Spiff ultimately. --- crc/services/email_service.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crc/services/email_service.py b/crc/services/email_service.py index 86810faa..20006827 100644 --- a/crc/services/email_service.py +++ b/crc/services/email_service.py @@ -47,9 +47,11 @@ class EmailService(object): msg.attach(file['name'], file['type'], file_data.data) mail.send(msg) + except Exception as e: app.logger.error('An exception happened in EmailService', exc_info=True) app.logger.error(str(e)) + raise e db.session.add(email_model) db.session.commit() From 771019d9cad2310c063ab3f7d4905fad660f8d1d Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 13 Oct 2021 12:05:08 -0400 Subject: [PATCH 14/21] Clarify call to email service. We now use the words content and content_html --- crc/api/tools.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crc/api/tools.py b/crc/api/tools.py index e889f73b..64ad8a0b 100644 --- a/crc/api/tools.py +++ b/crc/api/tools.py @@ -96,11 +96,12 @@ def evaluate_python_expression(body): except Exception as e: return {"result": False, "expression": body['expression'], "key": body['key'], "error": str(e)} + def send_test_email(subject, address, message, data=None): - rendered, wrapped = EmailService().get_rendered_content(message, data) + content, content_html = EmailService().get_rendered_content(message, data) EmailService.add_email( subject=subject, sender=DEFAULT_SENDER, recipients=[address], - content=rendered, - content_html=wrapped) + content=content, + content_html=content_html) From b3d515bf680b7a5a4c271632acfcc90897b9a97d Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 13 Oct 2021 13:23:18 -0400 Subject: [PATCH 15/21] Test for condition where email_service.add_email raises an exception. --- tests/emails/test_email_service.py | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/tests/emails/test_email_service.py b/tests/emails/test_email_service.py index 1cb36525..b045118f 100644 --- a/tests/emails/test_email_service.py +++ b/tests/emails/test_email_service.py @@ -3,6 +3,7 @@ from tests.base_test import BaseTest from crc import session from crc.models.email import EmailModel from crc.services.email_service import EmailService +from unittest.mock import patch class TestEmailService(BaseTest): @@ -10,7 +11,6 @@ class TestEmailService(BaseTest): def test_add_email(self): self.load_example_data() study = self.create_study() - workflow = self.create_workflow('random_fact') subject = 'Email Subject' sender = 'sender@sartography.com' @@ -42,3 +42,25 @@ class TestEmailService(BaseTest): self.assertEqual(email_model.content, content) self.assertEqual(email_model.content_html, content_html) self.assertEqual(email_model.study, None) + + @patch('crc.services.email_service.EmailService.add_email') + def test_add_email_with_error(self, mock_response): + + self.load_example_data() + mock_response.return_value.ok = True + mock_response.side_effect = Exception("This is my exception!") + + study = self.create_study() + + subject = 'Email Subject' + sender = 'sender@sartography.com' + recipients = ['recipient@sartography.com', 'back@sartography.com'] + content = 'Content for this email' + content_html = '

Hypertext Markup Language content for this email

' + + # Make sure we generate an error + with self.assertRaises(Exception) as e: + EmailService.add_email(subject=subject, sender=sender, recipients=recipients, + content=content, content_html=content_html, study_id=study.id) + # Make sure it's the error we want + self.assertEqual('This is my exception!', e.exception.args[0]) From 2f3fe59a0f96beabc1195ce42314a74a43194fe5 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 13 Oct 2021 13:40:14 -0400 Subject: [PATCH 16/21] Test for Exception when email service fails --- tests/emails/test_email_script.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/emails/test_email_script.py b/tests/emails/test_email_script.py index b84e8bbd..8176a9e0 100644 --- a/tests/emails/test_email_script.py +++ b/tests/emails/test_email_script.py @@ -2,6 +2,7 @@ from tests.base_test import BaseTest from crc import mail from crc.models.email import EmailModel import datetime +from unittest.mock import patch class TestEmailScript(BaseTest): @@ -49,3 +50,21 @@ class TestEmailScript(BaseTest): # Make sure timestamp is UTC self.assertEqual(db_emails[0].timestamp.tzinfo, datetime.timezone.utc) + + @patch('crc.services.email_service.EmailService.add_email') + def test_email_raises_exception(self, mock_response): + self.load_example_data() + mock_response.return_value.ok = True + mock_response.side_effect = Exception("This is my exception!") + + workflow = self.create_workflow('email') + + task_data = { + 'PIComputingID': 'dhf8r@virginia.edu', + 'ApprvlApprvr1': 'lb3dp@virginia.edu' + } + task = self.get_workflow_api(workflow).next_task + + with mail.record_messages() as outbox: + with self.assertRaises(Exception) as e: + self.complete_form(workflow, task, task_data) From 8015d35424ac9bb1800cc0d06dcf341a4065306f Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 13 Oct 2021 15:36:37 -0400 Subject: [PATCH 17/21] Script to get localtime from a UTC datetime --- crc/scripts/get_localtime.py | 19 ++++++ tests/data/get_localtime/get_localtime.bpmn | 72 +++++++++++++++++++++ tests/scripts/test_get_localtime.py | 21 ++++++ 3 files changed, 112 insertions(+) create mode 100644 crc/scripts/get_localtime.py create mode 100644 tests/data/get_localtime/get_localtime.bpmn create mode 100644 tests/scripts/test_get_localtime.py diff --git a/crc/scripts/get_localtime.py b/crc/scripts/get_localtime.py new file mode 100644 index 00000000..b5c46f0e --- /dev/null +++ b/crc/scripts/get_localtime.py @@ -0,0 +1,19 @@ +from crc.scripts.script import Script +from crc.api.common import ApiError + + +class GetLocaltime(Script): + + def get_description(self): + return """Converts a UTC Datetime object into a Datetime object with a different timezone. + Defaults to US/Eastern""" + + def do_task_validate_only(self, task, study_id, workflow_id, **kwargs): + if len(kwargs): + return True + raise ApiError(code='missing_timestamp', + message='You must include a timestamp to convert.') + + def do_task(self, task, study_id, workflow_id, **kwargs): + pass + diff --git a/tests/data/get_localtime/get_localtime.bpmn b/tests/data/get_localtime/get_localtime.bpmn new file mode 100644 index 00000000..ff8d1d2f --- /dev/null +++ b/tests/data/get_localtime/get_localtime.bpmn @@ -0,0 +1,72 @@ + + + + + Flow_0lnc9x0 + + + + timestamp = email_model.timestamp +localtime = get_localtime(timestamp) + Flow_0gtgzcf + Flow_0k1hbif + + + + # Timestamp +{{ timestamp }} + + +# Localtime +{{ localtime }} + Flow_0k1hbif + Flow_0kgtoh1 + + + + Flow_0kgtoh1 + + + + This is my email + Flow_0lnc9x0 + Flow_0gtgzcf + email_model = email(subject='My Email Subject', recipients='user@example.com') + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/scripts/test_get_localtime.py b/tests/scripts/test_get_localtime.py new file mode 100644 index 00000000..460481fa --- /dev/null +++ b/tests/scripts/test_get_localtime.py @@ -0,0 +1,21 @@ +from tests.base_test import BaseTest +from crc.models.file import FileDataModel +from crc import session + + +class TestGetLocaltime(BaseTest): + + def test_get_localtime(self): + self.load_example_data() + # file_model = session.query(FileDataModel).first() + # test_time = file_model.date_created + + workflow = self.create_workflow('get_localtime') + workflow_api = self.get_workflow_api(workflow) + task = workflow_api.next_task + # workflow_api = self.complete_form(workflow, task, {'timestamp': test_time}) + + # local_time = + + print('test_get_localtime') + From c2c79bd0144ad1be8c675c62b56f74d389484d21 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Thu, 14 Oct 2021 11:02:16 -0400 Subject: [PATCH 18/21] Convert UTC datetime object to a different timezone. JSON doesn't know about dates, so we have to return a string --- crc/scripts/get_localtime.py | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/crc/scripts/get_localtime.py b/crc/scripts/get_localtime.py index b5c46f0e..83fab223 100644 --- a/crc/scripts/get_localtime.py +++ b/crc/scripts/get_localtime.py @@ -1,5 +1,8 @@ -from crc.scripts.script import Script from crc.api.common import ApiError +from crc.scripts.script import Script + +import dateparser +import pytz class GetLocaltime(Script): @@ -8,12 +11,24 @@ class GetLocaltime(Script): return """Converts a UTC Datetime object into a Datetime object with a different timezone. Defaults to US/Eastern""" - def do_task_validate_only(self, task, study_id, workflow_id, **kwargs): - if len(kwargs): + def do_task_validate_only(self, task, study_id, workflow_id, *args, **kwargs): + if 'timestamp' in kwargs: return True raise ApiError(code='missing_timestamp', message='You must include a timestamp to convert.') - def do_task(self, task, study_id, workflow_id, **kwargs): - pass + def do_task(self, task, study_id, workflow_id, *args, **kwargs): + if 'timestamp' in kwargs: + timestamp = kwargs['timestamp'] + if 'timezone' in kwargs: + timezone = kwargs['timezone'] + else: + timezone = 'US/Eastern' + parsed_timestamp = dateparser.parse(timestamp) + localtime = parsed_timestamp.astimezone(pytz.timezone(timezone)) + return str(localtime) + + else: + raise ApiError(code='missing_timestamp', + message='You must include a timestamp to convert.') From fc3e7f818324894934c7a1c4cbaca07326b3c888 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Thu, 14 Oct 2021 11:02:51 -0400 Subject: [PATCH 19/21] Simple test for get_localtime script --- tests/data/get_localtime/get_localtime.bpmn | 46 +++++++++++---------- tests/scripts/test_get_localtime.py | 12 ++---- 2 files changed, 28 insertions(+), 30 deletions(-) diff --git a/tests/data/get_localtime/get_localtime.bpmn b/tests/data/get_localtime/get_localtime.bpmn index ff8d1d2f..43a136d5 100644 --- a/tests/data/get_localtime/get_localtime.bpmn +++ b/tests/data/get_localtime/get_localtime.bpmn @@ -5,23 +5,7 @@ Flow_0lnc9x0 - - timestamp = email_model.timestamp -localtime = get_localtime(timestamp) - Flow_0gtgzcf - Flow_0k1hbif - - - # Timestamp -{{ timestamp }} - - -# Localtime -{{ localtime }} - Flow_0k1hbif - Flow_0kgtoh1 - Flow_0kgtoh1 @@ -33,6 +17,24 @@ localtime = get_localtime(timestamp) Flow_0gtgzcf email_model = email(subject='My Email Subject', recipients='user@example.com') + + timestamp = email_model.timestamp +localtime = get_localtime(str(timestamp)) + Flow_0gtgzcf + Flow_0k1hbif + timestamp=email_model.timestamp +localtime = get_localtime(timestamp=timestamp) + + + # Timestamp +{{ timestamp }} + + +# Localtime +{{ localtime }} + Flow_0k1hbif + Flow_0kgtoh1 + @@ -55,18 +57,18 @@ localtime = get_localtime(timestamp) - - - - - - + + + + + + diff --git a/tests/scripts/test_get_localtime.py b/tests/scripts/test_get_localtime.py index 460481fa..7e5b489b 100644 --- a/tests/scripts/test_get_localtime.py +++ b/tests/scripts/test_get_localtime.py @@ -1,21 +1,17 @@ from tests.base_test import BaseTest -from crc.models.file import FileDataModel -from crc import session +from crc.scripts.get_localtime import GetLocaltime class TestGetLocaltime(BaseTest): def test_get_localtime(self): self.load_example_data() - # file_model = session.query(FileDataModel).first() - # test_time = file_model.date_created workflow = self.create_workflow('get_localtime') workflow_api = self.get_workflow_api(workflow) task = workflow_api.next_task - # workflow_api = self.complete_form(workflow, task, {'timestamp': test_time}) - # local_time = - - print('test_get_localtime') + timestamp = task.data['timestamp'] + localtime = task.data['localtime'] + self.assertEqual(localtime, GetLocaltime().do_task(None, None, None, timestamp=timestamp)) From 8de05b8fb687e3b95878a69d18e34404eb60133f Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 18 Oct 2021 11:19:56 -0400 Subject: [PATCH 20/21] Don't process attachments if argument is None or '' This can happen during workflow processing --- crc/scripts/email.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crc/scripts/email.py b/crc/scripts/email.py index bca72827..307cbda3 100644 --- a/crc/scripts/email.py +++ b/crc/scripts/email.py @@ -58,7 +58,8 @@ email(subject="My Subject", recipients="user@example.com", attachments=['Study_A bcc = self.get_email_addresses(kwargs['bcc'], study_id) if 'reply_to' in kwargs: reply_to = kwargs['reply_to'] - if 'attachments' in kwargs: + # Don't process if attachments is None or '' + if 'attachments' in kwargs and kwargs['attachments'] is not None and kwargs['attachments'] != '': files = self.get_files(kwargs['attachments'], study_id) else: From dc7b39b5c8ca652a9de7c1f85a9772f9b7d1fe06 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 18 Oct 2021 11:21:24 -0400 Subject: [PATCH 21/21] Tests and workflows for the cases where attachments is None or '' --- .../email_attachment_empty_string.bpmn | 84 +++++++++++++++++++ .../email_none_attachment.bpmn | 84 +++++++++++++++++++ tests/emails/test_email_script.py | 45 ++++++++++ 3 files changed, 213 insertions(+) create mode 100644 tests/data/email_attachment_empty_string/email_attachment_empty_string.bpmn create mode 100644 tests/data/email_none_attachment/email_none_attachment.bpmn diff --git a/tests/data/email_attachment_empty_string/email_attachment_empty_string.bpmn b/tests/data/email_attachment_empty_string/email_attachment_empty_string.bpmn new file mode 100644 index 00000000..e93e34a6 --- /dev/null +++ b/tests/data/email_attachment_empty_string/email_attachment_empty_string.bpmn @@ -0,0 +1,84 @@ + + + + + Flow_1synsig + + + # Dear Approver +## you have been requested for approval + + +--- +New request submitted by {{ PIComputingID }} + +Email content to be delivered to {{ ApprvlApprvr1 }} + +--- +**Test Some Formatting** + _UVA Tracking Number:_ {{ 321 }} + Flow_08n2npe + Flow_1ch3gt4 + attachments = '' +email_id = email(subject=subject,recipients=recipients, attachments=attachments) + + + + + + + + + + + Flow_1synsig + Flow_08n2npe + + + + Flow_1gei5cf + + + + # Email +{{ email_id }} + Flow_1ch3gt4 + Flow_1gei5cf + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/data/email_none_attachment/email_none_attachment.bpmn b/tests/data/email_none_attachment/email_none_attachment.bpmn new file mode 100644 index 00000000..ebfbc897 --- /dev/null +++ b/tests/data/email_none_attachment/email_none_attachment.bpmn @@ -0,0 +1,84 @@ + + + + + Flow_1synsig + + + # Dear Approver +## you have been requested for approval + + +--- +New request submitted by {{ PIComputingID }} + +Email content to be delivered to {{ ApprvlApprvr1 }} + +--- +**Test Some Formatting** + _UVA Tracking Number:_ {{ 321 }} + Flow_08n2npe + Flow_1ch3gt4 + attachments = None +email_id = email(subject=subject,recipients=recipients, attachments=attachments) + + + + + + + + + + + Flow_1synsig + Flow_08n2npe + + + + Flow_1gei5cf + + + + # Email +{{ email_id }} + Flow_1ch3gt4 + Flow_1gei5cf + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/emails/test_email_script.py b/tests/emails/test_email_script.py index 0b1ec499..cdc96f4e 100644 --- a/tests/emails/test_email_script.py +++ b/tests/emails/test_email_script.py @@ -37,3 +37,48 @@ class TestEmailScript(BaseTest): db_emails = EmailModel.query.count() self.assertEqual(db_emails, 1) + + def test_email_with_none_attachment(self): + # This workflow specifically sends `attachments = None` as a parameter + # to the email script + workflow = self.create_workflow('email_none_attachment') + workflow_api = self.get_workflow_api(workflow) + task = workflow_api.next_task + + with mail.record_messages() as outbox: + + workflow_api = self.complete_form(workflow, task, {'subject': 'My Test Email', + 'recipients': 'user@example.com'}) + task = workflow_api.next_task + # Make sure 'attachments' is in task.data, and is None + self.assertIn('attachments', task.data) + self.assertEqual(task.data['attachments'], None) + + # Make sure we still send an email + self.assertIn('email_id', task.data) + + self.assertEqual(len(outbox), 1) + self.assertEqual(outbox[0].subject, "My Test Email") + self.assertEqual(outbox[0].recipients, ['user@example.com']) + + def test_email_attachment_empty_string(self): + # This workflow specifically sends `attachments = ''` as a parameter + # to the email script + workflow = self.create_workflow('email_attachment_empty_string') + workflow_api = self.get_workflow_api(workflow) + task = workflow_api.next_task + + with mail.record_messages() as outbox: + workflow_api = self.complete_form(workflow, task, {'subject': 'My Test Email', + 'recipients': 'user@example.com'}) + task = workflow_api.next_task + # Make sure 'attachments' is in task.data, and is None + self.assertIn('attachments', task.data) + self.assertEqual(task.data['attachments'], '') + + # Make sure we still send an email + self.assertIn('email_id', task.data) + + self.assertEqual(len(outbox), 1) + self.assertEqual(outbox[0].subject, "My Test Email") + self.assertEqual(outbox[0].recipients, ['user@example.com'])