feat: set_item action: optionally target an index or id

[jfmcquade]

PR Checklist

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

Description

Adds two new optional parameters for the set_item action: _id and _index. These are used to target a specific item in the data items loop to set the a value on, for example, click | set_item | _index: 2, completed: true or click | set_item | _id: my_some_id, completed: true.

_index

NB, the index supplied is evaluated in terms of the standard 0-based indexing of arrays, where the first element of an array has index 0, the second element has index 1, and so on.

JS expressions can be used to specify the target index. Trivially 1 + 2, for example, to target the item at index 3, but also these expressions can use the @item._index to refer to the index of the current item in the loop. For example, set_item: @item._index + 1 will target the item that appears after the current item in the data items iterator.

Testing

Experiment with Example 4 in the the comp_data_items template – this isn't the prettiest example, but should demonstrate the functionality. And with the example_task_group_stepper template. See screenshots below.

Git Issues

Closes #

Screenshots/Videos

Action

Updated comp_data_items

Use case demo

example_task_group_stepper template and demo. This is our use case for this new feature.

https://github.com/IDEMSInternational/parenting-app-ui/assets/64838927/9ba57955-e9bc-4f80-92c5-b4058ce6aa3a

New tests passing

[esmeetewinkel]

In order to reference the index of the current item, I originally imagined using the syntax @item.index at the template level. However there are various complications with this. As the @item. syntax is generally used to refer to columns/keys of the item in the data list, @item.index would be needed to be treated as a special case. Additionally, as @item.index can only be evaluated in the context of of the data items loop in which it finds itself, it would need to pass through, unparsed, until being handled by the data-items component. So using a keyword, item_index, was much more straightforward to implement. If this is acceptable, we could merge this implementation and plan to support another syntax in the future.

Happy with this for now. One reason why @item.index would make sense to me: It would occasionally be useful to me on any loop (also non-dynamic begin_items) to be able to refer to the index of an item in a list. Currently I'm achieving this by adding +1 to a local variable each time the loop sets it, but that feels more artificial / error prone.

[chrismclarke]

In order to reference the index of the current item, I originally imagined using the syntax @item.index at the template level. However there are various complications with this. As the @item. syntax is generally used to refer to columns/keys of the item in the data list, @item.index would be needed to be treated as a special case. Additionally, as @item.index can only be evaluated in the context of of the data items loop in which it finds itself, it would need to pass through, unparsed, until being handled by the data-items component. So using a keyword, item_index, was much more straightforward to implement. If this is acceptable, we could merge this implementation and plan to support another syntax in the future.

As mentioned in the comment, all items already have a readonly @item._index property which can be used instead of any new variables.

A possible alternative syntax (not implemented) came up during development: click | set_item_at_index | index: item_index + 1, active:true or even click | set_item | index: item_index + 1, active:true (as index would be provided explicitly, it may not be confusing to include as an optional parameter on the existing set_item action. This could be substituted for the currently implemented syntax outlined above if it was deemed preferable.

I like the look of this, although I would again refer to _index readonly property. We should clarify when to use comma-separated or semi-colon separated (possibly following #2211, as may want to parse action params in a similar way as required). Ideally we want to use a syntax that automatically gets parsed as a json object

So assuming syntax something like:

 click | set_item | _index: @item._index + 1; active:true

I would also be tempted to let the user pick an item by id, e.g.

 click | set_item | id: my_some_id; active:true

And updating the logic to first extract {_index, id, ...writeableProps } = arg, using the _index or id if available to select item, with default current item in list if not provided.

The benefit of doing things this way is we could then use pretty much identical syntax when adding support for updating items from outside the item list itself (currently not supported but expect a nice-to-have).

[chrismclarke]

@jfmcquade - might be worth revisiting this one now #2220 is merged. When you get a chance to resolve conflicts let me know and I can give a quick additional check

[jfmcquade]

Thanks for the reminder, @chrismclarke, I believe this is ready for review

[github-actions[bot]]

Visit the preview URL for this PR (updated for commit 74a3efb):

https://plh-teens-app1--pr2215-feat-set-item-at-ind-on4graoj.web.app

_{(expires Tue, 14 May 2024 05:50:59 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: e4c0bab6b08dd290fbf002fd6e07987fa4b5fce1}

[jfmcquade]

Thanks @chrismclarke, see my reply to your latest inline comment.

In terms of tests, I've added some for the set_item logic, and some minimal tests for the template-variables service. The logic of the template-variables service is pretty complex, and it's hard to generate minimal examples of the the context object for testing (I logged out some actual context for a minimal sheet and copied the result for the tests I have written). I think more extensive tests are probably outside the scope of this PR, but the spec file is there and the tests can be run now at least. There is a comment in the app-data-variable service that it should ideally be extended to replace the template-variables service, so potentially there's a more substantial TODO to handle this refactor, including adding extensive tests. Let me know if you think some more tests here are minimally required for this PR.

[chrismclarke]

Thanks @chrismclarke, see my reply to your latest inline comment.

In terms of tests, I've added some for the set_item logic, and some minimal tests for the template-variables service. The logic of the template-variables service is pretty complex, and it's hard to generate minimal examples of the the context object for testing (I logged out some actual context for a minimal sheet and copied the result for the tests I have written). I think more extensive tests are probably outside the scope of this PR, but the spec file is there and the tests can be run now at least. There is a comment in the app-data-variable service that it should ideally be extended to replace the template-variables service, so potentially there's a more substantial TODO to handle this refactor, including adding extensive tests. Let me know if you think some more tests here are minimally required for this PR.

Thanks for adding the necessary scaffolding and starting to build out the item tests - agreed the service really isn't set up very well for long term maintainability (lots of complex code and weird caveats - which probably makes testing more important but beyond scope of this PR for sure). I've added 149c868 to try and tidy up a little and add a few more basic public method tests, but will stop there as fair chance we'll want to do a larger refactor at some point in the future.

But at least for now it covers the new introduced behaviour so I think should be good to go if you're happy with the changes to 149c868ab3fe44cd163bcfc840f8e9e70e0997cd @jfmcquade