Feat: set data action standalone

[chrismclarke]

PR Checklist

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

Description

Add actions to support data_list manipulation from outside of data_items context loops, originally discussed in RFC 21. Data Actions

This PR originally started in #2454, but has been separated out. This does not include any changes to set_item syntax or data_items loop handling (will be addressed in follow-ups)

Main Changes

• add set_data action
• add reset_data action
• add tests

Additional Changes

• Create new feat_data_actions sheet for demo
• Minor test tidying

Future Follow-ups

• Add insert_data action
• Add remove_data action
• Remove hackParseTemplatedParams method added (requires update to items loop code)
• Update dynamic data documentation to include use-cases and examples
• Support additional operators to change what data actions apply on, e.g. filter, sort, limit

Author Notes

Syntax
See examples in sheet feat_data_actions

Question
We previously said set_data would be the preferred name for actions to align with set_field , set_local etc. Although do we still want to keep this convention given we will likely need several follow-up actions, namely:
set_data, reset_data, insert_data, remove_data
vs
data_set, data_reset, data_insert, data_remove

Key Point
This PR does not change how data_lists are updated from within data_items loops. For now authors can still use set_item, but be aware it uses different code to the set_data action and so might behave slightly differently. Follow-up PRs are planned to unify the handling so that the set_item code can function in the same way as set_data, using the context of the item loop to set default _list_id and _id variables for the item (so author only needs to write explicit values to update)

Key Point
When setting data from a parameter_list, by default all the key-value pairs defined in the parameter list are parsed as strings (unless set as an intermediate local variable first). As this has high potential to confuse authoring, additional code has been added to try and convert the string into the correct data type, using the original data_list as source of which columns contain which type of data. As such it means the first row of any data_list currently needs to contain values for this to work correctly. It might be worth creating a follow-up issue to provide alternate ways to define the schema of a data_list, for example allowing a top-row metadata row that just specifies data types.

Review Notes

Functional test of feat_data_actions

For code tests, see updated specs (will need to run individually)

Dev Notes

This includes quite a few updates to tests, however as the angular tests are still not built into CI (due to #2196) they all need to be run manually. As we continue to work on refactoring service code it would be really useful to get these tests unblocked so that we can identify any regressions introduced.

A few of the mock tests have been moved to their own folder instead of the original spec file just so that the corresponding tests aren't triggered when importing from the file (looks like there's a few tests that will have been broken recently, likely with changes to deployment and app config - but beyond scope to fix). So again, would be good to get the full angular test suite passing so that we can include in CI

Some of the existing code and tests within the dynamic-data.service file has been migrated into the new data-actions.service, and further developed from there. This is to try and keep code a little bit more separated and easy to iterate on

I remember some of the initial discussion for the set_data action started in #2388, however I lost track of that PR when creating the various follow-ups. So I'm assuming the code here (and in other draft PRs) should supercede, but I'll hold off closing that for now until I do another follow-up on data_items lists in #2454

While developing I realised the issue with all parameter_list values being parsed as strings and added the workaround to coerce data types. I think generally this is useful to have and could be build on in the future to include more things like data validation (or any other rxdb schema options that we want to expose to authors). I haven't tried to address object or array types as I don't think there's a way for authors to include those data_types in lists (although maybe if a column ends _list...?). I think more advanced data type handling could possibly be saved for follow-up

You'll notice I've update the resetFlow method of the dynamic-data.service. Previously the method would delete the entire table and recreate from scratch. The problem with doing this when resetting data is that any existing data subscriptions also get destroyed, so the data-items component would somehow need to know to resubscribe once recreated. Instead it now deletes all rows and repopulates. This is more desirable than just trying to reverting values within existing rows for future cases where we have the ability to append new rows.

Git Issues

Closes #

Screenshots/Videos

Screenity.video.-.Nov.9.2024.webm

[github-actions]

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

https://plh-teens-app1--pr2513-feat-set-data-action-fga52kw1.web.app

