Fixes #6008 Update Content Activities

[jtkech]

Fixes #6008

@duncanhoggan could you re-do some tests by forking this branch or just copy UpdateContentTask and the little change in DefaultContentManagerSession needed in some scenarios.

• You could retry it when triggered by a ContentCreatedEvent. We use DraftRequired so that the content manager returns a new draft version or not by checking if the type is Versionable. Most of the time after a content event we retrieved a cached instance, but not here.

  So here, after having mutated this version this is not our responsability to publish it, it will be done or not by the "caller" that triggered the WF, e.g the admin controller => content handler => ContentCreatedEvent => the item is published or not by the controller.

• Then if related to an existing item and not triggered by a content event, e.g an HttpRequestEvent, as in your last WF sample in Fixed content create event order #6009, after the UpdateContentTask it's up to you to use or not a PublishContentTask.

• Then when using an UserTaskEvent edit action that adds a button when editing the related content item, when clicking on it this is always a submit.save that is posted, not a submit.publish, so the draft version is just saved, so here also it is up to you to use or not a PublishContentTask.

  So in your last WF sample in Fixed content create event order #6009 you would have to not use PublishContentTask after the UpdateContentTask but after the UserTaskEvent. Otherwise, after clicking the edit user button, it will always use the existing draft or create new one but without publishing it, as you saw.

  Then, if you didn't see any update, i think that there is another problem, maybe with your ContentProperties expression as i tried quite the same WF on my side and it was working

[duncanhoggan]

@jtkech I got round to doing some testing, below are my findings.

• ContentCreated -> UpdateContent this results in an updated item in drat mode if i call PublishContent after it updates correctly ⭐.
• HttpRequestEvent -> UpdateContent All works as expected in isolation ⭐. Although if i call a HttpRequestEvent -> UpdateContent from another workflow (nested) i get database lock exceptions ❌.
• UserTaskEvent -> UpdateContent -> PublishContent works great ⭐.

Your changes look great, we just need to address that nested case and i think it could be good to add a Publish option to the UpdateContent task to ensure that the updated item is published in those cases where it isn't published (the first and last case that you mentioned).

[jtkech]

@duncanhoggan

• For your 2nd test we already have this issue, e.g. sqlite doesn't support concurrent writes so they wait for each other, here both worflows are not executing in the same scope, so that would be okay, but both are still waiting for each other because WF1 will only commit its transaction at the end of its scope, but it is waiting for WF2 that is also waiting for WF1 to commit its transaction.

  See CommitTransactionTask workflow activity #4530 and my last comments in Creating content in a nested workflow fails (database table is locked) #4255.

  So here you need to use a CommitTransactionTask activity after the UpdateContentTask and before triggering your nested workflow.

• I added the Publish option

[jtkech]

@duncanhoggan and @deanmarcussen as you will have to review my changes related to the new ValidateAsync().

Sorry but because of some recent changes that have been merged, now an update content activity may do a session cancel through a new ValidateAsync(), so it is important to add the validation errors to the underlying model state. Otherwise the controller, not aware of the failing validation, will do another save and still persist the item.

We already saw this case: 1. session save - 2. query + mutation - 3. we need a save to take into account the last mutation. Here, because the cancel can be done earlier, the new one is: 1. session cancel - 2. query - 3. session save => the item is persisted even the session has been canceled in 1.

Todo: This use cases need to be supported transparently, but in a separate PR.

So we really need to check if the UpdateContentTask activity is executing in a "regular" mode, or as an inline handler from the ContentsHandler, or as an inline driver from the UserTaskEventContentDriver.

So i did some updates and re-do all the tests that was working on my side.

• If used in a "regular" mode e.g. after an HttpRequestEvent, the UpdateContentTask works as before and i added a Publish property to publish or not the item after updating.

• If used e.g. after a ContentCreatedEvent, the UpdateContentTask will update the item and, because here it acts as an handler, it will neither call the content manager UpdateAsync() nor the PublishAsync(), this is the responsability of the underlying controller. And if the validation fails, it reports the errors to the underlying model state that will be displayed in the admin UI.

