From c1fa3a07738f27143d45295b99eb7c5aeeb50df8 Mon Sep 17 00:00:00 2001 From: Agus Hilman Date: Wed, 2 Dec 2020 20:29:47 +0700 Subject: [PATCH 1/3] When indexed-repeat is used within a repeat, 1st, 2nd, 4th, and 6th argument not using relative path. --- pyxform/survey.py | 81 ++++++++++++++++++++++++--------- pyxform/tests_v1/test_repeat.py | 58 +++++++++++++++++++++++ 2 files changed, 118 insertions(+), 21 deletions(-) diff --git a/pyxform/survey.py b/pyxform/survey.py index de0f5f9cf..f9f45794c 100644 --- a/pyxform/survey.py +++ b/pyxform/survey.py @@ -877,26 +877,9 @@ def _var_repl_function( Given a dictionary of xpaths, return a function we can use to replace ${varname} with the xpath to varname. """ - name = matchobj.group(2) - last_saved = matchobj.group(1) is not None - intro = ( - "There has been a problem trying to replace %s with the " - "XPath to the survey element named '%s'." % (matchobj.group(0), name) - ) - if name not in self._xpath: - raise PyXFormError(intro + " There is no survey element with this name.") - if self._xpath[name] is None: - raise PyXFormError( - intro + " There are multiple survey elements" " with this name." - ) - if ( - not last_saved - and context - and not ( - context["type"] == "calculate" - and "indexed-repeat" in context["bind"]["calculate"] - ) - ): + + def _relative_path(name): + return_path = None xpath, context_xpath = self._xpath[name], context.get_xpath() # share same root i.e repeat_a from /data/repeat_a/... if ( @@ -912,7 +895,63 @@ def _var_repl_function( ref_path = ref_path if ref_path.endswith(name) else "/%s" % name prefix = " current()/" if use_current else " " - return prefix + "/".join([".."] * steps) + ref_path + " " + return_path = prefix + "/".join([".."] * steps) + ref_path + " " + + return return_path + + name = matchobj.group(2) + last_saved = matchobj.group(1) is not None + intro = ( + "There has been a problem trying to replace %s with the " + "XPath to the survey element named '%s'." % (matchobj.group(0), name) + ) + if name not in self._xpath: + raise PyXFormError(intro + " There is no survey element with this name.") + if self._xpath[name] is None: + raise PyXFormError( + intro + " There are multiple survey elements" " with this name." + ) + + return_relative_path = False + if not last_saved and context: + if context["type"] != "calculate": + indexed_repeat = None + if ( + "bind" in context + and "calculate" in context["bind"] + and "indexed-repeat" in context["bind"]["calculate"] + ): + indexed_repeat = context["bind"]["calculate"] + elif "default" in context and "indexed-repeat" in context["default"]: + indexed_repeat = context["default"] + + if "type" in context and context["type"] == "text": + indexed_repeat_name_index = None + if indexed_repeat is not None: + indexed_repeat_parameters = indexed_repeat.strip()[ + len("indexed-repeat") : + ][1:-1].split(",") + name_param = "${{{0}}}".format(name) + for idx, param in enumerate(indexed_repeat_parameters): + if name_param in param.strip(): + indexed_repeat_name_index = idx + + if ( + indexed_repeat_name_index is not None + and indexed_repeat_name_index not in [0, 1, 3, 5] + ) or indexed_repeat is None: + return_relative_path = True + else: + if indexed_repeat is None: + return_relative_path = True + else: + if "indexed-repeat" not in context["bind"]["calculate"]: + return_relative_path = True + + if return_relative_path: + relative_path = _relative_path(name) + if relative_path is not None: + return relative_path last_saved_prefix = ( "instance('" + LAST_SAVED_INSTANCE_NAME + "')" if last_saved else "" diff --git a/pyxform/tests_v1/test_repeat.py b/pyxform/tests_v1/test_repeat.py index 92f808921..449891873 100644 --- a/pyxform/tests_v1/test_repeat.py +++ b/pyxform/tests_v1/test_repeat.py @@ -458,3 +458,61 @@ def test_choice_from_previous_repeat_answers_in_nested_repeat_uses_current(self) '', ], ) + + def test_indexed_repeat_regular_calculation_relative_path_exception(self): + """Test relative path exception (absolute path) in indexed-repeat() using regular calculation.""" + self.assertPyxformXform( + name="data", + title="regular calculation indexed-repeat 1st, 2nd, 4th, and 6th parameters is using absolute path", + md=""" + | survey | | | | | + | | type | name | label | calculation | + | | begin_repeat | person | Person | | + | | integer | pos | | position(..) | + | | text | name | Enter name | | + | | text | prev_name | Name in previous repeat instance | indexed-repeat(${name}, ${person}, ${pos}-1) | + | | end repeat | | | | + """, # noqa pylint: disable=line-too-long + model__contains=[ + """""" # noqa pylint: disable=line-too-long + ], + ) + + def test_indexed_repeat_dynamic_default_relative_path_exception(self): + """Test relative path exception (absolute path) in indexed-repeat() using dynamic default.""" + self.assertPyxformXform( + name="data", + title="dynamic default indexed-repeat 1st, 2nd, 4th, and 6th parameters is using absolute path", + md=""" + | survey | | | | | + | | type | name | label | default | + | | begin_repeat | person | Person | | + | | text | name | Enter name | | + | | text | prev_name | Name in previous repeat instance | indexed-repeat(${name}, ${person}, position(..)-1) | + | | end repeat | | | | + """, # noqa pylint: disable=line-too-long + xml__contains=[ + """""" # noqa pylint: disable=line-too-long + ], + ) + + def test_indexed_repeat_dreaded_nested_repeat_relative_path_exception(self): + """Test relative path exception (absolute path) in indexed-repeat() using dreaded nested repeat.""" + self.assertPyxformXform( + name="data", + title="dreaded nested repeat indexed-repeat 1st, 2nd, 4th, and 6th parameters is using absolute path", + md=""" + | survey | | | | | + | | type | name | label | default | + | | begin_repeat | family | Family | | + | | integer | members_number | How many members in this family? | | + | | begin_repeat | person | Person | | + | | text | name | Enter name | | + | | text | prev_name | Non-sensible previous name in first family, 2nd person | indexed-repeat(${name}, ${family}, 1, ${person}, 2) | + | | end repeat | | | | + | | end repeat | | | | + """, # noqa pylint: disable=line-too-long + xml__contains=[ + """""" # noqa pylint: disable=line-too-long + ], + ) From f208727df7e69d9a1040ebdc7e71dcc24834561b Mon Sep 17 00:00:00 2001 From: Agus Hilman Date: Fri, 4 Dec 2020 21:28:26 +0700 Subject: [PATCH 2/3] Refactor code and add additional tests. --- pyxform/survey.py | 100 +++++++++++++++++++------------- pyxform/tests_v1/test_repeat.py | 72 +++++++++++++++++++++++ 2 files changed, 132 insertions(+), 40 deletions(-) diff --git a/pyxform/survey.py b/pyxform/survey.py index f9f45794c..829ae5ce8 100644 --- a/pyxform/survey.py +++ b/pyxform/survey.py @@ -878,7 +878,14 @@ def _var_repl_function( replace ${varname} with the xpath to varname. """ + name = matchobj.group(2) + last_saved = matchobj.group(1) is not None + is_indexed_repeat = matchobj.string.find("indexed-repeat(") > -1 + indexed_repeat_regex = re.compile(r"indexed-repeat\([^)]+\)") + function_args_regex = re.compile(r"\b[^()]+\((.*)\)$") + def _relative_path(name): + """Given name in ${name}, return relative xpath to ${name}.""" return_path = None xpath, context_xpath = self._xpath[name], context.get_xpath() # share same root i.e repeat_a from /data/repeat_a/... @@ -899,8 +906,57 @@ def _relative_path(name): return return_path - name = matchobj.group(2) - last_saved = matchobj.group(1) is not None + def _is_return_relative_path(): + """Determine condition to return relative xpath of current ${name}.""" + indexed_repeat_relative_path_args_index = [0, 1, 3, 5] + current_matchobj = matchobj + + if not last_saved and context: + if context["type"] == "text": + + if not is_indexed_repeat: + return True + + # It is possible to have multiple indexed-repeat in an expression + for indexed_repeat in indexed_repeat_regex.finditer( + matchobj.string + ): + + # Make sure current ${name} is in the correct indexed-repeat + if current_matchobj.end() > indexed_repeat.end(): + continue + + # ${name} outside of indexed-repeat always using relative path + if ( + current_matchobj.end() < indexed_repeat.start() + or current_matchobj.start() > indexed_repeat.end() + ): + return True + + indexed_repeat_name_index = None + indexed_repeat_args = ( + function_args_regex.match(indexed_repeat.group()) + .group(1) + .split(",") + ) + name_arg = "${{{0}}}".format(name) + for idx, arg in enumerate(indexed_repeat_args): + if name_arg in arg.strip(): + indexed_repeat_name_index = idx + + return ( + indexed_repeat_name_index is not None + and indexed_repeat_name_index + not in indexed_repeat_relative_path_args_index + ) + else: + return not ( + context["type"] == "calculate" + and "indexed-repeat" in context["bind"]["calculate"] + ) + + return False + intro = ( "There has been a problem trying to replace %s with the " "XPath to the survey element named '%s'." % (matchobj.group(0), name) @@ -912,45 +968,9 @@ def _relative_path(name): intro + " There are multiple survey elements" " with this name." ) - return_relative_path = False - if not last_saved and context: - if context["type"] != "calculate": - indexed_repeat = None - if ( - "bind" in context - and "calculate" in context["bind"] - and "indexed-repeat" in context["bind"]["calculate"] - ): - indexed_repeat = context["bind"]["calculate"] - elif "default" in context and "indexed-repeat" in context["default"]: - indexed_repeat = context["default"] - - if "type" in context and context["type"] == "text": - indexed_repeat_name_index = None - if indexed_repeat is not None: - indexed_repeat_parameters = indexed_repeat.strip()[ - len("indexed-repeat") : - ][1:-1].split(",") - name_param = "${{{0}}}".format(name) - for idx, param in enumerate(indexed_repeat_parameters): - if name_param in param.strip(): - indexed_repeat_name_index = idx - - if ( - indexed_repeat_name_index is not None - and indexed_repeat_name_index not in [0, 1, 3, 5] - ) or indexed_repeat is None: - return_relative_path = True - else: - if indexed_repeat is None: - return_relative_path = True - else: - if "indexed-repeat" not in context["bind"]["calculate"]: - return_relative_path = True - - if return_relative_path: + if _is_return_relative_path(): relative_path = _relative_path(name) - if relative_path is not None: + if relative_path: return relative_path last_saved_prefix = ( diff --git a/pyxform/tests_v1/test_repeat.py b/pyxform/tests_v1/test_repeat.py index 449891873..c68bcc782 100644 --- a/pyxform/tests_v1/test_repeat.py +++ b/pyxform/tests_v1/test_repeat.py @@ -516,3 +516,75 @@ def test_indexed_repeat_dreaded_nested_repeat_relative_path_exception(self): """""" # noqa pylint: disable=line-too-long ], ) + + def test_indexed_repeat_math_epression_dreaded_nested_repeat_relative_path_exception( + self, + ): + """Test relative path exception (absolute path) in indexed-repeat() with math expression using dreaded nested repeat.""" + self.assertPyxformXform( + name="data", + title="dreaded nested repeat indexed-repeat 1st, 2nd, 4th, and 6th parameters is using absolute path", + md=""" + | survey | | | | | + | | type | name | label | default | + | | begin_repeat | family | Family | | + | | integer | members_number | How many members in this family? | | + | | begin_repeat | person | Person | | + | | text | name | Enter name | | + | | integer | age | Enter age | | + | | text | prev_name | Expression label | 7 * indexed-repeat(${age}, ${family}, 1, ${person}, 2) | + | | end repeat | | | | + | | end repeat | | | | + """, # noqa pylint: disable=line-too-long + xml__contains=[ + """""" # noqa pylint: disable=line-too-long + ], + ) + + def test_multiple_indexed_repeat_in_epression_dreaded_nested_repeat_relative_path_exception( + self, + ): + """Test relative path exception (absolute path) in multiple indexed-repeat() inside an expression using dreaded nested repeat.""" + self.assertPyxformXform( + name="data", + title="dreaded nested repeat indexed-repeat 1st, 2nd, 4th, and 6th parameters is using absolute path", + md=""" + | survey | | | | | + | | type | name | label | default | + | | begin_repeat | family | Family | | + | | integer | members_number | How many members in this family? | | + | | begin_repeat | person | Person | | + | | text | name | Enter name | | + | | integer | age | Enter age | | + | | text | prev_name | Expression label | concat(indexed-repeat(${name}, ${family}, 1, ${person}, 2), indexed-repeat(${age}, ${family}, 1, ${person}, 2)) | + | | end repeat | | | | + | | end repeat | | | | + """, # noqa pylint: disable=line-too-long + xml__contains=[ + """""" # noqa pylint: disable=line-too-long + ], + ) + + def test_indexed_repeat_math_epression_with_double_variable_in_dreaded_nested_repeat_relative_path_exception( + self, + ): + """Test relative path exception (absolute path) in indexed-repeat() with math expression and double variable using dreaded nested repeat.""" + self.assertPyxformXform( + name="data", + title="dreaded nested repeat indexed-repeat 1st, 2nd, 4th, and 6th parameters is using absolute path", + md=""" + | survey | | | | | + | | type | name | label | default | + | | begin_repeat | family | Family | | + | | integer | members_number | How many members in this family? | | + | | begin_repeat | person | Person | | + | | text | name | Enter name | | + | | integer | age | Enter age | | + | | text | prev_name | Expression label | ${age} > indexed-repeat(${age}, ${family}, 1, ${person}, 2) | + | | end repeat | | | | + | | end repeat | | | | + """, # noqa pylint: disable=line-too-long + xml__contains=[ + """""" # noqa pylint: disable=line-too-long + ], + ) From c01d8e3a53173351ccf878f1721665eeed3de636 Mon Sep 17 00:00:00 2001 From: Agus Hilman Date: Sat, 5 Dec 2020 17:54:34 +0700 Subject: [PATCH 3/3] Addressed review and fixed bug on expression with indexed-repeat and ${name} as arguments --- pyxform/survey.py | 11 ++++-- pyxform/tests_v1/test_repeat.py | 66 ++++++++++++++++++++++----------- 2 files changed, 52 insertions(+), 25 deletions(-) diff --git a/pyxform/survey.py b/pyxform/survey.py index 829ae5ce8..c56f8c101 100644 --- a/pyxform/survey.py +++ b/pyxform/survey.py @@ -918,13 +918,18 @@ def _is_return_relative_path(): return True # It is possible to have multiple indexed-repeat in an expression - for indexed_repeat in indexed_repeat_regex.finditer( + indexed_repeats_iter = indexed_repeat_regex.finditer( matchobj.string - ): + ) + for indexed_repeat in indexed_repeats_iter: # Make sure current ${name} is in the correct indexed-repeat if current_matchobj.end() > indexed_repeat.end(): - continue + try: + next(indexed_repeats_iter) + continue + except StopIteration: + return True # ${name} outside of indexed-repeat always using relative path if ( diff --git a/pyxform/tests_v1/test_repeat.py b/pyxform/tests_v1/test_repeat.py index c68bcc782..dc100d132 100644 --- a/pyxform/tests_v1/test_repeat.py +++ b/pyxform/tests_v1/test_repeat.py @@ -463,7 +463,7 @@ def test_indexed_repeat_regular_calculation_relative_path_exception(self): """Test relative path exception (absolute path) in indexed-repeat() using regular calculation.""" self.assertPyxformXform( name="data", - title="regular calculation indexed-repeat 1st, 2nd, 4th, and 6th parameters is using absolute path", + title="regular calculation indexed-repeat 1st, 2nd, 4th, and 6th argument is using absolute path", md=""" | survey | | | | | | | type | name | label | calculation | @@ -482,7 +482,7 @@ def test_indexed_repeat_dynamic_default_relative_path_exception(self): """Test relative path exception (absolute path) in indexed-repeat() using dynamic default.""" self.assertPyxformXform( name="data", - title="dynamic default indexed-repeat 1st, 2nd, 4th, and 6th parameters is using absolute path", + title="dynamic default indexed-repeat 1st, 2nd, 4th, and 6th argument is using absolute path", md=""" | survey | | | | | | | type | name | label | default | @@ -496,11 +496,11 @@ def test_indexed_repeat_dynamic_default_relative_path_exception(self): ], ) - def test_indexed_repeat_dreaded_nested_repeat_relative_path_exception(self): - """Test relative path exception (absolute path) in indexed-repeat() using dreaded nested repeat.""" + def test_indexed_repeat_nested_repeat_relative_path_exception(self): + """Test relative path exception (absolute path) in indexed-repeat() using nested repeat.""" self.assertPyxformXform( name="data", - title="dreaded nested repeat indexed-repeat 1st, 2nd, 4th, and 6th parameters is using absolute path", + title="In nested repeat, indexed-repeat 1st, 2nd, 4th, and 6th argument is using absolute path", md=""" | survey | | | | | | | type | name | label | default | @@ -517,16 +517,14 @@ def test_indexed_repeat_dreaded_nested_repeat_relative_path_exception(self): ], ) - def test_indexed_repeat_math_epression_dreaded_nested_repeat_relative_path_exception( - self, - ): - """Test relative path exception (absolute path) in indexed-repeat() with math expression using dreaded nested repeat.""" + def test_indexed_repeat_math_epression_nested_repeat_relative_path_exception(self,): + """Test relative path exception (absolute path) in indexed-repeat() with math expression using nested repeat.""" self.assertPyxformXform( name="data", - title="dreaded nested repeat indexed-repeat 1st, 2nd, 4th, and 6th parameters is using absolute path", + title="In nested repeat, indexed-repeat 1st, 2nd, 4th, and 6th argument is using absolute path", md=""" | survey | | | | | - | | type | name | label | default | + | | type | name | label | calculation | | | begin_repeat | family | Family | | | | integer | members_number | How many members in this family? | | | | begin_repeat | person | Person | | @@ -537,20 +535,20 @@ def test_indexed_repeat_math_epression_dreaded_nested_repeat_relative_path_excep | | end repeat | | | | """, # noqa pylint: disable=line-too-long xml__contains=[ - """""" # noqa pylint: disable=line-too-long + """""" # noqa pylint: disable=line-too-long ], ) - def test_multiple_indexed_repeat_in_epression_dreaded_nested_repeat_relative_path_exception( + def test_multiple_indexed_repeat_in_epression_nested_repeat_relative_path_exception( self, ): - """Test relative path exception (absolute path) in multiple indexed-repeat() inside an expression using dreaded nested repeat.""" + """Test relative path exception (absolute path) in multiple indexed-repeat() inside an expression using nested repeat.""" self.assertPyxformXform( name="data", - title="dreaded nested repeat indexed-repeat 1st, 2nd, 4th, and 6th parameters is using absolute path", + title="In nested repeat, indexed-repeat 1st, 2nd, 4th, and 6th argument is using absolute path", md=""" | survey | | | | | - | | type | name | label | default | + | | type | name | label | required | | | begin_repeat | family | Family | | | | integer | members_number | How many members in this family? | | | | begin_repeat | person | Person | | @@ -561,20 +559,44 @@ def test_multiple_indexed_repeat_in_epression_dreaded_nested_repeat_relative_pat | | end repeat | | | | """, # noqa pylint: disable=line-too-long xml__contains=[ - """""" # noqa pylint: disable=line-too-long + """""" # noqa pylint: disable=line-too-long + ], + ) + + def test_mixed_variables_and_indexed_repeat_in_epression_nested_repeat_relative_path_exception( + self, + ): + """Test relative path exception (absolute path) in an expression contains variables and indexed-repeat() using nested repeat.""" + self.assertPyxformXform( + name="data", + title="In nested repeat, indexed-repeat 1st, 2nd, 4th, and 6th argument is using absolute path", + md=""" + | survey | | | | | + | | type | name | label | calculation | + | | begin_repeat | family | Family | | + | | integer | members_number | How many members in this family? | | + | | begin_repeat | person | Person | | + | | text | name | Enter name | | + | | integer | age | Enter age | | + | | text | prev_name | Expression label | concat(${name}, indexed-repeat(${age}, ${family}, 1, ${person}, 2), ${age}) | + | | end repeat | | | | + | | end repeat | | | | + """, # noqa pylint: disable=line-too-long + xml__contains=[ + """""" # noqa pylint: disable=line-too-long ], ) - def test_indexed_repeat_math_epression_with_double_variable_in_dreaded_nested_repeat_relative_path_exception( + def test_indexed_repeat_math_epression_with_double_variable_in_nested_repeat_relative_path_exception( self, ): - """Test relative path exception (absolute path) in indexed-repeat() with math expression and double variable using dreaded nested repeat.""" + """Test relative path exception (absolute path) in indexed-repeat() with math expression and double variable using nested repeat.""" self.assertPyxformXform( name="data", - title="dreaded nested repeat indexed-repeat 1st, 2nd, 4th, and 6th parameters is using absolute path", + title="In nested repeat, indexed-repeat 1st, 2nd, 4th, and 6th argument is using absolute path", md=""" | survey | | | | | - | | type | name | label | default | + | | type | name | label | relevant | | | begin_repeat | family | Family | | | | integer | members_number | How many members in this family? | | | | begin_repeat | person | Person | | @@ -585,6 +607,6 @@ def test_indexed_repeat_math_epression_with_double_variable_in_dreaded_nested_re | | end repeat | | | | """, # noqa pylint: disable=line-too-long xml__contains=[ - """""" # noqa pylint: disable=line-too-long + """""" # noqa pylint: disable=line-too-long ], )