fix: action parameter self-references

[jfmcquade]

PR Checklist

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

Description

Currently, there is a workaround for handling action arguments that are self-references (arguments whose value is a reference to the value of the current row, e.g. @local.current_row_name and this.value). For example, changed | set_local: my_var: this.value. This PR extends this workaround to additionally cover action parameters (key value pairs that are passed to an action after a pipe |). For example, changed | set_data | _list_id: my_list, field_1: this.value.

This functionality applies only to cases where the whole value is a self-reference. Values that are an operation that includes a self-reference, e.g. this.value + 1 or @local.some_var + this.value, are not supported (this is the case for the current action arg workaround). A separate follow-up PR will handle the specific case of using the self-reference value in a concatenation, as is required for the plh_facilitator_mx deployment.

Notes

The difference between action arguments and action parameters is somewhat arbitrary, and isn't something authors should need to be aware of. The terms are used within the code to differentiate between actions which are passed just a list of values (no pipe), e.g. set_local: var_1: value_1; and those which are passed a list of key/value pairs (following a pipe), e.g. set_data | _list_id: my_list, field_1: value_1;. In general, older actions tend to use an "argument" based syntax whereas actions added more recently tend to use the more robust, parameter based syntax. It would be good to rationalise these different syntaxes at some point, likely moving all actions over to using the key/value "parameter" syntax.

Git Issues

Provides a workaround for some cases relating to #2745, but does not resolve the underlying issue.

Screenshots/Videos

debug_set_data_changed

[chrismclarke]

Thanks @jfmcquade ,
I think it's a good idea to implement this immediate fix ahead of any refactoring (which I've started work on but will likely take some time to fully sort out)

Given you've picked up on 2 edge cases would it be possible to add tests to the related parser and template service to cover these, so that they are less likely to regress in the future? If possible also include a brief comment like you've included in the notes and PR just to remind ourselves why things need to work in such a way

[jfmcquade]

Thanks @chrismclarke. I've updated to add some minor tests for the parser changes, and some explanatory comments.

I'm struggling to configure testing for the template action service – the way the Injector is used to get the global services makes it difficult to know how to mock. I considered extracting the hackUpdateActionSelfReferenceValues() method to a utils file to make testing easier, but it does really belong on the TemplateActionService class.

I will return to this later this week, but for now I think the plh_facilitator_mx deployment can target the branch of #2750 to include the relevant bug fix.

[chrismclarke]

Thanks @chrismclarke. I've updated to add some minor tests for the parser changes, and some explanatory comments.

Great, yeah overall I think things are looking good. I've added one tidying commit c490b14 which does the following:

1. Replace Object.fromEntries calls with simpler loops
   In this case I don't think there's any real reason to create new objects (particularly given the equiv array methods map existing values), and I always find Object.fromEntries(Object.entries... a bit clunky. But more just a personal preference thing

2. Tidy up the this. checks to only check for this.value. I think initially we had an idea that you could refer to anything in the same row, however the parser overrides only applies to this.value and I can't think of any other use cases we have. I'm not sure if I'd call this a breaking change as I really don't think we have ever really supported something like an action_list referring to this.style_list or some other self-reference.

I'm struggling to configure testing for the template action service – the way the Injector is used to get the global services makes it difficult to know how to mock. I considered extracting the hackUpdateActionSelfReferenceValues() method to a utils file to make testing easier, but it does really belong on the TemplateActionService class.

I will return to this later this week, but for now I think the plh_facilitator_mx deployment can target the branch of #2750 to include the relevant bug fix.

Oh yeah, I can see we don't have any base tests for service at all. I've added a base suite that covers these explicit use cases with 6f16674

It's a little bit hacky in places but better than nothing. I'm keen not too spend too much time on these right now as we still have #2742 open which may already have some related changes. But hopefully what is there makes sense.

[chrismclarke]

I think this is now looking good to me, so feel free to merge once you've had a chance to check the commits I added and if you're happy with them

[jfmcquade]

Great, yeah overall I think things are looking good. I've added one tidying commit c490b14 which does the following:

1. Replace Object.fromEntries calls with simpler loops
   In this case I don't think there's any real reason to create new objects (particularly given the equiv array methods map existing values), and I always find Object.fromEntries(Object.entries... a bit clunky. But more just a personal preference thing
2. Tidy up the this. checks to only check for this.value. I think initially we had an idea that you could refer to anything in the same row, however the parser overrides only applies to this.value and I can't think of any other use cases we have. I'm not sure if I'd call this a breaking change as I really don't think we have ever really supported something like an action_list referring to this.style_list or some other self-reference.

Thanks, yeh definitely makes sense.

Oh yeah, I can see we don't have any base tests for service at all. I've added a base suite that covers these explicit use cases with 6f16674

It's a little bit hacky in places but better than nothing. I'm keen not too spend too much time on these right now as we still have #2742 open which may already have some related changes. But hopefully what is there makes sense.

Great, definitely nice to have something there and I can see how we can extend for future test cases in the future. Can confirm that both new sets of tests are passing for me.

I think I will merge now to move things forward.