From 157c429b43cf03c3037f591cdeb50c921762ae22 Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Thu, 26 Sep 2024 15:06:05 -0400 Subject: [PATCH 1/8] add new XLSForm question type called background-geopoint which saves an odk:setgeopoint action in response to an xforms-value-changed event --- pyxform/builder.py | 24 +++++++------ pyxform/question.py | 52 ++++++++++++++++++++--------- pyxform/question_type_dictionary.py | 4 +++ pyxform/survey.py | 10 ++++-- 4 files changed, 61 insertions(+), 29 deletions(-) diff --git a/pyxform/builder.py b/pyxform/builder.py index 6675df70..a9089bd3 100644 --- a/pyxform/builder.py +++ b/pyxform/builder.py @@ -85,6 +85,8 @@ def __init__(self, **kwargs): # dictionary of setvalue target and value tuple indexed by triggering element self.setvalues_by_triggering_ref = {} + # dictionary of setgeopoint target and value tuple indexed by triggering element + self.setgeopoint_by_triggering_ref = {} # For tracking survey-level choices while recursing through the survey. self._choices: dict[str, Any] = {} @@ -117,6 +119,7 @@ def create_survey_element_from_dict( if d[const.TYPE] == const.SURVEY: section.setvalues_by_triggering_ref = self.setvalues_by_triggering_ref + section.setgeopoint_by_triggering_ref = self.setgeopoint_by_triggering_ref section.choices = self._choices return section @@ -137,28 +140,27 @@ def create_survey_element_from_dict( return ExternalInstance(**d) elif d[const.TYPE] == "entity": return EntityDeclaration(**d) + elif d[const.TYPE] == "background-geopoint": + self._save_trigger_and_remove_calculate(d, self.setgeopoint_by_triggering_ref) + return self._create_question_from_dict( + d, copy_json_dict(QUESTION_TYPE_DICT), self._add_none_option + ) else: - self._save_trigger_as_setvalue_and_remove_calculate(d) - + self._save_trigger_and_remove_calculate(d, self.setvalues_by_triggering_ref) return self._create_question_from_dict( d, copy_json_dict(QUESTION_TYPE_DICT), self._add_none_option ) - def _save_trigger_as_setvalue_and_remove_calculate(self, d): + def _save_trigger_and_remove_calculate(self, d, target_dict): if "trigger" in d: triggering_ref = re.sub(r"\s+", "", d["trigger"]) value = "" if const.BIND in d and "calculate" in d[const.BIND]: value = d[const.BIND]["calculate"] - - if triggering_ref in self.setvalues_by_triggering_ref: - self.setvalues_by_triggering_ref[triggering_ref].append( - (d[const.NAME], value) - ) + if triggering_ref in target_dict: + target_dict[triggering_ref].append((d[const.NAME], value)) else: - self.setvalues_by_triggering_ref[triggering_ref] = [ - (d[const.NAME], value) - ] + target_dict[triggering_ref] = [(d[const.NAME], value)] @staticmethod def _create_question_from_dict( diff --git a/pyxform/question.py b/pyxform/question.py index 9c4810c6..e7c3384b 100644 --- a/pyxform/question.py +++ b/pyxform/question.py @@ -34,6 +34,11 @@ def validate(self): # question type dictionary. if self.type not in QUESTION_TYPE_DICT: raise PyXFormError(f"Unknown question type '{self.type}'.") + if self.type == "background-geopoint": + if not self.trigger: + raise PyXFormError( + f"background-geopoint question '{self.name}' must have a non-null trigger." + ) def xml_instance(self, **kwargs): survey = self.get_root() @@ -50,7 +55,15 @@ def xml_control(self): if self.type == "calculate" or ( ("calculate" in self.bind or self.trigger) and not (self.label or self.hint) ): - nested_setvalues = self.get_root().get_setvalues_for_question_name(self.name) + if self.type == "background-geopoint": + if "calculate" in self.bind: + raise PyXFormError( + f"'{self.name}' is triggered by a geopoint action, so the calculation must be null." + ) + + nested_setvalues = self.get_root().get_trigger_values_for_question_name( + self.name, "setvalue" + ) if nested_setvalues: for setvalue in nested_setvalues: msg = ( @@ -64,29 +77,36 @@ def xml_control(self): xml_node = self.build_xml() if xml_node: - self.nest_setvalues(xml_node) + # Get nested setvalue and setgeopoint items + setvalue_items = self.get_root().get_trigger_values_for_question_name( + self.name, "setvalue" + ) + setgeopoint_items = self.get_root().get_trigger_values_for_question_name( + self.name, "setgeopoint" + ) - return xml_node + # Only call nest_set_nodes if the respective nested items list is not empty + if setvalue_items: + self.nest_set_nodes(xml_node, "setvalue", setvalue_items) + if setgeopoint_items: + self.nest_set_nodes(xml_node, "odk:setgeopoint", setgeopoint_items) - def nest_setvalues(self, xml_node): - nested_setvalues = self.get_root().get_setvalues_for_question_name(self.name) + return xml_node - if nested_setvalues: - for setvalue in nested_setvalues: - setvalue_attrs = { + def nest_set_nodes(self, xml_node, tag, nested_items): + if nested_items: + for item in nested_items: + node_attrs = { "ref": self.get_root() - .insert_xpaths(f"${{{setvalue[0]}}}", self.get_root()) + .insert_xpaths(f"${{{item[0]}}}", self.get_root()) .strip(), "event": "xforms-value-changed", } - if not setvalue[1] == "": - setvalue_attrs["value"] = self.get_root().insert_xpaths( - setvalue[1], self - ) - - setvalue_node = node("setvalue", **setvalue_attrs) + if item[1]: + node_attrs["value"] = self.get_root().insert_xpaths(item[1], self) - xml_node.appendChild(setvalue_node) + set_node = node(tag, **node_attrs) + xml_node.appendChild(set_node) def build_xml(self): return None diff --git a/pyxform/question_type_dictionary.py b/pyxform/question_type_dictionary.py index 669504f1..b22e0b7d 100644 --- a/pyxform/question_type_dictionary.py +++ b/pyxform/question_type_dictionary.py @@ -382,4 +382,8 @@ def generate_new_dict(): "bind": {"type": "binary"}, "action": {"name": "odk:recordaudio", "event": "odk-instance-load"}, }, + "background-geopoint": { + "control": {"tag": "trigger"}, + "bind": {"type": "geopoint"}, + }, } diff --git a/pyxform/survey.py b/pyxform/survey.py index 8f1a69c8..f989e205 100644 --- a/pyxform/survey.py +++ b/pyxform/survey.py @@ -192,6 +192,7 @@ class Survey(Section): "_xpath": dict, "_created": datetime.now, # This can't be dumped to json "setvalues_by_triggering_ref": dict, + "setgeopoint_by_triggering_ref": dict, "title": str, "id_string": str, "sms_keyword": str, @@ -297,8 +298,13 @@ def xml(self): **nsmap, ) - def get_setvalues_for_question_name(self, question_name): - return self.setvalues_by_triggering_ref.get(f"${{{question_name}}}") + def get_trigger_values_for_question_name(self, question_name, trigger_type): + trigger_map = { + "setvalue": self.setvalues_by_triggering_ref, + "setgeopoint": self.setgeopoint_by_triggering_ref, + } + + return trigger_map.get(trigger_type, {}).get(f"${{{question_name}}}") def _generate_static_instances(self, list_name, choice_list) -> InstanceInfo: """ From f5022548537562ea1cfc8028bebf54e4b26f06aa Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Fri, 27 Sep 2024 13:22:23 -0400 Subject: [PATCH 2/8] add tests for background-geopoint questions --- pyxform/question.py | 7 +++ pyxform/survey.py | 9 ++++ tests/test_background_geopoint.py | 71 +++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+) create mode 100644 tests/test_background_geopoint.py diff --git a/pyxform/question.py b/pyxform/question.py index e7c3384b..91255e2a 100644 --- a/pyxform/question.py +++ b/pyxform/question.py @@ -34,11 +34,18 @@ def validate(self): # question type dictionary. if self.type not in QUESTION_TYPE_DICT: raise PyXFormError(f"Unknown question type '{self.type}'.") + # ensure that background-geopoint questions have non-null triggers that correspond to an exsisting questions if self.type == "background-geopoint": if not self.trigger: raise PyXFormError( f"background-geopoint question '{self.name}' must have a non-null trigger." ) + trigger_cleaned = self.trigger.strip("${}") + if not self.get_root().question_exists(trigger_cleaned): + raise PyXFormError( + f"Trigger '{trigger_cleaned}' for background-geopoint question '{self.name}' " + "does not correspond to an existing question." + ) def xml_instance(self, **kwargs): survey = self.get_root() diff --git a/pyxform/survey.py b/pyxform/survey.py index f989e205..136e4623 100644 --- a/pyxform/survey.py +++ b/pyxform/survey.py @@ -306,6 +306,15 @@ def get_trigger_values_for_question_name(self, question_name, trigger_type): return trigger_map.get(trigger_type, {}).get(f"${{{question_name}}}") + def question_exists(self, question_name: str) -> bool: + """ + Check if a question with the given name exists in the survey. + """ + for element in self.iter_descendants(): + if isinstance(element, Question) and element.name == question_name: + return True + return False + def _generate_static_instances(self, list_name, choice_list) -> InstanceInfo: """ Generate elements for static data (e.g. choices for selects) diff --git a/tests/test_background_geopoint.py b/tests/test_background_geopoint.py new file mode 100644 index 00000000..f7c2b56b --- /dev/null +++ b/tests/test_background_geopoint.py @@ -0,0 +1,71 @@ +from tests.pyxform_test_case import PyxformTestCase + + +class BackgroundGeopointTest(PyxformTestCase): + """Test background-geopoint question type.""" + + def test_background_geopoint(self): + self.assertPyxformXform( + name="data", + md=""" + | survey | | | | + | | type | name | label | trigger | + | | integer | temp | Enter the current temperature | | + | | background-geopoint| temp_geo | | ${temp} | + | | note | show_temp_geo | location: ${temp_geo} | | + """, + xml__contains=[ + '', + '', + ], + ) + + def test_background_geopoint_missing_trigger(self): + """Test that background-geopoint question raises error when trigger is empty.""" + self.assertPyxformXform( + name="data", + md=""" + | survey | | | | + | | type | name | label | trigger | + | | integer | temp | Enter the current temperature | | + | | background-geopoint| temp_geo | | | + | | note | show_temp_geo | location: ${temp_geo} | | + """, + errored=True, + error__contains=[ + "background-geopoint question 'temp_geo' must have a non-null trigger" + ], + ) + + def test_invalid_trigger_background_geopoint(self): + self.assertPyxformXform( + name="data", + md=""" + | survey | | | | + | | type | name | label | trigger | + | | integer | temp | Enter the current temperature | | + | | background-geopoint| temp_geo | | ${invalid_trigger} | + | | note | show_temp_geo | location: ${temp_geo} | | + """, + errored=True, + error__contains=[ + "Trigger 'invalid_trigger' for background-geopoint question 'temp_geo' does not correspond to an existing question" + ], + ) + + def test_background_geopoint_requires_null_calculation(self): + """Test that background-geopoint raises an error if there is a calculation.""" + self.assertPyxformXform( + name="data", + md=""" + | survey | | | | | + | | type | name | label | trigger | calculation | + | | integer | temp | Enter the current temperature | | | + | | background-geopoint| temp_geo | | ${temp} | 5 * temp | + | | note | show_temp_geo | location: ${temp_geo} | | | + """, + errored=True, + error__contains=[ + "'temp_geo' is triggered by a geopoint action, so the calculation must be null." + ], + ) From f762598f73eaeb00f526a6dfa99fa55988b31f7f Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Wed, 9 Oct 2024 13:05:32 -0400 Subject: [PATCH 3/8] formatting and improve error messages --- pyxform/question.py | 5 ++-- tests/test_background_geopoint.py | 50 +++++++++++++++---------------- 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/pyxform/question.py b/pyxform/question.py index 91255e2a..f65e7dc6 100644 --- a/pyxform/question.py +++ b/pyxform/question.py @@ -43,8 +43,7 @@ def validate(self): trigger_cleaned = self.trigger.strip("${}") if not self.get_root().question_exists(trigger_cleaned): raise PyXFormError( - f"Trigger '{trigger_cleaned}' for background-geopoint question '{self.name}' " - "does not correspond to an existing question." + f"background-geopoint question '{self.name}' must have a trigger corresponding to an existing question." ) def xml_instance(self, **kwargs): @@ -65,7 +64,7 @@ def xml_control(self): if self.type == "background-geopoint": if "calculate" in self.bind: raise PyXFormError( - f"'{self.name}' is triggered by a geopoint action, so the calculation must be null." + f"'{self.name}' is triggered by a geopoint action, please remove the calculation from this question." ) nested_setvalues = self.get_root().get_trigger_values_for_question_name( diff --git a/tests/test_background_geopoint.py b/tests/test_background_geopoint.py index f7c2b56b..e0deedcd 100644 --- a/tests/test_background_geopoint.py +++ b/tests/test_background_geopoint.py @@ -8,15 +8,15 @@ def test_background_geopoint(self): self.assertPyxformXform( name="data", md=""" - | survey | | | | - | | type | name | label | trigger | - | | integer | temp | Enter the current temperature | | - | | background-geopoint| temp_geo | | ${temp} | - | | note | show_temp_geo | location: ${temp_geo} | | + | survey | | | | + | | type | name | label | trigger | + | | integer | temp | Enter the current temperature | | + | | background-geopoint| temp_geo | | ${temp} | + | | note | show_temp_geo | location: ${temp_geo} | | """, - xml__contains=[ - '', - '', + xml__xpath_match=[ + '/h:html/h:head/x:model/x:bind[@nodeset="/data/temp_geo" and @type="geopoint"]', + '/h:html/h:body//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/temp_geo"]', ], ) @@ -25,11 +25,11 @@ def test_background_geopoint_missing_trigger(self): self.assertPyxformXform( name="data", md=""" - | survey | | | | - | | type | name | label | trigger | - | | integer | temp | Enter the current temperature | | - | | background-geopoint| temp_geo | | | - | | note | show_temp_geo | location: ${temp_geo} | | + | survey | | | | + | | type | name | label | trigger | + | | integer | temp | Enter the current temperature | | + | | background-geopoint| temp_geo | | | + | | note | show_temp_geo | location: ${temp_geo} | | """, errored=True, error__contains=[ @@ -41,15 +41,15 @@ def test_invalid_trigger_background_geopoint(self): self.assertPyxformXform( name="data", md=""" - | survey | | | | - | | type | name | label | trigger | - | | integer | temp | Enter the current temperature | | - | | background-geopoint| temp_geo | | ${invalid_trigger} | - | | note | show_temp_geo | location: ${temp_geo} | | + | survey | | | | + | | type | name | label | trigger | + | | integer | temp | Enter the current temperature | | + | | background-geopoint| temp_geo | | ${invalid_trigger} | + | | note | show_temp_geo | location: ${temp_geo} | | """, errored=True, error__contains=[ - "Trigger 'invalid_trigger' for background-geopoint question 'temp_geo' does not correspond to an existing question" + "background-geopoint question 'temp_geo' must have a trigger corresponding to an existing question" ], ) @@ -58,14 +58,14 @@ def test_background_geopoint_requires_null_calculation(self): self.assertPyxformXform( name="data", md=""" - | survey | | | | | - | | type | name | label | trigger | calculation | - | | integer | temp | Enter the current temperature | | | - | | background-geopoint| temp_geo | | ${temp} | 5 * temp | - | | note | show_temp_geo | location: ${temp_geo} | | | + | survey | | | | | + | | type | name | label | trigger | calculation | + | | integer | temp | Enter the current temperature | | | + | | background-geopoint| temp_geo | | ${temp} | 5 * temp | + | | note | show_temp_geo | location: ${temp_geo} | | | """, errored=True, error__contains=[ - "'temp_geo' is triggered by a geopoint action, so the calculation must be null." + "'temp_geo' is triggered by a geopoint action, please remove the calculation from this question." ], ) From 389c31cd53810449e86c900e4c0fb3f34a605ef1 Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Wed, 9 Oct 2024 17:47:04 -0400 Subject: [PATCH 4/8] move background-geopoint non-null validation to workbook_to_json --- pyxform/question.py | 6 +----- pyxform/xls2json.py | 8 ++++++++ 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/pyxform/question.py b/pyxform/question.py index f65e7dc6..e3bff785 100644 --- a/pyxform/question.py +++ b/pyxform/question.py @@ -34,12 +34,8 @@ def validate(self): # question type dictionary. if self.type not in QUESTION_TYPE_DICT: raise PyXFormError(f"Unknown question type '{self.type}'.") - # ensure that background-geopoint questions have non-null triggers that correspond to an exsisting questions + # ensure that background-geopoint questions have triggers that correspond to an existing questions if self.type == "background-geopoint": - if not self.trigger: - raise PyXFormError( - f"background-geopoint question '{self.name}' must have a non-null trigger." - ) trigger_cleaned = self.trigger.strip("${}") if not self.get_root().question_exists(trigger_cleaned): raise PyXFormError( diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py index 2d19fdad..6abd3981 100644 --- a/pyxform/xls2json.py +++ b/pyxform/xls2json.py @@ -1493,6 +1493,14 @@ def workbook_to_json( continue # TODO: Consider adding some question_type validation here. + # Ensure that background-geopoint questions have non-null triggers + if question_type == "background-geopoint": + new_dict = row.copy() + if "trigger" not in new_dict or not new_dict["trigger"]: + raise PyXFormError( + f"background-geopoint question '{new_dict['name']}' must have a non-null trigger." + ) + # Put the row in the json dict as is: parent_children_array.append(row) From ed213cd2f6ef57bd20920c81b47848172ad46d58 Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Wed, 9 Oct 2024 18:50:23 -0400 Subject: [PATCH 5/8] consolidate validation for trigger values with validation for section name uniqueness in survey class --- pyxform/question.py | 7 ------- pyxform/survey.py | 30 +++++++++++++++++++----------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/pyxform/question.py b/pyxform/question.py index e3bff785..09b4d9e2 100644 --- a/pyxform/question.py +++ b/pyxform/question.py @@ -34,13 +34,6 @@ def validate(self): # question type dictionary. if self.type not in QUESTION_TYPE_DICT: raise PyXFormError(f"Unknown question type '{self.type}'.") - # ensure that background-geopoint questions have triggers that correspond to an existing questions - if self.type == "background-geopoint": - trigger_cleaned = self.trigger.strip("${}") - if not self.get_root().question_exists(trigger_cleaned): - raise PyXFormError( - f"background-geopoint question '{self.name}' must have a trigger corresponding to an existing question." - ) def xml_instance(self, **kwargs): survey = self.get_root() diff --git a/pyxform/survey.py b/pyxform/survey.py index 136e4623..2f00d641 100644 --- a/pyxform/survey.py +++ b/pyxform/survey.py @@ -224,12 +224,21 @@ def validate(self): if self.id_string in [None, "None"]: raise PyXFormError("Survey cannot have an empty id_string") super().validate() - self._validate_uniqueness_of_section_names() + self._validate_section_and_trigger_names() - def _validate_uniqueness_of_section_names(self): + def _validate_section_and_trigger_names(self): root_node_name = self.name section_names = [] + existing_question_names = set() + bg_geopoint_elements = [] + for element in self.iter_descendants(): + if "name" in element: + existing_question_names.add(element["name"]) + + if element["type"] == "background-geopoint": + bg_geopoint_elements.append(element) + if isinstance(element, Section): if element.name in section_names: if element.name == root_node_name: @@ -245,6 +254,14 @@ def _validate_uniqueness_of_section_names(self): raise PyXFormError(msg) section_names.append(element.name) + # Ensure that background-geopoint questions have triggers that correspond to an existing questions + for bg_geo in bg_geopoint_elements: + trigger_cleaned = bg_geo.get("trigger", "").strip("${}") + if trigger_cleaned not in existing_question_names: + raise PyXFormError( + f"background-geopoint question '{bg_geo['name']}' must have a trigger corresponding to an existing question." + ) + def get_nsmap(self): """Add additional namespaces""" namespaces = getattr(self, constants.NAMESPACES, "") @@ -306,15 +323,6 @@ def get_trigger_values_for_question_name(self, question_name, trigger_type): return trigger_map.get(trigger_type, {}).get(f"${{{question_name}}}") - def question_exists(self, question_name: str) -> bool: - """ - Check if a question with the given name exists in the survey. - """ - for element in self.iter_descendants(): - if isinstance(element, Question) and element.name == question_name: - return True - return False - def _generate_static_instances(self, list_name, choice_list) -> InstanceInfo: """ Generate elements for static data (e.g. choices for selects) From 402c6695392048f65d4c4695000d06a64f2f73ad Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Fri, 11 Oct 2024 09:35:44 -0400 Subject: [PATCH 6/8] add more tests for background-geopoint questions when used with other setvalue question types and also when used in groups and repeats --- tests/test_background_geopoint.py | 214 +++++++++++++++++++++++++++--- 1 file changed, 193 insertions(+), 21 deletions(-) diff --git a/tests/test_background_geopoint.py b/tests/test_background_geopoint.py index e0deedcd..316db05d 100644 --- a/tests/test_background_geopoint.py +++ b/tests/test_background_geopoint.py @@ -8,11 +8,11 @@ def test_background_geopoint(self): self.assertPyxformXform( name="data", md=""" - | survey | | | | - | | type | name | label | trigger | - | | integer | temp | Enter the current temperature | | - | | background-geopoint| temp_geo | | ${temp} | - | | note | show_temp_geo | location: ${temp_geo} | | + | survey | | | | + | | type | name | label | trigger | + | | integer | temp | Enter the current temperature | | + | | background-geopoint | temp_geo | | ${temp} | + | | note | show_temp_geo | location: ${temp_geo} | | """, xml__xpath_match=[ '/h:html/h:head/x:model/x:bind[@nodeset="/data/temp_geo" and @type="geopoint"]', @@ -25,11 +25,11 @@ def test_background_geopoint_missing_trigger(self): self.assertPyxformXform( name="data", md=""" - | survey | | | | - | | type | name | label | trigger | - | | integer | temp | Enter the current temperature | | - | | background-geopoint| temp_geo | | | - | | note | show_temp_geo | location: ${temp_geo} | | + | survey | | | | + | | type | name | label | trigger | + | | integer | temp | Enter the current temperature | | + | | background-geopoint | temp_geo | | | + | | note | show_temp_geo | location: ${temp_geo} | | """, errored=True, error__contains=[ @@ -41,12 +41,12 @@ def test_invalid_trigger_background_geopoint(self): self.assertPyxformXform( name="data", md=""" - | survey | | | | - | | type | name | label | trigger | - | | integer | temp | Enter the current temperature | | - | | background-geopoint| temp_geo | | ${invalid_trigger} | - | | note | show_temp_geo | location: ${temp_geo} | | - """, + | survey | | | | + | | type | name | label | trigger | + | | integer | temp | Enter the current temperature | | + | | background-geopoint | temp_geo | | ${invalid_trigger} | + | | note | show_temp_geo | location: ${temp_geo} | | + """, errored=True, error__contains=[ "background-geopoint question 'temp_geo' must have a trigger corresponding to an existing question" @@ -58,14 +58,186 @@ def test_background_geopoint_requires_null_calculation(self): self.assertPyxformXform( name="data", md=""" - | survey | | | | | - | | type | name | label | trigger | calculation | - | | integer | temp | Enter the current temperature | | | - | | background-geopoint| temp_geo | | ${temp} | 5 * temp | - | | note | show_temp_geo | location: ${temp_geo} | | | + | survey | | | | | + | | type | name | label | trigger | calculation | + | | integer | temp | Enter the current temperature | | | + | | background-geopoint | temp_geo | | ${temp} | 5 * temp | + | | note | show_temp_geo | location: ${temp_geo} | | | """, errored=True, error__contains=[ "'temp_geo' is triggered by a geopoint action, please remove the calculation from this question." ], ) + + def test_combined_background_geopoint_and_setvalue(self): + """Test a form with both a background-geopoint and setvalue triggered by the same question.""" + self.assertPyxformXform( + name="data", + md=""" + | survey | | | | | + | | type | name | label | trigger | calculation | + | | integer | temp | Enter the current temperature | | | + | | background-geopoint | temp_geo | | ${temp} | | + | | calculate | temp_doubled | | ${temp} | ${temp} * 2 | + | | note | show_temp_geo | location: ${temp_geo} | | | + | | note | show_temp | doubled temp: ${temp_doubled} | | | + """, + xml__xpath_match=[ + '/h:html/h:body//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/temp_geo"]', + '/h:html/h:body//x:setvalue[@event="xforms-value-changed" and @ref="/data/temp_doubled" and normalize-space(@value)="/data/temp * 2"]', + ], + ) + + def test_setgeopoint_trigger_target_outside_group(self): + """Verify the correct structure for a setgeopoint trigger and target when neither is in a group.""" + self.assertPyxformXform( + name="data", + md=""" + | survey | | | | + | | type | name | label | trigger | + | | integer | temp | Enter the current temperature | | + | | background-geopoint | temp_geo | | ${temp} | + | | note | show_temp_geo | location: ${temp_geo} | | + """, + xml__xpath_match=[ + '/h:html/h:body//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/temp_geo"]', + '/h:html/h:body//x:input[@ref="/data/show_temp_geo"]/x:label[contains(text(), "location:")]/x:output[@value=" /data/temp_geo "]', + ], + ) + + def test_setgeopoint_trigger_target_in_non_repeating_group(self): + """Verify the correct structure for a setgeopoint trigger and target in a non-repeating group.""" + self.assertPyxformXform( + name="data", + md=""" + | survey | | | | + | | type | name | label | trigger | + | | begin_group | groupA | | | + | | integer | temp | Enter the current temperature | | + | | background-geopoint | temp_geo | | ${temp} | + | | note | show_temp_geo | location: ${temp_geo} | | + | | end_group | | | | + """, + xml__xpath_match=[ + '/h:html/h:body/x:group[@ref="/data/groupA"]', + '/h:html/h:body/x:group[@ref="/data/groupA"]/x:input[@ref="/data/groupA/temp"]//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/groupA/temp_geo"]', + '/h:html/h:body/x:group[@ref="/data/groupA"]/x:input[@ref="/data/groupA/show_temp_geo"]/x:label[contains(text(), "location:")]/x:output[@value=" /data/groupA/temp_geo "]', + ], + ) + + def test_setgeopoint_trigger_target_separate_groups(self): + """Verify the correct structure for a setgeopoint trigger and target in two separate groups.""" + self.assertPyxformXform( + name="data", + md=""" + | survey | | | | + | | type | name | label | trigger | + | | begin_group | groupA | | | + | | integer | temp | Enter the current temperature | | + | | end_group | | | | + | | begin_group | groupB | | | + | | background-geopoint | temp_geo | | ${temp} | + | | note | show_temp_geo | location: ${temp_geo} | | + | | end_group | | | | + """, + xml__xpath_match=[ + '/h:html/h:body/x:group[@ref="/data/groupA"]', + '/h:html/h:body/x:group[@ref="/data/groupB"]', + '/h:html/h:body/x:group[@ref="/data/groupA"]/x:input[@ref="/data/groupA/temp"]//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/groupB/temp_geo"]', + '/h:html/h:body/x:group[@ref="/data/groupB"]/x:input[@ref="/data/groupB/show_temp_geo"]/x:label/x:output[@value=" /data/groupB/temp_geo "]', + ], + ) + + def test_setgeopoint_trigger_target_in_same_repeating_group(self): + """Verify the correct structure for a setgeopoint trigger and target in the same repeating group.""" + self.assertPyxformXform( + name="data", + md=""" + | survey | | | | + | | type | name | label | trigger | + | | begin_repeat | groupA | | | + | | integer | temp | Enter the current temperature | | + | | background-geopoint | temp_geo | | ${temp} | + | | note | show_temp_geo | location: ${temp_geo} | | + | | end_repeat | | | | + """, + xml__xpath_match=[ + '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]', + '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]/x:input[@ref="/data/groupA/temp"]//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/groupA/temp_geo"]', + '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]/x:input[@ref="/data/groupA/show_temp_geo"]/x:label/x:output[@value=" ../temp_geo "]', + ], + ) + + def test_setgeopoint_trigger_target_in_separate_repeating_groups(self): + """Verify the correct structure for a setgeopoint trigger and target in separate repeating groups.""" + self.assertPyxformXform( + name="data", + md=""" + | survey | | | | + | | type | name | label | trigger | + | | begin_repeat | groupA | | | + | | integer | temp | Enter the current temperature | | + | | end_repeat | | | | + | | begin_repeat | groupB | | | + | | background-geopoint | temp_geo | | ${temp} | + | | note | show_temp_geo | location: ${temp_geo} | | + | | end_repeat | | | | + """, + xml__xpath_match=[ + '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]', + '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]/x:input[@ref="/data/groupA/temp"]//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/groupB/temp_geo"]', + '/h:html/h:body/x:group[@ref="/data/groupB"]/x:repeat[@nodeset="/data/groupB"]', + '/h:html/h:body/x:group[@ref="/data/groupB"]/x:repeat[@nodeset="/data/groupB"]/x:input[@ref="/data/groupB/show_temp_geo"]/x:label/x:output[@value=" ../temp_geo "]', + ], + ) + + def test_setgeopoint_trigger_in_repeating_group_target_in_non_repeating_groups( + self, + ): + """Verify the correct structure for a setgeopoint trigger in a repeating group and a target in a non-repeating group.""" + self.assertPyxformXform( + name="data", + md=""" + | survey | | | | + | | type | name | label | trigger | + | | begin_repeat | groupA | | | + | | integer | temp | Enter the current temperature | | + | | end_repeat | | | | + | | begin_group | groupB | | | + | | background-geopoint | temp_geo | | ${temp} | + | | note | show_temp_geo | location: ${temp_geo} | | + | | end_group | | | | + """, + xml__xpath_match=[ + '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]', + '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]/x:input[@ref="/data/groupA/temp"]//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/groupB/temp_geo"]', + '/h:html/h:body/x:group[@ref="/data/groupB"]', + '/h:html/h:body/x:group[@ref="/data/groupB"]/x:input[@ref="/data/groupB/show_temp_geo"]/x:label/x:output[@value=" /data/groupB/temp_geo "]', + ], + ) + + def test_setgeopoint_trigger_in_non_repeating_group_target_in_repeating_group( + self, + ): + """Verify the correct structure for a setgeopoint trigger in a non-repeating group and a target in a repeating group.""" + self.assertPyxformXform( + name="data", + md=""" + | survey | | | | + | | type | name | label | trigger | + | | begin_group | groupA | | | + | | integer | temp | Enter the current temperature | | + | | end_group | | | | + | | begin_repeat | groupB | | | + | | background-geopoint | temp_geo | | ${temp} | + | | note | show_temp_geo | location: ${temp_geo} | | + | | end_repeat | | | | + """, + xml__xpath_match=[ + '/h:html/h:body/x:group[@ref="/data/groupA"]', + '/h:html/h:body/x:group[@ref="/data/groupA"]/x:input[@ref="/data/groupA/temp"]//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/groupB/temp_geo"]', + '/h:html/h:body/x:group[@ref="/data/groupB"]/x:repeat[@nodeset="/data/groupB"]', + '/h:html/h:body/x:group[@ref="/data/groupB"]/x:repeat[@nodeset="/data/groupB"]/x:input[@ref="/data/groupB/show_temp_geo"]/x:label/x:output[@value=" ../temp_geo "]', + ], + ) From 705174b45d260fe83427310f16e00fa5ea28745a Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Sat, 19 Oct 2024 11:41:54 +1100 Subject: [PATCH 7/8] chg: improvements for the addition of background-geopoint - builder.py: - de-duplicate elif call to create_question_from_dict - use relevant class dictionaries directly instead of passing around - replace regex with strip() to avoid replacing whitespace everywhere in the question name (whitespace invalid in question names). - question.py: - avoid calling self.get_root() repeatedly as it iterates the form (up the tree from the current question). - remove `if` block in nest_set_nodes since it is only called when there are items to be processed. - question_types.py: - add validation functions for workbook_to_json. used external strings for re-use, easier testing, and (maybe someday) i18n. - xls2json.py: - move background-geopoint questions to workbook_to_json from survey.py and question.py to allow row references in errors - test_background_geopoint.py: - change the test class name to match unittest idiom / test discovery - remove `note` question from all tests since this is not relevant to the feature being tested or any specific edge cases of interest. - remove xpath expressions testing only the presence of groups, since these are tested already as part of the xpath to find the inputs within the groups. - readability improvements: - structured test method names - state the overall test case expectation in docstring - indicate the topic of the xpaths - wrap long xpaths and use single-quotes for predicates - shorten test fixture names and tidy up markdown formatting --- pyxform/builder.py | 24 +- pyxform/question.py | 54 ++- pyxform/survey.py | 25 +- pyxform/validators/pyxform/question_types.py | 45 +++ pyxform/xls2json.py | 19 +- tests/test_background_geopoint.py | 375 ++++++++++--------- 6 files changed, 295 insertions(+), 247 deletions(-) create mode 100644 pyxform/validators/pyxform/question_types.py diff --git a/pyxform/builder.py b/pyxform/builder.py index a9089bd3..0ade9150 100644 --- a/pyxform/builder.py +++ b/pyxform/builder.py @@ -4,7 +4,7 @@ import copy import os -import re +from collections import defaultdict from typing import TYPE_CHECKING, Any, Union from pyxform import constants as const @@ -84,9 +84,9 @@ def __init__(self, **kwargs): self.set_sections(kwargs.get("sections", {})) # dictionary of setvalue target and value tuple indexed by triggering element - self.setvalues_by_triggering_ref = {} + self.setvalues_by_triggering_ref = defaultdict(list) # dictionary of setgeopoint target and value tuple indexed by triggering element - self.setgeopoint_by_triggering_ref = {} + self.setgeopoint_by_triggering_ref = defaultdict(list) # For tracking survey-level choices while recursing through the survey. self._choices: dict[str, Any] = {} @@ -140,27 +140,23 @@ def create_survey_element_from_dict( return ExternalInstance(**d) elif d[const.TYPE] == "entity": return EntityDeclaration(**d) - elif d[const.TYPE] == "background-geopoint": - self._save_trigger_and_remove_calculate(d, self.setgeopoint_by_triggering_ref) - return self._create_question_from_dict( - d, copy_json_dict(QUESTION_TYPE_DICT), self._add_none_option - ) else: - self._save_trigger_and_remove_calculate(d, self.setvalues_by_triggering_ref) + self._save_trigger(d=d) return self._create_question_from_dict( d, copy_json_dict(QUESTION_TYPE_DICT), self._add_none_option ) - def _save_trigger_and_remove_calculate(self, d, target_dict): + def _save_trigger(self, d: dict) -> None: if "trigger" in d: - triggering_ref = re.sub(r"\s+", "", d["trigger"]) + triggering_ref = d["trigger"].strip() value = "" if const.BIND in d and "calculate" in d[const.BIND]: value = d[const.BIND]["calculate"] - if triggering_ref in target_dict: - target_dict[triggering_ref].append((d[const.NAME], value)) + question_ref = (d[const.NAME], value) + if d[const.TYPE] == "background-geopoint": + self.setgeopoint_by_triggering_ref[triggering_ref].append(question_ref) else: - target_dict[triggering_ref] = [(d[const.NAME], value)] + self.setvalues_by_triggering_ref[triggering_ref].append(question_ref) @staticmethod def _create_question_from_dict( diff --git a/pyxform/question.py b/pyxform/question.py index 09b4d9e2..6c8f6a91 100644 --- a/pyxform/question.py +++ b/pyxform/question.py @@ -14,7 +14,12 @@ from pyxform.errors import PyXFormError from pyxform.question_type_dictionary import QUESTION_TYPE_DICT from pyxform.survey_element import SurveyElement -from pyxform.utils import PYXFORM_REFERENCE_REGEX, default_is_dynamic, node +from pyxform.utils import ( + PYXFORM_REFERENCE_REGEX, + DetachableElement, + default_is_dynamic, + node, +) class Question(SurveyElement): @@ -47,16 +52,11 @@ def xml_instance(self, **kwargs): return node(self.name, **attributes) def xml_control(self): + survey = self.get_root() if self.type == "calculate" or ( ("calculate" in self.bind or self.trigger) and not (self.label or self.hint) ): - if self.type == "background-geopoint": - if "calculate" in self.bind: - raise PyXFormError( - f"'{self.name}' is triggered by a geopoint action, please remove the calculation from this question." - ) - - nested_setvalues = self.get_root().get_trigger_values_for_question_name( + nested_setvalues = survey.get_trigger_values_for_question_name( self.name, "setvalue" ) if nested_setvalues: @@ -73,37 +73,35 @@ def xml_control(self): if xml_node: # Get nested setvalue and setgeopoint items - setvalue_items = self.get_root().get_trigger_values_for_question_name( + setvalue_items = survey.get_trigger_values_for_question_name( self.name, "setvalue" ) - setgeopoint_items = self.get_root().get_trigger_values_for_question_name( + setgeopoint_items = survey.get_trigger_values_for_question_name( self.name, "setgeopoint" ) # Only call nest_set_nodes if the respective nested items list is not empty if setvalue_items: - self.nest_set_nodes(xml_node, "setvalue", setvalue_items) + self.nest_set_nodes(survey, xml_node, "setvalue", setvalue_items) if setgeopoint_items: - self.nest_set_nodes(xml_node, "odk:setgeopoint", setgeopoint_items) + self.nest_set_nodes( + survey, xml_node, "odk:setgeopoint", setgeopoint_items + ) return xml_node - def nest_set_nodes(self, xml_node, tag, nested_items): - if nested_items: - for item in nested_items: - node_attrs = { - "ref": self.get_root() - .insert_xpaths(f"${{{item[0]}}}", self.get_root()) - .strip(), - "event": "xforms-value-changed", - } - if item[1]: - node_attrs["value"] = self.get_root().insert_xpaths(item[1], self) - - set_node = node(tag, **node_attrs) - xml_node.appendChild(set_node) - - def build_xml(self): + def nest_set_nodes(self, survey, xml_node, tag, nested_items): + for item in nested_items: + node_attrs = { + "ref": survey.insert_xpaths(f"${{{item[0]}}}", survey).strip(), + "event": "xforms-value-changed", + } + if item[1]: + node_attrs["value"] = survey.insert_xpaths(item[1], self) + set_node = node(tag, **node_attrs) + xml_node.appendChild(set_node) + + def build_xml(self) -> DetachableElement | None: return None diff --git a/pyxform/survey.py b/pyxform/survey.py index 2f00d641..2b6e21ed 100644 --- a/pyxform/survey.py +++ b/pyxform/survey.py @@ -224,21 +224,12 @@ def validate(self): if self.id_string in [None, "None"]: raise PyXFormError("Survey cannot have an empty id_string") super().validate() - self._validate_section_and_trigger_names() + self._validate_uniqueness_of_section_names() - def _validate_section_and_trigger_names(self): + def _validate_uniqueness_of_section_names(self): root_node_name = self.name - section_names = [] - existing_question_names = set() - bg_geopoint_elements = [] - + section_names = set() for element in self.iter_descendants(): - if "name" in element: - existing_question_names.add(element["name"]) - - if element["type"] == "background-geopoint": - bg_geopoint_elements.append(element) - if isinstance(element, Section): if element.name in section_names: if element.name == root_node_name: @@ -252,15 +243,7 @@ def _validate_section_and_trigger_names(self): raise PyXFormError(msg) msg = f"There are two sections with the name {element.name}." raise PyXFormError(msg) - section_names.append(element.name) - - # Ensure that background-geopoint questions have triggers that correspond to an existing questions - for bg_geo in bg_geopoint_elements: - trigger_cleaned = bg_geo.get("trigger", "").strip("${}") - if trigger_cleaned not in existing_question_names: - raise PyXFormError( - f"background-geopoint question '{bg_geo['name']}' must have a trigger corresponding to an existing question." - ) + section_names.add(element.name) def get_nsmap(self): """Add additional namespaces""" diff --git a/pyxform/validators/pyxform/question_types.py b/pyxform/validators/pyxform/question_types.py new file mode 100644 index 00000000..48f49db8 --- /dev/null +++ b/pyxform/validators/pyxform/question_types.py @@ -0,0 +1,45 @@ +""" +Validations for question types. +""" + +import re + +from pyxform.errors import PyXFormError +from pyxform.parsing.expression import is_single_token_expression +from pyxform.utils import PYXFORM_REFERENCE_REGEX + +BACKGROUND_GEOPOINT_CALCULATION = "[row : {r}] For 'background-geopoint' questions, the 'calculation' column must be empty." +TRIGGER_INVALID = ( + "[row : {r}] For '{t}' questions, the 'trigger' column must be a reference to another " + "question that exists, in the format ${{question_name_here}}." +) + + +def validate_background_geopoint_calculation(row: dict, row_num: int) -> bool: + """A background-geopoint must not have a calculation.""" + try: + row["bind"]["calculate"] + except KeyError: + return True + else: + raise PyXFormError(BACKGROUND_GEOPOINT_CALCULATION.format(r=row_num)) + + +def validate_background_geopoint_trigger(row: dict, row_num: int) -> bool: + """A background-geopoint must have a trigger.""" + if not row.get("trigger", False) or not is_single_token_expression( + expression=row["trigger"], token_types=["PYXFORM_REF"] + ): + raise PyXFormError(TRIGGER_INVALID.format(r=row_num, t=row["type"])) + return True + + +def validate_references(referrers: list[tuple[dict, int]], questions: set[str]) -> bool: + """Triggers must refer to a question that exists.""" + for row, row_num in referrers: + matches = re.match(PYXFORM_REFERENCE_REGEX, row["trigger"]) + if matches is not None: + trigger = matches.groups()[0] + if trigger not in questions: + raise PyXFormError(TRIGGER_INVALID.format(r=row_num, t=row["type"])) + return True diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py index 6abd3981..37c3ca1c 100644 --- a/pyxform/xls2json.py +++ b/pyxform/xls2json.py @@ -24,6 +24,7 @@ from pyxform.parsing.expression import is_single_token_expression from pyxform.utils import PYXFORM_REFERENCE_REGEX, coalesce, default_is_dynamic from pyxform.validators.pyxform import parameters_generic, select_from_file +from pyxform.validators.pyxform import question_types as qt from pyxform.validators.pyxform.android_package_name import validate_android_package_name from pyxform.validators.pyxform.translations_checks import SheetTranslations from pyxform.xls2json_backends import csv_to_dict, xls_to_dict, xlsx_to_dict @@ -697,9 +698,11 @@ def workbook_to_json( # Rows from the survey sheet that should be nested in meta survey_meta = [] + # To check that questions with triggers refer to other questions that exist. + question_names = set() + trigger_references = [] - # row by row, validate questions, throwing errors and adding warnings - # where needed. + # row by row, validate questions, throwing errors and adding warnings where needed. for row in survey_sheet.data: row_number += 1 if stack[-1] is not None: @@ -727,6 +730,7 @@ def workbook_to_json( # Get question type question_type = row.get(constants.TYPE) question_name = row.get(constants.NAME) + question_names.add(question_name) if not question_type: # if name and label are also missing, @@ -1493,18 +1497,17 @@ def workbook_to_json( continue # TODO: Consider adding some question_type validation here. - # Ensure that background-geopoint questions have non-null triggers + # Ensure that if question_type == "background-geopoint": - new_dict = row.copy() - if "trigger" not in new_dict or not new_dict["trigger"]: - raise PyXFormError( - f"background-geopoint question '{new_dict['name']}' must have a non-null trigger." - ) + qt.validate_background_geopoint_trigger(row=row, row_num=row_number) + qt.validate_background_geopoint_calculation(row=row, row_num=row_number) + trigger_references.append((row, row_number)) # Put the row in the json dict as is: parent_children_array.append(row) sheet_translations.or_other_check(warnings=warnings) + qt.validate_references(referrers=trigger_references, questions=question_names) if len(stack) != 1: raise PyXFormError( diff --git a/tests/test_background_geopoint.py b/tests/test_background_geopoint.py index 316db05d..a7705fac 100644 --- a/tests/test_background_geopoint.py +++ b/tests/test_background_geopoint.py @@ -1,243 +1,266 @@ +from pyxform.validators.pyxform import question_types as qt + from tests.pyxform_test_case import PyxformTestCase -class BackgroundGeopointTest(PyxformTestCase): +class TestBackgroundGeopoint(PyxformTestCase): """Test background-geopoint question type.""" - def test_background_geopoint(self): - self.assertPyxformXform( - name="data", - md=""" - | survey | | | | - | | type | name | label | trigger | - | | integer | temp | Enter the current temperature | | - | | background-geopoint | temp_geo | | ${temp} | - | | note | show_temp_geo | location: ${temp_geo} | | - """, - xml__xpath_match=[ - '/h:html/h:head/x:model/x:bind[@nodeset="/data/temp_geo" and @type="geopoint"]', - '/h:html/h:body//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/temp_geo"]', - ], - ) - - def test_background_geopoint_missing_trigger(self): - """Test that background-geopoint question raises error when trigger is empty.""" + def test_error__missing_trigger(self): + """Should raise an error if the question trigger is empty.""" + md = """ + | survey | + | | type | name | label | trigger | + | | integer | temp | Enter temp | | + | | background-geopoint | temp_geo | | | + """ self.assertPyxformXform( name="data", - md=""" - | survey | | | | - | | type | name | label | trigger | - | | integer | temp | Enter the current temperature | | - | | background-geopoint | temp_geo | | | - | | note | show_temp_geo | location: ${temp_geo} | | - """, + md=md, errored=True, - error__contains=[ - "background-geopoint question 'temp_geo' must have a non-null trigger" - ], + error__contains=[qt.TRIGGER_INVALID.format(r=3, t="background-geopoint")], ) - def test_invalid_trigger_background_geopoint(self): + def test_error__invalid_trigger(self): + """Should raise an error if the question trigger does not refer to a question.""" + md = """ + | survey | + | | type | name | label | trigger | + | | integer | temp | Enter temp | | + | | background-geopoint | temp_geo | | ${invalid} | + """ self.assertPyxformXform( name="data", - md=""" - | survey | | | | - | | type | name | label | trigger | - | | integer | temp | Enter the current temperature | | - | | background-geopoint | temp_geo | | ${invalid_trigger} | - | | note | show_temp_geo | location: ${temp_geo} | | - """, + md=md, errored=True, - error__contains=[ - "background-geopoint question 'temp_geo' must have a trigger corresponding to an existing question" - ], + error__contains=[qt.TRIGGER_INVALID.format(r=3, t="background-geopoint")], ) - def test_background_geopoint_requires_null_calculation(self): - """Test that background-geopoint raises an error if there is a calculation.""" + def test_error__calculation_exists(self): + """Should raise an error if a calculation exists for the question.""" + md = """ + | survey | + | | type | name | label | trigger | calculation | + | | integer | temp | Enter temp | | | + | | background-geopoint | temp_geo | | ${temp} | 5 * temp | + """ self.assertPyxformXform( name="data", - md=""" - | survey | | | | | - | | type | name | label | trigger | calculation | - | | integer | temp | Enter the current temperature | | | - | | background-geopoint | temp_geo | | ${temp} | 5 * temp | - | | note | show_temp_geo | location: ${temp_geo} | | | - """, + md=md, errored=True, - error__contains=[ - "'temp_geo' is triggered by a geopoint action, please remove the calculation from this question." - ], + error__contains=[qt.BACKGROUND_GEOPOINT_CALCULATION.format(r=3)], ) - def test_combined_background_geopoint_and_setvalue(self): - """Test a form with both a background-geopoint and setvalue triggered by the same question.""" + def test_question_no_group__trigger_no_group(self): + """Should find geopoint binding, and setgeopoint action on the triggering item.""" + md = """ + | survey | + | | type | name | label | trigger | + | | integer | temp | Enter temp | | + | | background-geopoint | temp_geo | | ${temp} | + """ self.assertPyxformXform( name="data", - md=""" - | survey | | | | | - | | type | name | label | trigger | calculation | - | | integer | temp | Enter the current temperature | | | - | | background-geopoint | temp_geo | | ${temp} | | - | | calculate | temp_doubled | | ${temp} | ${temp} * 2 | - | | note | show_temp_geo | location: ${temp_geo} | | | - | | note | show_temp | doubled temp: ${temp_doubled} | | | - """, + md=md, xml__xpath_match=[ - '/h:html/h:body//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/temp_geo"]', - '/h:html/h:body//x:setvalue[@event="xforms-value-changed" and @ref="/data/temp_doubled" and normalize-space(@value)="/data/temp * 2"]', + # background-geopoint bind/control + """/h:html/h:head/x:model/x:bind[@nodeset='/data/temp_geo' and @type='geopoint']""", + """ + /h:html/h:body/x:input[@ref='/data/temp'] + /odk:setgeopoint[@event='xforms-value-changed' and @ref='/data/temp_geo'] + """, ], ) - def test_setgeopoint_trigger_target_outside_group(self): - """Verify the correct structure for a setgeopoint trigger and target when neither is in a group.""" + def test_question_no_group__trigger_no_group__with_calculate_same_trigger(self): + """Should find the behaviour is unchanged by a calculate question with same trigger.""" + md = """ + | survey | + | | type | name | label | trigger | calculation | + | | integer | temp | Enter temp | | | + | | background-geopoint | temp_geo | | ${temp} | | + | | calculate | temp_doubled | | ${temp} | ${temp} * 2 | + """ self.assertPyxformXform( name="data", - md=""" - | survey | | | | - | | type | name | label | trigger | - | | integer | temp | Enter the current temperature | | - | | background-geopoint | temp_geo | | ${temp} | - | | note | show_temp_geo | location: ${temp_geo} | | - """, + md=md, xml__xpath_match=[ - '/h:html/h:body//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/temp_geo"]', - '/h:html/h:body//x:input[@ref="/data/show_temp_geo"]/x:label[contains(text(), "location:")]/x:output[@value=" /data/temp_geo "]', + # background-geopoint bind/control + """/h:html/h:head/x:model/x:bind[@nodeset='/data/temp_geo' and @type='geopoint']""", + """ + /h:html/h:body/x:input[@ref='/data/temp'] + /odk:setgeopoint[@event='xforms-value-changed' and @ref='/data/temp_geo'] + """, + # calculate bind/control + """/h:html/h:head/x:model/x:bind[@nodeset='/data/temp_doubled' and @type='string']""", + """ + /h:html/h:body/x:input[@ref='/data/temp'] + /x:setvalue[@event='xforms-value-changed' + and @ref='/data/temp_doubled' + and normalize-space(@value)='/data/temp * 2' + ] + """, ], ) - def test_setgeopoint_trigger_target_in_non_repeating_group(self): - """Verify the correct structure for a setgeopoint trigger and target in a non-repeating group.""" + def test_question_in_nonrep_group__trigger_in_same_nonrep_group(self): + """Should find the behaviour is unchanged by nesting in a non-repeating group.""" + md = """ + | survey | + | | type | name | label | trigger | + | | begin_group | groupA | | | + | | integer | temp | Enter temp | | + | | background-geopoint | temp_geo | | ${temp} | + | | end_group | | | | + """ self.assertPyxformXform( name="data", - md=""" - | survey | | | | - | | type | name | label | trigger | - | | begin_group | groupA | | | - | | integer | temp | Enter the current temperature | | - | | background-geopoint | temp_geo | | ${temp} | - | | note | show_temp_geo | location: ${temp_geo} | | - | | end_group | | | | - """, + md=md, xml__xpath_match=[ - '/h:html/h:body/x:group[@ref="/data/groupA"]', - '/h:html/h:body/x:group[@ref="/data/groupA"]/x:input[@ref="/data/groupA/temp"]//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/groupA/temp_geo"]', - '/h:html/h:body/x:group[@ref="/data/groupA"]/x:input[@ref="/data/groupA/show_temp_geo"]/x:label[contains(text(), "location:")]/x:output[@value=" /data/groupA/temp_geo "]', + # background-geopoint bind/control + """/h:html/h:head/x:model/x:bind[@nodeset='/data/groupA/temp_geo' and @type='geopoint']""", + """ + /h:html/h:body/x:group[@ref='/data/groupA'] + /x:input[@ref='/data/groupA/temp']/odk:setgeopoint[ + @event='xforms-value-changed' and @ref='/data/groupA/temp_geo' + ] + """, ], ) - def test_setgeopoint_trigger_target_separate_groups(self): - """Verify the correct structure for a setgeopoint trigger and target in two separate groups.""" + def test_question_in_nonrep_group__trigger_in_different_nonrep_group(self): + """Should find the behaviour is unchanged by nesting in different non-repeating groups.""" + md = """ + | survey | + | | type | name | label | trigger | + | | begin_group | groupA | | | + | | integer | temp | Enter temp | | + | | end_group | | | | + | | begin_group | groupB | | | + | | background-geopoint | temp_geo | | ${temp} | + | | end_group | | | | + """ self.assertPyxformXform( name="data", - md=""" - | survey | | | | - | | type | name | label | trigger | - | | begin_group | groupA | | | - | | integer | temp | Enter the current temperature | | - | | end_group | | | | - | | begin_group | groupB | | | - | | background-geopoint | temp_geo | | ${temp} | - | | note | show_temp_geo | location: ${temp_geo} | | - | | end_group | | | | - """, + md=md, xml__xpath_match=[ - '/h:html/h:body/x:group[@ref="/data/groupA"]', - '/h:html/h:body/x:group[@ref="/data/groupB"]', - '/h:html/h:body/x:group[@ref="/data/groupA"]/x:input[@ref="/data/groupA/temp"]//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/groupB/temp_geo"]', - '/h:html/h:body/x:group[@ref="/data/groupB"]/x:input[@ref="/data/groupB/show_temp_geo"]/x:label/x:output[@value=" /data/groupB/temp_geo "]', + # background-geopoint bind/control + """/h:html/h:head/x:model/x:bind[@nodeset='/data/groupB/temp_geo' and @type='geopoint']""", + """ + /h:html/h:body/x:group[@ref="/data/groupA"] + /x:input[@ref="/data/groupA/temp"] + /odk:setgeopoint[ + @event="xforms-value-changed" and @ref="/data/groupB/temp_geo" + ] + """, ], ) - def test_setgeopoint_trigger_target_in_same_repeating_group(self): - """Verify the correct structure for a setgeopoint trigger and target in the same repeating group.""" + def test_question_in_rep_group__trigger_in_same_rep_group(self): + """Should find the behaviour is unchanged by nesting in a repeating group.""" + md = """ + | survey | + | | type | name | label | trigger | + | | begin_repeat | groupA | | | + | | integer | temp | Enter temp | | + | | background-geopoint | temp_geo | | ${temp} | + | | end_repeat | | | | + """ self.assertPyxformXform( name="data", - md=""" - | survey | | | | - | | type | name | label | trigger | - | | begin_repeat | groupA | | | - | | integer | temp | Enter the current temperature | | - | | background-geopoint | temp_geo | | ${temp} | - | | note | show_temp_geo | location: ${temp_geo} | | - | | end_repeat | | | | - """, + md=md, xml__xpath_match=[ - '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]', - '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]/x:input[@ref="/data/groupA/temp"]//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/groupA/temp_geo"]', - '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]/x:input[@ref="/data/groupA/show_temp_geo"]/x:label/x:output[@value=" ../temp_geo "]', + # background-geopoint bind/control + """/h:html/h:head/x:model/x:bind[@nodeset='/data/groupA/temp_geo' and @type='geopoint']""", + """ + /h:html/h:body/x:group[@ref='/data/groupA'] + /x:repeat[@nodeset='/data/groupA']/x:input[@ref='/data/groupA/temp'] + /odk:setgeopoint[ + @event='xforms-value-changed' and @ref='/data/groupA/temp_geo' + ] + """, ], ) - def test_setgeopoint_trigger_target_in_separate_repeating_groups(self): - """Verify the correct structure for a setgeopoint trigger and target in separate repeating groups.""" + def test_question_in_rep_group__trigger_in_different_rep_group(self): + """Should find the behaviour is unchanged by nesting in different repeating groups.""" + md = """ + | survey | + | | type | name | label | trigger | + | | begin_repeat | groupA | | | + | | integer | temp | Enter temp | | + | | end_repeat | | | | + | | begin_repeat | groupB | | | + | | background-geopoint | temp_geo | | ${temp} | + | | end_repeat | | | | + """ self.assertPyxformXform( name="data", - md=""" - | survey | | | | - | | type | name | label | trigger | - | | begin_repeat | groupA | | | - | | integer | temp | Enter the current temperature | | - | | end_repeat | | | | - | | begin_repeat | groupB | | | - | | background-geopoint | temp_geo | | ${temp} | - | | note | show_temp_geo | location: ${temp_geo} | | - | | end_repeat | | | | - """, + md=md, xml__xpath_match=[ - '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]', - '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]/x:input[@ref="/data/groupA/temp"]//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/groupB/temp_geo"]', - '/h:html/h:body/x:group[@ref="/data/groupB"]/x:repeat[@nodeset="/data/groupB"]', - '/h:html/h:body/x:group[@ref="/data/groupB"]/x:repeat[@nodeset="/data/groupB"]/x:input[@ref="/data/groupB/show_temp_geo"]/x:label/x:output[@value=" ../temp_geo "]', + # background-geopoint bind/control + """/h:html/h:head/x:model/x:bind[@nodeset='/data/groupB/temp_geo' and @type='geopoint']""", + """ + /h:html/h:body/x:group[@ref='/data/groupA'] + /x:repeat[@nodeset='/data/groupA']/x:input[@ref='/data/groupA/temp'] + /odk:setgeopoint[ + @event='xforms-value-changed' and @ref='/data/groupB/temp_geo' + ] + """, ], ) - def test_setgeopoint_trigger_in_repeating_group_target_in_non_repeating_groups( - self, - ): - """Verify the correct structure for a setgeopoint trigger in a repeating group and a target in a non-repeating group.""" + def test_question_in_nonrep_group__trigger_in_different_rep_group(self): + """Should find the behaviour is unchanged by nesting in different non/repeating groups.""" + md = """ + | survey | + | | type | name | label | trigger | + | | begin_repeat | groupA | | | + | | integer | temp | Enter temp | | + | | end_repeat | | | | + | | begin_group | groupB | | | + | | background-geopoint | temp_geo | | ${temp} | + | | end_group | | | | + """ self.assertPyxformXform( name="data", - md=""" - | survey | | | | - | | type | name | label | trigger | - | | begin_repeat | groupA | | | - | | integer | temp | Enter the current temperature | | - | | end_repeat | | | | - | | begin_group | groupB | | | - | | background-geopoint | temp_geo | | ${temp} | - | | note | show_temp_geo | location: ${temp_geo} | | - | | end_group | | | | - """, + md=md, xml__xpath_match=[ - '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]', - '/h:html/h:body/x:group[@ref="/data/groupA"]/x:repeat[@nodeset="/data/groupA"]/x:input[@ref="/data/groupA/temp"]//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/groupB/temp_geo"]', - '/h:html/h:body/x:group[@ref="/data/groupB"]', - '/h:html/h:body/x:group[@ref="/data/groupB"]/x:input[@ref="/data/groupB/show_temp_geo"]/x:label/x:output[@value=" /data/groupB/temp_geo "]', + # background-geopoint bind/control + """/h:html/h:head/x:model/x:bind[@nodeset='/data/groupB/temp_geo' and @type='geopoint']""", + """ + /h:html/h:body/x:group[@ref='/data/groupA'] + /x:repeat[@nodeset='/data/groupA']/x:input[@ref='/data/groupA/temp'] + /odk:setgeopoint[ + @event='xforms-value-changed' and @ref='/data/groupB/temp_geo' + ] + """, ], ) - def test_setgeopoint_trigger_in_non_repeating_group_target_in_repeating_group( - self, - ): - """Verify the correct structure for a setgeopoint trigger in a non-repeating group and a target in a repeating group.""" + def test_question_in_rep_group__trigger_in_different_nonrep_group(self): + """Should find the behaviour is unchanged by nesting in different non/repeating groups.""" + md = """ + | survey | + | | type | name | label | trigger | + | | begin_group | groupA | | | + | | integer | temp | Enter temp | | + | | end_group | | | | + | | begin_repeat | groupB | | | + | | background-geopoint | temp_geo | | ${temp} | + | | end_repeat | | | | + """ self.assertPyxformXform( name="data", - md=""" - | survey | | | | - | | type | name | label | trigger | - | | begin_group | groupA | | | - | | integer | temp | Enter the current temperature | | - | | end_group | | | | - | | begin_repeat | groupB | | | - | | background-geopoint | temp_geo | | ${temp} | - | | note | show_temp_geo | location: ${temp_geo} | | - | | end_repeat | | | | - """, + md=md, xml__xpath_match=[ - '/h:html/h:body/x:group[@ref="/data/groupA"]', - '/h:html/h:body/x:group[@ref="/data/groupA"]/x:input[@ref="/data/groupA/temp"]//odk:setgeopoint[@event="xforms-value-changed" and @ref="/data/groupB/temp_geo"]', - '/h:html/h:body/x:group[@ref="/data/groupB"]/x:repeat[@nodeset="/data/groupB"]', - '/h:html/h:body/x:group[@ref="/data/groupB"]/x:repeat[@nodeset="/data/groupB"]/x:input[@ref="/data/groupB/show_temp_geo"]/x:label/x:output[@value=" ../temp_geo "]', + # background-geopoint bind/control + """/h:html/h:head/x:model/x:bind[@nodeset='/data/groupB/temp_geo' and @type='geopoint']""", + """ + /h:html/h:body/x:group[@ref='/data/groupA'] + /x:input[@ref='/data/groupA/temp']/odk:setgeopoint[ + @event='xforms-value-changed' and @ref='/data/groupB/temp_geo' + ] + """, ], ) From f673525ee6d299397dba4d026b88af30aca0ab7a Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Sat, 19 Oct 2024 11:45:32 +1100 Subject: [PATCH 8/8] fix: update github actions versions to silence warnings about node --- .github/workflows/release.yml | 2 +- .github/workflows/verify.yml | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 191c0de2..6d1f38f9 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -19,7 +19,7 @@ jobs: python-version: ${{ matrix.python }} # Install dependencies. - - uses: actions/cache@v3 + - uses: actions/cache@v4 name: Python cache with dependencies. id: python-cache with: diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index 87721363..1044e6c7 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -17,7 +17,7 @@ jobs: python-version: ${{ matrix.python }} # Install dependencies. - - uses: actions/cache@v3 + - uses: actions/cache@v4 name: Python cache with dependencies. id: python-cache with: @@ -52,7 +52,7 @@ jobs: python-version: ${{ matrix.python }} # Install dependencies. - - uses: actions/cache@v3 + - uses: actions/cache@v4 name: Python cache with dependencies. id: python-cache with: @@ -76,7 +76,7 @@ jobs: flit --debug build --no-use-vcs - name: Upload sdist and wheel. if: success() - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: pyxform--on-${{ matrix.os }}--py${{ matrix.python }} path: ${{ github.workspace }}/dist/pyxform*