search

IDEMSInternational / open-app-builder

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

feat: data-items local context (alt) #2314

Closed chrismclarke closed 1 month ago

chrismclarke commented 2 months ago

PR Checklist

Description

Alternate implementation to #2304

Author Notes

To address the issue seen in the debug_kids template I've modified the sheet slightly (see comments on sheet)

I couldn't get the stop_loop condition working properly (I think the data doesn't update frequently enough to work as expected), so instead just added a condition to the display_group (so slightly less efficient as will still process all items before stopping).

I think this can also be handled by using the parameter_list to provide a filter operation (can't find debug sheet but pretty sure we included that functionality previously)

Dev Notes

I haven't added any tests as they would likely need to be at the data-items.component.spec.ts level which we haven't scaffolded out (but might be good to do at some time given the complex nature of the component)

I'm not sure if any of the tests introduced in #2304 would be good to migrate over or not

I'm also not sure if there's any potential knock-ons from no longer populating the item rows to the parent templateRowMap variable. I expect not as the only reason to do so would be referring from outside the items to a row inside, and as far as I'm aware we haven't added that functionality yet (i.e. have text at the end of the page with reference to a specific item value, @local.item_2.value - previously the lookup would only have reference to 2 instances of @local.item_{{@item.id}} ). Although if useful we could make a follow-up to tell the processor to update the parent map after processing

closes #2304

Git Issues

Closes #

Screenshots/Videos

debug_data_items - all items show local context variable image

debug_kids - now correctly renders just single row image

esmeetewinkel commented 2 months ago

To address the issue seen in the debug_kids template I've modified the sheet slightly (see comments on sheet)

I couldn't get the stop_loop condition working properly (I think the data doesn't update frequently enough to work as expected), so instead just added a condition to the display_group (so slightly less efficient as will still process all items before stopping).

I realised my debug sheet wasn't entirely clear because I was using the id as an example column to check for, which has unique values. I've updated the debug sheet now to reflect the use case where there is multiple rows that match the condition (e.g. looking for rows of a particular type, which there are multiple of). I do actually need the loop to stop after the first time the condition is met - or I need another way to find "the first row where the condition is met".

I think this can also be handled by using the parameter_list to provide a filter operation (can't find debug sheet but pretty sure we included that functionality previously)

We do have the parameter list filter operation on data_items, however, I believe it doesn't accept comparisons to local variables e.g. filter: @item.type == @local.type (or at least items doesn't https://github.com/IDEMSInternational/parenting-app-ui/issues/1598) so it's not of use here. If that were possible, however, I think that would avoid the stop_loop issue as well, because I could simply get away with using a parameter list on the begin_data_items row being filter: @item.type == @local.type; limit: 1.

Edited: @jfmcquade pointed out on Mattermost a temporary (hacky) way to do this using javascript functions, example here image

jfmcquade commented 2 months ago

Thanks, @chrismclarke, I definitely think this implementation is preferable to #2304. I've copied over a test from #2304 (https://github.com/IDEMSInternational/parenting-app-ui/pull/2314/commits/79323f0cb99c87e6ed9d81b61d68536414cabe72), I think the rest of that PR can be abandoned. In terms of adding tests for the data items logic, I did add some tests to data-items.component.spec.ts as part of #2309, I agree it would be good to expand these at some point. As part of that follow-up, do you think it would be sensible to extract the items related logic out of both the data-items component and the dynamic data service and into a new data-items service (as floated here)? This would likely make testing easier (for example if some of the complex logic of the data-items component was in public methods of a standalone service).

chrismclarke commented 2 months ago

Thanks, @chrismclarke, I definitely think this implementation is preferable to #2304. I've copied over a test from #2304 (79323f0), I think the rest of that PR can be abandoned. In terms of adding tests for the data items logic, I did add some tests to data-items.component.spec.ts as part of #2309, I agree it would be good to expand these at some point. As part of that follow-up, do you think it would be sensible to extract the items related logic out of both the data-items component and the dynamic data service and into a new data-items service (as floated here)? This would likely make testing easier (for example if some of the complex logic of the data-items component was in public methods of a standalone service).

I've started to stub out some tests in https://github.com/IDEMSInternational/parenting-app-ui/pull/2315 as I think might get a bit messy if adding here

chrismclarke commented 2 months ago

We do have the parameter list filter operation on data_items, however, I believe it doesn't accept comparisons to local variables e.g. filter: @item.type == @local.type (or at least items doesn't #1598) so it's not of use here. If that were possible, however, I think that would avoid the stop_loop issue as well, because I could simply get away with using a parameter list on the begin_data_items row being filter: @item.type == @local.type; limit: 1.

Edited: @jfmcquade pointed out on Mattermost a temporary (hacky) way to do this using javascript functions, example here

Thanks for the clarifications @esmeetewinkel . That makes sense, I think probably best to handle in a follow-up PR once merged

esmeetewinkel commented 1 month ago

Closes #2303