Skip to content

Commit

Permalink
fix(jexl): properly cache is_hidden/is_required
Browse files Browse the repository at this point in the history
To speed up is_hidden/is_required evaluations, we cache the
result. However, the cache key used must be local to the document of
the answer, as otherwise the value might be wrong for table answers
(assume row 1 evaluated to another result than row 2 would, then we
wouldn't notice)
  • Loading branch information
David Vogt committed Apr 27, 2020
1 parent 97e2302 commit c09a133
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 9 deletions.
25 changes: 16 additions & 9 deletions caluma/caluma_form/jexl.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,15 +114,20 @@ def _all_containers_hidden(self, question):

return res

def _cache_key(self, question_slug):
field = self._structure.get_field(question_slug)
return (field.document.pk, question_slug)

def is_hidden(self, question):
"""Return True if the given question is hidden.
This checks whether the dependency questions are hidden, then
evaluates the question's is_hidden expression itself.
"""
cache_key = self._cache_key(question.pk)

if question.pk in self._cache["hidden"]:
return self._cache["hidden"][question.pk]
if cache_key in self._cache["hidden"]:
return self._cache["hidden"][cache_key]
# Check visibility of dependencies before actually evaluating the `is_hidden`
# expression. If all dependencies are hidden,
# there is no way to evaluate our own visibility, so we default to
Expand All @@ -136,23 +141,25 @@ def is_hidden(self, question):
self.is_hidden(self._question(dep)) for dep in deps
)
if all_deps_hidden:
self._cache["hidden"][question.pk] = True
self._cache["hidden"][cache_key] = True
return True

# Also check if the question is hidden indirectly,
# for example via parent formquestion.
if self._all_containers_hidden(question):
# no way this is shown somewhere
self._cache["hidden"][question.pk] = True
self._cache["hidden"][cache_key] = True
return True
# if the question is visible-in-context and not hidden by invisible dependencies,
# we can evaluate it's own is_hidden expression
self._cache["hidden"][question.pk] = self.evaluate(question.is_hidden)
return self._cache["hidden"][question.pk]
self._cache["hidden"][cache_key] = self.evaluate(question.is_hidden)
return self._cache["hidden"][cache_key]

def is_required(self, question):
if question.pk in self._cache["required"]:
return self._cache["required"][question.pk]
cache_key = self._cache_key(question.pk)

if cache_key in self._cache["required"]:
return self._cache["required"][cache_key]

deps = list(self.extract_referenced_questions(question.is_required))

Expand All @@ -162,5 +169,5 @@ def is_required(self, question):
ret = False
else:
ret = self.evaluate(question.is_required)
self._cache["required"][question.pk] = ret
self._cache["required"][cache_key] = ret
return ret
34 changes: 34 additions & 0 deletions caluma/caluma_form/tests/test_jexl.py
Original file line number Diff line number Diff line change
Expand Up @@ -347,3 +347,37 @@ def test_answer_transform_on_hidden_question_types(
)

assert qj.is_hidden(questions["form_question"])


def test_answer_transform_in_tables(info, form_and_document, form_question_factory):
# Bug test: When a JEXL expression references two answers, and one of them
# is hidden, the `answer` transform must return None for the hidden question
# to ensure correct evaluation.

form, document, questions, answers = form_and_document(
use_table=True, use_subform=False
)

table_question = questions["table_question"]

question_col1 = questions["column"]
question_col2 = form_question_factory(
form=table_question.row_form,
question__slug="column2",
question__is_required="'column'|answer == 5",
).question

table_answer = answers["table_question"]

ans_doc_1 = table_answer.documents.first()
ans_doc_2 = table_answer.documents.create(form=table_question.row_form)

ans_row1_col1 = ans_doc_1.answers.get(question_id="column")
ans_row2_col1 = ans_doc_2.answers.create(question_id="column", value=5)

validator = validators.DocumentValidator()

# we expect this to fail, as in the second row, the 'column' answer is 5,
# so 'column2' should be required
with pytest.raises(validators.CustomValidationError):
validator.validate(document, info)

0 comments on commit c09a133

Please sign in to comment.