fix: Handle multiple external portals as siblings #2217
Conversation
| ob[id] = { | ||
| ...node, | ||
| type: TYPES.InternalPortal, | ||
| data: { text: slug }, | ||
| }; | ||
| } else if (node.type === TYPES.ExternalPortal && !ob[node.data.flowId]) { |
There was a problem hiding this comment.
There's a little rearranging / renaming here but breaking up this condition is what's fixed the bug. As is, this flattens the first external portal it hits, but skips subsequent ones.
|
Removed vultr server and associated DNS entries |
DafyddLlyr
force-pushed
the
dp/external-portal-bug
branch
from
d4059a5 to
45edb4c
Compare
There was a problem hiding this comment.
Thanks for this fix & tests - pizza working as expected for me!
Still a bit hung up on where we've introduced this (not totally convinced beginning of dataMerged because flows published a couple months ago are ok?)
One half-baked thought while sifting through changes is perhaps that the newish FlowGraph type from planx-core is being misused somewhere to type unpublished flows (which it actually can't handle because it doesn't account for data.flowId in a Node) when it should only be used to type published flows? Just noticing the type right now, and haven't confirmed if any actual nodes were getting dropped/overwritten on sort or order, but just a thought! It's such a strange one.
| if (nodeId === "_root" && Object.keys(ob).length > 0) { | ||
| const isExternalPortalRoot = nodeId === "_root" && Object.keys(ob).length > 0; | ||
| const isExternalPortal = node.type === TYPES.ExternalPortal; | ||
| const isMerged = ob[node.data?.flowId]; |
There was a problem hiding this comment.
nit: is isNotMerged a more clear name here since node.data?.flowId indicates it's still an external portal reference, not flattened representation of individual nodes?
There was a problem hiding this comment.
So here ob is the returned item (the flattened flow). If ob[node.data?.flowId] == true then the flow has already been flattened and ob[example-uuid-of-portal] exists. Does that make sense?
There was a problem hiding this comment.
TBH I think some of the language here is a little fuzzy around merging / flattening
I actually think something along these lines is more likely but I've also not been able to prove anything / work it out 🤔 |
|
Thanks for the review! As anything graph related is high impact I'll request PO testing on the Pizza before merging. |
DafyddLlyr
added
the
UAT
|
Merged - PO testing done by Silvia and Sam https://trello.com/c/23Wi60zZ/2551-blank-screen-shows-when-passing-through-external-portal |
What does this PR do?
dataMerged()When was this bug introduced?
It appears to have been there since
dataMerged()was written as far as I can tell which is a little surprising - I don't have total confidence in this but it's my best guess.Why not moved
dataMerged()to a shared location likeplanx-core?We could do this, but there's not real need to - if more flow management / logic goes there then sure, but for now we can continue to live with the duplication here I think.
Test flows
dataMerged().