_{(expires Tue, 03 Dec 2024 00:16:04 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: e4c0bab6b08dd290fbf002fd6e07987fa4b5fce1}

[esmeetewinkel]

Functional test passed. I still don't particularly like the syntax

click | set_data | 

_list_id: feat_data_actions_list,
_index: @local.last_index,
completed:!@item.completed

because the conditions (_list_id: feat_data_actions_list, _index: @local.last_index,) and the action that should happen when the conditions are satisfied (completed:!@item.completed) appear in the same list - but appreciate it might be the most straightforward way that respects our conventions so far.

We previously said set_data would be the preferred name for actions to align with set_field , set_local etc. Although do we still want to keep this convention given we will likely need several follow-up actions, namely:
set_data, reset_data, insert_data, remove_data

This naming makes sense to me and seems consistent (also reset_field and remove_field could make sense as operations - I don't have an immediate use case though).

For now authors can still use set_item, but be aware it uses different code to the set_data action and so might behave slightly differently.

Noted.

As such it means the first row of any data_list currently needs to contain values for this to work correctly. It might be worth creating a follow-up issue to provide alternate ways to define the schema of a data_list, for example allowing a top-row metadata row that just specifies data types.

Noted, follow-up issue here.

[chrismclarke]

Functional test passed. I still don't particularly like the syntax

click | set_data | 

_list_id: feat_data_actions_list,
_index: @local.last_index,
_filter: @index < 5,
_limit: 2,
completed:!@item.completed,
number: 5,
string: this is updated

because the conditions (_list_id: feat_data_actions_list, _index: @local.last_index,) and the action that should happen when the conditions are satisfied (completed:!@item.completed) appear in the same list - but appreciate it might be the most straightforward way that respects our conventions so far.

The main reason for keeping it limited to everything in a 3rd argument is so that the set_data action can be consistent with the set_item action (which doesn't require you to specify a list or id, but can optionally include). E.g.

click | set_data | _index: @item.index+1, completed:!@item.completed

click | set_item | _index: @item.index+1, completed:!@item.completed
click | set_item | completed:!@item.completed

If we move to using 4 arguments by default then set_item becomes weird as either it would need to leave the 3rd argument as empty, or we would need to specify the args in the order of data changes before data selection, i.e.

click | set_data | completed:!@item.completed |  _list_id: feat_data_actions_list, _index: @local.last_index

click | set_item | completed:!@item.completed 

I'm not totally opposed to this, but then it does raise the question if we start to add more functionality (like filter/limit operations) whether we keep adding on more arguments. In the past I've found going over 3 arguments to get pretty confusing, but ultimately we're trying to include a huge amount of information in a single cell so never going to be perfect.

Another option might even to be having a dedicated data_list subtype actions_list that can be referred to by name, e.g. action_list column could just have something like toggle_data_completed and the actions data_list be along the lines of

id	trigger	action	target	changes
toggle_data_completed	click	set_data	_list_id: feat_data_actions_list, _index:@local.last_index	completed: !@item.completed

Where essentially the list can use named columns that will just be passed as ordered arguments (although this gets suuuper confusing if still referring to things in the local template).

This naming makes sense to me and seems consistent (also reset_field and remove_field could make sense as operations - I don't have an immediate use case though).

Sounds good

Noted, follow-up issue here.

Thanks

[ChrisMarsh82]

Slight issue although may be with tasks and not this.
Problem: Ability to run through and keep progress of tasks for each user group
This can be solved by users selecting a user group. Then all tasks will update 'completed' and 'completed[user_group]'
Then when users change user group an action runs 'completed' = 'completed[user_group]'

The problem comes with the multiple datalists that need to be maintained to allow tasks to work and that we don't want to be restrictive on sub levels. Therefore the action (completed = completed[user_group]) needs to run on an unspecified number of datalists (i.e. Modules, Modules_[sub_module] x Number of sub-modules, Modules_[sub_module]_[sub_sub_module] ..)

Possible solutions:

1. Add task_group filter on tasks so that it would be looking at a subset of a datalist
2. Allow action to take a @Local variable so that loops can be created to build up the complicated action that will be over multiple datalists

Personally, I would like the ability to do both of these. Option 1 feels the better solution for this issue as it lowers the complexity of generating multiple datalists. The 2nd option would mean that any datalist actions can be centralised as there may be multiple places we want to perform the same action.

[jfmcquade]

Looks good, really nice to have the tests and demo templates. I've run the tests manually with positive results.

Question
We previously said set_data would be the preferred name for actions to align with set_field , set_local etc. Although do we still want to keep this convention given we will likely need several follow-up actions, namely:
set_data, reset_data, insert_data, remove_data
vs
data_set, data_reset, data_insert, data_remove

I don't have a strong opinion here, I can see advantages to each. I suppose another option would be to separate the actions within a "data" namespace, so they were called like:

click | data: set | _list_id: feat_data_actions_list, _index: @local.last_index, completed: false;
click | data: reset | _list_id: feat_data_actions_list, _index: @local.last_index, completed: false;
click | data: insert | _list_id: feat_data_actions_list, _index: @local.last_index, completed: false;

etc.

This would still be breaking from the implied convention of set_field, set_local etc.

[jfmcquade]

@esmeetewinkel noticed that this warning has been appearing, see #2528. I can't understand what's causing this as the comparison appears to be between two strings. It doesn't seem to cause any problems for functionality, but just flagging here

[chrismclarke]

Yeah the console log is wrong as up till this point we've checked:

• strict equivalent (a===b) \ true
• different types \ false
• equivalent arrays and equivalent object literals \ true

so what we are left with are non-equivalent but same type (strings, numbers etc.), or additional primative types not already checked (e.g. date objects, bigints etc.).

So if we wanted we could add a type check for string, number and boolean types and confidently return false, and then add a log just to catch other types; but for now I've just dropped the log and added a note on the function limitation. I don't see this being of practical consequence as there's currently no way to construct other primitive types from the code.

[jfmcquade]

So the point of this test case is just to demonstrate that using the _updates param (which should only be considered from within the code) requires any dynamic values to be evaluated already?

[chrismclarke]

Yes, part of the idea with the parallel work on items loops is that the item loop handles all its own data update processing (it already has an active data_list query subscription, so doesn't to run a single db query like a regular set_data operation needs to).

Although part of me is thinking that this inefficiency might actually be better just to simplify the set_items handling, and given that the data_list query is only making a single call to in-memory db so really not an expensive operation to duplicate. So I might decide to remove this functionality in a follow-up PR, but for now it does act as a bypass.

[jfmcquade]

debug log could be tidied or removed

[chrismclarke]

Looks good, really nice to have the tests and demo templates. I've run the tests manually with positive results.

Question
We previously said set_data would be the preferred name for actions to align with set_field , set_local etc. Although do we still want to keep this convention given we will likely need several follow-up actions, namely:
set_data, reset_data, insert_data, remove_data
vs
data_set, data_reset, data_insert, data_remove

I don't have a strong opinion here, I can see advantages to each. I suppose another option would be to separate the actions within a "data" namespace, so they were called like:

click | data: set | _list_id: feat_data_actions_list, _index: @local.last_index, completed: false;
click | data: reset | _list_id: feat_data_actions_list, _index: @local.last_index, completed: false;
click | data: insert | _list_id: feat_data_actions_list, _index: @local.last_index, completed: false;

etc.

This would still be breaking from the implied convention of set_field, set_local etc.

Yeah I think at this point I think the more natural way of expression an action like set_data probably outweighs the more extensible way of defining within namespaces data_set or data: set. If we want to revisit in the future then we can try to do so more broadly, but for now good to try and stay consistent with other data manipulation actions.

[ChrisMarsh82]

@chrismclarke Am I right in thinking there is no way to dynamically specify the field to update i.e.
click | set_data | @local.completed_field:true

also when using a combobox change | is there a way of getting the selected value? I have tried @local.[name] which works in the preceding line but doesn't work in the change

[jfmcquade]

also when using a combobox change | is there a way of getting the selected value? I have tried @local.[name] which works in the preceding line but doesn't work in the change

Referring to the combo_box instance's name works for setting fields etc. I believe, see the example on row 6 here. If it doesn't work like that for set_data, it could be worth trying writing this.value to reference the value of the combo box instance as a temporary workaround, although I'm not sure that will work either.

[ChrisMarsh82]

@jfmcquade

also when using a combobox change | is there a way of getting the selected value? I have tried @local.[name] which works in the preceding line but doesn't work in the change

Referring to the combo_box instance's name works for setting fields etc. I believe, see the example on row 6 here. If it doesn't work like that for set_data, it could be worth trying writing this.value to reference the value of the combo box instance as a temporary workaround, although I'm not sure that will work either.

I have just got the combobox to work and was coming back to delete my message before anyone saw it. Still having issues with setting field dynamically but, again, this could be an issue my side

[chrismclarke]

@chrismclarke Am I right in thinking there is no way to dynamically specify the field to update i.e. click | set_data | @local.completed_field:true

I've just tested this use-case and can confirm it is not currently working. I'll raise a PR to address
#2597