Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hotfix: action parameter self-reference concatenation #2750

Merged
merged 8 commits into from
Jan 31, 2025

Conversation

jfmcquade
Copy link
Collaborator

@jfmcquade jfmcquade commented Jan 27, 2025

PR Checklist

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

Description

Follow-up to #2749.

Adds a workaround to handle a specific use case required on the plh_facilitator_mx deployment but not currently supported: authoring an action with a parameter value that uses the current value of its row in a concatenation. From menu_task_flow:

changed | set_data |
_list_id: sub_items,
completed: @item.@local.completed_prefix+this.value;

Here the this.value reference is expected to be a string which will then be concatenated with the @local.completed_prefix value to ultimately reference a particular property on the current item, equivalent to authoring, for example, completed: @item.completed_1.

The included workaround is quite brittle: it requires the specific authoring syntax of +this.value (with no spaces before or after the + symbol) in order to make the substitution.

Git Issues

Provides a hacky workaround for #2745, but does not resolve the underlying issue.
Closes #2745

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

@github-actions github-actions bot added the scripts Work on backend scripts and devops label Jan 27, 2025
Copy link
Collaborator

@ChrisMarsh82 ChrisMarsh82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes my issue in facilitator MX

Copy link
Member

@chrismclarke chrismclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to put a blocker on this as I believe it builds on top of #2749 and so shouldn't be merged until after

@jfmcquade
Copy link
Collaborator Author

@chrismclarke I've tagged you for review again. The changes are much clearer after the updates to #2749

Copy link
Member

@chrismclarke chrismclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfmcquade
I've pushed 3bb2c31 to tidy up the code a bit and add a test case. I've modified the replacer function to look for {this.value} as I think this is more likely to be supported by default with future changes (so should be able to remove workaround at future stage). I've also updated the #2753 TODO items to include check against this case

I think this should reasonably address the issue flagged in #2745 so have marked this PR to close it, although if you think something is still not covered feel free to re-open or create a new issue to capture what parts are still missing

I've also updated the debug sheet to minimally capture what is now working (as well as what is not)
debug_set_data_changed

Screenity.video.-.Jan.29.2025.mp4

@chrismclarke
Copy link
Member

chrismclarke commented Jan 29, 2025

@ChrisMarsh82
The included changes now require to use templated expression, e.g. {@local.some_value}_{this.value} (instead of previous +this.value) syntax, so I'll request a re-review from you to ensure still works for your use case. Examples in debug_set_data_changed

@jfmcquade
Copy link
Collaborator Author

Thanks, @chrismclarke, the updates look good. I agree that it makes sense for this PR to close #2745.

@ChrisMarsh82 – since the MX deployment is targeting this branch, I've already updated the syntax in the menu_task_flow template to reflect the changes (cell D14). I believe that was the only case of the +this.value syntax, but correct me if not.

Copy link
Collaborator

@ChrisMarsh82 ChrisMarsh82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked on MX

@ChrisMarsh82 ChrisMarsh82 merged commit 92f3d68 into master Jan 31, 2025
6 checks passed
@ChrisMarsh82 ChrisMarsh82 deleted the hotfix/action-param-self-reference-concat branch January 31, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scripts Work on backend scripts and devops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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