Calculations for repeating group items not working like before

[eprager412]

Describe the bug
Calculations that should display information for each repeat group are not executing as expect/at all. The first repeat of the repeat group should show the position1 for item "" but this field is blank.

To Reproduce

1. Open 'repeatgroup_simple.txt' in mode /single/fs/
2. Click on 'yes' for the first question
3. Review question "This is a Repeating Group entry:".
4. Field is empty

Expected behavior
Item "This is a Repeating Group entry:" should have a value of 1.

Screenshots

Browser and OS:
-Windows
-Browser - chrome

Notes:
When I cut the xform down to 2 items, Item 1: "Was blood pressure measured?" and item 2: "This is a Repeating Group entry:" (item 2 is part of the repeat group) the calculation works as expected and I see the value 1 for "This is a Repeating Group entry:".

I was able to simplify the form down to item "Was blood pressure measured?" and then 2 items in the repeat group. 1 that is a none calculate type item "This is Repeating Group entry :". And the second must be an item with the type calculate. This form did not work and calculations were not show.

If you put two integer items or two non-calculate items, there will be no issue. Once you add a calculate item within the repeat group all other item calculations are broken.

[MartijnR]

Looks pretty serious.

• try to reproduce on https://getodk.org/xlsform - cannot
• file bug at https://github.com/enketo/enketo (unless it is already reported as one of these: https://github.com/enketo/enketo/issues?q=is%3Aissue+is%3Aopen+repeat)

[MartijnR]

@pbowen-oc, @kkrumlian, for expediency sake, could somebody at your side do the 2 things I mentioned above? Since it doesn't deal with submissions, it doesn't require using a backend.

[MartijnR]

Martijn to do

This appears to be caused by a customization in this fork.

In enketo-express-oc: http://localhost:8005/preview/?xform=http://localhost:3000/form/repeatgroup_simple/form.xml

