From e87e7b5b8b19214f620ce469eaec97f3207dcfab Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 8 Feb 2021 10:18:41 -0500 Subject: [PATCH 1/7] Form fields that can be hidden and required must now have a default value or value_expression. Also, if a field is hidden but not required, it should not produce a value. --- crc/services/workflow_service.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index 74d070c0..1c7aa670 100644 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -154,8 +154,14 @@ class WorkflowService(object): result = WorkflowService.evaluate_property(Task.FIELD_PROP_LABEL_EXPRESSION, field, task) field.label = result - # If the field is hidden, it should not produce a value. - if field.has_property(Task.FIELD_PROP_HIDE_EXPRESSION): + # If a field is hidden and required, it must have a default value or value_expression + if field.has_property(Task.FIELD_PROP_HIDE_EXPRESSION) and field.has_validation(Task.FIELD_CONSTRAINT_REQUIRED): + if not field.has_property(Task.FIELD_PROP_VALUE_EXPRESSION) or not (hasattr(field, 'default_value')): + raise ApiError(code='hidden and required field missing default', + message='Fields that are required but can be hidden must have either a default value or a value_expression') + + # If the field is hidden and not required, it should not produce a value. + if field.has_property(Task.FIELD_PROP_HIDE_EXPRESSION) and not field.has_validation(Task.FIELD_CONSTRAINT_REQUIRED): if WorkflowService.evaluate_property(Task.FIELD_PROP_HIDE_EXPRESSION, field, task): continue From eb504465f6ab5fc8ee1cf931370c01d79fd6f160 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 8 Feb 2021 10:20:05 -0500 Subject: [PATCH 2/7] Test and workflow for new code. We test 2 things. - If a field can be hidden and required, it must have a default value or value_expression - If a field is hidden and required, make sure we use the default value --- .../hidden_required_field.bpmn | 116 ++++++++++++++++++ .../test_workflow_hidden_required_field.py | 37 ++++++ 2 files changed, 153 insertions(+) create mode 100644 tests/data/hidden_required_field/hidden_required_field.bpmn create mode 100644 tests/workflow/test_workflow_hidden_required_field.py diff --git a/tests/data/hidden_required_field/hidden_required_field.bpmn b/tests/data/hidden_required_field/hidden_required_field.bpmn new file mode 100644 index 00000000..5e4ace06 --- /dev/null +++ b/tests/data/hidden_required_field/hidden_required_field.bpmn @@ -0,0 +1,116 @@ + + + + + Flow_0zt7wv5 + + + + + + + + + + + + + + + + Flow_0fb4w15 + Flow_0c2rym0 + + + + Flow_0cm6imh + Flow_0fb4w15 + if not 'require_yes_no' in globals(): + require_yes_no = True +if not 'hide_yes_no' in globals(): + hide_yes_no = True + + + + <H1>Good Bye{% if name %} {{ name }}{% endif %}</H1> + Flow_1qkjkbh + Flow_1udbzd6 + + + Flow_1udbzd6 + + + + <H1>Hello</H1> + Flow_0zt7wv5 + Flow_0cm6imh + + + + + + + + + + + + + + + + + Flow_0c2rym0 + Flow_1qkjkbh + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/workflow/test_workflow_hidden_required_field.py b/tests/workflow/test_workflow_hidden_required_field.py new file mode 100644 index 00000000..fb22f6b9 --- /dev/null +++ b/tests/workflow/test_workflow_hidden_required_field.py @@ -0,0 +1,37 @@ +from tests.base_test import BaseTest +import json + + +class TestWorkflowHiddenRequiredField(BaseTest): + + def test_require_default(self): + # We have a field that can be hidden and required. + # Validation should fail if we don't have a default value. + spec_model = self.load_test_spec('hidden_required_field') + rv = self.app.get('/v1.0/workflow-specification/%s/validate' % spec_model.id, headers=self.logged_in_headers()) + + json_data = json.loads(rv.get_data(as_text=True)) + self.assertEqual(json_data[0]['code'], 'hidden and required field missing default') + + def test_default_used(self): + # If a field is hidden and required, make sure we use the default value + + workflow = self.create_workflow('hidden_required_field') + workflow_api = self.get_workflow_api(workflow) + + first_task = workflow_api.next_task + self.assertEqual('Activity_Hello', first_task.name) + workflow_api = self.get_workflow_api(workflow) + + self.complete_form(workflow_api, first_task, {}) + workflow_api = self.get_workflow_api(workflow) + + second_task = workflow_api.next_task + self.assertEqual('Activity_HiddenField', second_task.name) + self.complete_form(workflow_api, second_task, {}) + workflow_api = self.get_workflow_api(workflow) + + # The color field is hidden and required. Make sure we use the default value + third_task = workflow_api.next_task + self.assertEqual('Activity_CheckDefault', third_task.name) + self.assertEqual('Gray', third_task.data['color']) From b42b843f7df8c9e7ebac8b2c85d37c70488ba3a6 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 8 Feb 2021 15:10:53 -0500 Subject: [PATCH 3/7] It was possible for an enum field lookup to return an empty list. This meant there were no options for the list. We now test for this and raise an error. --- crc/services/workflow_service.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index 74d070c0..139882c6 100644 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -316,7 +316,11 @@ class WorkflowService(object): data = db.session.query(LookupDataModel).filter( LookupDataModel.lookup_file_model == lookup_model).limit(10).all() options = [{"value": d.value, "label": d.label, "data": d.data} for d in data] - return random.choice(options) + if len(options) > 0: + return random.choice(options) + else: + raise ApiError.from_task("invalid enum", "You specified an enumeration field (%s)," + " with no options" % field.id, task) else: raise ApiError.from_task("unknown_lookup_option", "The settings for this auto complete field " "are incorrect: %s " % field.id, task) From 11177b4296f46f70a0655ca590db1f8558d5a52e Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Mon, 8 Feb 2021 15:12:11 -0500 Subject: [PATCH 4/7] Test, workflow, and empty spreadsheet for the new code. The workflow references the spreadsheet, which has no rows. --- .../enum_empty_list/empty_spreadsheet.xls | Bin 0 -> 9728 bytes .../data/enum_empty_list/enum_empty_list.bpmn | 62 ++++++++++++++++++ .../workflow/test_workflow_enum_empty_list.py | 13 ++++ 3 files changed, 75 insertions(+) create mode 100644 tests/data/enum_empty_list/empty_spreadsheet.xls create mode 100644 tests/data/enum_empty_list/enum_empty_list.bpmn create mode 100644 tests/workflow/test_workflow_enum_empty_list.py diff --git a/tests/data/enum_empty_list/empty_spreadsheet.xls b/tests/data/enum_empty_list/empty_spreadsheet.xls new file mode 100644 index 0000000000000000000000000000000000000000..dad3355b4948e54e9e33efa0be2a195d987d9c44 GIT binary patch literal 9728 zcmeHMYiwM_6+W}xwbypy>^cdgK+LiUjvYUU?G)UEc$d5s3*tmE$N^G;F}CBz)YLJE zNK|YBX=zC+)6i6_rm9jYstOV+DiVK);`}O7%`=3mNQDp~gv6f&fhZDq`+aBb-CbYb zbxirgW5+%-bH6z=XJ*dKnK`qsy&64t{JAUM6Hn-n5}C@BOKBc%oV=zH5&R4D83YzZ zojkvb;oC6-J7ffZV-k~1@>8UvvJYD9*+G5IC0r&U)jvnewGayjwAzHNyjhb}$JuwD z`=d#{`-fX@oqEfVFUiG&U}T#NOFwd_?%18O>qrac7*$BXP&TD9^{eK}byM;zau|(l z!1(Wz`_YekUA%;F_y^j(?}qY7eB3${xt39`?*8%eTyxA4KU~<+HEB zD%%|TVio4w`lUewD-nPeDch_2D^PvwP~Ygt-jRW^*rtd2`iEmZBcprv?(Xj&L%mF< zeRJn}c0|jXyNNfqUs`A$M|l_vsWc>KB9mo=X|LYHkp31ttphuc@AdlKYdFw#h>aWr zQYA^{IIMA8<0`~PUQo`r6*(k9AV%(1?6e8XX-pYMyH2XnUX$$X_}~WHXCpfUHNiIQ zh|9&qWx2#Se9$k~*N%OLn28Cb$k=zp7BRJ7ov<$i4F8SsF-4nJk^z zKr>{|BK>PAALCz(fz`=MiD6hxAUjNANzK;;_hAawh#_Q0PUhUak)13F|g2FFGpr?@vg&(VzS9*Q>U_6Q{s{+9V?E_XrR{*%)$z zNPA%`cjJ_}4kH?cimd$$frq;S$&BFIT{lgDV~fKTl8tD$Py5iXC(zvbvv;Vkclfp) zx3oL*DVt%2Z~(4cH@y+qc$RSkdaz!8h)wcHbU|15_6J7qAKBaAx$)-yeKHbV-h!f5 zFswql35P`!h+pfGTP?crtz;W#o6XNS=bt#~^nMNvDl0nlBV-<2y*im#m5g_F-!wYf zyD#3J%eGvf%eSuXYis2v>wX>i=bV#}D+Tt)2@^5mCEPm1AC5*VLcb10g5`>nN%4n> zR|22sgI6eiBoLHHswBfSnkv<_Iu+D(VX911Nr|S5QlZRm<-DC#IXQ7kPMkO)i9~`U z1YxB3VS8AMhqJGNGi+%kqZH}E2S5ffX4(y zPbWh8bjDO(cox`xkjMdzAJzCVjSp)4ON}4b_*WV~q46oHkV;9Zoc+92$pPezoH9|| z?I+B({U`PGGxAT<-Y4?1a=xPRYZ||<@f&)>JEQTN8lTnpEkjfEFO4&1i7b%>fB=kq z{Ng;69MZ8ImN%agNlMK{oa{0mCJ$xd%R!&P<@f?;30ommh}nQ^+g8~wH^H#p4*TDU z%{*@=+y#cM*uELmfkniUro0M}y+1^z-5=(obpwt7PTU2WkE*-~>VEOpi*IbbXM5r< z#aAlcpz#4kQrs_2xE5>P$ZyPkl$;LUhB%_TZ82`|@U3>%dDtHm5Vs@-@@bZI0J6XS;$vub^E8Xq@F zK&E3@`!g^eKVCjw1-)3A4@QM~`M49k7?+C|tMtLRw7h&+#xpR^m6s12eFnxE^zza4 zc`;r`z1Tt@jINKDZ?OmaQa6XtC?VJ{gD)z_1WGlu2I63OiVkM|! zbJU-kb1~}AVxcS_^=C24vpMR|&AAx$XR&aWkNUG%c^0GoEC$2jWvsiVOkNUIN!YoGpS!_`jqy8+m zSh0>kwLH%Wlq4f3EQL9vMi6wDA=O14k)r;;r}`J`e_0>26GlHS#_OQwyJ)j}qWO&t z!7FSn?Dypy#hfQM-2p4?>*eaYPG%4M!g@bHFULC5!^;WC`F1&ex9Ot)`c3vk@aI}>psoIL_V1t&V$U4zYxw={rJDjxRDr#=LC128|*y%*NfK|z#9wT zD+}P|zBr{7{nvc|w&=eW{nw)Z>iDg&4n_YJK6lZ7E&8vecu;fw&7%K`SBoh;zlQYb z!o9QDly|+r*_Y2AYrZma|tR15rA$r@0l?W9sW8H9~V}E*cJo7 z8QhePJnJy9&i@k%^`GsYJNT!MKi{@H^30Ry|BAo=n|;yOW27vnO#TYX{xRjl3?E|n zZor2YKCJK|g@%KD=EDddLb!1F(82nA$l${TA1XK=J|ytb2=BEj>jy~rJMfht{=Bdh zR0Fyi6a!rYii4JcmV>SZ)q+-lIEVG1m7oStBd7_~47v`)@w9+iL3m)t&oP1@X+*9E y-2kFq_?yZa&{`13xDIKDlTSJ+;O}kVx}}aeHzdDMNdNA-`O>da5&Krm!2bYd0<
+ + + + Flow_08cjvuw + + + + + + + + + + + + + + + Flow_08cjvuw + Flow_0qm71qa + + + + Flow_0ynk21r + + + + <H1>Good Bye</H1> + Flow_0qm71qa + Flow_0ynk21r + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/workflow/test_workflow_enum_empty_list.py b/tests/workflow/test_workflow_enum_empty_list.py new file mode 100644 index 00000000..5d81c1d8 --- /dev/null +++ b/tests/workflow/test_workflow_enum_empty_list.py @@ -0,0 +1,13 @@ +from tests.base_test import BaseTest +import json + + +class TestEmptyEnumList(BaseTest): + + def test_empty_enum_list(self): + + spec_model = self.load_test_spec('enum_empty_list') + rv = self.app.get('/v1.0/workflow-specification/%s/validate' % spec_model.id, headers=self.logged_in_headers()) + json_data = json.loads(rv.get_data(as_text=True)) + + self.assertEqual(json_data[0]['code'], 'invalid enum') From 3e32cdedc456ed397b69538988fd924895503162 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 9 Feb 2021 16:35:57 -0500 Subject: [PATCH 5/7] New error handler for non Api errors. Ticket 187 --- crc/api/common.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crc/api/common.py b/crc/api/common.py index 78cbc018..ba250313 100644 --- a/crc/api/common.py +++ b/crc/api/common.py @@ -1,6 +1,7 @@ from SpiffWorkflow import WorkflowException from SpiffWorkflow.exceptions import WorkflowTaskExecException from flask import g +from werkzeug.exceptions import InternalServerError from crc import ma, app @@ -77,3 +78,9 @@ def handle_invalid_usage(error): return response, error.status_code +@app.errorhandler(InternalServerError) +def handle_internal_server_error(e): + original = getattr(e, "original_exception", None) + api_error = ApiError(code='Internal Server Error (500)', message=str(original)) + response = ApiErrorSchema().dump(api_error) + return response, 500 From 777cfbdecd3bf9676e52174bd9593936f377d776 Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Tue, 9 Feb 2021 17:37:55 -0500 Subject: [PATCH 6/7] We now test whether we have a valid StudyModel when getting a study by study model. We raise an ApiError if the model is None or empty. --- crc/models/study.py | 15 +++++++++------ tests/study/test_get_study_from_model.py | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 6 deletions(-) create mode 100644 tests/study/test_get_study_from_model.py diff --git a/crc/models/study.py b/crc/models/study.py index e56ef8d6..55744073 100644 --- a/crc/models/study.py +++ b/crc/models/study.py @@ -8,7 +8,7 @@ from marshmallow_enum import EnumField from sqlalchemy import func from crc import db, ma -from crc.api.common import ApiErrorSchema +from crc.api.common import ApiErrorSchema, ApiError from crc.models.file import FileModel, SimpleFileSchema, FileSchema from crc.models.protocol_builder import ProtocolBuilderStatus, ProtocolBuilderStudy from crc.models.workflow import WorkflowSpecCategoryModel, WorkflowState, WorkflowStatus, WorkflowSpecModel, \ @@ -165,11 +165,14 @@ class Study(object): @classmethod def from_model(cls, study_model: StudyModel): - id = study_model.id # Just read some value, in case the dict expired, otherwise dict may be empty. - args = dict((k, v) for k, v in study_model.__dict__.items() if not k.startswith('_')) - args['events_history'] = study_model.events_history # For some reason this attribute is not picked up - instance = cls(**args) - return instance + if study_model is not None and len(study_model.__dict__.items()) > 0: + args = dict((k, v) for k, v in study_model.__dict__.items() if not k.startswith('_')) + args['events_history'] = study_model.events_history # For some reason this attribute is not picked up + instance = cls(**args) + return instance + else: + raise ApiError(code='empty_study_model', + message='There was a problem retrieving your study. StudyModel is empty.') def model_args(self): """Arguments that can be passed into the Study Model to update it.""" diff --git a/tests/study/test_get_study_from_model.py b/tests/study/test_get_study_from_model.py new file mode 100644 index 00000000..38c30e89 --- /dev/null +++ b/tests/study/test_get_study_from_model.py @@ -0,0 +1,18 @@ +from tests.base_test import BaseTest +from crc import session +from crc.models.study import StudyModel +import json + + +class TestGetStudyFromModel(BaseTest): + + def test_get_study_from_model(self): + + self.load_example_data() + study = session.query(StudyModel).order_by(StudyModel.id.desc()).first() + id = study.id + 1 + result = self.app.get('/v1.0/study/%i' % id, + headers=self.logged_in_headers()) + json_data = json.loads(result.get_data(as_text=True)) + self.assertIn('code', json_data) + self.assertEqual('empty_study_model', json_data['code']) \ No newline at end of file From 39e874504f89c1c3bbe311d898c049b3ae11bdcd Mon Sep 17 00:00:00 2001 From: mike cullerton Date: Wed, 10 Feb 2021 14:35:58 -0500 Subject: [PATCH 7/7] We now allow value_expression to evaluate to an empty string ("") so that we *do not* set a default value for enums in some cases. This was causing an error because "" was not an option for the enum. --- crc/services/workflow_service.py | 2 + .../test_value_expression.bpmn | 89 +++++++++++++++++++ .../test_workflow_value_expression.py | 31 +++++++ 3 files changed, 122 insertions(+) create mode 100644 tests/data/test_value_expression/test_value_expression.bpmn create mode 100644 tests/workflow/test_workflow_value_expression.py diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index 74d070c0..ba5c4cd3 100644 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -256,6 +256,8 @@ class WorkflowService(object): return None if field.type == "enum" and not has_lookup: + if isinstance(default, str) and default.strip() == '': + return default_option = next((obj for obj in field.options if obj.id == default), None) if not default_option: raise ApiError.from_task("invalid_default", "You specified a default value that does not exist in " diff --git a/tests/data/test_value_expression/test_value_expression.bpmn b/tests/data/test_value_expression/test_value_expression.bpmn new file mode 100644 index 00000000..ee6c09aa --- /dev/null +++ b/tests/data/test_value_expression/test_value_expression.bpmn @@ -0,0 +1,89 @@ + + + + + Flow_1nc3qi5 + + + + + + <H1>Hello</H1> + Flow_1nc3qi5 + Flow_1t2lo17 + + + Flow_1t2lo17 + Flow_1hhfj67 + if not 'value_expression_value' in globals(): + value_expression_value = "" + + + + + + + + + + + + + + Flow_1hhfj67 + Flow_1skkg5a + + + + <H1>Good Bye</H1> + Flow_1skkg5a + Flow_057as2q + + + Flow_057as2q + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/workflow/test_workflow_value_expression.py b/tests/workflow/test_workflow_value_expression.py new file mode 100644 index 00000000..e059dfef --- /dev/null +++ b/tests/workflow/test_workflow_value_expression.py @@ -0,0 +1,31 @@ +from tests.base_test import BaseTest + + +class TestValueExpression(BaseTest): + + def test_value_expression_no_default(self): + + workflow = self.create_workflow('test_value_expression') + + workflow_api = self.get_workflow_api(workflow) + first_task = workflow_api.next_task + self.complete_form(workflow_api, first_task, {'value_expression_value': ''}) + + workflow_api = self.get_workflow_api(workflow) + second_task = workflow_api.next_task + self.assertEqual('', second_task.data['value_expression_value']) + self.assertNotIn('color', second_task.data) + + def test_value_expression_with_default(self): + + workflow = self.create_workflow('test_value_expression') + + workflow_api = self.get_workflow_api(workflow) + first_task = workflow_api.next_task + self.complete_form(workflow_api, first_task, {'value_expression_value': 'black'}) + + workflow_api = self.get_workflow_api(workflow) + second_task = workflow_api.next_task + self.assertEqual('black', second_task.data['value_expression_value']) + self.assertIn('color', second_task.data) + self.assertEqual('black', second_task.data['color']['value'])