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.

Sign up for GitHub

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

Jump to bottom

V10/bugfix/14543 publish descendants #14763

Merged
merged 7 commits into from
Sep 5, 2023
Merged

V10/bugfix/14543 publish descendants #14763

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8cc6aee
WIP: Fix publish descendants and related notifications
Migaroez Aug 21, 2023
c50cd02
Removed related entitities from publish notification
Migaroez Aug 30, 2023
561ff4f
Fixed root not being saved when publishingWithDescendants
Migaroez Aug 30, 2023
32eaa92
Updated integrationtests to reflect the update view on when to save t…
Migaroez Sep 4, 2023
6b18273
PR formatting fix
Migaroez Sep 5, 2023
ab451ac
PR Cleanup
Migaroez Sep 5, 2023
433b6f3
Spicing up the codebase with some PR pattern matching
Migaroez Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Umbraco.Core/Notifications/NotificationExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
using Umbraco.Cms.Core.Models;

namespace Umbraco.Cms.Core.Notifications;
Migaroez marked this conversation as resolved.
Show resolved Hide resolved

public static class NotificationExtensions
Expand Down
42 changes: 31 additions & 11 deletions src/Umbraco.Core/Services/ContentService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Microsoft.Extensions.DependencyInjection;

Check notice on line 1 in src/Umbraco.Core/Services/ContentService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v10/dev)

ℹ Getting worse: Overall Code Complexity 

The mean cyclomatic complexity increases from 5.39 to 5.40, threshold = 4. This file has many conditional statements (e.g. if, for, while) across its implementation, leading to lower code health. Avoid adding more conditionals.
using Microsoft.Extensions.Logging;
using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Core.Exceptions;
Expand Down Expand Up @@ -207,7 +207,7 @@

/// <summary>
/// Used to bulk update the permissions set for a content item. This will replace all permissions
/// assigned to an entity with a list of user id & permission pairs.

Check warning on line 210 in src/Umbraco.Core/Services/ContentService.cs

View workflow job for this annotation

GitHub Actions / CodeQL-Build 

XML comment has badly formed XML -- 'Whitespace is not allowed at this location.'
/// </summary>
/// <param name="permissionSet"></param>
public void SetPermissions(EntityPermissionSet permissionSet)
Expand Down Expand Up @@ -1950,7 +1950,7 @@
cultures = new HashSet<string>(); // empty means 'already published'
}

if (edited)
if (isRoot || edited)

Check warning on line 1953 in src/Umbraco.Core/Services/ContentService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v10/dev)

❌ Getting worse: Complex Method 

SaveAndPublishBranch_ShouldPublish increases in cyclomatic complexity from 9 to 10, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
{
cultures.Add(c); // <culture> means 'republish this culture'
}
Expand Down Expand Up @@ -2105,11 +2105,13 @@
}

// deal with the branch root - if it fails, abort
PublishResult? result = SaveAndPublishBranchItem(scope, document, shouldPublish, publishCultures, true, publishedDocuments, eventMessages, userId, allLangs);
if (result != null)
var rootPublishNotificationState = new Dictionary<string, object?>();
PublishResult? rootResult = SaveAndPublishBranchItem(scope, document, shouldPublish, publishCultures, true,
publishedDocuments, eventMessages, userId, allLangs,rootPublishNotificationState);
Migaroez marked this conversation as resolved.
Show resolved Hide resolved
if (rootResult != null)
{
results.Add(result);
if (!result.Success)
results.Add(rootResult);
if (!rootResult.Success)
{
return results;
}
Expand All @@ -2122,6 +2124,7 @@
int count;
var page = 0;
const int pageSize = 100;
PublishResult? result = null;
do
{
count = 0;
Expand All @@ -2140,7 +2143,8 @@
}

// no need to check path here, parent has to be published here
result = SaveAndPublishBranchItem(scope, d, shouldPublish, publishCultures, false, publishedDocuments, eventMessages, userId, allLangs);
result = SaveAndPublishBranchItem(scope, d, shouldPublish, publishCultures, false,
publishedDocuments, eventMessages, userId, allLangs,null);
if (result != null)
{
results.Add(result);
Expand All @@ -2164,7 +2168,12 @@
// (SaveAndPublishBranchOne does *not* do it)
scope.Notifications.Publish(
new ContentTreeChangeNotification(document, TreeChangeTypes.RefreshBranch, eventMessages));
scope.Notifications.Publish(new ContentPublishedNotification(publishedDocuments, eventMessages));
if (rootResult?.Success == true)
Migaroez marked this conversation as resolved.
Show resolved Hide resolved
{
scope.Notifications.Publish(
new ContentPublishedNotification(rootResult!.Content!, eventMessages)
.WithState(rootPublishNotificationState));
}

scope.Complete();
}
Expand All @@ -2175,6 +2184,9 @@
// shouldPublish: a function determining whether the document has changes that need to be published
// note - 'force' is handled by 'editing'
// publishValues: a function publishing values (using the appropriate PublishCulture calls)
/// <param name="rootPublishingNotificationState">Only set this when processing a the root of the branch
/// Published notification will not be send when this property is set</param>
/// <returns></returns>
private PublishResult? SaveAndPublishBranchItem(
ICoreScope scope,
IContent document,
Expand All @@ -2185,7 +2197,8 @@
ICollection<IContent> publishedDocuments,
EventMessages evtMsgs,
int userId,
IReadOnlyCollection<ILanguage> allLangs)
IReadOnlyCollection<ILanguage> allLangs,
IDictionary<string, object?>? rootPublishingNotificationState)
{
HashSet<string>? culturesToPublish = shouldPublish(document);

Expand Down Expand Up @@ -2214,10 +2227,17 @@
return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, document);
}

