OpenClinica / enketo-oc

OpenClinica's fork of the Enketo web forms monorepo
Apache License 2.0
0 stars 1 forks source link

Relevant logic using indexed-repeat does not re-evaluate after a repeating group occurrence is added #109

Open pbowen-oc opened 3 years ago

pbowen-oc commented 3 years ago

The attached form has examples of relevant logic on repeating group items that a) looks at corresponding rows in a different repeating group and b) looks at values in the previous row of the current repeating group. When entering data initially, it appears that the relevant logic is evaluated when a new repeat is added, but not when data in the source items is updated. If the form is closed and reopened with the existing data, the relevant logic is loaded as expected.

Scenario 1 Steps:

  1. Open the attached form - relevant indexed repeat v2.xml.txt
  2. Enter 1 in the first number item
  3. Observe that the text2 item in the second repeating group becomes relevant
  4. Add a new repeat
  5. Enter 1 in the number item in the second repeat

Expected: The text2 item in the second repeat becomes relevant.

Actual: The text2 item in the second repeat remains non-relevant.

Scenario 2 Steps:

  1. Open the attached form again (new form)
  2. Enter 'x' in text3 item
  3. Add a new repeat
  4. Observe that text3 item is relevant in the second repeat when it first appears
  5. Add a new repeat
  6. Enter 'x' in text3 item in the second repeat

Expected: The text3 item in the third repeat becomes relevant.

Actual: The text3 item in the third repeat remains non-relevant.

In both cases, reopening the form with existing data will cause the relevant logic to be evaluated as expected, but further data entry has the original problem again.

MartijnR commented 3 years ago

First scenario:

This is a deliberate bug as a result of a performance optimization. Changing of the number1 value does not trigger re-evaluation in a different repeat. It's been like this for many years. Very easy to fix (and greatly simplify Enketo's code), but it means throwing away a huge performance improvement. Generally, logic dependencies between repeats do not work (except during the creation of the repeat, or loading a record).

Perhaps the getRelatedNodes function can look at other repeats, but only the first of each series to assess whether there are any logic dependencies. If there are it could include the whole series (including nested repeats). This would keep the performance degradation small unless such a logic dependency is found (with a large series). Pretty complex to do though, and will be prone to bugs.

pbowen-oc commented 3 years ago

@MartijnR - Did the more recent updates have any impact on this? If not and if the potential fix is complex and risky, let's defer this for now so we can keep the build more stable.

MartijnR commented 3 years ago

Did the more recent updates have any impact on this?

No.

Ok, let's defer. The simple 1 minute fix would not be risky, but just significantly degrade performance for forms with many repeat instances.

pbowen-oc commented 3 years ago

Works for me. Thanks.

I suspect we'll get requests for this sort of functionality, but we can see what happens.

pbowen-oc commented 3 years ago

@MartijnR - Is the expected behavior now that re-evaluation is not triggered in other repeats for any type of logic (constraint, required, calculation) and not just for relevant? It looks like at least constraints and required may be behaving the same way as relevant.

MartijnR commented 3 years ago

yes, it should be the same for all logic