From b9ff353c265a4021c49839f95c90eafa6cd1f166 Mon Sep 17 00:00:00 2001 From: Duncan Watson Date: Tue, 7 Nov 2023 12:23:51 +0000 Subject: [PATCH] EES-4654 - stopping Data Blocks from being copied when creating a Release from a Release Template. Handled part of EES-4639, separating Release Template creation out from ReleaseAmendmentExtensions. --- .../Services/ReleaseServiceTests.cs | 90 +++++++++++-------- .../Services/ReleaseService.cs | 57 ++++++------ .../ContentSection.cs | 21 ++++- .../Extensions/ReleaseAmendmentExtensions.cs | 19 ---- .../Release.cs | 10 +-- 5 files changed, 99 insertions(+), 98 deletions(-) diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseServiceTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseServiceTests.cs index c60e7dd7f38..639f4e605bb 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseServiceTests.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseServiceTests.cs @@ -203,11 +203,13 @@ await context.AddAsync( await context.SaveChangesAsync(); } + Guid newReleaseId = Guid.Empty; + await using (var context = InMemoryApplicationDbContext(contextId)) { var releaseService = BuildReleaseService(context); - var result = releaseService.CreateRelease( + var result = await releaseService.CreateRelease( new ReleaseCreateRequest { PublicationId = new Guid("403d3c5d-a8cd-4d54-a029-0c74c86c55b2"), @@ -218,12 +220,18 @@ await context.AddAsync( } ); + var newRelease = result.AssertRight(); + newReleaseId = newRelease.Id; + } + + await using (var context = InMemoryApplicationDbContext(contextId)) + { // Do an in depth check of the saved release var newRelease = context .Releases .Include(release => release.Content) .ThenInclude(section => section.Content) - .Single(r => r.Id == result.Result.Right.Id); + .Single(r => r.Id == newReleaseId); var contentSections = newRelease.GenericContent.ToList(); Assert.Single(contentSections); @@ -232,12 +240,18 @@ await context.AddAsync( Assert.Single(contentSections); Assert.Equal(1, contentSections[0].Order); - // Content should not be copied when create from template + // Content should not be copied when created from a template. Assert.Empty(contentSections[0].Content); Assert.Empty(contentSections[0].Content.AsReadOnly()); Assert.Equal(ContentSectionType.ReleaseSummary, newRelease.SummarySection.Type); Assert.Equal(ContentSectionType.Headlines, newRelease.HeadlinesSection.Type); Assert.Equal(ContentSectionType.KeyStatisticsSecondary, newRelease.KeyStatisticsSecondarySection.Type); + + // Data Blocks should not be copied when created from a template. + Assert.Equal(2, context.DataBlocks.Count()); + Assert.Empty(context + .DataBlocks + .Where(dataBlock => dataBlock.ReleaseId == newReleaseId)); } } @@ -742,13 +756,13 @@ public async Task GetRelease() release } }; - + var nonPreReleaseUserRole = new UserReleaseRole { Role = ReleaseRole.Contributor, Release = release }; - + var nonPreReleaseUserInvite = new UserReleaseInvite { Role = ReleaseRole.Contributor, @@ -821,7 +835,7 @@ public async Task GetRelease_WithPreReleaseUsers() Role = ReleaseRole.PrereleaseViewer, Release = release }; - + var contextId = Guid.NewGuid().ToString(); await using (var context = InMemoryApplicationDbContext(contextId)) @@ -864,7 +878,7 @@ public async Task GetRelease_WithPreReleaseInvites() Role = ReleaseRole.PrereleaseViewer, Release = release }; - + var contextId = Guid.NewGuid().ToString(); await using (var context = InMemoryApplicationDbContext(contextId)) @@ -1616,21 +1630,21 @@ public async Task UpdateReleasePublished_ConvertsPublishedFromLocalToUniversalTi public class ListUsersReleasesForApproval { private readonly DataFixture _fixture = new(); - + [Fact] public async Task UserHasApproverRoleOnRelease() { var contextId = Guid.NewGuid().ToString(); var otherUser = new User(); - + var publications = _fixture .DefaultPublication() .WithReleases(_ => _fixture .DefaultRelease() .WithApprovalStatuses(ListOf( - ReleaseApprovalStatus.Draft, - ReleaseApprovalStatus.HigherLevelReview, + ReleaseApprovalStatus.Draft, + ReleaseApprovalStatus.HigherLevelReview, ReleaseApprovalStatus.Approved)) .GenerateList()) .GenerateList(4); @@ -1641,30 +1655,30 @@ public async Task UserHasApproverRoleOnRelease() .WithRole(ReleaseRole.Contributor) .WithReleases(publications[0].Releases) .GenerateList(); - + var approverReleaseRolesForUser = _fixture .DefaultUserReleaseRole() .WithUser(User) .WithRole(ReleaseRole.Approver) .WithReleases(publications[1].Releases) .GenerateList(); - + var prereleaseReleaseRolesForUser = _fixture .DefaultUserReleaseRole() .WithUser(User) .WithRole(ReleaseRole.PrereleaseViewer) .WithReleases(publications[2].Releases) .GenerateList(); - + var approverReleaseRolesForOtherUser = _fixture .DefaultUserReleaseRole() .WithUser(otherUser) .WithRole(ReleaseRole.Approver) .WithReleases(publications.SelectMany(publication => publication.Releases)) .GenerateList(); - + var higherReviewReleaseWithApproverRoleForUser = publications[1].Releases[1]; - + await using (var context = InMemoryApplicationDbContext(contextId)) { await context.Publications.AddRangeAsync(publications); @@ -1674,7 +1688,7 @@ public async Task UserHasApproverRoleOnRelease() await context.UserReleaseRoles.AddRangeAsync(approverReleaseRolesForOtherUser); await context.SaveChangesAsync(); } - + await using (var context = InMemoryApplicationDbContext(contextId)) { var service = BuildReleaseService(context); @@ -1682,35 +1696,35 @@ public async Task UserHasApproverRoleOnRelease() var result = await service.ListUsersReleasesForApproval(); var viewModels = result.AssertRight(); - + // Assert that the only Release returned for this user is the Release where they have a direct // Approver role on and it is in Higher Review. var viewModel = Assert.Single(viewModels); Assert.Equal(higherReviewReleaseWithApproverRoleForUser.Id, viewModel.Id); - + // Assert that we have a fully populated ReleaseSummaryViewModel, including details from the owning // Publication. Assert.Equal( - higherReviewReleaseWithApproverRoleForUser.Publication.Title, + higherReviewReleaseWithApproverRoleForUser.Publication.Title, viewModel.Publication!.Title); } } - + [Fact] public async Task UserHasApproverRoleOnPublications() { var contextId = Guid.NewGuid().ToString(); var otherUser = new User(); - + var publications = _fixture .DefaultPublication() .WithReleases(_ => _fixture .DefaultRelease() .WithApprovalStatuses(ListOf( - ReleaseApprovalStatus.Draft, - ReleaseApprovalStatus.HigherLevelReview, - ReleaseApprovalStatus.Approved, + ReleaseApprovalStatus.Draft, + ReleaseApprovalStatus.HigherLevelReview, + ReleaseApprovalStatus.Approved, ReleaseApprovalStatus.HigherLevelReview)) .GenerateList()) .GenerateList(3); @@ -1721,42 +1735,42 @@ public async Task UserHasApproverRoleOnPublications() .WithRole(PublicationRole.Owner) .WithPublication(publications[0]) .Generate(); - + var approverPublicationRoleForUser = _fixture .DefaultUserPublicationRole() .WithUser(User) .WithRole(PublicationRole.Approver) .WithPublication(publications[1]) .Generate(); - + var ownerPublicationRolesForOtherUser = _fixture .DefaultUserPublicationRole() .WithUser(otherUser) .WithRole(PublicationRole.Owner) .WithPublications(publications) .GenerateList(); - + var approverPublicationRolesForOtherUser = _fixture .DefaultUserPublicationRole() .WithUser(otherUser) .WithRole(PublicationRole.Approver) .WithPublications(publications) .GenerateList(); - + var release1WithApproverRoleForUser = publications[1].Releases[1]; var release2WithApproverRoleForUser = publications[1].Releases[3]; - + await using (var context = InMemoryApplicationDbContext(contextId)) { await context.Publications.AddRangeAsync(publications); await context.UserPublicationRoles.AddRangeAsync( - ownerPublicationRoleForUser, + ownerPublicationRoleForUser, approverPublicationRoleForUser); await context.UserPublicationRoles.AddRangeAsync(ownerPublicationRolesForOtherUser); await context.UserPublicationRoles.AddRangeAsync(approverPublicationRolesForOtherUser); await context.SaveChangesAsync(); } - + await using (var context = InMemoryApplicationDbContext(contextId)) { var service = BuildReleaseService(context); @@ -1764,7 +1778,7 @@ await context.UserPublicationRoles.AddRangeAsync( var result = await service.ListUsersReleasesForApproval(); var viewModels = result.AssertRight(); - + // Assert that the only Releases returned for this user are the Releases where they have Approver // role on the overarching Publication and the Releases are in Higher Review. Assert.Equal(2, viewModels.Count); @@ -1772,7 +1786,7 @@ await context.UserPublicationRoles.AddRangeAsync( Assert.Equal(release2WithApproverRoleForUser.Id, viewModels[1].Id); } } - + [Fact] public async Task UserIsPublicationAndReleaseApprover_NoDuplication() { @@ -1785,7 +1799,7 @@ public async Task UserIsPublicationAndReleaseApprover_NoDuplication() .WithApprovalStatus(ReleaseApprovalStatus.HigherLevelReview) .Generate(1)) .Generate(); - + var approverReleaseRolesForUser = _fixture .DefaultUserReleaseRole() .WithUser(User) @@ -1799,7 +1813,7 @@ public async Task UserIsPublicationAndReleaseApprover_NoDuplication() .WithRole(PublicationRole.Approver) .WithPublication(publication) .Generate(); - + await using (var context = InMemoryApplicationDbContext(contextId)) { await context.Publications.AddRangeAsync(publication); @@ -1807,7 +1821,7 @@ public async Task UserIsPublicationAndReleaseApprover_NoDuplication() await context.UserPublicationRoles.AddRangeAsync(approverPublicationRoleForUser); await context.SaveChangesAsync(); } - + await using (var context = InMemoryApplicationDbContext(contextId)) { var service = BuildReleaseService(context); @@ -1815,7 +1829,7 @@ public async Task UserIsPublicationAndReleaseApprover_NoDuplication() var result = await service.ListUsersReleasesForApproval(); var viewModels = result.AssertRight(); - + // Assert that the Release only appears once despite the user having approval directly via the // Release itself AND via the overarching Publication. Assert.Single(viewModels); diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs index 502469efd0c..82df2805f29 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs @@ -110,11 +110,11 @@ public async Task> GetRelease(Guid id) { PreReleaseUsersOrInvitesAdded = _context .UserReleaseRoles - .Any(role => role.ReleaseId == id + .Any(role => role.ReleaseId == id && role.Role == ReleaseRole.PrereleaseViewer) || _context .UserReleaseInvites - .Any(role => role.ReleaseId == id + .Any(role => role.ReleaseId == id && role.Role == ReleaseRole.PrereleaseViewer) }); } @@ -140,10 +140,7 @@ public async Task> CreateRelease(ReleaseC if (releaseCreate.TemplateReleaseId.HasValue) { - var context = new Release.CloneContext(newRelease); - await CreateGenericContentFromTemplate(releaseCreate.TemplateReleaseId.Value, context); - var copiedDataBlocks = await CopyDataBlocksFromTemplateRelease(releaseCreate.TemplateReleaseId.Value, context); - await _context.ContentBlocks.AddRangeAsync(copiedDataBlocks); + await CreateGenericContentFromTemplate(releaseCreate.TemplateReleaseId.Value, newRelease); } else { @@ -296,7 +293,7 @@ private async Task> CopyReleaseRoles(Guid original // manually removed from a Release as opposed to SoftDeleted, which is only set when a Release is // deleted) .IgnoreQueryFilters() - .Where(releaseRole => releaseRole.ReleaseId == originalReleaseId + .Where(releaseRole => releaseRole.ReleaseId == originalReleaseId && releaseRole.Role != ReleaseRole.PrereleaseViewer) .Select(releaseRole => releaseRole.CopyForAmendment(amendment)) .ToList(); @@ -313,7 +310,7 @@ private async Task> CreateBasicReleaseAmendment(Re .Where(block => block.ReleaseId == release.Id) .OfType() .ToListAsync(); - + var (amendment, amendedDataBlocks) = release.CreateAmendment(dataBlocks, DateTime.UtcNow, _userService.GetUserId()); await _context.Releases.AddAsync(amendment); await _context.ContentBlocks.AddRangeAsync(amendedDataBlocks); @@ -462,7 +459,7 @@ public async Task>> ListUsers .Where(role => role.UserId == userId && role.Role == ReleaseRole.Approver) .Select(role => role.ReleaseId) .ToListAsync(); - + var indirectReleasesWithApprovalRole = await _context .UserPublicationRoles .Where(role => role.UserId == userId && role.Role == PublicationRole.Approver) @@ -476,7 +473,7 @@ public async Task>> ListUsers var releasesForApproval = await _context .Releases .Include(release => release.Publication) - .Where(release => + .Where(release => release.ApprovalStatus == ReleaseApprovalStatus.HigherLevelReview && releaseIdsForApproval.Contains(release.Id)) .ToListAsync(); @@ -544,7 +541,7 @@ private static bool IsLatestVersionOfRelease(IEnumerable releases, Guid return !releases.Any(r => r.PreviousVersionId == releaseId && r.Id != releaseId); } - private async Task CreateGenericContentFromTemplate(Guid templateReleaseId, Release.CloneContext context) + private async Task CreateGenericContentFromTemplate(Guid templateReleaseId, Release newRelease) { var templateRelease = await _context .Releases @@ -552,27 +549,31 @@ private async Task CreateGenericContentFromTemplate(Guid templateReleaseId, Rele .Include(release => release.Content) .FirstAsync(r => r.Id == templateReleaseId); - templateRelease.CreateGenericContentFromTemplate(context); + newRelease.Content = templateRelease + .Content + .Where(section => section.Type == ContentSectionType.Generic) + .Select(section => CloneContentSectionFromReleaseTemplate(section, newRelease)) + .ToList(); } - // TODO EES-4467 - this can be incorporated back into Release as the newly fashioned - // DataBlock / DataBlockVersions tables when tackling EES-4467. For the time being though, - // this will need to be done separately from the cloning of the main Release entity. - private async Task> CopyDataBlocksFromTemplateRelease( - Guid templateReleaseId, - Release.CloneContext context) + private ContentSection CloneContentSectionFromReleaseTemplate( + ContentSection originalSection, + Release newRelease) { - var templateDataBlocks = await _context - .ContentBlocks - .AsNoTracking() - .Where(block => block.ReleaseId == templateReleaseId) - .OfType() - .ToListAsync(); + // Create a like-for-like copy of the template ContentSection. + var copy = originalSection.MemberwiseClone(); - return templateDataBlocks - .Select(dataBlock => dataBlock.Clone(context)) - .Cast() - .ToList(); + // Assign a new Id for the new ContentSection. + copy.Id = Guid.NewGuid(); + + // Assign the new ContentSection to the new Release. + copy.Release = newRelease; + copy.ReleaseId = newRelease.Id; + + // Do not copy over any existing Content from the original template ContentSection. + copy.Content = new List(); + + return copy; } public async Task> GetDeleteDataFilePlan(Guid releaseId, Guid fileId) diff --git a/src/GovUk.Education.ExploreEducationStatistics.Content.Model/ContentSection.cs b/src/GovUk.Education.ExploreEducationStatistics.Content.Model/ContentSection.cs index d1cb4a28772..d66f9c0afc2 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Content.Model/ContentSection.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Content.Model/ContentSection.cs @@ -27,14 +27,16 @@ public class ContentSection public List Content { get; set; } = new(); public Release Release { get; set; } - + public Guid ReleaseId { get; set; } - + [JsonIgnore] public ContentSectionType Type { get; set; } + // TODO EES-4639 - rewrite Release Amendment generation code to be localised to + // ReleaseAmendmentExtensions.CreateAmendment() or ReleaseService.CreateBasicReleaseAmendment(). public ContentSection Clone(Release.CloneContext context) { - var copy = MemberwiseClone() as ContentSection; + var copy = MemberwiseClone(); copy.Id = Guid.NewGuid(); copy.Release = context.NewRelease; @@ -48,9 +50,11 @@ public ContentSection Clone(Release.CloneContext context) return copy; } + // TODO EES-4639 - rewrite Methodology Amendment generation code to be localised to + // MethodologyVersion.CreateMethodologyAmendment() or MethodologyAmendmentService.CreateAndSaveAmendment(). public ContentSection Clone(DateTime createdDate) { - var copy = MemberwiseClone() as ContentSection; + var copy = MemberwiseClone(); copy.Id = Guid.NewGuid(); copy.Content = copy @@ -61,5 +65,14 @@ public ContentSection Clone(DateTime createdDate) copy.Content?.ForEach(c => c.ContentSectionId = copy.Id); return copy; } + + // TODO EES-4639 - adopt straight MemberwiseClone() usage during the generating of Release Amendments, + // Methodology Amendments and Releases from templates, then have each call site tailor the resulting + // cloned Content tree to their individual requirements, rather than support various Clone() methods in + // Release Content entities themselves. + public new ContentSection MemberwiseClone() + { + return base.MemberwiseClone() as ContentSection; + } } } diff --git a/src/GovUk.Education.ExploreEducationStatistics.Content.Model/Extensions/ReleaseAmendmentExtensions.cs b/src/GovUk.Education.ExploreEducationStatistics.Content.Model/Extensions/ReleaseAmendmentExtensions.cs index 3fc2ae953e3..4bfc361a8df 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Content.Model/Extensions/ReleaseAmendmentExtensions.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Content.Model/Extensions/ReleaseAmendmentExtensions.cs @@ -185,24 +185,5 @@ private static string ReplaceContent( } ); } - - public static void CreateGenericContentFromTemplate( - this Release originalRelease, - Release.CloneContext context) - { - var newRelease = context.NewRelease; - - newRelease.Content = originalRelease.Content - .Where(section => section.Type == ContentSectionType.Generic) - .ToList(); - - newRelease.Content = originalRelease - .Content - .Select(section => section.Clone(context)) - .ToList(); - - // TODO EES-4467 - we can add the cloning of Data Blocks back in here rather than being doing separately - // when the new DataBlock / DataBlockVersion tables are added. - } } } \ No newline at end of file diff --git a/src/GovUk.Education.ExploreEducationStatistics.Content.Model/Release.cs b/src/GovUk.Education.ExploreEducationStatistics.Content.Model/Release.cs index eee90bde572..812d2be8c52 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Content.Model/Release.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Content.Model/Release.cs @@ -200,21 +200,13 @@ public Release Clone() return MemberwiseClone() as Release; } - public record CloneContext + public record CloneContext(Release NewRelease) { // Maps old content block references to new content blocks // Ideally we want to try and get rid of this completely as we // shouldn't have to deal with the same content blocks being // referenced in multiple places. - // TODO: EES-1306 may be possible to remove this as part of this ticket public Dictionary OriginalToAmendmentContentBlockMap { get; } = new(); - - public Release NewRelease { get; } - - public CloneContext(Release newRelease) - { - NewRelease = newRelease; - } } } }