From eda2d7efa344ffe4fb0d6bfab7d46cede645741e Mon Sep 17 00:00:00 2001 From: Michael Petrinolis Date: Sat, 1 Feb 2025 03:29:34 +0200 Subject: [PATCH 1/9] Improve handling of publish/unpublish operations Updated AdminController to handle success/failure of publish/unpublish operations more robustly. Modified IContentManager interface to return Task for PublishAsync and UnpublishAsync methods. Implemented these changes in DefaultContentManager, including handling operation cancellations. Enhanced user feedback based on operation results. --- .../Controllers/AdminController.cs | 60 +++++++++++++++---- .../IContentManager.cs | 4 +- .../DefaultContentManager.cs | 19 ++++-- 3 files changed, 66 insertions(+), 17 deletions(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs index 33f3ba8ff67..ace2f374548 100644 --- a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs +++ b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs @@ -469,13 +469,23 @@ public async Task EditAndPublishPOST( return await EditPOST(contentItemId, returnUrl, stayOnSamePage, async contentItem => { - await _contentManager.PublishAsync(contentItem); + var published = await _contentManager.PublishAsync(contentItem); var typeDefinition = await _contentDefinitionManager.GetTypeDefinitionAsync(contentItem.ContentType); - await _notifier.SuccessAsync(string.IsNullOrWhiteSpace(typeDefinition?.DisplayName) + if (published) + { + await _notifier.SuccessAsync(string.IsNullOrWhiteSpace(typeDefinition?.DisplayName) ? H["Your content has been published."] : H["Your {0} has been published.", typeDefinition.DisplayName]); + } + else + { + await _notifier.ErrorAsync(string.IsNullOrWhiteSpace(typeDefinition?.DisplayName) + ? H["Your content could not be published."] + : H["Your {0} could not be published.", typeDefinition.DisplayName]); + + } }); } @@ -585,17 +595,32 @@ public async Task Publish(string contentItemId, string returnUrl) return Forbid(); } - await _contentManager.PublishAsync(contentItem); + var published = await _contentManager.PublishAsync(contentItem); var typeDefinition = await _contentDefinitionManager.GetTypeDefinitionAsync(contentItem.ContentType); - if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) + if (published) { - await _notifier.SuccessAsync(H["That content has been published."]); + if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) + { + await _notifier.SuccessAsync(H["That content has been published."]); + } + else + { + await _notifier.SuccessAsync(H["That {0} has been published.", typeDefinition.DisplayName]); + } } else { - await _notifier.SuccessAsync(H["That {0} has been published.", typeDefinition.DisplayName]); + if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) + { + await _notifier.ErrorAsync(H["That content could not be published."]); + } + else + { + await _notifier.ErrorAsync(H["That {0} could not be published.", typeDefinition.DisplayName]); + } + } return Url.IsLocalUrl(returnUrl) @@ -618,17 +643,32 @@ public async Task Unpublish(string contentItemId, string returnUr return Forbid(); } - await _contentManager.UnpublishAsync(contentItem); + var unpublished = await _contentManager.UnpublishAsync(contentItem); var typeDefinition = await _contentDefinitionManager.GetTypeDefinitionAsync(contentItem.ContentType); - if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) + if (unpublished) { - await _notifier.SuccessAsync(H["The content has been unpublished."]); + if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) + { + await _notifier.SuccessAsync(H["The content has been unpublished."]); + } + else + { + await _notifier.SuccessAsync(H["The {0} has been unpublished.", typeDefinition.DisplayName]); + } } else { - await _notifier.SuccessAsync(H["The {0} has been unpublished.", typeDefinition.DisplayName]); + if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) + { + await _notifier.ErrorAsync(H["The content could not be unpublished."]); + } + else + { + await _notifier.ErrorAsync(H["The {0} could not be unpublished.", typeDefinition.DisplayName]); + } + } return Url.IsLocalUrl(returnUrl) diff --git a/src/OrchardCore/OrchardCore.ContentManagement.Abstractions/IContentManager.cs b/src/OrchardCore/OrchardCore.ContentManagement.Abstractions/IContentManager.cs index 14e7a052754..0a20d345fcd 100644 --- a/src/OrchardCore/OrchardCore.ContentManagement.Abstractions/IContentManager.cs +++ b/src/OrchardCore/OrchardCore.ContentManagement.Abstractions/IContentManager.cs @@ -120,9 +120,9 @@ public interface IContentManager /// Task SaveDraftAsync(ContentItem contentItem); - Task PublishAsync(ContentItem contentItem); + Task PublishAsync(ContentItem contentItem); - Task UnpublishAsync(ContentItem contentItem); + Task UnpublishAsync(ContentItem contentItem); Task PopulateAspectAsync(IContent content, TAspect aspect); diff --git a/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs b/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs index e4caf00f02b..07977351c79 100644 --- a/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs +++ b/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs @@ -361,13 +361,13 @@ public async Task SaveDraftAsync(ContentItem contentItem) await ReversedHandlers.InvokeAsync((handler, context) => handler.DraftSavedAsync(context), context, _logger); } - public async Task PublishAsync(ContentItem contentItem) + public async Task PublishAsync(ContentItem contentItem) { ArgumentNullException.ThrowIfNull(contentItem); if (contentItem.Published) { - return; + return true; } // Create a context for the item and it's previous published record @@ -385,7 +385,7 @@ public async Task PublishAsync(ContentItem contentItem) if (context.Cancel) { - return; + return false; } if (previous != null) @@ -398,9 +398,11 @@ public async Task PublishAsync(ContentItem contentItem) await _session.SaveAsync(contentItem, checkConcurrency: true); await ReversedHandlers.InvokeAsync((handler, context) => handler.PublishedAsync(context), context, _logger); + + return true; } - public async Task UnpublishAsync(ContentItem contentItem) + public async Task UnpublishAsync(ContentItem contentItem) { ArgumentNullException.ThrowIfNull(contentItem); @@ -425,7 +427,7 @@ public async Task UnpublishAsync(ContentItem contentItem) if (publishedItem == null) { // No published version exists. no work to perform. - return; + return true; } // Create a context for the item. the publishing version is null in this case @@ -438,11 +440,18 @@ public async Task UnpublishAsync(ContentItem contentItem) await Handlers.InvokeAsync((handler, context) => handler.UnpublishingAsync(context), context, _logger); + if (context.Cancel) + { + return false; + } + publishedItem.Published = false; publishedItem.ModifiedUtc = _clock.UtcNow; await _session.SaveAsync(publishedItem, checkConcurrency: true); await ReversedHandlers.InvokeAsync((handler, context) => handler.UnpublishedAsync(context), context, _logger); + + return true; } protected async Task BuildNewVersionAsync(ContentItem existingContentItem) From cacfea05c2993b394ceaa2ae70d8a537f2d9388b Mon Sep 17 00:00:00 2001 From: Michael Petrinolis Date: Sat, 1 Feb 2025 10:53:27 +0200 Subject: [PATCH 2/9] Update src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs Co-authored-by: Hisham Bin Ateya --- .../OrchardCore.Contents/Controllers/AdminController.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs index ace2f374548..f5c488f2f00 100644 --- a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs +++ b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs @@ -484,7 +484,6 @@ await _notifier.SuccessAsync(string.IsNullOrWhiteSpace(typeDefinition?.DisplayNa await _notifier.ErrorAsync(string.IsNullOrWhiteSpace(typeDefinition?.DisplayName) ? H["Your content could not be published."] : H["Your {0} could not be published.", typeDefinition.DisplayName]); - } }); } From 1c998ce54b13e9b4c2469da0fcefc76c27fee945 Mon Sep 17 00:00:00 2001 From: Michael Petrinolis Date: Sat, 1 Feb 2025 10:53:35 +0200 Subject: [PATCH 3/9] Update src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs Co-authored-by: Hisham Bin Ateya --- .../OrchardCore.Contents/Controllers/AdminController.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs index f5c488f2f00..670d49e3d9c 100644 --- a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs +++ b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs @@ -619,7 +619,6 @@ public async Task Publish(string contentItemId, string returnUrl) { await _notifier.ErrorAsync(H["That {0} could not be published.", typeDefinition.DisplayName]); } - } return Url.IsLocalUrl(returnUrl) From 3379355c10faf2c5c6507bc8949a3d47022b8c93 Mon Sep 17 00:00:00 2001 From: Michael Petrinolis Date: Sat, 1 Feb 2025 10:53:41 +0200 Subject: [PATCH 4/9] Update src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs Co-authored-by: Hisham Bin Ateya --- .../OrchardCore.Contents/Controllers/AdminController.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs index 670d49e3d9c..b229823d459 100644 --- a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs +++ b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs @@ -666,7 +666,6 @@ public async Task Unpublish(string contentItemId, string returnUr { await _notifier.ErrorAsync(H["The {0} could not be unpublished.", typeDefinition.DisplayName]); } - } return Url.IsLocalUrl(returnUrl) From 430f011a1b92c93efcf25ee26dc49eed2f1d4b79 Mon Sep 17 00:00:00 2001 From: Michael Petrinolis Date: Sat, 8 Feb 2025 21:39:15 +0200 Subject: [PATCH 5/9] Improve bulk action handling and user notifications Introduce `authorized` variable in `AdminController` to track authorization status during bulk actions, redirecting to list view or returning forbidden response as needed. Update `conditionallyPublish` to return `Task` for better control flow in `CreateInternalAsync` and `EditInternalAsync`. Enhance `DefaultContentManager` with `INotifier` and `IHtmlLocalizer` dependencies to provide user notifications when publishing or unpublishing is canceled, including content type and display text in messages. --- .../Controllers/AdminController.cs | 74 ++++++------------- .../DefaultContentManager.cs | 33 ++++++++- 2 files changed, 55 insertions(+), 52 deletions(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs index 239f44d366d..5e25c2bea7f 100644 --- a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs +++ b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs @@ -256,7 +256,7 @@ public async Task ListPOST(ContentOptionsViewModel options, long[] var checkedContentItems = await _session.Query() .Where(x => x.DocumentId.IsIn(itemIds) && x.Latest) .ListAsync(_contentManager); - + bool authorized = false; switch (options.BulkAction) { case ContentsBulkAction.None: @@ -264,28 +264,24 @@ public async Task ListPOST(ContentOptionsViewModel options, long[] case ContentsBulkAction.PublishNow: foreach (var item in checkedContentItems) { - if (!await _authorizationService.AuthorizeAsync(User, CommonPermissions.PublishContent, item)) + if (!(authorized = await _authorizationService.AuthorizeAsync(User, CommonPermissions.PublishContent, item)) || !await _contentManager.PublishAsync(item)) { await _notifier.WarningAsync(H["Couldn't publish selected content."]); await _session.CancelAsync(); - return Forbid(); + return authorized ? RedirectToAction(nameof(List)) : Forbid(); } - - await _contentManager.PublishAsync(item); } await _notifier.SuccessAsync(H["Content published successfully."]); break; case ContentsBulkAction.Unpublish: foreach (var item in checkedContentItems) { - if (!await IsAuthorizedAsync(CommonPermissions.PublishContent, item)) + if (!(authorized = await IsAuthorizedAsync(CommonPermissions.PublishContent, item)) || !await _contentManager.UnpublishAsync(item)) { await _notifier.WarningAsync(H["Couldn't unpublish selected content."]); await _session.CancelAsync(); - return Forbid(); + return authorized ? RedirectToAction(nameof(List)) : Forbid(); } - - await _contentManager.UnpublishAsync(item); } await _notifier.SuccessAsync(H["Content unpublished successfully."]); break; @@ -349,6 +345,8 @@ public Task CreatePOST( await _notifier.SuccessAsync(string.IsNullOrWhiteSpace(typeDefinition?.DisplayName) ? H["Your content draft has been saved."] : H["Your {0} draft has been saved.", typeDefinition.DisplayName]); + + return true; }); } @@ -374,13 +372,17 @@ public async Task CreateAndPublishPOST( return await CreateInternalAsync(id, returnUrl, stayOnSamePage, async contentItem => { - await _contentManager.PublishAsync(contentItem); + if (await _contentManager.PublishAsync(contentItem)) + { + var typeDefinition = await _contentDefinitionManager.GetTypeDefinitionAsync(contentItem.ContentType); - var typeDefinition = await _contentDefinitionManager.GetTypeDefinitionAsync(contentItem.ContentType); + await _notifier.SuccessAsync(string.IsNullOrWhiteSpace(typeDefinition.DisplayName) + ? H["Your content has been published."] + : H["Your {0} has been published.", typeDefinition.DisplayName]); - await _notifier.SuccessAsync(string.IsNullOrWhiteSpace(typeDefinition.DisplayName) - ? H["Your content has been published."] - : H["Your {0} has been published.", typeDefinition.DisplayName]); + return true; + } + return false; }); } @@ -443,6 +445,8 @@ public Task EditPOST( await _notifier.SuccessAsync(string.IsNullOrWhiteSpace(typeDefinition?.DisplayName) ? H["Your content draft has been saved."] : H["Your {0} draft has been saved.", typeDefinition.DisplayName]); + + return true; }); } @@ -481,12 +485,8 @@ await _notifier.SuccessAsync(string.IsNullOrWhiteSpace(typeDefinition?.DisplayNa ? H["Your content has been published."] : H["Your {0} has been published.", typeDefinition.DisplayName]); } - else - { - await _notifier.ErrorAsync(string.IsNullOrWhiteSpace(typeDefinition?.DisplayName) - ? H["Your content could not be published."] - : H["Your {0} could not be published.", typeDefinition.DisplayName]); - } + + return published; }); } @@ -611,17 +611,6 @@ public async Task Publish(string contentItemId, string returnUrl) await _notifier.SuccessAsync(H["That {0} has been published.", typeDefinition.DisplayName]); } } - else - { - if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) - { - await _notifier.ErrorAsync(H["That content could not be published."]); - } - else - { - await _notifier.ErrorAsync(H["That {0} could not be published.", typeDefinition.DisplayName]); - } - } return Url.IsLocalUrl(returnUrl) ? this.LocalRedirect(returnUrl, true) @@ -658,17 +647,6 @@ public async Task Unpublish(string contentItemId, string returnUr await _notifier.SuccessAsync(H["The {0} has been unpublished.", typeDefinition.DisplayName]); } } - else - { - if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) - { - await _notifier.ErrorAsync(H["The content could not be unpublished."]); - } - else - { - await _notifier.ErrorAsync(H["The {0} could not be unpublished.", typeDefinition.DisplayName]); - } - } return Url.IsLocalUrl(returnUrl) ? this.LocalRedirect(returnUrl, true) @@ -679,7 +657,7 @@ private async Task CreateInternalAsync( string id, string returnUrl, bool stayOnSamePage, - Func conditionallyPublish) + Func> conditionallyPublish) { var contentItem = await CreateContentItemOwnedByCurrentUserAsync(id); @@ -695,14 +673,12 @@ private async Task CreateInternalAsync( await _contentManager.CreateAsync(contentItem, VersionOptions.Draft); } - if (!ModelState.IsValid) + if (!ModelState.IsValid || !await conditionallyPublish(contentItem)) { await _session.CancelAsync(); return View(model); } - await conditionallyPublish(contentItem); - if (!string.IsNullOrEmpty(returnUrl) && !stayOnSamePage) { return this.LocalRedirect(returnUrl, true); @@ -722,7 +698,7 @@ private async Task EditInternalAsync( string contentItemId, string returnUrl, bool stayOnSamePage, - Func conditionallyPublish) + Func> conditionallyPublish) { var contentItem = await _contentManager.GetAsync(contentItemId, VersionOptions.DraftRequired); @@ -738,14 +714,12 @@ private async Task EditInternalAsync( var model = await _contentItemDisplayManager.UpdateEditorAsync(contentItem, this, false); - if (!ModelState.IsValid) + if (!ModelState.IsValid || !await conditionallyPublish(contentItem)) { await _session.CancelAsync(); return View(nameof(Edit), model); } - await conditionallyPublish(contentItem); - if (returnUrl == null) { return RedirectToAction(nameof(Edit), new RouteValueDictionary diff --git a/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs b/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs index 743fbcb2690..133b5627ac2 100644 --- a/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs +++ b/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs @@ -1,6 +1,8 @@ using System.ComponentModel.DataAnnotations; using System.Text.Json.Nodes; using System.Text.Json.Settings; +using Microsoft.AspNetCore.Mvc.Localization; +using Microsoft.Extensions.Localization; using Microsoft.Extensions.Logging; using OrchardCore.ContentManagement.CompiledQueries; using OrchardCore.ContentManagement.Handlers; @@ -8,6 +10,8 @@ using OrchardCore.ContentManagement.Metadata.Builders; using OrchardCore.ContentManagement.Metadata.Models; using OrchardCore.ContentManagement.Records; +using OrchardCore.DisplayManagement.ModelBinding; +using OrchardCore.DisplayManagement.Notify; using OrchardCore.Modules; using YesSql; using YesSql.Services; @@ -29,6 +33,9 @@ public class DefaultContentManager : IContentManager private readonly IContentManagerSession _contentManagerSession; private readonly IContentItemIdGenerator _idGenerator; private readonly IClock _clock; + private readonly INotifier _notifier; + protected readonly IHtmlLocalizer H; + public DefaultContentManager( IContentDefinitionManager contentDefinitionManager, @@ -37,7 +44,9 @@ public DefaultContentManager( ISession session, IContentItemIdGenerator idGenerator, ILogger logger, - IClock clock) + IClock clock, + INotifier notifier, + IHtmlLocalizer localizer) { _contentDefinitionManager = contentDefinitionManager; Handlers = handlers; @@ -46,6 +55,8 @@ public DefaultContentManager( _contentManagerSession = contentManagerSession; _logger = logger; _clock = clock; + _notifier = notifier; + H = localizer; } public IEnumerable Handlers { get; private set; } @@ -382,9 +393,18 @@ public async Task PublishAsync(ContentItem contentItem) // Invoke handlers to acquire state, or at least establish lazy loading callbacks. await Handlers.InvokeAsync((handler, context) => handler.PublishingAsync(context), context, _logger); - + if (context.Cancel) { + var typeDefinition = await _contentDefinitionManager.GetTypeDefinitionAsync(contentItem.ContentType); + if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) + { + await _notifier.ErrorAsync(H["Publishing '{0}' was cancelled.", contentItem.DisplayText]); + } + else + { + await _notifier.ErrorAsync(H["Publishing {0} '{1}' was cancelled.", typeDefinition.DisplayName, contentItem.DisplayText]); + } return false; } @@ -444,6 +464,15 @@ public async Task UnpublishAsync(ContentItem contentItem) if (context.Cancel) { + var typeDefinition = await _contentDefinitionManager.GetTypeDefinitionAsync(contentItem.ContentType); + if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) + { + await _notifier.ErrorAsync(H["Unpublishing '{0}' was cancelled.", contentItem.DisplayText]); + } + else + { + await _notifier.ErrorAsync(H["Unpublishing {0} '{1}' was cancelled.", typeDefinition.DisplayName, contentItem.DisplayText]); + } return false; } From 2ecaa188cde3cea29e94424be462c140e0998f7c Mon Sep 17 00:00:00 2001 From: Michael Petrinolis Date: Sun, 9 Feb 2025 23:03:59 +0200 Subject: [PATCH 6/9] Refactor error handling and localization in controllers Updated AdminController to handle ModelState errors across redirects using TempData. Added RedirectToListActionWithModelState method. Updated List.cshtml to display validation errors. Replaced INotifier and IHtmlLocalizer with IUpdateModelAccessor and IStringLocalizer in DefaultContentManager. Ensured ModelUpdater is checked before adding ModelState errors. --- .../Controllers/AdminController.cs | 52 ++++++++++++++++--- .../Views/Admin/List.cshtml | 1 + .../DefaultContentManager.cs | 50 ++++++++++-------- 3 files changed, 75 insertions(+), 28 deletions(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs index 5e25c2bea7f..73c3102c93c 100644 --- a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs +++ b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs @@ -1,9 +1,12 @@ using System.Security.Claims; +using System.Text.Json; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Localization; +using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.AspNetCore.Mvc.Rendering; using Microsoft.AspNetCore.Routing; +using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.Extensions.Localization; using Microsoft.Extensions.Options; using OrchardCore.Admin; @@ -219,6 +222,21 @@ public async Task List( viewModel.Header = header; }); + if (TempData.TryGetValue(nameof(ModelState), out var modelStateJson) && modelStateJson is string) + { + var errors = JsonSerializer.Deserialize>((string)modelStateJson); + if (errors != null) + { + foreach (var error in errors) + { + foreach (var errorMessage in error.Value) + { + ModelState.AddModelError(error.Key, errorMessage); + } + } + } + } + return View(shapeViewModel); } @@ -268,7 +286,8 @@ public async Task ListPOST(ContentOptionsViewModel options, long[] { await _notifier.WarningAsync(H["Couldn't publish selected content."]); await _session.CancelAsync(); - return authorized ? RedirectToAction(nameof(List)) : Forbid(); + + return authorized ? RedirectToListActionWithModelState() : Forbid(); } } await _notifier.SuccessAsync(H["Content published successfully."]); @@ -280,7 +299,7 @@ public async Task ListPOST(ContentOptionsViewModel options, long[] { await _notifier.WarningAsync(H["Couldn't unpublish selected content."]); await _session.CancelAsync(); - return authorized ? RedirectToAction(nameof(List)) : Forbid(); + return authorized ? RedirectToListActionWithModelState() : Forbid(); } } await _notifier.SuccessAsync(H["Content unpublished successfully."]); @@ -303,7 +322,6 @@ public async Task ListPOST(ContentOptionsViewModel options, long[] return BadRequest(); } } - return RedirectToAction(nameof(List)); } @@ -611,10 +629,14 @@ public async Task Publish(string contentItemId, string returnUrl) await _notifier.SuccessAsync(H["That {0} has been published.", typeDefinition.DisplayName]); } } + else if (Url.IsLocalUrl(returnUrl)) + { + await _notifier.ErrorAsync(H["The operation was canceled."]); + } return Url.IsLocalUrl(returnUrl) ? this.LocalRedirect(returnUrl, true) - : RedirectToAction(nameof(List)); + : RedirectToListActionWithModelState(); } [HttpPost] @@ -647,10 +669,15 @@ public async Task Unpublish(string contentItemId, string returnUr await _notifier.SuccessAsync(H["The {0} has been unpublished.", typeDefinition.DisplayName]); } } + else if (Url.IsLocalUrl(returnUrl)) + { + await _notifier.ErrorAsync(H["The operation was canceled."]); + } + return Url.IsLocalUrl(returnUrl) ? this.LocalRedirect(returnUrl, true) - : RedirectToAction(nameof(List)); + : RedirectToListActionWithModelState(); } private async Task CreateInternalAsync( @@ -714,7 +741,7 @@ private async Task EditInternalAsync( var model = await _contentItemDisplayManager.UpdateEditorAsync(contentItem, this, false); - if (!ModelState.IsValid || !await conditionallyPublish(contentItem)) + if (!ModelState.IsValid || !(await conditionallyPublish(contentItem))) { await _session.CancelAsync(); return View(nameof(Edit), model); @@ -807,4 +834,17 @@ private async Task IsAuthorizedAsync(Permission permission) private async Task IsAuthorizedAsync(Permission permission, object resource) => await _authorizationService.AuthorizeAsync(User, permission, resource); + + private RedirectToActionResult RedirectToListActionWithModelState() + { + var errors = ModelState + .Where(ms => ms.Value.Errors.Any()) + .ToDictionary( + kvp => kvp.Key, + kvp => kvp.Value.Errors.Select(e => e.ErrorMessage).ToArray() + ); + + TempData[nameof(ModelState)] = JsonSerializer.Serialize(errors); + return RedirectToAction(nameof(List)); + } } diff --git a/src/OrchardCore.Modules/OrchardCore.Contents/Views/Admin/List.cshtml b/src/OrchardCore.Modules/OrchardCore.Contents/Views/Admin/List.cshtml index 085d9f9f657..3aa0b1c5ecc 100644 --- a/src/OrchardCore.Modules/OrchardCore.Contents/Views/Admin/List.cshtml +++ b/src/OrchardCore.Modules/OrchardCore.Contents/Views/Admin/List.cshtml @@ -13,5 +13,6 @@