• Idem if used after an UserTaskEvent but here because the UpdateContentTask acts as a driver. Here also this is the underlying controller that has the responsability to save the item and then publish it or not. Anyway here we are executing before the regular display drivers that will overwrite your updates if related to regular parts / fields. But still useful to add some specific data as we do through some custom drivers. Then about publishing, the problem was that before an user action button was always sending a submit.Save, now when you define an edit user action e.g. Go, you will have 2 buttons Go and Save and Go and Publish.

• Finally, we now support new scenarios, you now can use an ContentUpdatedEvent followed by an UpdateContentTask, this because here it will act as an inline handler and then not call the content manager UpdateAsync(), so there is no more an infinite loop in this case.

Maybe i will need to also update other content task activities, but not sure, will see tomorrow.

[deanmarcussen]

• If used e.g. after a ContentCreatedEvent, the UpdateContentTask will update the item and, because here it acts as an handler, it will neither call the content manager UpdateAsync() nor the PublishAsync(), this is the responsability of the underlying controller. And if the validation fails, it reports the errors to the underlying model state that will be displayed in the admin UI.

Ok, makes sense @jtkech

I wanted the ValidateAsync to be disconnected from a ModelState style of errors, because in some cases there is no ModelState that we want to work with.

But where there is (and there is a need for it), it makes good sense to join these errors into the ModelState as you have done.

(The other reason was because it was about validation failures, not successes, so didn't want to list all the succeeded validations, just the errors, or we could have just used a ModelState dictionary).

call the content manager UpdateAsync(), so there is no more an infinite loop in this case.
👍

Maybe i will need to also update other content task activities, but not sure, will see tomorrow.

[jtkech]

@deanmarcussen when you will have time

@agriffard requested a review from you

[jtkech]

@sebastienros and others

So we discussed about preventing e.g. a WF infinite loop (#6455) but at another level, but it can't supersede all what is done in this PR specifically for content events / tasks. So here, as a reminder that may be used in another triage meeting, some uses cases to show what i mean.

• You are saving a draft through the admin UI => drivers update is successful => handlers => a WF content event => a WF content task that does a publish => You are still notified that a draft has been saved even the content item has been published.

  Here, when executed inline from a content handler related to the same content item id we don't call content manager methods, so the content item is not published.

• You are saving a draft through the admin UI => drivers update is successful => handlers => a WF content event => WF update content task where ValidateAsync() fails => But you will still see that the draft has been saved successfully even the session has been canceled.

  Here, when executed inline from a content handler related to the same content item id, if the validation fails we add errors to the underlying model state, so you will be notified that the related WF task failed to update the content item, and with error messages.

• Idem with the ApiController where e.g. you may wrongly return Ok(contentItem).

• Note: Here when triggering a content event we not only pass the content item to the WF input but also a ContentEventContext with more infos, this in a normalized way that should be used for any future content event, as i did for the below UserTaskEvent that is a content event.

• The UserTaskEventalso belongs to the WF content events,it allows to add a button(s) e.g. Go when you edit the related item, the event is then triggered when you submit by using this button, but the submit was always a submit.save, here we have 2 options Go and save and Go and publish.

• Here i also added a DraftSavedAsync event, and its related WF ContentDraftSaved event, that is triggered when you save an item as a draft but only if the content item has been validated.

[jtkech]

Fixes #6596, fixes #6597, fixes #6598, fixes #6599

[jtkech]

@deanmarcussen

You are still a pending reviewer for this one ;) but only if you have time. I could merge it myself but because we talked about it in a recent triage meeting i prefer that someone else approves it (or not) before merging it (or just closing it).

In the meeting it was related to infinite loops but this PR supports many other scenarios. If needed i can help you to understand what i did, and to do some simple repros see my previous comment or the recent related issues i opened.

[deanmarcussen]

@jtkech sorry, took some time to think about this one.

For the detecting and cancelling infinite loops all seems to make sense.

For the Inline (from driver, and external) it looks like you have changed the shape of this pr for the better.

Can you explain though?

Previously when I looked at it, you were detecting (with some cleverness) when a workflow event came from a driver, or a controller, from memory to decide whether to save, or not?

And for knowing whether to add model state errors etc?

From the triage conversation this was the area of concern that was blocking it, as Seb was concerned that workflows should have to know about these things.

It looks however you have changed it so that the workflow does not figure it out itself, but is passed some information informing it?

Can you just explain how it is operating in those cases? I'm looking but there are so many other scenarios you have fixed with this one, that I'm not quite sure I'm following the logic there

[jtkech]

@deanmarcussen no problem, thanks to look at it again

I didn't have any time as #6356 was not so easy to fix ;) but i will give you soon more infos step by step

[jtkech]

@deanmarcussen

If i'm not clear / incomplete as i didn't have enough time we could have a chat / talk on gitter / skipe. So here some more infos as i can in a first step that may indirectly answer to your questions

• The main place where content events are triggered is in this content handler, there is also one triggered in this driver that is specific to the UserTaskEvent. That's why before i was talking about inline from an handler or inline from a driver, but i harmonized the way a content event is triggered, so here let's only talk about inline from a content handler, but we could just talk about inline from a content event.

• In fact we don't care from where e.g. the above content handler has been called, can be a controller, an api controller or another WF, the main concern is to know if this event is followed by a content activity without any intermediate waiting activity, and correlated to the same content item id, meaning that this activity is executed inline from a content event and then is not the original initiator of the whole operation on a given content item.

• When we trigger a content event with WF input data, before starting / resuming a related WF instance, the WF manager sends an OnInputReceivedAsync() event to all its activities. This is in the ContentActivity base class that we capture this WF input containing content event data. Then, because the WF manager rebuild all activities of a given WF instance when it resumes it, if there is an intermediate waiting activity we will loose these content event data, otherwise in the content activity we can use them, and this is how we detect that we are executing inline from a content event, if it is a starting event, if we are acting on the same correlated content item id and so on, and we can decide what we do.

• So, if a content activity is executed inline from a correlated content event, it is not the original initiator and we don't allow to call content manager methods, for the same reason that e.g. a content handler is not allowed to do so. If it fails it populates the underlying model state in case the original initiator is e.g. a controller / api controller, if the initiator is another WF it will only populate the model state of a NullUpdater, for nothing but without failing (we could check if it is a null updater), and also pass the errors to the WF lastResult.

• If not executed inline from a correlated content event, it means that the content activity is the original initiator by acting e.g. as a mini controller, e.g. by retrieveing a given content item id by itself using its expression property, and is allowed to call content manager methods, if it fails it doesn't populate any model state, it only returns errors through the WF context lastResult. Then by calling content manager methods and then handlers, this content activity may be the original initiator of another correlated content activity that will not be allowed to call content manager methods.

[deanmarcussen]

@jtkech I am reading and thinking ;)