PublishResult result = CommitDocumentChangesInternal(scope, document, evtMsgs, allLangs, savingNotification.State, userId, true, isRoot);
if (result.Success)
var notificationState = rootPublishingNotificationState ?? new Dictionary<string, object?>();
PublishResult result = CommitDocumentChangesInternal(scope, document, evtMsgs, allLangs, notificationState, userId, true, isRoot);
if (!result.Success)
{
return result;
}

publishedDocuments.Add(document);
if (rootPublishingNotificationState == null)
{
publishedDocuments.Add(document);
scope.Notifications.Publish(new ContentPublishedNotification(result.Content!, evtMsgs).WithState(notificationState));
}

return result;
Expand Down
12 changes: 6 additions & 6 deletions ...aco.Tests.Integration/Umbraco.Infrastructure/Services/ContentServicePublishBranchTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ public void Can_Publish_Invariant_Branch(int method)
AssertPublishResults(
r,
x => x.Result,
PublishResultType.SuccessPublishAlready,
PublishResultType.SuccessPublish, // During branch publishing, the change detection of the root branch runs AFTER the check to process the branchItem => root is always saved&Published as the intent requires it.
PublishResultType.SuccessPublishAlready,
PublishResultType.SuccessPublishAlready);

Expand Down Expand Up @@ -138,7 +138,7 @@ public void Can_Publish_Invariant_Branch(int method)
AssertPublishResults(
r,
x => x.Result,
PublishResultType.SuccessPublishAlready,
PublishResultType.SuccessPublish, // During branch publishing, the change detection of the root branch runs AFTER the check to process the branchItem => root is always saved&Published as the intent requires it.
PublishResultType.SuccessPublishAlready,
PublishResultType.SuccessPublishAlready,
PublishResultType.SuccessPublish,
Expand Down Expand Up @@ -183,7 +183,7 @@ public void Can_Publish_Variant_Branch_When_No_Changes_On_Root_All_Cultures()

var r = ContentService.SaveAndPublishBranch(vRoot, false)
.ToArray(); // no culture specified so "*" is used, so all cultures
Assert.AreEqual(PublishResultType.SuccessPublishAlready, r[0].Result);
Assert.AreEqual(PublishResultType.SuccessPublishCulture, r[0].Result); // During branch publishing, the change detection of the root branch runs AFTER the check to process the branchItem => root is always saved&Published as the intent requires it.
Assert.AreEqual(PublishResultType.SuccessPublishCulture, r[1].Result);
}

Expand Down Expand Up @@ -219,7 +219,7 @@ public void Can_Publish_Variant_Branch_When_No_Changes_On_Root_Specific_Culture(
var saveResult = ContentService.Save(iv1);

var r = ContentService.SaveAndPublishBranch(vRoot, false, "de").ToArray();
Assert.AreEqual(PublishResultType.SuccessPublishAlready, r[0].Result);
Assert.AreEqual(PublishResultType.SuccessPublishCulture, r[0].Result); // During branch publishing, the change detection of the root branch runs AFTER the check to process the branchItem => root is always saved&Published as the intent requires it.
Assert.AreEqual(PublishResultType.SuccessPublishCulture, r[1].Result);
}

Expand Down Expand Up @@ -379,7 +379,7 @@ public void Can_Publish_Mixed_Branch_1()
AssertPublishResults(
r,
x => x.Result,
PublishResultType.SuccessPublishAlready,
PublishResultType.SuccessPublish, // During branch publishing, the change detection of the root branch runs AFTER the check to process the branchItem => root is always saved&Published as the intent requires it.
PublishResultType.SuccessPublish,
PublishResultType.SuccessPublishCulture);

Expand All @@ -405,7 +405,7 @@ public void Can_Publish_MixedBranch_2()
AssertPublishResults(
r,
x => x.Result,
PublishResultType.SuccessPublishAlready,
PublishResultType.SuccessPublish, // During branch publishing, the change detection of the root branch runs AFTER the check to process the branchItem => root is always saved&Published as the intent requires it.
PublishResultType.SuccessPublish,
PublishResultType.SuccessPublishCulture);

Expand Down
Loading