Skip to content
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

Improve handling of publish/unpublish operations #17441

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MichaelPetrinolis
Copy link
Contributor

This pull request introduces changes to the content publishing and unpublishing functionality in the OrchardCore.Contents module. The key changes involve updating the methods to return a boolean indicating success or failure and adding user notifications based on the outcome.

Updates to Publishing and Unpublishing Methods:

Interface and Implementation Changes:

closes #17440

Updated AdminController to handle success/failure of publish/unpublish operations more robustly. Modified IContentManager interface to return Task<bool> for PublishAsync and UnpublishAsync methods. Implemented these changes in DefaultContentManager, including handling operation cancellations. Enhanced user feedback based on operation results.
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nicely prepared PR, thanks for the helpful description.

I'd wait for the triage, because I'm really unsure about the IContentManager changes.

Comment on lines +485 to +486
? H["Your content could not be published."]
: H["Your {0} could not be published.", typeDefinition.DisplayName]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these messages would be more helpful with some information about why it wasn't published. Since at this point we can't really know that, maybe this is not the best way to surface this information, or to prepare the UI message.

Perhaps a handler, that's at the end of the handler list, would be a better place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanx @Piedone for the suggestion. I also thought to extend the Context to support something like Validation Result and change the signature of the method in the interface to return the list of errors, but that would be a bigger change.

Maybe at Triage we decide to make more robust changes e.g. we could introduce Cancel to more operations, support error messages etc. - in this PR or another. My intension was to fix that Success messages appeared to the user, although the publish/unpublish was canceled.

PS.
How another handler at the end of the handler list would help (Sorry, I don't get it)? Keep in mind that the Cancel property can be set/unset from any Publishing/Unpublishing handler, based on the business rules of the domain.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, the notification could be displayed from a last handler if Cancel is true. Because it would actually know that publishing was canceled excplicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. I don't like the 'last' though, because the module that checks a business rule and cancels the operation will depend on the registration order. If cancel is set to true, a message should be displayed with the reason. If another independent rule is introduced, the other handler could display a notification that the operation is 'forced' to complete.

My concern is that I could not find any IContentHandler, IContentPartHandler implementation using INotifier to display a message to the user. If you think that using INotifier does not break the separation of concerns, we could make that change. Although the Controller that displays the Success notification should also inform the user that the operation failed. If a handler that cancels the operation does not send a notification, the user won't see any message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Displaying a notification from a handler is OK, I think, though it's not that I'm too fond of that either.

It's actually better to have a notification in a controller, but then the context is not there. Unless instead of a bool IContentManager returns some result object with bool if the publish was canceled or not, and with a cancellation reason. Which looks overcomplicated and not nice either.

I'm really not sure. I see the problem, but I don't yet see good solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error messages can be provided by filling errors in the ModelState as the controller will then display these. Or forced using INotifier.

The current session will still be committed, unless some errors were written in the ModelState

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ModelState, of course. So, please add a message to it in DefaultContentManager. You can access the ModelState via an injected IUpdateModel.

Copy link
Member

@hishamco hishamco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will leave the final approval to @Piedone after addressing his feedback

Copy link
Contributor

github-actions bot commented Feb 6, 2025

This pull request has merge conflicts. Please resolve those before requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PublishContext support cancellation but this is not visible to User
4 participants