[deanmarcussen]

If not executed inline from a correlated content event, it means that the content activity is the original initiator by acting e.g. as a mini controller, e.g. by retrieveing a given content item id by itself using its expression property, and is allowed to call content manager methods, if it fails it doesn't populate any model state, it only returns errors through the WF context lastResult.

@jtketch I had to leave triage early last night, so we did not get to talk about this.

We do need to talk about it before merging, my concern is that we are fixing something, on top of fixing something else, with regard to the inline event, and the workflow acting as a mini controller. All else is fine.

It seems to me we have a fatal flaw in the idea of a workflow being a mini controller, and calling these content manager methods.

I went to use these a couple of days ago, and they were not adding up to me.

Let's talk about a CreatedEvent, or an UpdatedEvent, then followed by an UpdateContentTask. To me it makes sense, that the two would follow each other, and that the UpdateContentTask would not be calling the content manager to retrieve an item.

It seems a little weird that a workflow would be it's own initiator, or mini controller.

I realise we have recently added some of these tasks, RetrieveContent and possibly the UpdateContent, but that the PublishContent has existed for some time. But it concerns me that maybe the design of these new tasks is the issue here, requiring us to apply more fixes, on top of them.

However I did not write these tasks, nor design workflows, so maybe I am wrong here.

I would have to look back at O1 to see how the event -> task flow worked better.

[jtkech]

@deanmarcussen

@jtketch I had to leave triage early last night, so we did not get to talk about this.

No problem, nothing urgent for me

For the other concerns i understand and i have no closed opinion, it only depends on what we allow to do.

Here some oher infos / examples

The UpdateContentTask has a script expression property to provide a contenItemId, if we are just after a content event the script can just retrieves it from the WF context input, and here in fact, as we are executing inline from the content event, we will use the content item of the WF input if its contentItemId it is the same as the one provided by the script. And if so we are not allowed to call content manager methods, not only GetAsync() but also UpdateAsync() and so on.

