Skip to content

Conversation

@dehall
Copy link
Contributor

@dehall dehall commented Mar 16, 2023

Fixes the issue in #323 where removing an Observation will leave a null in the list.

The short version of the root cause here is that there is some magic behind the scenes to filter out these nulls, but the function wasn't being invoked properly to trigger that.

Full details for posterity in case this happens again:
The this.props.onChange function invoked on the changed line points to this:

onChange = (update, path=[]) => {
if(typeof update !== 'object') {
return (val) => this.onChange(val, [].concat(path, [update]))
}
this.props.editNode(update, path)
}

which is a recursive closure, where every time it gets called with a string it creates the next level of the function with a path element appended to it. Path is an array containing the sequence of fields in the JSON to get to a given location. For example ["covid19/measurements_daily", "states.CBC_Differential", "conditional_transition", "[0]"] (Don't ask why states.CBC_Differential is one entry instead of two, I didn't dig that far)
Anyway once this function gets called with an object, that object is used to set the value at the given path. If the object has a null then that means "delete".

Ultimately it works its way to the reducer:

case 'EDIT_NODE':

And more specifically this section will pick up the parent field, check if it's an array, and if so filter out any "falsy" values such as null.

if(Array.isArray(newVal)) {
_.set(newState.modules, parent, newVal.filter(x => x));
}

Because the onChange function up above was previously being called with observation.[{i}], the "parent" field (at the second-to-last index in the path array) wasn't correctly identified and so it wasn't looking at the observations array to filter out nulls.
The fix is to change the call to onChange to call it once for each level of the path, ie, onChange('observations')(`[${i}]`) instead of onChange(`observations.[${i}]`)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants