Open eprager412 opened 7 months ago
MartijnR
commented
6 months ago Looks pretty serious.
MartijnR
commented
6 months ago @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
commented
5 months ago 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
commented
5 months ago 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
commented
5 months ago @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
commented
5 months ago 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
commented
5 months ago 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 OpenClinica/enketo-oc#16.
MartijnR
commented
5 months ago My findings:
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.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).
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
commented
5 months ago @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
commented
5 months ago @theywa - If the workaround solution addresses the calculation issue and does not cause other issues, then it works for me.
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
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.