Removing the 'init' calculation (that doesn't actually do anything) indeed avoids the issue. Very strange.

Cause appears to be in the calculate module customization. Something to do with first clearing the calculation values (due to non-relevancy) as desired by OC. It looks like the new enketo-core no longer re-calculates them when the repeat becomes relevant. Why this doesn't happen when the init no-op question is removed is still a mystery and may mean my diagnosis is wrong.

[theywa]

Hi @MartijnR ~

I tried to debug the issue, and here are my findings :
comparing two xls files that are similar I found that is related to the calculate type item(name: bp_rg_num_init)

it's in line with your comment above about calculate.

in HTML it looks like this

this input(bp_rg_num_init) is called first (I think due to its deep is lower than other input) when this line 83 in calculate.js

the problem is when the code(line 454 in calculate.js) that get the ancestorGroup from that input(bp_rg_num_init),

the element that choosed as it's ancestor is section.or-group-data.or-branch.pre-init.or-appearance-no-collapse the ancestor from another input [name=/data/page2/bp_rg/bp_row]

The result value on line 489 in calculate.js is false for input(bp_rg_num_init) then it's ancestor(section.or-group-data.or-branch.pre-init.or-appearance-no-collapse) is saved in the this.preInitRelevance with false value (line 499 in calculate.js)

This caused an issue when the other input [name=/data/page2/bp_rg/bp_row] called next. Since it's ancestor is already saved in the map(this.preInitRelevance) then it will return false due to line 486 in calculate.js.

the false above will be value of this._isRelevant(props) line 329 in calculate.js then it will make the empty to be true

empty = true will make the value result line 341 in calculate.js will be ''(empty char).
CMIIW, I think that caused the issue with empty value on calculating group.

Do you think we can exclude the #or-calculated-item child from getting the ancestor or any suggestion to get the correct ancestor for it?

I'm not quite familiar with the logic code so I'm sorry if my deduction is wrong, can you elaborate more about the calculation you mentioned above?
Thanks

[MartijnR]

@theywa, quite an investigation!

I still have to read through it a bit more, but I wanted to quickly comment on the ancestorGroup of bp_rg_num_init. Even though it seems odd in the DOM due to the weird placement of calculations without form controls (in the #or-calculated-items group), it is correct that the .or-group with name attribute ending on /bp_rg is the ancestorGroup. This is determined by the path of bp_rg_num_init which is /data/page2/bp_rg/bp_rg_num_init.

This may not necessarily be helpful, as I may have misunderstood one of your comments. Will try to read through your comments more thoroughly later.

[MartijnR]

Do you think we can exclude the #or-calculated-item child from getting the ancestor or any suggestion to get the correct ancestor for it?

I don't know. I don't think so.

I am looking at the code too. I don't see where preInitRelevance is reset after the element becomes relevant. I only see one resetInitRelevant.set() statement (which is only called once). This makes me think we should be able to reproduce this issue on enketo/enketo after all. Might have something to do with config.forceClearNonRelevant. I will check again about reproducing this in enketo/enketo.

[pbowen-oc]

I observed this issue in the Blood Pressure repeating group on my test form on the first pass through as a new form. However, when I reopened the form as an existing record, the calculations in the repeating group ran as expected. So, this looks like the same issue as #16.

[MartijnR]

My findings:

• I cannot reproduce in enketo/enketo because I don't think it is possible any more to configure enketo/enketo to clear non-relevant values. The clear() function in relevant.js is probably a no-op in enketo/enketo, and only used by OpenClinica. Similarly, I suspect the forceClearNonRelevant in relevant.js is never used (not even in this OC fork possibly). So some reorganization of the 2 repos is in order, cleaning up enketo/enketo in the process.
• The preInitRelevance map in calculate.js is never updated (it's not meant to) and I think this is okay in enketo/enketo because emptyNonRelevant is never true (so const empty = emptyNonRelevant ? !this._isRelevant(props) : false; always returns false without running _isRelevant()) again in ODK Enketo world. In the OC world emptyNonRelevant is sometimestrue (see calculate.js in the fork) and this is causing this bug. The _isRelevant function (in Enketo Core) doesn't return the correct result after relevancy changes to true.

One potential fix in enketo-core, calculate.js, _isRelevant fn is changing line 486 to:

                        if (preInitRelevance != null && !this.initialized) {
                            return preInitRelevance;
                        }

I think this could be done in enketo/enketo as well but we need to first identify a real bug in Enketo/Enketo, I think (no tests failing in enketo-core when doing this).

• create PR for enketo/enketo for a proper fix to a bug that is still be defined for enketo/enketo => Fixed _isRelevant function in calculation module returns incorrect ca… enketo/enketo#1296
• remove this workaround in enketo-oc (monorepo) after PR is approved and merged > Fix/move calculate customizations #8

For now, in this fork, let's just overwrite the preInitRelevance cache feature by adding the following line to calculate.js in this fork (the update function):

    // We're disabling this map because it is causing this bug
    // https://github.com/OpenClinica/enketo-oc/issues/22
    // which we don't quite understand, but need to fix urgently.
    this.preInitRelevance = new WeakMap();

We'll likely incur a performance hit by disabling this cache until the PR to enketo/enketo can be created and merged.

So @theywa, while looking into your analysis it appears you found the cause, and I think I stumbled upon a temporary "fix" and figured it was easiest to create a PR for it myself. I'll ask you to review the PR!

[theywa]

@MartijnR ~ Thank you for the explanation. Ah yes, that is much easier to reset the map than figure out how to handle the #or-calculated-item as I asked before. Also, I tested your workaround on dev and it fixes the initial value for the calculated issue. Personally, I approve the workaround since this is urgent.
What do you think @pbowen-oc ?
I also checked the vital form, and the blood pressure average was calculated correctly in the initial load(first-time load)

[pbowen-oc]

@theywa - If the workaround solution addresses the calculation issue and does not cause other issues, then it works for me.