-
Notifications
You must be signed in to change notification settings - Fork 2
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
migrate: find+replace Calculate output
prop with fn
#4083
base: main
Are you sure you want to change the base?
Conversation
@@ -19,7 +19,6 @@ export const getFlowSchema = async (flowId: string) => { | |||
planx_variable: | |||
nodeData.data?.fn || | |||
nodeData.data?.val || | |||
nodeData.data?.output || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a key benefit - we only need to account for nodeData.data?.fn
when getting a flow's "schema" ✔️
@@ -9,7 +9,6 @@ const generalData: SearchFacets = ["data.fn", "data.val"]; | |||
const fileUploadAndLabelData: SearchFacets = ["data.fileTypes.fn"]; | |||
|
|||
const calculateData: SearchFacets = [ | |||
"data.output", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another key benefit - data.output
is now simply picked up by generalData
search facets ✔️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked through the Calculate comp and the code in your branch and couldn't find any more refs to .output
and looks like you got all the right fields changed.
Only comment I have is on that merge conflict, to do with the autocomplete data fields work, imagine we would want to take in that change into this PR?
@RODO94 now rebased to |
output
prop with fn
output
prop with fn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sick! Thanks so much
Queueing up per migration plan: https://docs.google.com/spreadsheets/d/1Vtxp5BLweDPDooQoNhgOCYjCIBPRYIcOuyArGJRqOkI/edit?gid=0#gid=0
Planx-core counterpart here theopensystemslab/planx-core#593