IDEMSInternational / open-app-builder

PLH App Frontend
GNU General Public License v3.0
5 stars 24 forks source link

fix: handle undefined values in answer lists #2204

Closed jfmcquade closed 4 months ago

jfmcquade commented 5 months ago

PR Checklist

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:

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:

Screenshot 2024-02-14 at 14 59 38

Data list debug_answer_list_partial_data:

Screenshot 2024-02-14 at 15 00 12

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.

https://github.com/IDEMSInternational/parenting-app-ui/assets/64838927/db92d4bc-9a8c-46e8-a3e5-290354780d18

chrismclarke commented 4 months ago

Just to quickly summarise from the call today

jfmcquade commented 4 months ago

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 commented 4 months ago

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.

jfmcquade commented 4 months ago

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 commented 4 months ago

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?