diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs index 39917cabe543..76d6737c59ff 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs @@ -127,5 +127,8 @@ protected virtual void DefinePlan() // To 16.3.0 To("{A917FCBC-C378-4A08-A36C-220C581A6581}"); To("{FB7073AF-DFAF-4AC1-800D-91F9BD5B5238}"); + + // To 16.4.0 + To("{6A7D3B80-8B64-4E41-A7C0-02EC39336E97}"); } } diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabs.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabs.cs new file mode 100644 index 000000000000..a6ebafd07564 --- /dev/null +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabs.cs @@ -0,0 +1,135 @@ +using Microsoft.Extensions.Logging; +using NPoco; +using Umbraco.Cms.Infrastructure.Persistence; +using Umbraco.Cms.Infrastructure.Persistence.Dtos; +using Umbraco.Extensions; + +namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_16_4_0; + +/// +/// Creates missing tabs on content types when a tab is referenced by both a composition and the content type's own groups. +/// +/// +/// In v13, if a tab had groups in both a composition and the content type, the tab might not exist on the content type itself. +/// Newer versions require such tabs to also exist directly on the content type. This migration ensures those tabs are created. +/// +[Obsolete("Remove in Umbraco 18.")] +public class CreateMissingTabs : AsyncMigrationBase +{ + private readonly ILogger _logger; + + /// + /// Initializes a new instance of the class. + /// + public CreateMissingTabs(IMigrationContext context, ILogger logger) + : base(context) => _logger = logger; + + /// + protected override async Task MigrateAsync() + { + await ExecuteMigration(Database, _logger); + Context.Complete(); + } + + /// + /// Performs the migration to create missing tabs. + /// + /// + /// Extracted into an internal static method to support integration testing. + /// + internal static async Task ExecuteMigration(IUmbracoDatabase database, ILogger logger) + { + // 1. Find all property groups (type 0) and extract their tab alias (the part before the first '/'). + // This helps identify which groups are referencing tabs. + Sql groupsSql = database.SqlContext.Sql() + .SelectDistinct("g", pt => pt.ContentTypeNodeId) + .AndSelect(GetTabAliasQuery(database.DatabaseType, "g.alias") + " AS tabAlias") + .From(alias: "p") + .InnerJoin(alias: "g").On( + (pt, ptg) => pt.PropertyTypeGroupId == ptg.Id && pt.ContentTypeId == ptg.ContentTypeNodeId, + aliasLeft: "p", + "g") + .Where(x => x.Type == 0, alias: "g") + .Where(CheckIfContainsTabAliasQuery(database.DatabaseType, "g.alias")); + + // 2. Get all existing tabs (type 1) for all content types. + Sql tabsSql = database.SqlContext.Sql() + .Select("g2", g => g.UniqueId, g => g.ContentTypeNodeId, g => g.Alias) + .From(alias: "g2") + .Where(x => x.Type == 1, alias: "g2"); + + // 3. Identify groups that reference a tab alias which does not exist as a tab for their content type. + // These are the "missing tabs" that need to be created. + Sql missingTabsSql = database.SqlContext.Sql() + .Select("groups", g => g.ContentTypeNodeId) + .AndSelect("groups.tabAlias") + .From() + .AppendSubQuery(groupsSql, "groups") + .LeftJoin(tabsSql, "tabs") + .On("groups.ContentTypeNodeId = tabs.ContentTypeNodeId AND tabs.alias = groups.tabAlias") + .WhereNull(ptg => ptg.UniqueId, "tabs"); + + // 4. For each missing tab, find the corresponding tab details (text, alias, sort order) + // from the parent content type (composition) that already has this tab. + Sql missingTabsWithDetailsSql = database.SqlContext.Sql() + .Select("missingTabs", ptg => ptg.ContentTypeNodeId) + .AndSelect("tg", ptg => ptg.Alias) + .AndSelect("MIN(text) AS text", "MIN(sortorder) AS sortOrder") + .From() + .AppendSubQuery(missingTabsSql, "missingTabs") + .InnerJoin(alias: "ct2ct") + .On( + (ptg, ct2Ct) => ptg.ContentTypeNodeId == ct2Ct.ChildId, + "missingTabs", + "ct2ct") + .InnerJoin(alias: "tg") + .On( + (ct2Ct, ptg) => ct2Ct.ParentId == ptg.ContentTypeNodeId, + "ct2ct", + "tg") + .Append("AND tg.alias = missingTabs.tabAlias") + .GroupBy("missingTabs", ptg => ptg.ContentTypeNodeId) + .AndBy("tg", ptg => ptg.Alias); + + List missingTabsWithDetails = + await database.FetchAsync(missingTabsWithDetailsSql); + + // 5. Create and insert new tab records for each missing tab, using the details from the parent/composition. + IEnumerable newTabs = missingTabsWithDetails + .Select(missingTabWithDetails => new PropertyTypeGroupDto + { + UniqueId = Guid.CreateVersion7(), + ContentTypeNodeId = missingTabWithDetails.ContentTypeNodeId, + Type = 1, + Text = missingTabWithDetails.Text, + Alias = missingTabWithDetails.Alias, + SortOrder = missingTabWithDetails.SortOrder, + }); + await database.InsertBatchAsync(newTabs); + + logger.LogInformation( + "Created {MissingTabCount} tab records to migrate property group information for content types derived from compositions.", + missingTabsWithDetails.Count); + } + + private static string GetTabAliasQuery(DatabaseType databaseType, string columnName) => + databaseType == DatabaseType.SQLite + ? $"substr({columnName}, 1, INSTR({columnName},'/') - 1)" + : $"SUBSTRING({columnName}, 1, CHARINDEX('/', {columnName}) - 1)"; + + private static string CheckIfContainsTabAliasQuery(DatabaseType databaseType, string columnName) => + databaseType == DatabaseType.SQLite + ? $"INSTR({columnName}, '/') > 0" + : $"CHARINDEX('/', {columnName}) > 0"; + + private class MissingTabWithDetails + { + public required int ContentTypeNodeId { get; set; } + + public required string Alias { get; set; } + + public required string Text { get; set; } + + public required int SortOrder { get; set; } + } +} diff --git a/src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs b/src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs index d2c7aa87ffd5..bf2e1eb72377 100644 --- a/src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs +++ b/src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs @@ -393,6 +393,26 @@ public static Sql GroupBy(this Sql sql, params E return sql.GroupBy(columns); } + /// + /// Appends a GROUP BY clause to the Sql statement. + /// + /// The type of the Dto. + /// The Sql statement. + /// A table alias. + /// Expression specifying the fields. + /// The Sql statement. + public static Sql GroupBy( + this Sql sql, + string tableAlias, + params Expression>[] fields) + { + ISqlSyntaxProvider sqlSyntax = sql.SqlContext.SqlSyntax; + var columns = fields.Length == 0 + ? sql.GetColumns(withAlias: false) + : fields.Select(x => sqlSyntax.GetFieldName(x, tableAlias)).ToArray(); + return sql.GroupBy(columns); + } + /// /// Appends more ORDER BY or GROUP BY fields to the Sql statement. /// @@ -548,7 +568,8 @@ public static Sql.SqlJoinClause LeftJoin(this Sql.SqlJoinClause(sql); } /// @@ -801,6 +822,29 @@ public static Sql SelectDistinct(this Sql sql, p return sql; } + /// + /// Creates a SELECT DISTINCT Sql statement. + /// + /// The type of the DTO to select. + /// The origin sql. + /// A table alias. + /// Expressions indicating the columns to select. + /// The Sql statement. + /// + /// If is empty, all columns are selected. + /// + public static Sql SelectDistinct(this Sql sql, string tableAlias, params Expression>[] fields) + { + if (sql == null) + { + throw new ArgumentNullException(nameof(sql)); + } + + var columns = sql.GetColumns(tableAlias: tableAlias, columnExpressions: fields); + sql.Append("SELECT DISTINCT " + string.Join(", ", columns)); + return sql; + } + public static Sql SelectDistinct(this Sql sql, params object[] columns) { sql.Append("SELECT DISTINCT " + string.Join(", ", columns)); diff --git a/tests/Umbraco.Tests.Common/Builders/PropertyGroupBuilder.cs b/tests/Umbraco.Tests.Common/Builders/PropertyGroupBuilder.cs index 5a958f1389a9..39732f4fe8bf 100644 --- a/tests/Umbraco.Tests.Common/Builders/PropertyGroupBuilder.cs +++ b/tests/Umbraco.Tests.Common/Builders/PropertyGroupBuilder.cs @@ -45,6 +45,7 @@ public class PropertyGroupBuilder private int? _sortOrder; private bool? _supportsPublishing; private DateTime? _updateDate; + private PropertyGroupType? _type; public PropertyGroupBuilder(TParent parentBuilder) : base(parentBuilder) @@ -99,6 +100,12 @@ string IWithNameBuilder.Name set => _updateDate = value; } + public PropertyGroupBuilder WithType(PropertyGroupType type) + { + _type = type; + return this; + } + public PropertyGroupBuilder WithPropertyTypeCollection(PropertyTypeCollection propertyTypeCollection) { _propertyTypeCollection = propertyTypeCollection; @@ -122,6 +129,7 @@ public override PropertyGroup Build() var name = _name ?? Guid.NewGuid().ToString(); var sortOrder = _sortOrder ?? 0; var supportsPublishing = _supportsPublishing ?? false; + var type = _type ?? PropertyGroupType.Group; PropertyTypeCollection propertyTypeCollection; if (_propertyTypeCollection != null) @@ -145,7 +153,8 @@ public override PropertyGroup Build() Name = name, SortOrder = sortOrder, CreateDate = createDate, - UpdateDate = updateDate + UpdateDate = updateDate, + Type = type, }; } } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabsTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabsTest.cs new file mode 100644 index 000000000000..7e733eb95500 --- /dev/null +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabsTest.cs @@ -0,0 +1,208 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using Microsoft.Extensions.Logging.Abstractions; +using NPoco; +using NUnit.Framework; +using Umbraco.Cms.Api.Management.ViewModels.DocumentType; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Mapping; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_16_4_0; +using Umbraco.Cms.Infrastructure.Persistence; +using Umbraco.Cms.Infrastructure.Persistence.Dtos; +using Umbraco.Cms.Infrastructure.Scoping; +using Umbraco.Cms.Tests.Common.Builders; +using Umbraco.Cms.Tests.Common.Builders.Extensions; +using Umbraco.Cms.Tests.Integration.TestServerTest; + +namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Migrations.Upgrade.V16_4_0; + +[TestFixture] +internal sealed class CreateMissingTabsTest : UmbracoTestServerTestBase +{ + private IScopeProvider ScopeProvider => GetRequiredService(); + + private IContentTypeService ContentTypeService => GetRequiredService(); + + private IUmbracoMapper UmbracoMapper => GetRequiredService(); + + /// + /// A verification integration test for the solution to https://github.com/umbraco/Umbraco-CMS/issues/20058 + /// provided in https://github.com/umbraco/Umbraco-CMS/pull/20303. + /// + [Test] + public async Task Can_Create_Missing_Tabs() + { + // Prepare a base and composed content type. + (IContentType baseContentType, IContentType composedContentType) = await PrepareTestData(); + + // Assert the groups and properties are created in the database and that the content type model is as expected. + await AssertValidDbGroupsAndProperties(baseContentType.Id, composedContentType.Id); + await AssertValidContentTypeModel(composedContentType.Key); + + // Prepare the database state as it would have been in Umbraco 13. + await PreparePropertyGroupPersistedStateForUmbraco13(composedContentType); + + // Assert that the content type groups are now without a parent tab. + await AssertInvalidContentTypeModel(composedContentType.Key); + + // Run the migration to add the missing tab back. + await ExecuteMigration(); + + // Re-retrieve the content types and assert that the groups and types are as expected. + await AssertValidContentTypeModel(composedContentType.Key); + + // Verify in the database that the migration has re-added only the record we removed in the setup. + await AssertValidDbGroupsAndProperties(baseContentType.Id, composedContentType.Id); + } + + private async Task<(IContentType BaseContentType, IContentType ComposedContentType)> PrepareTestData() + { + // Prepare document types as per reproduction steps described here: https://github.com/umbraco/Umbraco-CMS/issues/20058#issuecomment-3332742559 + // - Create a new composition with a tab "Content" and inside add a group "Header" with a "Text 1" property inside. + // - Save the composition. + // - Create a new document type and inherit the composition created in step 2. + // - Add a new property "Text 2" to the Content > Header group. + // - Create a new group "Home Content", inside the "Content" tab, and add a property "Text 3". + // - Save the document type. + + // Create base content type. + var baseContentType = new ContentTypeBuilder() + .WithAlias("baseType") + .WithName("Base Type") + .AddPropertyGroup() + .WithAlias("content") + .WithName("Content") + .WithType(PropertyGroupType.Tab) + .Done() + .AddPropertyGroup() + .WithAlias("content/header") + .WithName("Header") + .WithType(PropertyGroupType.Group) + .AddPropertyType() + .WithAlias("text1") + .WithName("Text 1") + .Done() + .Done() + .Build(); + await ContentTypeService.CreateAsync(baseContentType, Constants.Security.SuperUserKey); + baseContentType = await ContentTypeService.GetAsync(baseContentType.Key); + + // Create composed content type. + var composedContentType = new ContentTypeBuilder() + .WithAlias("composedType") + .WithName("Composed Type") + .AddPropertyGroup() + .WithAlias("content") + .WithName("Content") + .WithType(PropertyGroupType.Tab) + .Done() + .AddPropertyGroup() + .WithAlias("content/header") + .WithName("Header") + .WithType(PropertyGroupType.Group) + .AddPropertyType() + .WithAlias("text2") + .WithName("Text 2") + .Done() + .Done() + .AddPropertyGroup() + .WithAlias("content/homeContent") + .WithName("Home Content") + .WithType(PropertyGroupType.Group) + .AddPropertyType() + .WithAlias("text3") + .WithName("Text 3") + .Done() + .Done() + .Build(); + composedContentType.ContentTypeComposition = [baseContentType]; + await ContentTypeService.CreateAsync(composedContentType, Constants.Security.SuperUserKey); + composedContentType = await ContentTypeService.GetAsync(composedContentType.Key); + return (baseContentType, composedContentType); + } + + private async Task AssertValidDbGroupsAndProperties(int baseContentTypeId, int composedContentTypeId) + { + using IScope scope = ScopeProvider.CreateScope(); + Sql groupsSql = scope.Database.SqlContext.Sql() + .Select() + .From() + .WhereIn(x => x.ContentTypeNodeId, new[] { baseContentTypeId, composedContentTypeId }); + var groups = await scope.Database.FetchAsync(groupsSql); + Assert.AreEqual(5, groups.Count); + + Assert.AreEqual(1, groups.Count(x => x.ContentTypeNodeId == baseContentTypeId && x.Type == (int)PropertyGroupType.Tab)); + Assert.AreEqual(1, groups.Count(x => x.ContentTypeNodeId == baseContentTypeId && x.Type == (int)PropertyGroupType.Group)); + + Assert.AreEqual(1, groups.Count(x => x.ContentTypeNodeId == composedContentTypeId && x.Type == (int)PropertyGroupType.Tab)); + Assert.AreEqual(2, groups.Count(x => x.ContentTypeNodeId == composedContentTypeId && x.Type == (int)PropertyGroupType.Group)); + + Sql propertiesSql = scope.Database.SqlContext.Sql() + .Select() + .From() + .WhereIn(x => x.ContentTypeId, new[] { baseContentTypeId, composedContentTypeId }); + var types = await scope.Database.FetchAsync(propertiesSql); + Assert.AreEqual(3, types.Count); + scope.Complete(); + } + + private async Task AssertValidContentTypeModel(Guid contentTypeKey) + { + var contentType = await ContentTypeService.GetAsync(contentTypeKey); + DocumentTypeResponseModel model = UmbracoMapper.Map(contentType)!; + Assert.AreEqual(3, model.Containers.Count()); + + var contentTab = model.Containers.FirstOrDefault(c => c.Name == "Content" && c.Type == nameof(PropertyGroupType.Tab)); + Assert.IsNotNull(contentTab); + + var headerGroup = model.Containers.FirstOrDefault(c => c.Name == "Header" && c.Type == nameof(PropertyGroupType.Group)); + Assert.IsNotNull(headerGroup); + Assert.IsNotNull(headerGroup.Parent); + Assert.AreEqual(contentTab.Id, headerGroup.Parent.Id); + + var homeContentGroup = model.Containers.FirstOrDefault(c => c.Name == "Home Content" && c.Type == nameof(PropertyGroupType.Group)); + Assert.IsNotNull(homeContentGroup); + Assert.IsNotNull(homeContentGroup.Parent); + Assert.AreEqual(contentTab.Id, homeContentGroup.Parent.Id); + } + + private async Task PreparePropertyGroupPersistedStateForUmbraco13(IContentType composedContentType) + { + // Delete one of the tab records so we get to the 13 state. + using IScope scope = ScopeProvider.CreateScope(); + Sql deleteTabSql = scope.Database.SqlContext.Sql() + .Delete() + .Where(x => x.Type == (int)PropertyGroupType.Tab && x.ContentTypeNodeId == composedContentType.Id); + var deletedCount = await scope.Database.ExecuteAsync(deleteTabSql); + scope.Complete(); + Assert.AreEqual(1, deletedCount); + } + + private async Task AssertInvalidContentTypeModel(Guid contentTypeKey) + { + var contentType = await ContentTypeService.GetAsync(contentTypeKey); + DocumentTypeResponseModel model = UmbracoMapper.Map(contentType)!; + Assert.AreEqual(2, model.Containers.Count()); + + var contentTab = model.Containers.FirstOrDefault(c => c.Name == "Content" && c.Type == nameof(PropertyGroupType.Tab)); + Assert.IsNull(contentTab); + + var headerGroup = model.Containers.FirstOrDefault(c => c.Name == "Header" && c.Type == nameof(PropertyGroupType.Group)); + Assert.IsNotNull(headerGroup); + Assert.IsNull(headerGroup.Parent); + + var homeContentGroup = model.Containers.FirstOrDefault(c => c.Name == "Home Content" && c.Type == nameof(PropertyGroupType.Group)); + Assert.IsNotNull(homeContentGroup); + Assert.IsNull(homeContentGroup.Parent); + } + + private async Task ExecuteMigration() + { + using IScope scope = ScopeProvider.CreateScope(); + await CreateMissingTabs.ExecuteMigration(scope.Database, new NullLogger()); + scope.Complete(); + } +}