From a1906800e23e3ae00dbf4394537c88224dc74836 Mon Sep 17 00:00:00 2001 From: Duncan Watson Date: Thu, 16 Nov 2023 22:00:13 +0000 Subject: [PATCH 1/2] EES-4680 - moved SendPreReleaseUserInviteEmails() from public method in PreReleaseUserService into private ReleaseApprovalService method. Made successful approval (or other approval changes) reliant on email sending and Publisher notification success, otherwise prevent approval from going through. Added rollback in the event of a failed Approval. --- .../PreReleaseUserServicePermissionTests.cs | 23 +- .../Services/PreReleaseUserServiceTests.cs | 95 +--- .../ReleaseApprovalServicePermissionTests.cs | 120 +++-- .../Services/ReleaseApprovalServiceTests.cs | 481 +++++++++++++++++- .../ReleaseChecklistServicePermissionTests.cs | 39 +- .../Services/ReleaseChecklistServiceTests.cs | 2 - .../Services/ReleaseServicePermissionTests.cs | 4 +- .../Security/SecurityPolicies.cs | 3 +- .../Security/StartupSecurityConfiguration.cs | 5 +- .../Interfaces/IPreReleaseUserService.cs | 8 +- .../Security/UserServiceExtensions.cs | 8 +- .../Services/PreReleaseUserService.cs | 45 +- .../Services/ReleaseApprovalService.cs | 205 +++++--- .../Services/ReleaseChecklistService.cs | 25 +- .../Services/ReleaseService.cs | 7 +- .../Model/EitherTest.cs | 30 ++ .../Model/Either.cs | 16 + .../SubjectMetaService.cs | 16 +- .../robot-tests/tests/libs/admin-utilities.py | 4 +- 19 files changed, 793 insertions(+), 343 deletions(-) diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/PreReleaseUserServicePermissionTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/PreReleaseUserServicePermissionTests.cs index 0fe7374dc7f..54b2031dc98 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/PreReleaseUserServicePermissionTests.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/PreReleaseUserServicePermissionTests.cs @@ -33,7 +33,7 @@ public class PreReleaseUserServicePermissionTests public async Task GetPreReleaseUsers() { await PolicyCheckBuilder() - .SetupResourceCheckToFail(_release, CanAssignPrereleaseContactsToSpecificRelease) + .SetupResourceCheckToFail(_release, CanAssignPreReleaseUsersToSpecificRelease) .AssertForbidden( userService => { @@ -48,7 +48,7 @@ await PolicyCheckBuilder() public async Task GetPreReleaseUsersInvitePlan() { await PolicyCheckBuilder() - .SetupResourceCheckToFail(_release, CanAssignPrereleaseContactsToSpecificRelease) + .SetupResourceCheckToFail(_release, CanAssignPreReleaseUsersToSpecificRelease) .AssertForbidden( userService => { @@ -66,7 +66,7 @@ await PolicyCheckBuilder() public async Task InvitePreReleaseUsers() { await PolicyCheckBuilder() - .SetupResourceCheckToFail(_release, CanAssignPrereleaseContactsToSpecificRelease) + .SetupResourceCheckToFail(_release, CanAssignPreReleaseUsersToSpecificRelease) .AssertForbidden( userService => { @@ -84,7 +84,7 @@ await PolicyCheckBuilder() public async Task RemovePreReleaseUser() { await PolicyCheckBuilder() - .SetupResourceCheckToFail(_release, CanAssignPrereleaseContactsToSpecificRelease) + .SetupResourceCheckToFail(_release, CanAssignPreReleaseUsersToSpecificRelease) .AssertForbidden( userService => { @@ -95,21 +95,6 @@ await PolicyCheckBuilder() ); } - [Fact] - public async Task SendPreReleaseUserInviteEmails() - { - await PolicyCheckBuilder() - .SetupResourceCheckToFail(_release, CanUpdateSpecificRelease) - .AssertForbidden( - userService => - { - var service = SetupPreReleaseUserService( - userService: userService.Object); - return service.SendPreReleaseUserInviteEmails(_release.Id); - } - ); - } - private Mock> DefaultPersistenceHelperMock() { return MockPersistenceHelper(_release.Id, _release); diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/PreReleaseUserServiceTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/PreReleaseUserServiceTests.cs index f43876258b3..9dec3cd6abc 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/PreReleaseUserServiceTests.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/PreReleaseUserServiceTests.cs @@ -699,7 +699,7 @@ public async Task InvitePreReleaseUsers_FailsSendingEmail_ExistingUser_ApprovedR await using (var context = InMemoryApplicationDbContext(contextId)) { - Assert.Empty(context.UserReleaseRoles); + Assert.Empty(context.UserReleaseRoles); Assert.Empty(context.UserReleaseInvites); } } @@ -1739,99 +1739,6 @@ await userAndRolesDbContext.AddAsync( } } - [Fact] - public async Task SendPreReleaseUserInviteEmails() - { - var release = new Release - { - ReleaseName = "2020", - TimePeriodCoverage = TimeIdentifier.CalendarYear, - Publication = new Publication {Title = "Test publication"}, - PublishScheduled = PublishedScheduledStartOfDay - }; - var contextId = Guid.NewGuid().ToString(); - - await using (var context = InMemoryApplicationDbContext(contextId)) - { - await context.AddRangeAsync( - release, - new User - { - Email = "test@test.com", - }, - new UserReleaseInvite - { - Release = release, - Role = ReleaseRole.PrereleaseViewer, - Email = "test@test.com", - EmailSent = false, - }, - new User - { - Email = "test2@test.com" - }, - new UserReleaseInvite - { - Release = release, - Role = ReleaseRole.PrereleaseViewer, - Email = "test2@test.com", - EmailSent = true, - } - ); - - await context.SaveChangesAsync(); - } - - var emailService = new Mock(MockBehavior.Strict); - - var expectedTemplateValues = GetExpectedPreReleaseTemplateValues(release, newUser: false); - - emailService.Setup(mock => mock.SendEmail( - "test@test.com", - PreReleaseTemplateId, - expectedTemplateValues - )) - .Returns(Unit.Instance); - - var preReleaseService = new Mock(MockBehavior.Strict); - SetupGetPrereleaseWindow(preReleaseService, release); - - await using (var context = InMemoryApplicationDbContext(contextId)) - await using (var userAndRolesDbContext = InMemoryUserAndRolesDbContext(contextId)) - { - var service = SetupPreReleaseUserService( - context, - usersAndRolesDbContext: userAndRolesDbContext, - preReleaseService: preReleaseService.Object, - emailService: emailService.Object - ); - - var result = await service.SendPreReleaseUserInviteEmails(release.Id); - - emailService.Verify( - s => s.SendEmail( - "test@test.com", - PreReleaseTemplateId, - expectedTemplateValues - ), Times.Once - ); - - VerifyAllMocks(emailService, preReleaseService); - - result.AssertRight(); - } - - await using (var context = InMemoryApplicationDbContext(contextId)) - { - var updatedInvite = context.UserReleaseInvites - .Single(i => i.Email == "test@test.com"); - - Assert.Equal(release.Id, updatedInvite.ReleaseId); - Assert.Equal(ReleaseRole.PrereleaseViewer, updatedInvite.Role); - Assert.True(updatedInvite.EmailSent); - } - } - private static Dictionary GetExpectedPreReleaseTemplateValues(Release release, bool newUser) { return new() diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServicePermissionTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServicePermissionTests.cs index d65603b9dc8..3eab6c5ba36 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServicePermissionTests.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServicePermissionTests.cs @@ -9,7 +9,6 @@ using GovUk.Education.ExploreEducationStatistics.Common.Model; using GovUk.Education.ExploreEducationStatistics.Common.Services; using GovUk.Education.ExploreEducationStatistics.Common.Services.Interfaces.Security; -using GovUk.Education.ExploreEducationStatistics.Common.Utils; using GovUk.Education.ExploreEducationStatistics.Content.Model; using GovUk.Education.ExploreEducationStatistics.Content.Model.Database; using GovUk.Education.ExploreEducationStatistics.Content.Model.Repository.Interfaces; @@ -17,7 +16,7 @@ using Moq; using Xunit; using static GovUk.Education.ExploreEducationStatistics.Admin.Security.SecurityPolicies; -using static GovUk.Education.ExploreEducationStatistics.Common.Tests.Utils.MockUtils; +using static GovUk.Education.ExploreEducationStatistics.Admin.Tests.Services.DbUtils; using static GovUk.Education.ExploreEducationStatistics.Common.Tests.Utils.PermissionTestUtils; using IReleaseRepository = GovUk.Education.ExploreEducationStatistics.Admin.Services.Interfaces.IReleaseRepository; @@ -25,15 +24,10 @@ namespace GovUk.Education.ExploreEducationStatistics.Admin.Tests.Services { public class ReleaseApprovalServicePermissionTests { - private static readonly Publication Publication = new() - { - Id = Guid.NewGuid() - }; - private readonly Release _release = new() { Id = Guid.NewGuid(), - PublicationId = Publication.Id, + Publication= new Publication(), Published = DateTime.Now, TimePeriodCoverage = TimeIdentifier.April }; @@ -42,26 +36,43 @@ public class ReleaseApprovalServicePermissionTests public async Task GetReleaseStatuses() { await PolicyCheckBuilder() - .SetupResourceCheckToFail(_release, CanViewReleaseStatusHistory) - .AssertForbidden( - userService => + .SetupResourceCheckToFailWithMatcher(r => r.Id == _release.Id, CanViewReleaseStatusHistory) + .AssertForbidden(async userService => + { + var contentDbContextId = Guid.NewGuid().ToString(); + await using (var contentDbContext = InMemoryApplicationDbContext(contentDbContextId)) { - var service = BuildService(userService.Object); - return service.GetReleaseStatuses(_release.Id); + await contentDbContext.AddRangeAsync(_release); + await contentDbContext.SaveChangesAsync(); } - ); + + await using (var contentDbContext = InMemoryApplicationDbContext(contentDbContextId)) + { + var service = BuildService(contentDbContext: contentDbContext, userService.Object); + return await service.GetReleaseStatuses(_release.Id); + } + }); } [Fact] public async Task UpdateReleaseStatus_Draft() { await PolicyCheckBuilder() - .SetupResourceCheckToFail(_release, CanMarkSpecificReleaseAsDraft) - .AssertForbidden( - userService => + .SetupResourceCheckToFailWithMatcher(r => r.Id == _release.Id, CanMarkSpecificReleaseAsDraft) + .AssertForbidden(async userService => + { + var contentDbContextId = Guid.NewGuid().ToString(); + await using (var contentDbContext = InMemoryApplicationDbContext(contentDbContextId)) { - var service = BuildService(userService.Object); - return service.CreateReleaseStatus( + await contentDbContext.AddRangeAsync(_release); + await contentDbContext.SaveChangesAsync(); + } + + await using (var contentDbContext = InMemoryApplicationDbContext(contentDbContextId)) + { + var service = BuildService(contentDbContext: contentDbContext, userService.Object); + + return await service.CreateReleaseStatus( _release.Id, new ReleaseStatusCreateRequest { @@ -69,25 +80,35 @@ await PolicyCheckBuilder() } ); } - ); + }); } [Fact] public async Task UpdateReleaseStatus_HigherLevelReview() { await PolicyCheckBuilder() - .SetupResourceCheckToFail(_release, CanSubmitSpecificReleaseToHigherReview) - .AssertForbidden( - userService => + .SetupResourceCheckToFailWithMatcher(r => r.Id == _release.Id, CanSubmitSpecificReleaseToHigherReview) + .AssertForbidden(async userService => { - var service = BuildService(userService.Object); - return service.CreateReleaseStatus( - _release.Id, - new ReleaseStatusCreateRequest - { - ApprovalStatus = ReleaseApprovalStatus.HigherLevelReview - } - ); + var contentDbContextId = Guid.NewGuid().ToString(); + await using (var contentDbContext = InMemoryApplicationDbContext(contentDbContextId)) + { + await contentDbContext.AddRangeAsync(_release); + await contentDbContext.SaveChangesAsync(); + } + + await using (var contentDbContext = InMemoryApplicationDbContext(contentDbContextId)) + { + var service = BuildService(contentDbContext: contentDbContext, userService.Object); + + return await service.CreateReleaseStatus( + _release.Id, + new ReleaseStatusCreateRequest + { + ApprovalStatus = ReleaseApprovalStatus.HigherLevelReview + } + ); + } } ); } @@ -96,12 +117,20 @@ await PolicyCheckBuilder() public async Task UpdateReleaseStatus_Approve() { await PolicyCheckBuilder() - .SetupResourceCheckToFail(_release, CanApproveSpecificRelease) - .AssertForbidden( - userService => + .SetupResourceCheckToFailWithMatcher(r => r.Id == _release.Id, CanApproveSpecificRelease) + .AssertForbidden(async userService => + { + var contentDbContextId = Guid.NewGuid().ToString(); + await using (var contentDbContext = InMemoryApplicationDbContext(contentDbContextId)) { - var service = BuildService(userService.Object); - return service.CreateReleaseStatus( + await contentDbContext.AddRangeAsync(_release); + await contentDbContext.SaveChangesAsync(); + } + + await using (var contentDbContext = InMemoryApplicationDbContext(contentDbContextId)) + { + var service = BuildService(contentDbContext: contentDbContext, userService.Object); + return await service.CreateReleaseStatus( _release.Id, new ReleaseStatusCreateRequest { @@ -109,14 +138,13 @@ await PolicyCheckBuilder() } ); } - ); + }); } - private ReleaseApprovalService BuildService(IUserService userService) + private ReleaseApprovalService BuildService(ContentDbContext contentDbContext, IUserService userService) { return new ReleaseApprovalService( - Mock.Of(), - DefaultPersistenceHelperMock().Object, + contentDbContext, new DateTimeProvider(), userService, Mock.Of(), @@ -128,17 +156,9 @@ private ReleaseApprovalService BuildService(IUserService userService) Mock.Of(), Options.Create(new ReleaseApprovalOptions()), Mock.Of(), - Mock.Of() + Mock.Of(), + Mock.Of() ); } - - private Mock> DefaultPersistenceHelperMock() - { - var mock = MockPersistenceHelper(); - SetupCall(mock, _release.Id, _release); - SetupCall(mock, Publication.Id, Publication); - - return mock; - } } } diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServiceTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServiceTests.cs index 4e073b2282d..9f392d7cc68 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServiceTests.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServiceTests.cs @@ -13,12 +13,12 @@ using GovUk.Education.ExploreEducationStatistics.Common.Model; using GovUk.Education.ExploreEducationStatistics.Common.Services; using GovUk.Education.ExploreEducationStatistics.Common.Tests.Extensions; -using GovUk.Education.ExploreEducationStatistics.Common.Utils; using GovUk.Education.ExploreEducationStatistics.Content.Model; using GovUk.Education.ExploreEducationStatistics.Content.Model.Database; using GovUk.Education.ExploreEducationStatistics.Content.Model.Repository; using GovUk.Education.ExploreEducationStatistics.Content.Model.Repository.Interfaces; using GovUk.Education.ExploreEducationStatistics.Data.Model.Database; +using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Options; using Moq; @@ -636,7 +636,7 @@ public async Task CreateReleaseStatus_Approved_Scheduled_FailsPublishDateCannotB } [Fact] - public async Task + public async Task CreateReleaseStatus_Approved_Scheduled_FailsPublishDateCannotBeScheduled_SecondFunctionHasNoOccurrence() { var release = new Release @@ -750,10 +750,69 @@ public async Task CreateReleaseStatus_Approved_Scheduled() Version = 0, }; + var existingUser1 = new User + { + Email = "test@test.com", + }; + + var existingUser1Invite = new UserReleaseInvite + { + Release = release, + Role = ReleaseRole.PrereleaseViewer, + Email = existingUser1.Email, + EmailSent = false, + }; + + var existingUser2 = new User + { + Email = "test2@test.com" + }; + + var existingUser2SentInvite = new UserReleaseInvite + { + Release = release, + Role = ReleaseRole.PrereleaseViewer, + Email = existingUser2.Email, + EmailSent = true, + }; + + var existingUser3 = new User + { + Email = "test3@test.com", + }; + + var existingUser3NonPreReleaseInvite = new UserReleaseInvite + { + Release = release, + Role = ReleaseRole.Contributor, + Email = existingUser3.Email, + EmailSent = false, + }; + + var nonExistingUserPreReleaseInvite = new UserReleaseInvite + { + Release = release, + Role = ReleaseRole.PrereleaseViewer, + Email = "nonexistent1@test.com", + EmailSent = false, + }; + + var nonExistingUserNonPreReleaseInvite = new UserReleaseInvite + { + Release = release, + Role = ReleaseRole.Contributor, + Email = "nonexistent2@test.com", + EmailSent = false, + }; + var contextId = Guid.NewGuid().ToString(); await using (var context = InMemoryApplicationDbContext(contextId)) { await context.Releases.AddAsync(release); + await context.Users.AddRangeAsync(existingUser1, existingUser2, existingUser3); + await context.UserReleaseInvites.AddRangeAsync( + existingUser1Invite, existingUser2SentInvite, existingUser3NonPreReleaseInvite, + nonExistingUserPreReleaseInvite, nonExistingUserNonPreReleaseInvite); await context.SaveChangesAsync(); } @@ -781,10 +840,30 @@ public async Task CreateReleaseStatus_Approved_Scheduled() mock.GetContentBlocks(release.Id)) .ReturnsAsync(new List()); - preReleaseUserService.Setup(mock => - mock.SendPreReleaseUserInviteEmails(release.Id)) + preReleaseUserService + .Setup(mock => mock.SendPreReleaseInviteEmail( + It.Is(r => r.Id == release.Id), + existingUser1Invite.Email, + false)) + .ReturnsAsync(Unit.Instance); + + preReleaseUserService + .Setup(mock => mock.SendPreReleaseInviteEmail( + It.Is(r => r.Id == release.Id), + nonExistingUserPreReleaseInvite.Email, + true)) .ReturnsAsync(Unit.Instance); + preReleaseUserService + .Setup(mock => mock.MarkInviteEmailAsSent( + It.Is(i => i.Email == existingUser1Invite.Email))) + .Returns(Task.CompletedTask); + + preReleaseUserService + .Setup(mock => mock.MarkInviteEmailAsSent( + It.Is(i => i.Email == nonExistingUserPreReleaseInvite.Email))) + .Returns(Task.CompletedTask); + var nextReleaseDateEdited = new PartialDate { Month = "12", Year = "2000" }; await using (var context = InMemoryApplicationDbContext(contextId)) @@ -823,12 +902,12 @@ public async Task CreateReleaseStatus_Approved_Scheduled() .SingleAsync(r => r.Id == release.Id); Assert.Equal(ReleaseApprovalStatus.Approved, saved.ApprovalStatus); - + // PublishScheduled should have been set to the scheduled date specified in the request. Assert.Equal(new DateTime(2051, 6, 29, 23, 0, 0, DateTimeKind.Utc), saved.PublishScheduled); nextReleaseDateEdited.AssertDeepEqualTo(saved.NextReleaseDate); - + // NotifySubscribers should default to true for original releases Assert.True(saved.NotifySubscribers); Assert.False(saved.UpdatePublishedDate); @@ -840,7 +919,7 @@ public async Task CreateReleaseStatus_Approved_Scheduled() Assert.Equal("Test note", savedStatus.InternalReleaseNote); } } - + [Fact] public async Task CreateReleaseStatus_Approved_Immediately() { @@ -921,10 +1000,10 @@ public async Task CreateReleaseStatus_Approved_Immediately() .SingleAsync(r => r.Id == release.Id); Assert.Equal(ReleaseApprovalStatus.Approved, saved.ApprovalStatus); - + // PublishScheduled should have been set to "now". saved.PublishScheduled.AssertUtcNow(); - + nextReleaseDateEdited.AssertDeepEqualTo(saved.NextReleaseDate); // NotifySubscribers should default to true for original releases Assert.True(saved.NotifySubscribers); @@ -1280,6 +1359,240 @@ public async Task CreateReleaseStatus_ReleaseHasUnusedImages() } } + [Fact] + public async Task CreateReleaseStatus_Approved_Scheduled_FailsSendingPreReleaseInviteEmail() + { + var release = new Release + { + ApprovalStatus = ReleaseApprovalStatus.Draft, + Type = ReleaseType.AdHocStatistics, + Publication = new Publication(), + ReleaseName = "2030", + Slug = "2030", + Version = 0, + }; + + var invite1 = new UserReleaseInvite + { + Release = release, + Role = ReleaseRole.PrereleaseViewer, + Email = "test@test.com", + EmailSent = false, + }; + + var invite2 = new UserReleaseInvite + { + Release = release, + Role = ReleaseRole.PrereleaseViewer, + Email = "test2@test.com", + EmailSent = false, + }; + + var contextId = Guid.NewGuid().ToString(); + await using (var context = InMemoryApplicationDbContext(contextId)) + { + await context.Releases.AddAsync(release); + await context.UserReleaseInvites.AddRangeAsync(invite1, invite2); + await context.SaveChangesAsync(); + } + + var releaseChecklistService = new Mock(MockBehavior.Strict); + var contentService = new Mock(MockBehavior.Strict); + var preReleaseUserService = new Mock(MockBehavior.Strict); + + releaseChecklistService + .Setup(s => + s.GetErrors(It.Is(r => r.Id == release.Id))) + .ReturnsAsync( + new List() + ); + + contentService.Setup(mock => + mock.GetContentBlocks(release.Id)) + .ReturnsAsync(new List()); + + preReleaseUserService + .Setup(mock => mock.SendPreReleaseInviteEmail( + It.Is(r => r.Id == release.Id), + invite1.Email, + true)) + .ReturnsAsync(new BadRequestResult()); + + preReleaseUserService + .Setup(mock => mock.SendPreReleaseInviteEmail( + It.Is(r => r.Id == release.Id), + invite2.Email, + true)) + .ReturnsAsync(Unit.Instance); + + preReleaseUserService + .Setup(mock => mock.MarkInviteEmailAsSent( + It.Is(i => i.Email == invite2.Email))) + .Returns(Task.CompletedTask); + + var nextReleaseDateEdited = new PartialDate { Month = "12", Year = "2000" }; + + await using (var context = InMemoryApplicationDbContext(contextId)) + { + var releaseService = BuildService(contentDbContext: context, + releaseChecklistService: releaseChecklistService.Object, + contentService: contentService.Object, + preReleaseUserService: preReleaseUserService.Object); + + var result = await releaseService + .CreateReleaseStatus( + release.Id, + new ReleaseStatusCreateRequest + { + ApprovalStatus = ReleaseApprovalStatus.Approved, + InternalReleaseNote = "Test note", + PublishMethod = PublishMethod.Scheduled, + PublishScheduled = "2051-06-30", + NextReleaseDate = nextReleaseDateEdited + } + ); + + VerifyAllMocks(contentService, + preReleaseUserService, + releaseChecklistService); + + result.AssertLeft(); + } + + await using (var context = InMemoryApplicationDbContext(contextId)) + { + var saved = await context + .Releases + .HydrateReleaseForChecklist() + .Include(r => r.ReleaseStatuses) + .SingleAsync(r => r.Id == release.Id); + + // Assert that the failure to send emails prevented the Release from completing approval. + // The Release should remain unchanged from the original Release, as changes to it were rolled back. + // Additionally, no new ReleaseStatus entries were added. + saved.AssertDeepEqualTo(release); + + // Futhermore, we have proven that the Publisher was not informed of the Release change, as it + // did not complete. + } + } + + [Fact] + public async Task CreateReleaseStatus_Approved_Scheduled_FailsWhileInformingPublisher() + { + var release = new Release + { + ApprovalStatus = ReleaseApprovalStatus.Draft, + Type = ReleaseType.AdHocStatistics, + Publication = new Publication(), + ReleaseName = "2030", + Slug = "2030", + Version = 0, + }; + + var invite = new UserReleaseInvite + { + Release = release, + Role = ReleaseRole.PrereleaseViewer, + Email = "test@test.com", + EmailSent = false, + }; + + var contextId = Guid.NewGuid().ToString(); + await using (var context = InMemoryApplicationDbContext(contextId)) + { + await context.Releases.AddAsync(release); + await context.UserReleaseInvites.AddAsync(invite); + await context.SaveChangesAsync(); + } + + var releaseChecklistService = new Mock(MockBehavior.Strict); + var publishingService = new Mock(MockBehavior.Strict); + var contentService = new Mock(MockBehavior.Strict); + var preReleaseUserService = new Mock(MockBehavior.Strict); + + releaseChecklistService + .Setup(s => + s.GetErrors(It.Is(r => r.Id == release.Id))) + .ReturnsAsync( + new List() + ); + + preReleaseUserService + .Setup(mock => mock.SendPreReleaseInviteEmail( + It.Is(r => r.Id == release.Id), + invite.Email, + true)) + .ReturnsAsync(Unit.Instance); + + preReleaseUserService + .Setup(mock => mock.MarkInviteEmailAsSent( + It.Is(i => i.Email == invite.Email))) + .Returns(Task.CompletedTask); + + publishingService + .Setup(s => s.ReleaseChanged( + It.Is(g => g == release.Id), + It.IsAny(), + It.IsAny() + )) + .ReturnsAsync(new BadRequestResult()); + + contentService.Setup(mock => + mock.GetContentBlocks(release.Id)) + .ReturnsAsync(new List()); + + var nextReleaseDateEdited = new PartialDate { Month = "12", Year = "2000" }; + + await using (var context = InMemoryApplicationDbContext(contextId)) + { + var releaseService = BuildService(contentDbContext: context, + releaseChecklistService: releaseChecklistService.Object, + publishingService: publishingService.Object, + contentService: contentService.Object, + preReleaseUserService: preReleaseUserService.Object); + + var result = await releaseService + .CreateReleaseStatus( + release.Id, + new ReleaseStatusCreateRequest + { + ApprovalStatus = ReleaseApprovalStatus.Approved, + InternalReleaseNote = "Test note", + PublishMethod = PublishMethod.Scheduled, + PublishScheduled = "2051-06-30", + NextReleaseDate = nextReleaseDateEdited + } + ); + + VerifyAllMocks( + contentService, + publishingService, + releaseChecklistService, + preReleaseUserService); + + result.AssertLeft(); + } + + await using (var context = InMemoryApplicationDbContext(contextId)) + { + var saved = await context + .Releases + .HydrateReleaseForChecklist() + .Include(r => r.ReleaseStatuses) + .SingleAsync(r => r.Id == release.Id); + + // Assert that the failure to notify the Publisher prevented the Release from completing approval. + // The Release should remain unchanged from the original Release, as changes to it were rolled back. + // Additionally, no new ReleaseStatus entries were added. + saved.AssertDeepEqualTo(release); + + // We have also shown that unfortunately the invite emails would have been sent out despite the + // approval failing, but we have more importantly stopped a Release from only having been partially + // approved. + } + } + [Fact] public async Task GetReleaseStatuses() { @@ -1505,7 +1818,6 @@ public async Task GetReleaseStatuses_NoResults() await context.SaveChangesAsync(); } - await using (var context = InMemoryApplicationDbContext(contextId)) { var releaseRepository = new ReleaseRepository( @@ -1578,6 +1890,24 @@ public async Task CreateReleaseStatus_HigherReview_DoesNotSendEmailIfNoReleaseAp result.AssertRight(); } + + await using (var context = InMemoryApplicationDbContext(contextId)) + { + var saved = await context + .Releases + .Include(r => r.ReleaseStatuses) + .SingleAsync(r => r.Id == release.Id); + + // Assert that the Release has moved into Higher Level Review successfully. + Assert.Equal(ReleaseApprovalStatus.HigherLevelReview, saved.ApprovalStatus); + + // Assert a new ReleaseStatus entry is included. + var savedStatus = Assert.Single(saved.ReleaseStatuses); + Assert.Equal(release.Id, savedStatus.ReleaseId); + Assert.Equal(ReleaseApprovalStatus.HigherLevelReview, savedStatus.ApprovalStatus); + Assert.Equal(_userId, savedStatus.CreatedById); + Assert.Equal("Test internal note", savedStatus.InternalReleaseNote); + } } [Fact] @@ -1654,6 +1984,24 @@ public async Task CreateReleaseStatus_HigherReview_SendsEmailIfReleaseApprovers( VerifyAllMocks(contentService, userReleaseRoleService, emailTemplateService); result.AssertRight(); } + + await using (var context = InMemoryApplicationDbContext(contextId)) + { + var saved = await context + .Releases + .Include(r => r.ReleaseStatuses) + .SingleAsync(r => r.Id == release.Id); + + // Assert that the Release has moved into Higher Level Review successfully. + Assert.Equal(ReleaseApprovalStatus.HigherLevelReview, saved.ApprovalStatus); + + // Assert a new ReleaseStatus entry is included. + var savedStatus = Assert.Single(saved.ReleaseStatuses); + Assert.Equal(release.Id, savedStatus.ReleaseId); + Assert.Equal(ReleaseApprovalStatus.HigherLevelReview, savedStatus.ApprovalStatus); + Assert.Equal(_userId, savedStatus.CreatedById); + Assert.Equal("Test internal note", savedStatus.InternalReleaseNote); + } } [Fact] @@ -1723,6 +2071,115 @@ public async Task CreateReleaseStatus_HigherReview_SendsEmailIfPublicationReleas result.AssertRight(); } + + await using (var context = InMemoryApplicationDbContext(contextId)) + { + var saved = await context + .Releases + .Include(r => r.ReleaseStatuses) + .SingleAsync(r => r.Id == release.Id); + + // Assert that the Release has moved into Higher Level Review successfully. + Assert.Equal(ReleaseApprovalStatus.HigherLevelReview, saved.ApprovalStatus); + + // Assert a new ReleaseStatus entry is included. + var savedStatus = Assert.Single(saved.ReleaseStatuses); + Assert.Equal(release.Id, savedStatus.ReleaseId); + Assert.Equal(ReleaseApprovalStatus.HigherLevelReview, savedStatus.ApprovalStatus); + Assert.Equal(_userId, savedStatus.CreatedById); + Assert.Equal("Test internal note", savedStatus.InternalReleaseNote); + } + } + + [Fact] + public async Task CreateReleaseStatus_HigherReview_FailsSendingEmail() + { + var release = new Release + { + Type = ReleaseType.AdHocStatistics, + Publication = new Publication { Title = "Test publication" }, + ReleaseName = "2030", + Slug = "2030", + Version = 0, + }; + + var userReleaseRole1 = new UserReleaseRole + { + User = new User { Id = Guid.NewGuid(), Email = "test@test.com"}, + Release = release, + Role = ReleaseRole.Approver, + }; + + var userReleaseRole2 = new UserReleaseRole + { + User = new User { Id = Guid.NewGuid(), Email = "test2@test.com"}, + Release = release, + Role = ReleaseRole.Approver, + }; + + var contextId = Guid.NewGuid().ToString(); + + await using (var context = InMemoryApplicationDbContext(contextId)) + { + await context.Releases.AddAsync(release); + await context.SaveChangesAsync(); + } + + var contentService = new Mock(MockBehavior.Strict); + var userReleaseRoleService = new Mock(MockBehavior.Strict); + var emailTemplateService = new Mock(MockBehavior.Strict); + + await using (var context = InMemoryApplicationDbContext(contextId)) + { + contentService.Setup(mock => mock.GetContentBlocks(release.Id)) + .ReturnsAsync(new List()); + + userReleaseRoleService.Setup(mock => + mock.ListUserReleaseRolesByPublication(ReleaseRole.Approver, release.Publication.Id)) + .ReturnsAsync(ListOf(userReleaseRole1, userReleaseRole2)); + + emailTemplateService.Setup(mock => mock.SendReleaseHigherReviewEmail(userReleaseRole1.User.Email, It.Is(r => r.Id == release.Id))) + .Returns(Unit.Instance); + + emailTemplateService.Setup(mock => mock.SendReleaseHigherReviewEmail(userReleaseRole2.User.Email, It.Is(r => r.Id == release.Id))) + .Returns(new BadRequestResult()); + + var releaseService = BuildService( + contentDbContext: context, + contentService: contentService.Object, + userReleaseRoleService: userReleaseRoleService.Object, + emailTemplateService: emailTemplateService.Object + ); + + var result = await releaseService + .CreateReleaseStatus( + release.Id, new ReleaseStatusCreateRequest + { + PublishMethod = PublishMethod.Scheduled, + PublishScheduled = "2051-06-30", + ApprovalStatus = ReleaseApprovalStatus.HigherLevelReview, + InternalReleaseNote = "Test internal note", + NextReleaseDate = new PartialDate { Month = "12", Year = "2077" }, + }); + + VerifyAllMocks(contentService, userReleaseRoleService, emailTemplateService); + result.AssertLeft(); + } + + await using (var context = InMemoryApplicationDbContext(contextId)) + { + var saved = await context + .Releases + .Include(r => r.ReleaseStatuses) + .SingleAsync(r => r.Id == release.Id); + + // Assert that the failure to send emails prevented the Release from completing moving into Higher + // Level Review. + Assert.Equal(ReleaseApprovalStatus.Draft, saved.ApprovalStatus); + + // Assert no new ReleaseStatus entries were added. + Assert.Empty(saved.ReleaseStatuses); + } } private static IOptions DefaultReleaseApprovalOptions() @@ -1756,7 +2213,6 @@ private ReleaseApprovalService BuildService( return new ReleaseApprovalService( contentDbContext, - new PersistenceHelper(contentDbContext), dateTimeProvider ?? new DateTimeProvider(), userService.Object, publishingService ?? Mock.Of(MockBehavior.Strict), @@ -1768,7 +2224,8 @@ private ReleaseApprovalService BuildService( releaseRepository ?? Mock.Of(MockBehavior.Strict), options ?? DefaultReleaseApprovalOptions(), userReleaseRoleService ?? Mock.Of(MockBehavior.Strict), - emailTemplateService ?? Mock.Of(MockBehavior.Strict)); + emailTemplateService ?? Mock.Of(MockBehavior.Strict), + new UserRepository(contentDbContext)); } } } diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseChecklistServicePermissionTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseChecklistServicePermissionTests.cs index a687a1639e1..0d8b627d1e0 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseChecklistServicePermissionTests.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseChecklistServicePermissionTests.cs @@ -12,30 +12,43 @@ using GovUk.Education.ExploreEducationStatistics.Data.Model.Repository.Interfaces; using Moq; using Xunit; +using static GovUk.Education.ExploreEducationStatistics.Admin.Tests.Services.DbUtils; using static GovUk.Education.ExploreEducationStatistics.Common.Tests.Utils.PermissionTestUtils; namespace GovUk.Education.ExploreEducationStatistics.Admin.Tests.Services { public class ReleaseChecklistPermissionServiceTests { - private readonly Release _release = new Release + private readonly Release _release = new() { - Id = Guid.NewGuid() + Id = Guid.NewGuid(), + Publication = new Publication() }; - [Fact] public async Task GetChecklist() { await PolicyCheckBuilder() - .SetupResourceCheckToFail(_release, ContentSecurityPolicies.CanViewSpecificRelease) - .AssertForbidden( - userService => + .SetupResourceCheckToFailWithMatcher( + r => r.Id == _release.Id, + ContentSecurityPolicies.CanViewSpecificRelease) + .AssertForbidden(async userService => + { + var contentDbContextId = Guid.NewGuid().ToString(); + await using (var contentDbContext = InMemoryApplicationDbContext(contentDbContextId)) + { + await contentDbContext.AddRangeAsync(_release); + await contentDbContext.SaveChangesAsync(); + } + + await using (var contentDbContext = InMemoryApplicationDbContext(contentDbContextId)) { - var service = BuildReleaseChecklistService(userService: userService.Object); - return service.GetChecklist(_release.Id); + var service = BuildReleaseChecklistService( + contentDbContext: contentDbContext, + userService: userService.Object); + return await service.GetChecklist(_release.Id); } - ); + }); } private ReleaseChecklistService BuildReleaseChecklistService( @@ -52,7 +65,6 @@ private ReleaseChecklistService BuildReleaseChecklistService( return new( contentDbContext ?? new Mock().Object, - persistenceHelper ?? DefaultPersistenceHelperMock().Object, dataImportService ?? new Mock().Object, userService ?? MockUtils.AlwaysTrueUserService().Object, dataGuidanceService ?? new Mock().Object, @@ -62,12 +74,5 @@ private ReleaseChecklistService BuildReleaseChecklistService( dataBlockService ?? new Mock().Object ); } - - private Mock> DefaultPersistenceHelperMock() - { - var mock = MockUtils.MockPersistenceHelper(); - MockUtils.SetupCall(mock, _release.Id, _release); - return mock; - } } } diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseChecklistServiceTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseChecklistServiceTests.cs index fd517239821..a9f3836900f 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseChecklistServiceTests.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseChecklistServiceTests.cs @@ -10,7 +10,6 @@ using GovUk.Education.ExploreEducationStatistics.Common.Services.Interfaces.Security; using GovUk.Education.ExploreEducationStatistics.Common.Tests.Extensions; using GovUk.Education.ExploreEducationStatistics.Common.Tests.Utils; -using GovUk.Education.ExploreEducationStatistics.Common.Utils; using GovUk.Education.ExploreEducationStatistics.Content.Model; using GovUk.Education.ExploreEducationStatistics.Content.Model.Database; using GovUk.Education.ExploreEducationStatistics.Content.Model.Repository.Interfaces; @@ -674,7 +673,6 @@ private static ReleaseChecklistService BuildReleaseChecklistService( { return new( contentDbContext, - new PersistenceHelper(contentDbContext), dataImportService ?? new Mock().Object, userService ?? MockUtils.AlwaysTrueUserService().Object, dataGuidanceService ?? new Mock().Object, diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseServicePermissionTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseServicePermissionTests.cs index 5fc8791a942..04284b02f76 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseServicePermissionTests.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseServicePermissionTests.cs @@ -167,7 +167,7 @@ await PolicyCheckBuilder() async userService => { userService - .Setup(s => s.MatchesPolicy(list[0], CanAssignPrereleaseContactsToSpecificRelease)) + .Setup(s => s.MatchesPolicy(list[0], CanAssignPreReleaseUsersToSpecificRelease)) .ReturnsAsync(true); userService @@ -230,7 +230,7 @@ await PolicyCheckBuilder() async userService => { userService - .Setup(s => s.MatchesPolicy(list[0], CanAssignPrereleaseContactsToSpecificRelease)) + .Setup(s => s.MatchesPolicy(list[0], CanAssignPreReleaseUsersToSpecificRelease)) .ReturnsAsync(true); userService diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin/Security/SecurityPolicies.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin/Security/SecurityPolicies.cs index e903811eafe..ac4d0a7e7c7 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin/Security/SecurityPolicies.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin/Security/SecurityPolicies.cs @@ -56,8 +56,7 @@ public enum SecurityPolicies /** * Pre Release management */ - CanViewPrereleaseContacts, - CanAssignPrereleaseContactsToSpecificRelease, + CanAssignPreReleaseUsersToSpecificRelease, /** * Publication management diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin/Security/StartupSecurityConfiguration.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin/Security/StartupSecurityConfiguration.cs index 2e41dee1d02..8e507c8b9a6 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin/Security/StartupSecurityConfiguration.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin/Security/StartupSecurityConfiguration.cs @@ -134,10 +134,7 @@ public static void ConfigureAuthorizationPolicies(IServiceCollection services) /** * Pre Release management */ - options.AddPolicy(SecurityPolicies.CanViewPrereleaseContacts.ToString(), policy => - policy.RequireClaim(SecurityClaimTypes.CanViewPrereleaseContacts.ToString())); - - options.AddPolicy(SecurityPolicies.CanAssignPrereleaseContactsToSpecificRelease.ToString(), policy => + options.AddPolicy(SecurityPolicies.CanAssignPreReleaseUsersToSpecificRelease.ToString(), policy => policy.Requirements.Add(new AssignPrereleaseContactsToSpecificReleaseRequirement())); /** diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/Interfaces/IPreReleaseUserService.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/Interfaces/IPreReleaseUserService.cs index 24950f45134..00dc5490ab0 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/Interfaces/IPreReleaseUserService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/Interfaces/IPreReleaseUserService.cs @@ -4,6 +4,7 @@ using System.Threading.Tasks; using GovUk.Education.ExploreEducationStatistics.Admin.ViewModels; using GovUk.Education.ExploreEducationStatistics.Common.Model; +using GovUk.Education.ExploreEducationStatistics.Content.Model; using Microsoft.AspNetCore.Mvc; namespace GovUk.Education.ExploreEducationStatistics.Admin.Services.Interfaces @@ -22,6 +23,11 @@ Task>> InvitePreReleaseUsers( Task> RemovePreReleaseUser(Guid releaseId, string email); - Task> SendPreReleaseUserInviteEmails(Guid releaseId); + Task> SendPreReleaseInviteEmail( + Release release, + string email, + bool isNewUser); + + Task MarkInviteEmailAsSent(UserReleaseInvite invite); } } diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/Interfaces/Security/UserServiceExtensions.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/Interfaces/Security/UserServiceExtensions.cs index c67cdaa6416..b0d54bad245 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/Interfaces/Security/UserServiceExtensions.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/Interfaces/Security/UserServiceExtensions.cs @@ -278,16 +278,10 @@ public static Task> CheckCanMakeAmendmentOfRelease return userService.CheckPolicy(release, SecurityPolicies.CanMakeAmendmentOfSpecificRelease); } - public static Task> CheckCanViewPrereleaseContactsList( - this IUserService userService) - { - return userService.CheckPolicy(SecurityPolicies.CanViewPrereleaseContacts); - } - public static Task> CheckCanAssignPrereleaseContactsToRelease( this IUserService userService, Release release) { - return userService.CheckPolicy(release, SecurityPolicies.CanAssignPrereleaseContactsToSpecificRelease); + return userService.CheckPolicy(release, SecurityPolicies.CanAssignPreReleaseUsersToSpecificRelease); } public static Task> CheckCanViewPreReleaseSummary( diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/PreReleaseUserService.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/PreReleaseUserService.cs index 5236676b27b..d05cc5e0eed 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/PreReleaseUserService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/PreReleaseUserService.cs @@ -241,43 +241,6 @@ public async Task> RemovePreReleaseUser(Guid releaseI ); } - public async Task> SendPreReleaseUserInviteEmails(Guid releaseId) - { - return await _persistenceHelper.CheckEntityExists(releaseId) - .OnSuccess(_userService.CheckCanUpdateRelease) - .OnSuccess(async release => - { - var userReleaseInvites = await _context.UserReleaseInvites - .AsQueryable() - .Where(i => - i.ReleaseId == releaseId - && i.Role == PrereleaseViewer - && !i.EmailSent) - .ToListAsync(); - - var results = await userReleaseInvites - .ToAsyncEnumerable() - .SelectAwait(async invite => - { - var user = await _userRepository.FindByEmail(invite.Email); - return await SendPreReleaseInviteEmail( - release, - invite.Email.ToLower(), - isNewUser: user == null) - .OnSuccessDo(() => MarkInviteEmailAsSent(invite)); - }) - .ToListAsync(); - - var failure = results.FirstOrDefault(sendResult => sendResult.IsLeft)?.Left; - if (failure != null) - { - return failure; - } - - return Unit.Instance; - }); - } - private async Task> InvitePreReleaseUser(Release release, string email) { @@ -304,6 +267,8 @@ private async Task> CreateUserReleaseInvite(Release r var sendEmail = release.ApprovalStatus == ReleaseApprovalStatus.Approved; if (sendEmail) { + // TODO EES-4681 - we're not currently marking this email as having been sent using + // MarkInviteEmailAsSent, but should we be doing so? var emailResult = await SendPreReleaseInviteEmail(release, email, isNewUser: true); if (emailResult.IsLeft) { @@ -331,6 +296,8 @@ private async Task> CreateExistingUserReleaseInvite(R if (sendEmail) { + // TODO EES-4681 - we're not currently marking this email as having been sent using + // MarkInviteEmailAsSent, but should we be doing so? var emailResult = await SendPreReleaseInviteEmail(release, email, isNewUser: false); if (emailResult.IsLeft) { @@ -358,7 +325,7 @@ await _userReleaseRoleRepository.CreateIfNotExists( return Unit.Instance; } - private async Task> SendPreReleaseInviteEmail( + public async Task> SendPreReleaseInviteEmail( Release release, string email, bool isNewUser) @@ -402,7 +369,7 @@ await _context.Entry(release) return _emailService.SendEmail(email, template, emailValues); } - private async Task MarkInviteEmailAsSent(UserReleaseInvite invite) + public async Task MarkInviteEmailAsSent(UserReleaseInvite invite) { invite.EmailSent = true; _context.Update(invite); diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseApprovalService.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseApprovalService.cs index 411120aa6b1..7cdf9c3e69c 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseApprovalService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseApprovalService.cs @@ -14,7 +14,6 @@ using GovUk.Education.ExploreEducationStatistics.Common.Model; using GovUk.Education.ExploreEducationStatistics.Common.Services; using GovUk.Education.ExploreEducationStatistics.Common.Services.Interfaces.Security; -using GovUk.Education.ExploreEducationStatistics.Common.Utils; using GovUk.Education.ExploreEducationStatistics.Content.Model; using GovUk.Education.ExploreEducationStatistics.Content.Model.Database; using GovUk.Education.ExploreEducationStatistics.Content.Model.Repository.Interfaces; @@ -31,7 +30,6 @@ namespace GovUk.Education.ExploreEducationStatistics.Admin.Services public class ReleaseApprovalService : IReleaseApprovalService { private readonly ContentDbContext _context; - private readonly IPersistenceHelper _persistenceHelper; private readonly DateTimeProvider _dateTimeProvider; private readonly IUserService _userService; private readonly IPublishingService _publishingService; @@ -44,10 +42,10 @@ public class ReleaseApprovalService : IReleaseApprovalService private readonly ReleaseApprovalOptions _options; private readonly IUserReleaseRoleService _userReleaseRoleService; private readonly IEmailTemplateService _emailTemplateService; + private readonly IUserRepository _userRepository; public ReleaseApprovalService( ContentDbContext context, - IPersistenceHelper persistenceHelper, DateTimeProvider dateTimeProvider, IUserService userService, IPublishingService publishingService, @@ -59,11 +57,10 @@ public ReleaseApprovalService( IReleaseRepository releaseRepository, IOptions options, IUserReleaseRoleService userReleaseRoleService, - IEmailTemplateService emailTemplateService - ) + IEmailTemplateService emailTemplateService, + IUserRepository userRepository) { _context = context; - _persistenceHelper = persistenceHelper; _dateTimeProvider = dateTimeProvider; _userService = userService; _publishingService = publishingService; @@ -75,16 +72,19 @@ IEmailTemplateService emailTemplateService _releaseRepository = releaseRepository; _userReleaseRoleService = userReleaseRoleService; _emailTemplateService = emailTemplateService; + _userRepository = userRepository; _options = options.Value; } public async Task>> GetReleaseStatuses( Guid releaseId) { - return await _persistenceHelper - .CheckEntityExists(releaseId, release => - release.Include(r => r.ReleaseStatuses) - .ThenInclude(rs => rs.CreatedBy)) + return await _context + .Releases + .Include(r => r.ReleaseStatuses) + .ThenInclude(rs => rs.CreatedBy) + .SingleOrDefaultAsync(r => r.Id == releaseId) + .OrNotFound() .OnSuccess(_userService.CheckCanViewReleaseStatusHistory) .OnSuccess(async release => { @@ -113,92 +113,155 @@ public async Task> CreateReleaseStatus( Guid releaseId, ReleaseStatusCreateRequest request) { - return await _persistenceHelper - .CheckEntityExists(releaseId, ReleaseChecklistService.HydrateReleaseForChecklist) + return await _context + .Releases + .HydrateReleaseForChecklist() + .AsNoTracking() + .SingleOrDefaultAsync(r => r.Id == releaseId) + .OrNotFound() .OnSuccessDo(release => _userService.CheckCanUpdateReleaseStatus(release, request.ApprovalStatus)) .OnSuccessDo(() => ValidatePublishDate(request)) - .OnSuccess(async release => + .OnSuccess(async + // Keep a track of the original Release in the event that we need to roll back approval. + originalRelease => { - if (request.ApprovalStatus != ReleaseApprovalStatus.Approved && release.Published.HasValue) + if (request.ApprovalStatus != ReleaseApprovalStatus.Approved && originalRelease.Published.HasValue) { return ValidationActionResult(PublishedReleaseCannotBeUnapproved); } - var oldStatus = release.ApprovalStatus; + var oldStatus = originalRelease.ApprovalStatus; + + var releaseToUpdate = await _context + .Releases + .HydrateReleaseForChecklist() + .SingleAsync(r => r.Id == releaseId); - release.ApprovalStatus = request.ApprovalStatus; - release.NextReleaseDate = request.NextReleaseDate; - release.NotifySubscribers = release.Version == 0 || (request.NotifySubscribers ?? false); - release.UpdatePublishedDate = request.UpdatePublishedDate ?? false; - release.PublishScheduled = request.PublishMethod == PublishMethod.Immediate + releaseToUpdate.ApprovalStatus = request.ApprovalStatus; + releaseToUpdate.NextReleaseDate = request.NextReleaseDate; + releaseToUpdate.NotifySubscribers = originalRelease.Version == 0 || (request.NotifySubscribers ?? false); + releaseToUpdate.UpdatePublishedDate = request.UpdatePublishedDate ?? false; + releaseToUpdate.PublishScheduled = request.PublishMethod == PublishMethod.Immediate ? _dateTimeProvider.UtcNow : request.PublishScheduledDate; var releaseStatus = new ReleaseStatus { - Release = release, + Release = releaseToUpdate, InternalReleaseNote = request.InternalReleaseNote, ApprovalStatus = request.ApprovalStatus, CreatedById = _userService.GetUserId() }; - return await ValidateReleaseWithChecklist(release) - .OnSuccessDo(() => RemoveUnusedImages(release.Id)) - .OnSuccessVoid(async () => + return await + ValidateReleaseWithChecklist(releaseToUpdate) + .OnSuccess(() => RemoveUnusedImages(releaseToUpdate.Id)) + .OnSuccessDo(async () => { - _context.Releases.Update(release); + _context.Releases.Update(releaseToUpdate); await _context.AddAsync(releaseStatus); await _context.SaveChangesAsync(); - - // Only need to inform Publisher if changing release approval status to or from Approved - if (oldStatus == ReleaseApprovalStatus.Approved || - request.ApprovalStatus == ReleaseApprovalStatus.Approved) - { - await _publishingService.ReleaseChanged( - releaseId, - releaseStatus.Id, - request.PublishMethod == PublishMethod.Immediate - ); - } - - switch (request.ApprovalStatus) + }) + .OnSuccess(() => + SendEmailNotificationsAndInvites(request, releaseToUpdate) + .OnSuccess(() => NotifyPublisher(releaseId, request, oldStatus, releaseStatus)) + // If a failure occurs in either the sending of the emails or the Publisher notification, + // roll back changes to the Release and remove the added Release Status. + .OnFailureDo(async _ => { - case ReleaseApprovalStatus.Approved: - if (request.PublishMethod == PublishMethod.Scheduled) - { - await _preReleaseUserService.SendPreReleaseUserInviteEmails(release.Id); - } - break; - - case ReleaseApprovalStatus.HigherLevelReview: - var userReleaseRoles = - await _userReleaseRoleService.ListUserReleaseRolesByPublication( - ReleaseRole.Approver, - release.Publication.Id); - - var userPublicationRoles = await _context.UserPublicationRoles - .Include(upr => upr.User) - .Where(upr => upr.PublicationId == release.PublicationId - && upr.Role == PublicationRole.Approver) - .ToListAsync(); - - var notifyHigherReviewers = userReleaseRoles.Any() || userPublicationRoles.Any(); - if (notifyHigherReviewers) - { - userReleaseRoles.Select(urr => urr.User.Email) - .Concat(userPublicationRoles.Select(upr => upr.User.Email)) - .Distinct() - .ForEach(email => - { - _emailTemplateService.SendReleaseHigherReviewEmail(email, release); - }); - } - break; - } - }); + _context.Entry(releaseToUpdate).CurrentValues.SetValues(originalRelease); + _context.ReleaseStatus.Remove(releaseStatus); + await _context.SaveChangesAsync(); + })); }); } + private async Task> NotifyPublisher( + Guid releaseId, + ReleaseStatusCreateRequest request, + ReleaseApprovalStatus oldStatus, + ReleaseStatus releaseStatus) + { + // Only need to inform Publisher if changing release approval status to or from Approved + if (oldStatus == ReleaseApprovalStatus.Approved || + request.ApprovalStatus == ReleaseApprovalStatus.Approved) + { + return await _publishingService.ReleaseChanged( + releaseId, + releaseStatus.Id, + request.PublishMethod == PublishMethod.Immediate); + } + + return Unit.Instance; + } + + private async Task> SendEmailNotificationsAndInvites( + ReleaseStatusCreateRequest request, + Release release) + { + if (request.ApprovalStatus == ReleaseApprovalStatus.Approved + && request.PublishMethod == PublishMethod.Scheduled) + { + return await SendPreReleaseUserInviteEmails(release); + } + + if (request.ApprovalStatus == ReleaseApprovalStatus.HigherLevelReview) + { + var userReleaseRoles = + await _userReleaseRoleService.ListUserReleaseRolesByPublication( + ReleaseRole.Approver, + release.Publication.Id); + + var userPublicationRoles = await _context + .UserPublicationRoles + .Include(upr => upr.User) + .Where(upr => + upr.PublicationId == release.PublicationId + && upr.Role == PublicationRole.Approver) + .ToListAsync(); + + var notifyHigherReviewers = userReleaseRoles.Any() || userPublicationRoles.Any(); + + if (notifyHigherReviewers) + { + return userReleaseRoles + .Select(urr => urr.User.Email) + .Concat(userPublicationRoles.Select(upr => upr.User.Email)) + .Distinct() + .Select(email => _emailTemplateService.SendReleaseHigherReviewEmail(email, release)) + .OnSuccessAllReturnVoid(); + } + } + + return Unit.Instance; + } + + private async Task> SendPreReleaseUserInviteEmails(Release release) + { + var unsentUserReleaseInvites = await _context.UserReleaseInvites + .AsQueryable() + .Where(i => + i.ReleaseId == release.Id + && i.Role == ReleaseRole.PrereleaseViewer + && !i.EmailSent) + .ToListAsync(); + + var emailResults = await unsentUserReleaseInvites + .ToAsyncEnumerable() + .SelectAwait(async invite => + { + var user = await _userRepository.FindByEmail(invite.Email); + return await _preReleaseUserService.SendPreReleaseInviteEmail( + release, + invite.Email.ToLower(), + isNewUser: user == null) + .OnSuccessDo(() => _preReleaseUserService.MarkInviteEmailAsSent(invite)); + }) + .ToListAsync(); + + return emailResults.OnSuccessAllReturnVoid(); + } + private async Task> RemoveUnusedImages(Guid releaseId) { return await _contentService.GetContentBlocks(releaseId) diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseChecklistService.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseChecklistService.cs index 2ca44ac18bd..fac9e35ae01 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseChecklistService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseChecklistService.cs @@ -10,7 +10,6 @@ using GovUk.Education.ExploreEducationStatistics.Common.Extensions; using GovUk.Education.ExploreEducationStatistics.Common.Model; using GovUk.Education.ExploreEducationStatistics.Common.Services.Interfaces.Security; -using GovUk.Education.ExploreEducationStatistics.Common.Utils; using GovUk.Education.ExploreEducationStatistics.Content.Model; using GovUk.Education.ExploreEducationStatistics.Content.Model.Database; using GovUk.Education.ExploreEducationStatistics.Content.Model.Repository.Interfaces; @@ -25,7 +24,6 @@ namespace GovUk.Education.ExploreEducationStatistics.Admin.Services public class ReleaseChecklistService : IReleaseChecklistService { private readonly ContentDbContext _contentDbContext; - private readonly IPersistenceHelper _persistenceHelper; private readonly IDataImportService _dataImportService; private readonly IUserService _userService; private readonly IDataGuidanceService _dataGuidanceService; @@ -36,7 +34,6 @@ public class ReleaseChecklistService : IReleaseChecklistService public ReleaseChecklistService( ContentDbContext contentDbContext, - IPersistenceHelper persistenceHelper, IDataImportService dataImportService, IUserService userService, IDataGuidanceService dataGuidanceService, @@ -46,7 +43,6 @@ public ReleaseChecklistService( IDataBlockService dataBlockService) { _contentDbContext = contentDbContext; - _persistenceHelper = persistenceHelper; _dataImportService = dataImportService; _userService = userService; _dataGuidanceService = dataGuidanceService; @@ -58,7 +54,11 @@ public ReleaseChecklistService( public async Task> GetChecklist(Guid releaseId) { - return await _persistenceHelper.CheckEntityExists(releaseId, HydrateReleaseForChecklist) + return await _contentDbContext + .Releases + .HydrateReleaseForChecklist() + .SingleOrDefaultAsync(r => r.Id == releaseId) + .OrNotFound() .OnSuccess(_userService.CheckCanViewRelease) .OnSuccess( async release => new ReleaseChecklistViewModel( @@ -68,12 +68,6 @@ await GetWarnings(release) ); } - public static IQueryable HydrateReleaseForChecklist(IQueryable query) - { - return query.Include(r => r.Publication) - .Include(r => r.Updates); - } - public async Task> GetErrors(Release release) { var errors = new List(); @@ -250,4 +244,13 @@ private async Task HasFeaturedTable(Guid releaseId) .AnyAsync(ft => dataBlockIds.Contains(ft.DataBlockId)); } } + + public static class ReleaseChecklistQueryableExtensions + { + public static IQueryable HydrateReleaseForChecklist(this IQueryable query) + { + return query.Include(r => r.Publication) + .Include(r => r.Updates); + } + } } diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs index 82df2805f29..bd4d9fdb4da 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs @@ -342,8 +342,11 @@ public Task> GetReleaseP public async Task> UpdateRelease( Guid releaseId, ReleaseUpdateRequest request) { - return await _persistenceHelper - .CheckEntityExists(releaseId, ReleaseChecklistService.HydrateReleaseForChecklist) + return await _context + .Releases + .HydrateReleaseForChecklist() + .SingleOrDefaultAsync(r => r.Id == releaseId) + .OrNotFound() .OnSuccess(_userService.CheckCanUpdateRelease) .OnSuccessDo(async release => await ValidateReleaseSlugUniqueToPublication(request.Slug, release.PublicationId, releaseId)) .OnSuccess(async release => diff --git a/src/GovUk.Education.ExploreEducationStatistics.Common.Tests/Model/EitherTest.cs b/src/GovUk.Education.ExploreEducationStatistics.Common.Tests/Model/EitherTest.cs index 58e6be53e6c..15da69f674b 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Common.Tests/Model/EitherTest.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Common.Tests/Model/EitherTest.cs @@ -1652,5 +1652,35 @@ public async Task OnSuccessAll_Left() result.AssertLeft(500); } + + [Fact] + public void OnSuccessAllReturnVoid() + { + var either1 = new Either("either1"); + var either2 = new Either("either2"); + var either3 = new Either("either3"); + + var eitherList = ListOf(either1, either2, either3); + + var results = eitherList.OnSuccessAllReturnVoid(); + + Assert.True(results.IsRight); + Assert.Equal(Unit.Instance, results.Right); + } + + [Fact] + public void OnSuccessAllReturnVoid_Left() + { + var either1 = new Either("either1"); + var either2 = new Either(2); + var either3 = new Either(3); + + var eitherList = ListOf(either1, either2, either3); + + var results = eitherList.OnSuccessAllReturnVoid(); + + Assert.True(results.IsLeft); + Assert.Equal(2, results.Left); + } } } diff --git a/src/GovUk.Education.ExploreEducationStatistics.Common/Model/Either.cs b/src/GovUk.Education.ExploreEducationStatistics.Common/Model/Either.cs index f715cd9f51a..32c07ddc62a 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Common/Model/Either.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Common/Model/Either.cs @@ -66,6 +66,10 @@ public async Task> OnSuccess(Func>> func public static class EitherExtensions { + /// + /// If all Eithers in the provided list are successful, return a list of the successful results. Otherwise, + /// return the first failure. + /// public static Either> OnSuccessAll( this IEnumerable> items) { @@ -82,6 +86,18 @@ public static Either> OnSuccessAll( return result; } + /// + /// If all Eithers in the provided list are successful, return Unit.Instance. Otherwise, return the first + /// failure. + /// + public static Either OnSuccessAllReturnVoid( + this IEnumerable> items) + { + return items + .OnSuccessAll() + .OnSuccessVoid(); + } + public static Either OnSuccessVoid( this Either either) { diff --git a/src/GovUk.Education.ExploreEducationStatistics.Data.Services/SubjectMetaService.cs b/src/GovUk.Education.ExploreEducationStatistics.Data.Services/SubjectMetaService.cs index 1a1e8588285..45da17ef95c 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Data.Services/SubjectMetaService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Data.Services/SubjectMetaService.cs @@ -302,10 +302,10 @@ private async Task> ValidateFiltersForSubject( FiltersDifferFromSubject).OnSuccess(_ => { var requestMap = requestFilters.ToDictionary(filter => filter.Id); - return filters.Select(filter => + return filters + .Select(filter => ValidateFilterGroupsForSubject(filter, requestMap[filter.Id].FilterGroups)) - .OnSuccessAll() - .OnSuccessVoid(); + .OnSuccessAllReturnVoid(); }); } @@ -321,13 +321,14 @@ private static Either ValidateFilterGroupsForSubject( .OnSuccess(_ => { var requestMap = requestFilterGroups.ToDictionary(filterGroup => filterGroup.Id); - return filter.FilterGroups.Select(filterGroup => + return filter + .FilterGroups + .Select(filterGroup => AssertCollectionsAreSameIgnoringOrder( filterGroup.FilterItems.Select(filterItem => filterItem.Id), requestMap[filterGroup.Id].FilterItems, FilterItemsDifferFromSubject)) - .OnSuccessAll() - .OnSuccessVoid(); + .OnSuccessAllReturnVoid(); }); } @@ -349,8 +350,7 @@ private async Task> ValidateIndicatorGroupsForSubject indicatorGroup.Indicators.Select(indicator => indicator.Id), requestMap[indicatorGroup.Id].Indicators, IndicatorsDifferFromSubject)) - .OnSuccessAll() - .OnSuccessVoid(); + .OnSuccessAllReturnVoid(); }); } diff --git a/tests/robot-tests/tests/libs/admin-utilities.py b/tests/robot-tests/tests/libs/admin-utilities.py index 3c1d45c720c..2e57d1eadd0 100644 --- a/tests/robot-tests/tests/libs/admin-utilities.py +++ b/tests/robot-tests/tests/libs/admin-utilities.py @@ -14,9 +14,9 @@ sl = BuiltIn().get_library_instance("SeleniumLibrary") -def raise_assertion_error(self, err_msg): +def raise_assertion_error(err_msg): sl.failure_occurred() - self.logger.warn(err_msg) + logger.warn(err_msg) raise AssertionError(err_msg) From f223766c5a634101d37123552d77cc163d60fbdd Mon Sep 17 00:00:00 2001 From: Duncan Watson Date: Fri, 17 Nov 2023 15:47:43 +0000 Subject: [PATCH 2/2] EES-4680 - responding to PR comments. Refactored ReleaseApprovalService to not need a rollback but instead manually assign a ReleaseStatusId prior to sending the Publisher notification. Various cleanups and use of additional helpful methods. --- .../ReleaseApprovalServicePermissionTests.cs | 2 +- .../Services/ReleaseApprovalServiceTests.cs | 6 +- .../Services/ReleaseApprovalService.cs | 113 ++++++++---------- .../Services/ReleaseChecklistService.cs | 3 +- .../Services/ReleaseService.cs | 3 +- 5 files changed, 58 insertions(+), 69 deletions(-) diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServicePermissionTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServicePermissionTests.cs index 3eab6c5ba36..413069e984f 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServicePermissionTests.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServicePermissionTests.cs @@ -27,7 +27,7 @@ public class ReleaseApprovalServicePermissionTests private readonly Release _release = new() { Id = Guid.NewGuid(), - Publication= new Publication(), + Publication = new Publication(), Published = DateTime.Now, TimePeriodCoverage = TimeIdentifier.April }; diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServiceTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServiceTests.cs index 9f392d7cc68..9d5972a926a 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServiceTests.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/ReleaseApprovalServiceTests.cs @@ -1468,11 +1468,11 @@ public async Task CreateReleaseStatus_Approved_Scheduled_FailsSendingPreReleaseI .SingleAsync(r => r.Id == release.Id); // Assert that the failure to send emails prevented the Release from completing approval. - // The Release should remain unchanged from the original Release, as changes to it were rolled back. + // The Release should remain unchanged from the original Release. // Additionally, no new ReleaseStatus entries were added. saved.AssertDeepEqualTo(release); - // Futhermore, we have proven that the Publisher was not informed of the Release change, as it + // Furthermore, we have proven that the Publisher was not informed of the Release change, as it // did not complete. } } @@ -1583,7 +1583,7 @@ public async Task CreateReleaseStatus_Approved_Scheduled_FailsWhileInformingPubl .SingleAsync(r => r.Id == release.Id); // Assert that the failure to notify the Publisher prevented the Release from completing approval. - // The Release should remain unchanged from the original Release, as changes to it were rolled back. + // The Release should remain unchanged from the original Release. // Additionally, no new ReleaseStatus entries were added. saved.AssertDeepEqualTo(release); diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseApprovalService.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseApprovalService.cs index 7cdf9c3e69c..7a2f11a51b0 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseApprovalService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseApprovalService.cs @@ -83,8 +83,7 @@ public async Task>> GetRelease .Releases .Include(r => r.ReleaseStatuses) .ThenInclude(rs => rs.CreatedBy) - .SingleOrDefaultAsync(r => r.Id == releaseId) - .OrNotFound() + .SingleOrNotFoundAsync(r => r.Id == releaseId) .OnSuccess(_userService.CheckCanViewReleaseStatusHistory) .OnSuccess(async release => { @@ -116,61 +115,46 @@ public async Task> CreateReleaseStatus( return await _context .Releases .HydrateReleaseForChecklist() - .AsNoTracking() - .SingleOrDefaultAsync(r => r.Id == releaseId) - .OrNotFound() + .SingleOrNotFoundAsync(r => r.Id == releaseId) .OnSuccessDo(release => _userService.CheckCanUpdateReleaseStatus(release, request.ApprovalStatus)) .OnSuccessDo(() => ValidatePublishDate(request)) - .OnSuccess(async - // Keep a track of the original Release in the event that we need to roll back approval. - originalRelease => + .OnSuccess(async release => { - if (request.ApprovalStatus != ReleaseApprovalStatus.Approved && originalRelease.Published.HasValue) + if (request.ApprovalStatus != ReleaseApprovalStatus.Approved && release.Published.HasValue) { return ValidationActionResult(PublishedReleaseCannotBeUnapproved); } - var oldStatus = originalRelease.ApprovalStatus; - - var releaseToUpdate = await _context - .Releases - .HydrateReleaseForChecklist() - .SingleAsync(r => r.Id == releaseId); + var oldStatus = release.ApprovalStatus; - releaseToUpdate.ApprovalStatus = request.ApprovalStatus; - releaseToUpdate.NextReleaseDate = request.NextReleaseDate; - releaseToUpdate.NotifySubscribers = originalRelease.Version == 0 || (request.NotifySubscribers ?? false); - releaseToUpdate.UpdatePublishedDate = request.UpdatePublishedDate ?? false; - releaseToUpdate.PublishScheduled = request.PublishMethod == PublishMethod.Immediate + release.ApprovalStatus = request.ApprovalStatus; + release.NextReleaseDate = request.NextReleaseDate; + release.NotifySubscribers = release.Version == 0 || (request.NotifySubscribers ?? false); + release.UpdatePublishedDate = request.UpdatePublishedDate ?? false; + release.PublishScheduled = request.PublishMethod == PublishMethod.Immediate ? _dateTimeProvider.UtcNow : request.PublishScheduledDate; var releaseStatus = new ReleaseStatus { - Release = releaseToUpdate, + Id = Guid.NewGuid(), + Release = release, InternalReleaseNote = request.InternalReleaseNote, ApprovalStatus = request.ApprovalStatus, CreatedById = _userService.GetUserId() }; return await - ValidateReleaseWithChecklist(releaseToUpdate) - .OnSuccess(() => RemoveUnusedImages(releaseToUpdate.Id)) - .OnSuccessDo(async () => - { - _context.Releases.Update(releaseToUpdate); - await _context.AddAsync(releaseStatus); - await _context.SaveChangesAsync(); - }) + ValidateReleaseWithChecklist(release) + .OnSuccess(() => RemoveUnusedImages(release.Id)) + .OnSuccess(() => - SendEmailNotificationsAndInvites(request, releaseToUpdate) + SendEmailNotificationsAndInvites(request, release) .OnSuccess(() => NotifyPublisher(releaseId, request, oldStatus, releaseStatus)) - // If a failure occurs in either the sending of the emails or the Publisher notification, - // roll back changes to the Release and remove the added Release Status. - .OnFailureDo(async _ => + .OnSuccessDo(async () => { - _context.Entry(releaseToUpdate).CurrentValues.SetValues(originalRelease); - _context.ReleaseStatus.Remove(releaseStatus); + _context.Releases.Update(release); + await _context.AddAsync(releaseStatus); await _context.SaveChangesAsync(); })); }); @@ -207,30 +191,37 @@ private async Task> SendEmailNotificationsAndInvites( if (request.ApprovalStatus == ReleaseApprovalStatus.HigherLevelReview) { - var userReleaseRoles = - await _userReleaseRoleService.ListUserReleaseRolesByPublication( - ReleaseRole.Approver, - release.Publication.Id); - - var userPublicationRoles = await _context - .UserPublicationRoles - .Include(upr => upr.User) - .Where(upr => - upr.PublicationId == release.PublicationId - && upr.Role == PublicationRole.Approver) - .ToListAsync(); - - var notifyHigherReviewers = userReleaseRoles.Any() || userPublicationRoles.Any(); - - if (notifyHigherReviewers) - { - return userReleaseRoles - .Select(urr => urr.User.Email) - .Concat(userPublicationRoles.Select(upr => upr.User.Email)) - .Distinct() - .Select(email => _emailTemplateService.SendReleaseHigherReviewEmail(email, release)) - .OnSuccessAllReturnVoid(); - } + return await SendHigherLevelReviewNotificationEmails(release); + } + + return Unit.Instance; + } + + private async Task> SendHigherLevelReviewNotificationEmails(Release release) + { + var userReleaseRoles = + await _userReleaseRoleService.ListUserReleaseRolesByPublication( + ReleaseRole.Approver, + release.Publication.Id); + + var userPublicationRoles = await _context + .UserPublicationRoles + .Include(upr => upr.User) + .Where(upr => + upr.PublicationId == release.PublicationId + && upr.Role == PublicationRole.Approver) + .ToListAsync(); + + var notifyHigherReviewers = userReleaseRoles.Any() || userPublicationRoles.Any(); + + if (notifyHigherReviewers) + { + return userReleaseRoles + .Select(urr => urr.User.Email) + .Concat(userPublicationRoles.Select(upr => upr.User.Email)) + .Distinct() + .Select(email => _emailTemplateService.SendReleaseHigherReviewEmail(email, release)) + .OnSuccessAllReturnVoid(); } return Unit.Instance; @@ -238,8 +229,8 @@ await _userReleaseRoleService.ListUserReleaseRolesByPublication( private async Task> SendPreReleaseUserInviteEmails(Release release) { - var unsentUserReleaseInvites = await _context.UserReleaseInvites - .AsQueryable() + var unsentUserReleaseInvites = await _context + .UserReleaseInvites .Where(i => i.ReleaseId == release.Id && i.Role == ReleaseRole.PrereleaseViewer diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseChecklistService.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseChecklistService.cs index fac9e35ae01..120eb4372ad 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseChecklistService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseChecklistService.cs @@ -57,8 +57,7 @@ public async Task> GetChecklist( return await _contentDbContext .Releases .HydrateReleaseForChecklist() - .SingleOrDefaultAsync(r => r.Id == releaseId) - .OrNotFound() + .SingleOrNotFoundAsync(r => r.Id == releaseId) .OnSuccess(_userService.CheckCanViewRelease) .OnSuccess( async release => new ReleaseChecklistViewModel( diff --git a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs index bd4d9fdb4da..f9044c07081 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs @@ -345,8 +345,7 @@ public async Task> UpdateRelease( return await _context .Releases .HydrateReleaseForChecklist() - .SingleOrDefaultAsync(r => r.Id == releaseId) - .OrNotFound() + .SingleOrNotFoundAsync(r => r.Id == releaseId) .OnSuccess(_userService.CheckCanUpdateRelease) .OnSuccessDo(async release => await ValidateReleaseSlugUniqueToPublication(request.Slug, release.PublicationId, releaseId)) .OnSuccess(async release =>