If there is an intermediate activity before, e.g. a timer event or any other waiting event, because when a WF instance is persisted we don't save the whole content item but only the contentItemId as a correlationId, the script can just be correlationId() meaning that it is retrieved from the WF context correlationId. But here we are not executing inline from the content event but in another scope, so we are allowed to call content manager methods including GetAsync() to retrieve the content item.

In place of saying as a mini controller / initiator maybe better to say as a standalone activity as it has to retrieve the content item by itself. And in the above case this is already the case but here the real initiator is still the content event but the UpdateContentTask is executed in another scope, the same of the last intermediate event.

Another exemple is an HttpRequestEvent providing a correlationId and followed by an UpdateContentTask using it to retrieve the content item by itself. Here we are not executing inline from a content event, so as a standalone activity but the real initiator is the request event, there is always a WF event (of any type) before an UpdateContentTask.

Finally, the script expression can just be an hard coded string 4eczzkt0yb4znw8hwmpv01bccw, maybe this is what we don't want but right now nothing prevents it. It's an higher level of autonomy but as said there is always a WF event before.

It seems a little weird that a workflow would be it's own initiator, or mini controller.

See the above

What i said about the script expression, providing a contenItemId, is the same for the RetrieveContentTask. Maybe this task can be removed as, at least here, all other content tasks can retrieve a content item by themselves.

[sturatcliffe]

Hi @jtkech - this line doesn't seem right as it prevents handlers being fired when adding a draft to an already published content item.

[jtkech]

@sturatcliffe Thanks for the feedback

• Normally only one content item version can be Published and only one version can be the Latest.

• An active version is Published OR the Latest (so can be both). So a version is not active if it is neither Published nor the Latest, there is a pending audit trail module to manage these archived versions, otherwise we don't take them into account because they are considered as soft deleted versions.

• So let's only talk about active versions, if a version is Published and Latest it means that there is no active draft version for the related content item. When we publish it again, a new version is created that is marked as Published and Latest, and we set to false the Published and Latest of the previous version that becomes an archived version.

• If we save as a draft this Published and Latest, a new version is created that is only marked as Latest, the published version is still the same but no more the Latest and we now have an active draft version. When we edit a content item we always edit the Latest version, so here it will be the new active draft version. If you save it again as a draft version, we don't create a new version, we just update this version that is still the active draft version.

Sorry for the long story and there are different ways to describe content item versions ;)

So what i can say here is that an active draft version is always the Latest and of course can't be Published.

And SaveDraftAsync() is intended to call the related handlers only if we are saving an active draft version.

I don't know in which context you are using SaveDraftAsync() but at this point your content item version needs to be the Latest and you have to take care that you have only one Latest. For this, e.g. when updating a content item, we first get this content item through the content manager by using the option DraftRequired, this will use a current active draft version or create a new non published version and that becomes the latest.

[lazcool]

Hi @jtkech,

Should these 2 be added to IContentPartHandler too?

Btw, we've been using this branch for a while now and we're very happy with it and had no issues.

[jtkech]

@lazcool Good catch, forgot it ;)

[jtkech]

Done ;)

Btw, we've been using this branch for a while now and we're very happy with it and had no issues.

Cool thanks. If you have time can you add some comments on your own usage of this branch in the main thread? It may help us to decide if it is worth to review this PR again before merging, or just close it.

[lazcool]

Sorry, it's took a week, work's been manic 😅.

We use the new draft saved event, to sync the content item to a preview graph database (we basically replicate content items as nodes and relationships to published and preview graphs).

We'll also be using the new draft saved event to publish external events to Azure Event Grid when a new draft version is saved. (We have micro-service UI apps that listen for the events and then update their local stores with the content from the graphs.)

We currently publish draft events using the existing Orchard Core events, such as updated, but without this PR, we've found it impossible not to publish false-positive events, because the context just isn't there (and we have to do delayed processing of the events, which is complicated, messy and racey).

I have another question for you:
Would it make sense to fire the new draft save events when the user adds or deletes a taxonomy term?

At the moment, as soon as they, for example, add a new term, a new draft version of the taxonomy is saved, but no draft saved event gets fired. That means we don't get an opportunity to sync the new item to our graph or publish an event off the back of it.