[BUG] set_data action on "changed" trigger does not have latest row value

[jfmcquade]

Describe the bug
Some components emit a "changed" event when their value changes. When using this event to trigger set_data action, and referencing the component row's value in that action, the value passed to this action does not reflect the latest value, but rather the value before the change (at least for combo_box and toggle_bar components).

App version
0.17.1

To Reproduce
Describe the steps to reproduce this bug.

Debug Sheet(s)
debug_set_data_changed

The combo box and toggle bars in this template are all configured to update the same local variable and dynamic data row. Note that on changes, the local var updates to the expected value, but the dynamic data value is always "behind", being updated to the previous value of the row. This is not an issue with the data-items not rendering the latest value, as debug logs show that the set_data action itself is being called with the incorrect value.

set.data.changed.bug.mov

notes
From logging out the component's _row.action_list, it appears that the arg of the set_local action remains in unevaluated form, containing a reference to the @local variable, whereas the param of the set_data action is already parsed to reflect the current value of that referenced local variable. It is this static value that is then passed through to the action, before it has been re-evaluated to reflect the new value of the row:

[chrismclarke]

Is this related to #2711 or do you think a separate issue?

[chrismclarke]

Also just testing and I'm not getting the same behaviour, is your issue based on branch for #2744? Currently whenever I change the combobox neither of the lower variables change. I'm also slightly confused by the toggles, is this basically just to demo with a component other than combo_box (which has its own issues)?

[chrismclarke]

Also just testing and I'm not getting the same behaviour, is your issue based on branch for #2744? Currently whenever I change the combobox neither of the lower variables change. I'm also slightly confused by the toggles, is this basically just to demo with a component other than combo_box (which has its own issues)?

nvm, have merged #2744 and see same behaviour. I'm pretty sure the reason for this is down to how actions are parsed in advance of being triggered, so triggering two successive actions will not pass changes from the first action to the second. There is currently a workaround in the code to specify this.value instead of referencing a rows own name, which looks up the value and updates with latest.

However this only applies to action args and we have an inconsistency where some actions use args and other params, e.g.

 "set_local: var_1: @local.combo_box_1;"

// args: ['var_1', '@local.combo_box_1']
// parameter_list: {}

vs

"set_data | field_1: value_1; field_2: value_2;"

// args:[],
// parameter_list: {field_1: "value_1", field_2: "value_2"}

I can't remember at the code level how we know whether an action is using args vs params (possibly searching for ; character?), but in any case we've historically only made workarounds for args and not params.

So I think the options here are either to

1. Re-author the action to use this.value and duplicate args workaround for param values
2. Refactor actions more generally to only evaluate at the point of being triggered, and to evaluate all variables in the same instance

I might quickly try explore (2) as a better long term option, but if a quick solution needed an alternate PR could be created to add parameter_list case to src/app/shared/components/template/services/instance/template-action.service.ts#L207

[ChrisMarsh82]

The issue with using this.value is that it needs to be preceded by @item. If I could specify the end of the text or start of 'this' i.e. @this then this.value would work.

current
completed: @item.@local.completed_prefix@local.selected_group;
should evaluate to:
completed: @item.completed_14

@local.selected_group cannot be replaced with this.value

[jfmcquade]

Thanks @chrismclarke.

Your inferences about the debug sheet were correct: #2744 was required for the combo-box to function correctly and manifest the bug, and the toggle bars were included just as an example of a different component that has the same issue.

I'm pretty sure the reason for this is down to how actions are parsed in advance of being triggered, so triggering two successive actions will not pass changes from the first action to the second. There is currently a workaround in the code to specify this.value instead of referencing a rows own name, which looks up the value and updates with latest.

However this only applies to action args and we have an inconsistency where some actions use args and other params,
[...]
So I think the options here are either to

1. Re-author the action to use this.value and duplicate args workaround for param values
2. Refactor actions more generally to only evaluate at the point of being triggered, and to evaluate all variables in the same instance

I think you're right. In addition to the case you've described of successive actions not being able to pass changes (which applies to the case of updating via an intermediary variable as in the debug sheet), there's the case of self-references not being up to date in general, which is handled with a parser workaround to replace @local. self-references with this. self references, in order to be caught by the existing workaround, as I understand it.

I've implemented a version of your suggestion for approach 1. over two PRs:

1. fix: action parameter self-references #2749

• This extends (both parts of) the existing workaround for action arg self-references to action param self-references

3. hotfix: action parameter self-reference concatenation #2750

• This adds a specific workaround for the case that @ChrisMarsh82 has on the plh_facilitator_mx app, where the self-referenced value is not just being used as a parameter value itself, but needs to be evaluated and then concatenated before being passed to the action logic. This is a very specific hack, but is likely required by this deployment at least until we have a longer term solution.