add sankey chart action server to client protos #21
Conversation
|
|
||
| // A state step provides the configuration used to name Sankey chart states. Each state step corresponds | ||
| // to a single transition between two states in the workflow. | ||
| message NodeStep { |
There was a problem hiding this comment.
For associating with the transition I had thought we could just embed it directly on the Transition proto / LogMatcher proto, that way we avoid having to duplicate all the state transitions and it ought to make it easier to construct the necessary bits on the workflow engine
That said I haven't spent time looking at the client code so lmk if you think this format works better
There was a problem hiding this comment.
from the perspective of the client It would me simpler to have this be a property on the LogMatcher. I wanted to avoid this though not to tie the implementation of the LogMatcher to any specific action but let me rework this.
| // to a single transition from a workflow origin state to a workflow target state. | ||
| message Node { | ||
| // The ID of the origin state. | ||
| string origin_workflow_state_id = 1 [(validate.rules).string = {min_len: 1}]; |
There was a problem hiding this comment.
would this be the generated client ID? Or the FE ID? Not sure how useful the generated ID would be since I it might vary between workflow constructions (I think)
There was a problem hiding this comment.
This is the ID that's generated server-side and yes, looking at the code it can change between workflow's constructions. We could probably change the logic so that the generate IDs stay the same between consecutive workflow constructions (I haven't verified that) but for now I will just remove the state_ids from here since they are not required for the absolute mvp
|
@snowp should be ready for re-review |
| } | ||
|
|
||
| // Extracts the value from a field. | ||
| message FieldExtracted { |
There was a problem hiding this comment.
I think we have this duplicated in a few places, wonder if we should reuse? Not a blocker, just curious
Fixes BIT-3836