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* diff --git a/pyxform/builder.py b/pyxform/builder.py index 6675df70..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,7 +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 = defaultdict(list) # 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 @@ -138,27 +141,22 @@ def create_survey_element_from_dict( elif d[const.TYPE] == "entity": return EntityDeclaration(**d) else: - self._save_trigger_as_setvalue_and_remove_calculate(d) - + 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_as_setvalue_and_remove_calculate(self, d): + 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 self.setvalues_by_triggering_ref: - self.setvalues_by_triggering_ref[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: - self.setvalues_by_triggering_ref[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 9c4810c6..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,10 +52,13 @@ 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) ): - nested_setvalues = self.get_root().get_setvalues_for_question_name(self.name) + nested_setvalues = survey.get_trigger_values_for_question_name( + self.name, "setvalue" + ) if nested_setvalues: for setvalue in nested_setvalues: msg = ( @@ -64,31 +72,36 @@ def xml_control(self): xml_node = self.build_xml() if xml_node: - self.nest_setvalues(xml_node) - - return xml_node - - def nest_setvalues(self, xml_node): - nested_setvalues = self.get_root().get_setvalues_for_question_name(self.name) - - if nested_setvalues: - for setvalue in nested_setvalues: - setvalue_attrs = { - "ref": self.get_root() - .insert_xpaths(f"${{{setvalue[0]}}}", self.get_root()) - .strip(), - "event": "xforms-value-changed", - } - if not setvalue[1] == "": - setvalue_attrs["value"] = self.get_root().insert_xpaths( - setvalue[1], self - ) + # Get nested setvalue and setgeopoint items + setvalue_items = survey.get_trigger_values_for_question_name( + self.name, "setvalue" + ) + setgeopoint_items = survey.get_trigger_values_for_question_name( + self.name, "setgeopoint" + ) - setvalue_node = node("setvalue", **setvalue_attrs) + # Only call nest_set_nodes if the respective nested items list is not empty + if setvalue_items: + self.nest_set_nodes(survey, xml_node, "setvalue", setvalue_items) + if setgeopoint_items: + self.nest_set_nodes( + survey, xml_node, "odk:setgeopoint", setgeopoint_items + ) - xml_node.appendChild(setvalue_node) + return xml_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/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..2b6e21ed 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, @@ -227,7 +228,7 @@ def validate(self): def _validate_uniqueness_of_section_names(self): root_node_name = self.name - section_names = [] + section_names = set() for element in self.iter_descendants(): if isinstance(element, Section): if element.name in section_names: @@ -242,7 +243,7 @@ def _validate_uniqueness_of_section_names(self): raise PyXFormError(msg) msg = f"There are two sections with the name {element.name}." raise PyXFormError(msg) - section_names.append(element.name) + section_names.add(element.name) def get_nsmap(self): """Add additional namespaces""" @@ -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: """ 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 2d19fdad..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,10 +1497,17 @@ def workbook_to_json( continue # TODO: Consider adding some question_type validation here. + # Ensure that + if question_type == "background-geopoint": + 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 new file mode 100644 index 00000000..a7705fac --- /dev/null +++ b/tests/test_background_geopoint.py @@ -0,0 +1,266 @@ +from pyxform.validators.pyxform import question_types as qt + +from tests.pyxform_test_case import PyxformTestCase + + +class TestBackgroundGeopoint(PyxformTestCase): + """Test background-geopoint question type.""" + + 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=md, + errored=True, + error__contains=[qt.TRIGGER_INVALID.format(r=3, t="background-geopoint")], + ) + + 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=md, + errored=True, + error__contains=[qt.TRIGGER_INVALID.format(r=3, t="background-geopoint")], + ) + + 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=md, + errored=True, + error__contains=[qt.BACKGROUND_GEOPOINT_CALCULATION.format(r=3)], + ) + + 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=md, + xml__xpath_match=[ + # 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_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=md, + xml__xpath_match=[ + # 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_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=md, + xml__xpath_match=[ + # 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_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=md, + xml__xpath_match=[ + # 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_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=md, + xml__xpath_match=[ + # 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_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=md, + xml__xpath_match=[ + # 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_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=md, + xml__xpath_match=[ + # 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_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=md, + xml__xpath_match=[ + # 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' + ] + """, + ], + )