@RenderTitleSegments(pageTitle)
+ @Html.ValidationSummary() @await DisplayAsync(Model)
diff --git a/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs b/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs index 133b5627ac2..abd9aff3b5e 100644 --- a/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs +++ b/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs @@ -33,8 +33,8 @@ public class DefaultContentManager : IContentManager private readonly IContentManagerSession _contentManagerSession; private readonly IContentItemIdGenerator _idGenerator; private readonly IClock _clock; - private readonly INotifier _notifier; - protected readonly IHtmlLocalizer H; + private readonly IUpdateModelAccessor _updateModelAccessor; + protected readonly IStringLocalizer S; public DefaultContentManager( @@ -45,8 +45,8 @@ public DefaultContentManager( IContentItemIdGenerator idGenerator, ILogger logger, IClock clock, - INotifier notifier, - IHtmlLocalizer localizer) + IUpdateModelAccessor updateModelAccessor, + IStringLocalizer localizer) { _contentDefinitionManager = contentDefinitionManager; Handlers = handlers; @@ -55,8 +55,8 @@ public DefaultContentManager( _contentManagerSession = contentManagerSession; _logger = logger; _clock = clock; - _notifier = notifier; - H = localizer; + _updateModelAccessor = updateModelAccessor; + S = localizer; } public IEnumerable Handlers { get; private set; } @@ -393,17 +393,20 @@ public async Task PublishAsync(ContentItem contentItem) // Invoke handlers to acquire state, or at least establish lazy loading callbacks. await Handlers.InvokeAsync((handler, context) => handler.PublishingAsync(context), context, _logger); - + if (context.Cancel) { - var typeDefinition = await _contentDefinitionManager.GetTypeDefinitionAsync(contentItem.ContentType); - if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) + if (_updateModelAccessor.ModelUpdater is not null) { - await _notifier.ErrorAsync(H["Publishing '{0}' was cancelled.", contentItem.DisplayText]); - } - else - { - await _notifier.ErrorAsync(H["Publishing {0} '{1}' was cancelled.", typeDefinition.DisplayName, contentItem.DisplayText]); + var typeDefinition = await _contentDefinitionManager.GetTypeDefinitionAsync(contentItem.ContentType); + if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) + { + _updateModelAccessor.ModelUpdater.ModelState.AddModelError("", S["Publishing '{0}' was cancelled.", contentItem.DisplayText]); + } + else + { + _updateModelAccessor.ModelUpdater.ModelState.AddModelError("", S["Publishing {0} '{1}' was cancelled.", typeDefinition.DisplayName, contentItem.DisplayText]); + } } return false; } @@ -464,17 +467,20 @@ public async Task UnpublishAsync(ContentItem contentItem) if (context.Cancel) { - var typeDefinition = await _contentDefinitionManager.GetTypeDefinitionAsync(contentItem.ContentType); - if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) + if (_updateModelAccessor.ModelUpdater is not null) { - await _notifier.ErrorAsync(H["Unpublishing '{0}' was cancelled.", contentItem.DisplayText]); - } - else - { - await _notifier.ErrorAsync(H["Unpublishing {0} '{1}' was cancelled.", typeDefinition.DisplayName, contentItem.DisplayText]); + var typeDefinition = await _contentDefinitionManager.GetTypeDefinitionAsync(contentItem.ContentType); + if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) + { + _updateModelAccessor.ModelUpdater.ModelState.AddModelError("", S["Unpublishing '{0}' was cancelled.", contentItem.DisplayText]); + } + else + { + _updateModelAccessor.ModelUpdater.ModelState.AddModelError("", S["Unpublishing {0} '{1}' was cancelled.", typeDefinition.DisplayName, contentItem.DisplayText]); + } } return false; - } + } publishedItem.Published = false; publishedItem.ModifiedUtc = _clock.UtcNow; From 766744bd29667081da1248ad2ae10fae062fde37 Mon Sep 17 00:00:00 2001 From: Michael Petrinolis Date: Mon, 10 Feb 2025 10:01:26 +0200 Subject: [PATCH 7/9] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Hisham Bin Ateya Co-authored-by: Zoltán Lehóczky --- .../Controllers/AdminController.cs | 19 +++++++++++-------- .../DefaultContentManager.cs | 3 ++- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs index 73c3102c93c..6aee4b120b1 100644 --- a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs +++ b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs @@ -225,14 +225,11 @@ public async Task List( if (TempData.TryGetValue(nameof(ModelState), out var modelStateJson) && modelStateJson is string) { var errors = JsonSerializer.Deserialize>((string)modelStateJson); - if (errors != null) + if (errors is not null) { - foreach (var error in errors) + foreach (var errorMessage in errors.SelectMany(e => e.Value)) { - foreach (var errorMessage in error.Value) - { - ModelState.AddModelError(error.Key, errorMessage); - } + ModelState.AddModelError(error.Key, errorMessage); } } } @@ -274,7 +271,7 @@ public async Task ListPOST(ContentOptionsViewModel options, long[] var checkedContentItems = await _session.Query() .Where(x => x.DocumentId.IsIn(itemIds) && x.Latest) .ListAsync(_contentManager); - bool authorized = false; + var isAuthorized = false; switch (options.BulkAction) { case ContentsBulkAction.None: @@ -299,7 +296,10 @@ public async Task ListPOST(ContentOptionsViewModel options, long[] { await _notifier.WarningAsync(H["Couldn't unpublish selected content."]); await _session.CancelAsync(); - return authorized ? RedirectToListActionWithModelState() : Forbid(); + + return authorized + ? RedirectToListActionWithModelState() + : Forbid(); } } await _notifier.SuccessAsync(H["Content unpublished successfully."]); @@ -400,6 +400,7 @@ await _notifier.SuccessAsync(string.IsNullOrWhiteSpace(typeDefinition.DisplayNam return true; } + return false; }); } @@ -620,6 +621,7 @@ public async Task Publish(string contentItemId, string returnUrl) if (published) { + if (string.IsNullOrEmpty(typeDefinition?.DisplayName)) { await _notifier.SuccessAsync(H["That content has been published."]); @@ -845,6 +847,7 @@ private RedirectToActionResult RedirectToListActionWithModelState() ); TempData[nameof(ModelState)] = JsonSerializer.Serialize(errors); + return RedirectToAction(nameof(List)); } } diff --git a/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs b/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs index abd9aff3b5e..2b5ceb123cb 100644 --- a/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs +++ b/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs @@ -36,7 +36,6 @@ public class DefaultContentManager : IContentManager private readonly IUpdateModelAccessor _updateModelAccessor; protected readonly IStringLocalizer S; - public DefaultContentManager( IContentDefinitionManager contentDefinitionManager, IContentManagerSession contentManagerSession, @@ -408,6 +407,7 @@ public async Task PublishAsync(ContentItem contentItem) _updateModelAccessor.ModelUpdater.ModelState.AddModelError("", S["Publishing {0} '{1}' was cancelled.", typeDefinition.DisplayName, contentItem.DisplayText]); } } + return false; } @@ -479,6 +479,7 @@ public async Task UnpublishAsync(ContentItem contentItem) _updateModelAccessor.ModelUpdater.ModelState.AddModelError("", S["Unpublishing {0} '{1}' was cancelled.", typeDefinition.DisplayName, contentItem.DisplayText]); } } + return false; } From f8be070ce62d61ba89944f1ea73703b1da58dcfc Mon Sep 17 00:00:00 2001 From: Michael Petrinolis Date: Mon, 10 Feb 2025 10:11:12 +0200 Subject: [PATCH 8/9] Refactor error handling and improve readability Refactor error handling in AdminController to iterate through error keys and messages before adding them to ModelState. Rename variable 'authorized' to 'isAuthorized' for better readability and reformat return statement for clarity. --- .../Controllers/AdminController.cs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs index 6aee4b120b1..77f74a7cca5 100644 --- a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs +++ b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs @@ -227,9 +227,12 @@ public async Task List( var errors = JsonSerializer.Deserialize>((string)modelStateJson); if (errors is not null) { - foreach (var errorMessage in errors.SelectMany(e => e.Value)) + foreach (var error in errors) { - ModelState.AddModelError(error.Key, errorMessage); + foreach (var errorMessage in error.Value) + { + ModelState.AddModelError(error.Key, errorMessage); + } } } } @@ -279,12 +282,14 @@ public async Task ListPOST(ContentOptionsViewModel options, long[] case ContentsBulkAction.PublishNow: foreach (var item in checkedContentItems) { - if (!(authorized = await _authorizationService.AuthorizeAsync(User, CommonPermissions.PublishContent, item)) || !await _contentManager.PublishAsync(item)) + if (!(isAuthorized = await _authorizationService.AuthorizeAsync(User, CommonPermissions.PublishContent, item)) || !await _contentManager.PublishAsync(item)) { await _notifier.WarningAsync(H["Couldn't publish selected content."]); await _session.CancelAsync(); - return authorized ? RedirectToListActionWithModelState() : Forbid(); + return isAuthorized + ? RedirectToListActionWithModelState() + : Forbid(); } } await _notifier.SuccessAsync(H["Content published successfully."]); @@ -292,12 +297,12 @@ public async Task ListPOST(ContentOptionsViewModel options, long[] case ContentsBulkAction.Unpublish: foreach (var item in checkedContentItems) { - if (!(authorized = await IsAuthorizedAsync(CommonPermissions.PublishContent, item)) || !await _contentManager.UnpublishAsync(item)) + if (!(isAuthorized = await IsAuthorizedAsync(CommonPermissions.PublishContent, item)) || !await _contentManager.UnpublishAsync(item)) { await _notifier.WarningAsync(H["Couldn't unpublish selected content."]); await _session.CancelAsync(); - return authorized + return isAuthorized ? RedirectToListActionWithModelState() : Forbid(); } From fc5a398c25b9edc622ed932ba5a710f1c878236c Mon Sep 17 00:00:00 2001 From: Michael Petrinolis Date: Tue, 11 Feb 2025 07:59:39 +0200 Subject: [PATCH 9/9] Refactor authorization checks in BulkAction switch The code refactor removes the `isAuthorized` variable and replaces it with an inline variable declaration using the `is var` pattern within the `if` statements. This change is applied to both the `PublishNow` and `Unpublish` cases within the `switch` statement handling `options.BulkAction`. The refactor simplifies the authorization and content management checks by combining them into a single `if` statement, improving readability. The return statements are updated to use the new `authorized` variable instead of the removed `isAuthorized` variable. --- .../Controllers/AdminController.cs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs index 77f74a7cca5..840fa31d2ce 100644 --- a/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs +++ b/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs @@ -274,7 +274,7 @@ public async Task ListPOST(ContentOptionsViewModel options, long[] var checkedContentItems = await _session.Query() .Where(x => x.DocumentId.IsIn(itemIds) && x.Latest) .ListAsync(_contentManager); - var isAuthorized = false; + switch (options.BulkAction) { case ContentsBulkAction.None: @@ -282,14 +282,15 @@ public async Task ListPOST(ContentOptionsViewModel options, long[] case ContentsBulkAction.PublishNow: foreach (var item in checkedContentItems) { - if (!(isAuthorized = await _authorizationService.AuthorizeAsync(User, CommonPermissions.PublishContent, item)) || !await _contentManager.PublishAsync(item)) + if (await _authorizationService.AuthorizeAsync(User, CommonPermissions.PublishContent, item) is var authorized + && !(authorized && await _contentManager.PublishAsync(item))) { await _notifier.WarningAsync(H["Couldn't publish selected content."]); await _session.CancelAsync(); - return isAuthorized - ? RedirectToListActionWithModelState() - : Forbid(); + return authorized + ? RedirectToListActionWithModelState() + : Forbid(); } } await _notifier.SuccessAsync(H["Content published successfully."]); @@ -297,13 +298,14 @@ public async Task ListPOST(ContentOptionsViewModel options, long[] case ContentsBulkAction.Unpublish: foreach (var item in checkedContentItems) { - if (!(isAuthorized = await IsAuthorizedAsync(CommonPermissions.PublishContent, item)) || !await _contentManager.UnpublishAsync(item)) + if (await IsAuthorizedAsync(CommonPermissions.PublishContent, item) is var authorized + && !(authorized && await _contentManager.UnpublishAsync(item))) { await _notifier.WarningAsync(H["Couldn't unpublish selected content."]); await _session.CancelAsync(); - return isAuthorized - ? RedirectToListActionWithModelState() + return authorized + ? RedirectToListActionWithModelState() : Forbid(); } }