Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dangling nulls in list when removing Observation from DiagnosticReport/MultiObservation #324

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

1 participant