OrchardCMS
Fix the BUG that CorrelationId is overwritten
The correlationId with the workflow will be assigned after the content is created (step1)
but ,when a content item is created in a workflow using a Create Content Activity , the contentItemId of the new content overrides the correlationId of the workflow . ( step2)
Causes step3 and step4 never to trigger
in step 2
Not sure I agree with this one, it fixes some scenarios but breaks other ones, e.g. after the execution of a RetrieveContentTask we may want (through the correlationId) to be in sync with the retrieved contentItemId even if an existing correlationId is already set. For other scenarios we have e.g. the Set Output and Set Property activities allowing to set and then retrieve a previous correlationId
For your scenario we have the oob User Task activity that halts the workflow until the user execute an action on a content item, in the activity settings you can define the action and the user role that can do the action, here the action could be named e.g. Approve, in that case if you edit the related content item you will see an Approve button that you will be able to click if you have the permission.
Hi @jtkech ，
after the execution of a RetrieveContentTask we may want (through the correlationId) to be in sync with the retrieved contentItemId even if an existing correlationId is already set.
I think this operation is a little too subtle, and no one knows it without documentation and without looking at the code
As a workflow node, it should not affect the global properties of the entire workflow, it should only take the input and output the appropriate content, Unless it is the Task explicitly to update correlationId,
Otherwise, the global properties of the workflow should not be modified implicitly within it, And they're just a Task not an Event
we can get all the information through LastResult
In addition, the approval I refer to is not just approval through OC interface, if I want users to approve by email. Some dynamic scripts cannot be executed
@hyzx86
Yes I understand your use case ;) But still not sure that the correlationId should always stay the same for the whole WF execution, particularly after the retrieve content task, maybe an activity option to not override an existing correlationId.
Maybe here your signal_url would need to be correlated with the global WorkflowId, not the ContentItemId, hmm can be done by inserting between step 1 and 2 a correlate task that just set the correlationId to null, in that case I think that the signal_url token will use the workflowId instead.
Anyway I don't want to block your PR, so I will let others triage it.
I remember that in the signal task if the corrlationID is empty, it creates a new ID instead of using the workflow ID directly. The workflow ID may not exist because the signal task can also start the workflow (too long ago to remember).
This approval process is just an example. If I need multiple levels of approval, I need to reset correlationid before each approval task.
@jtkech I will label this as dontmerge and let you decide how this should go, or let the others triage it
I remember that in the signal task if the corrlationID is empty, it creates a new ID instead of using the workflow ID directly.
There is no signal task, only a signal event, and we can generate a signal url through a SignalMethodProvider or a signal_url liquid filter (as here), that targets the HttpWorkflowController that will trigger the related workflow, in both ways to generate the url if CorrelationId is null or empty the WorkflowId is used in the payload.
The workflow ID may not exist because the signal task can also start the workflow
I don't think so, on each NewWorkflow() a new WorkflowId is generated.
Just tried to add just before step 2 a CorrelateTask with no value that I named "Decorrelate", yes when we edit the halted instance we still see the CorrelationId set to the 2nd contentItemId, but in the approval page the signal_url was already generated with the WorkflowId, then when I clicked on an Approve/Reject link the WF was resumed.
Is this something you'd like to revisit any time soon @hyzx86 or should we close?
Is this something you'd like to revisit any time soon @hyzx86 or should we close?
Hi @Piedone , Combined with the problems mentioned by JT, I don't think there will be any problems with my PR
The existing code will always update CorrelationId, I'm just adding a check
OK then! @hishamco @Skrypt you've reviewed this previously, please check this out if you still approve. Also, Hisham you added dontmerge, please check that's still applicable.
You can always still update the correlationId by using different methods as mentioned by @hyzx86 and J-T. Maybe it can potentially break someone's workflow but that's how it is when you do a change in these ... document the change in the release notes.
@Skrypt , Maybe I can add a script function to this PR
Just like setCorrelationId(string) ?
Yeah, maybe instead of setting it from server side we could have a script that would affect it. That would be non-breaking.
@Skrypt can you please elaborate on why do you think this needs triage? What should we be mindful of during the triage?
Waiting on @hyzx86 to know if he implemented what I suggested. If it is done then we need to review/test that code again to make sure it is fine.
It can be triaged on next thursday meeting and decided if it will be merged for current release or not.
Since this is a non-breaking change (right?) why wouldn't we merge if it otherwise looks good?