IDEMSInternational / open-app-builder

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

Fix: item parameter_list dynamic context #2325

Closed chrismclarke closed 3 weeks ago

chrismclarke commented 1 month ago

PR Checklist

TODO

Description

Add support for using dynamic values within the parameter list of items and data_items components. E.g.

filter: @item.number < @local.test_value
filter: @item.string === @local.some_string

Author Notes

Should confirm if passing tests cases from example_items_pipe_2 (see current output in screenshot below)

I expect this won't solve the issue in https://github.com/IDEMSInternational/parenting-app-ui/issues/2303 (but could be tested). If confirmed not working should probably handle in a follow-up instead of this PR

Review Notes

Worth testing against both items and data_items syntax to test if working as expected. The fix did have to change a couple parts of the items code that isn't very well documented or tested, and so does have some scope for unexpected errors. So would be good to test against current debug sheets and with cases where the item lists are dynamically updated

Dev Notes

As all the code changes are made within the template-row.service there weren't any more tests to to items or data-items specs, although I have tested that existing specs still pass. The template-row.service has no existing infrastructure, and given this is mostly a workaround it seemed beyond scope to start adding (but could be done as a follow-up).

Unpicking the issue was a bit of a challenge given the complexity of the template-row service and item implementation. See more details in the comments on #1598. This PR goes for the first proposed solution, replacing the app-string evaluator with a pure JS evaluator and attempting to pre-evaluate all non-item dynamic expressions in advance.

I also independently noticed an issue when using the app-string evaluator to evaluator any sort of string comparison (not-working). I've added a placeholder test to identify but beyond scope of this PR to fix (the more I unpick the more that starts to unravel - I think really a lot of this would be better-off with templating system refactor)

The part of the fix I'm least certain about is the parameter list revert hack that was included in both items and data-items component. The comments suggested this is because parameter lists would have item context replaced, although looking at the code it seems that it wouldn't be until render, so not sure if either the fix was legacy or I'm missing something (might be worth confirming against cases where the items get updated)

I'm not sure if related to this PR or not but starting to see issue with modifying final fields image

(update - seeing on main - possibly may want to address after merge #2323)

Git Issues

Closes #1598

Screenshots/Videos

output from example_items_pipe_2 localhost_4200_template

esmeetewinkel commented 1 month ago

Issue #2303 was closed by PR #2314, but we forgot to mark it as such when moving from the original PR #2304 to the refactor.