fix: handle undefined values in answer lists

[jfmcquade]

PR Checklist

• PR title descriptive (can be used in release notes)

Description

Handles the case where some values for text or image within an answer list may be undefined. When authoring an answer list that gets dynamically populated by various data lists, there will sometimes be undefined values that are complicated to handle at an authoring level (#2166). For example, for a generated answer list of length 4:

1. image/text values for some items may be blank
2. there may be fewer than 4 options (i.e. for some items both text and image is blank)

This PR adds answer list parsing such that:

• in case 1., the image/text component corresponding to an undefined image/text property does not render (previously a broken image link or the word "undefined" would be shown)
• In case 2., where an item in the answer list has no value for text or image, the whole item is not rendered.

This fix applies to all components that take answer_list parameters: combo_box, radio_button_grid and radio_group. See demos below.

Testing

Test the debug_answer_list_partial template (see screenshots below). Also consider testing example_answer_data, which was added in #1967 (the summer of love) to test answer lists coming directly from data lists, to ensure this still functions as expected.

Git Issues

Closes #2166

Screenshots/Videos

Template debug_answer_list_partial:

Data list debug_answer_list_partial_data:

debug_answer_list_partial demo.

Note that in this demo, the combo_box and radio_grid components displays a blank box for option_3. This is because option_3 does have a value for image in the answer list, and only those items with no value for all other fields besides name will be removed from the answer list. In actual applications, answer lists passed to a combo_box and radio_grid components would not typically contain any properties besides name and text (there would be no image). If we do want to handle undefined values in the case of an answer list that will be used for both combo_box and, for example, radio_button_grid components, that can be handled in a follow-up.

answer.list.partial.demo.mov

[chrismclarke]

I have a couple reservations when it comes to implementing in this way, see comments inline - happy to discuss on a call if useful

[chrismclarke]

nit(blocking)
Do you have any idea how a string "undefined" "NaN" or "null" could be populated? It would be much better to fix at the source, e.g. if there is some sort of getStringParam parsing these then we should apply the fix there instead. I'm guessing similarly we need a sensible response for NaN which I'm guessing comes from some parseNumberParam` or similar...

Additionally I'm not sure converting empty string to undefined makes sense. When thinking of answer lists there's sometimes a case where html components may want to use empty string to represent nothing selected in the context of something like a select dropdown.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/select

[jfmcquade]

Yeh I don't think this was sensible, I've updated to only convert the string "undefined" to undefined. It is supposed to catch precisely the case where the value is undefined because the answer list is being generated from a data list which may not have a value in the relevant column

[chrismclarke]

nit(blocking)
This logic seems a bit confusing to me, we're running a filter operation but then have an inner for loop which will escape before completion. Might make more sense as 2-step operation, e.g more like

const hadItemData = Object.entries(item).find(([key,value])=>(key !== "name" && value !== undefined)
return hadItemData  ? true : false

That being considered, I'm not sure if this makes total sense to implement - surely it's better to let broken images through so that an author can be aware of an authoring mistake instead of just hiding the data away? I'd probably lean more towards providing an authoring option to add a filter function to the specific component, e.g. filter_no_image:true or ensuring support for runtime filtering, e.g. @data.some_list.filter(item=>item.image!==undefined)

Additionaly we'd still end up with broken images in cases where text is supplied without image with the above filter.

[jfmcquade]

Your suggested logic is definitely clearer, I have refactored accordingly.

In terms of allowing through broken images, a broken image link will still be allowed through with the suggested implementation – an invalid path to an image will still display a broken image link. The image will only be filtered out if the value is undefined – which admittedly could be due to an authoring error that the author should be made aware of, but it's not clear to me that displaying a broken image would be necessary in such cases.

Either way, returning to the issue referenced in this PR, we want a way to author an answer list in a generic way where the source data may or may not contain image paths, and in the case where an item does not reference an image, to display no image (and not a broken image link). Filtering on the undefined values as suggested seems like a satisfactory solution, but I may be missing some associated problems, and there may be other approaches.

Ultimately, I think the best solution may be a way of defining arbitrary data structures, or at least answer lists, directly from data lists, handled at parse-time, in a way that can handle lists of multiple length and containing partial values (as in WIP RFC for a universal data list parser). But I think we want an acceptable solution sooner in order to help with the current authoring requirements.

[chrismclarke]

Just to quickly summarise from the call today

• Seems like a reasonable option to remove entries that have no data included whatsover
• Existing logic should be able to handle image/text removal if one if included and not the other (to be confirmed?)
• It would still be preferable to track down how "undefined" string ends up in the answer_list if possible (to reduce chance of similar issue re-occuring), or to create as a small follow-up issue if not easy to do (and apply workaround proposed in pr)

[jfmcquade]

It would still be preferable to track down how "undefined" string ends up in the answer_list if possible (to reduce chance of similar issue re-occuring), or to create as a small follow-up issue if not easy to do (and apply workaround proposed in pr)

I think this is actually quite straightforward: "undefined" would be a string in answer list in cases like that in the issue referenced in this PR (e.g. when explicitly defining an answer list referring to values which themselves may be undefined) because the answer list string does not receive any parsing additional to the parseAnswerList function.

#1967 introduced the parseAnswerList utility function, where previously components that used answer lists handled the parsing directly in the component code.

I've now added a commit to this PR, which incorporates this logic into a getAnswerListParamFromTemplateRow() utility function to closer resemble the other utility functions for getting params.
As an answer list before this parsing is just an array of strings, it is not surprising that the value "undefined" would come through as a string. The fix is exactly what is now included in this PR: explicitly converting "undefined" to undefined when it is the value of a property within an entry of an answer list. It may be that we want to do additional conversions on some more strings here (e.g. "null"), but it's not apparent to me how these would end up in the answer list string, and all of the properties of an answer list item are expected to be strings (so we don't need to parse numbers as such), so I think handling "undefined" should be fine for now.

[chrismclarke]

I think this is actually quite straightforward: "undefined" would be a string in answer list in cases like that in the issue referenced in this PR (e.g. when explicitly defining an answer list referring to values which themselves may be undefined) because the answer list string does not receive any parsing additional to the parseAnswerList function.

Ah - yeah I see now, yeah the answerlist is usually stored in an intermediate local variable which is an array (auto converted by parser for anything with _list in name), but whose elements are formatted string. So when the dynamic variables are replaced they are dropped into something like
["name: @local.name | text: @local.text"] -> ["name: undefined | text:undefined"]

So what would actually be better would be to process these within the parser to convert the string-array into an object-array. That way when replacing the localVariable it could already be formatted as:

[{
   "name": "@local.name",
   "text": "@local.text"
}]

Which when converted should replace the variables (local, data, field or anything else) it should be able to assign the proper value.

I think would make sense to merge what's here now to fix the issue in the short term, and then I'll try make a quick follow-up pr/issue to see if possible to update the parser as needed.

[chrismclarke]

nit(non-blocking) can remove temp logs

[chrismclarke]

Thanks @jfmcquade

I think looks good to fix the immediate issue (have suggested follow-up in separate comment). I think also makes sense to have the more unified way to extract answer list param so thanks for adding

[chrismclarke]

nice addition (although shame the git diff isn't showing very cleanly)
I think the rest of the code is the same as you updated previously, so looks good to me

[esmeetewinkel]

Functional test passed (debug and plh_kids_tz).

I'm seeing the parsed answer lists as red text (not errors though) in the console which I think can be helpful for debugging.

[jfmcquade]

I think would make sense to merge what's here now to fix the issue in the short term, and then I'll try make a quick follow-up pr/issue to see if possible to update the parser as needed.

I've created an issue here: #2210

I'm seeing the parsed answer lists as red text (not errors though) in the console which I think can be helpful for debugging.

This was actually a log I didn't mean to leave in. If it, or a similar log, would be useful to keep then we should probably add a label like Source answer_list:

[esmeetewinkel]

This was actually a log I didn't mean to leave in. If it, or a similar log, would be useful to keep then we should probably add a label like Source answer_list:

Okay, I guess it would be more consistent with the rest of the app to leave it out - and the console would be less cluttered. This does remind me of our discussion that it would be useful for debugging to "see" the values of local variables somehow.

Would it be possible/sensible to have logs of this sort visible in the console when the debug-mode is enabled?