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

Fix change node, not handling from field properly when using context #3754

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

Fadoli
Copy link
Contributor

@Fadoli Fadoli commented Jul 11, 2022

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

Fix a typo introduced in #2822 that breaks using context to replace a value.

The bug can be replicated with the following minimalist flow :

[{"id":"906bd15b.bf1a5","type":"change","z":"40930c8d.9ea28c","name":"","rules":[{"t":"set","p":"prefix","pt":"flow","to":"myFilePrefix/","tot":"str"}],"action":"","property":"","from":"","to":"","reg":false,"x":560,"y":1160,"wires":[["98a5ca24.feff1"]]},{"id":"98a5ca24.feff1","type":"change","z":"40930c8d.9ea28c","name":"","rules":[{"t":"change","p":"filename","pt":"msg","from":"prefix","fromt":"flow","to":"","tot":"str"}],"action":"","property":"","from":"","to":"","reg":false,"x":760,"y":1160,"wires":[["3341cce4.c0dc14"]]},{"id":"9ef6ce5a.decc","type":"inject","z":"40930c8d.9ea28c","name":"specific file","props":[{"p":"payload"},{"p":"filename","v":"myFilePrefix/TAM_HIST_20210521_110828.csv","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"","payloadType":"date","x":400,"y":1160,"wires":[["906bd15b.bf1a5"]]},{"id":"3341cce4.c0dc14","type":"debug","z":"40930c8d.9ea28c","name":"","active":true,"tosidebar":true,"console":false,"tostatus":false,"complete":"true","targetType":"full","statusVal":"","statusType":"auto","x":930,"y":1160,"wires":[]}]

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the forum/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 11, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Fadoli / name: Franck M. (7a048d5)

Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

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

I have tested this modification and it does correct the issue.

In future, could you please first raise an issue for discussion and triage before raising a PR.

Also, please sign the CLA before we can merge.

@Fadoli
Copy link
Contributor Author

Fadoli commented Jul 11, 2022

Also, please sign the CLA before we can merge.

Hey I've signed the CLA I don't know if it gets updated or if I need to push a new commit to have it detect the change ?

I have tested this modification and it does correct the issue.

In future, could you please first raise an issue for discussion and triage before raising a PR.

Yes I'd gladly take the time to follow the recommendations next time, but as it seemed to just be a typo I thought it wasn't necessarily worth to do it. But it's noted

@dceejay
Copy link
Member

dceejay commented Jul 11, 2022

/easycla

@Steve-Mcl
Copy link
Contributor

In future, could you please first raise an issue for discussion and triage before raising a PR.

Yes I'd gladly take the time to follow the recommendations next time, but as it seemed to just be a typo I thought it wasn't necessarily worth to do it. But it's noted

Its really for tracking of issues (they get labelled up accordingly, sometimes backported etc)

Thank you for taking the time to contribute - much apreciated 👍

@coveralls
Copy link

Coverage Status

Coverage remained the same at 68.311% when pulling 7a048d5 on braincube-io:fix-change-context into e14dd06 on node-red:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants