Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EES-5656 Release series tidy up #5444

Merged
merged 4 commits into from
Dec 10, 2024
Merged

EES-5656 Release series tidy up #5444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a46aef5
EES-5656 Release series tidy up
benoutram Dec 2, 2024
249bb0a
EES-5656 Release series unit test tidy up
benoutram Dec 2, 2024
b6e1c33
EES-5656 Further release series tidy up
benoutram Dec 3, 2024
504166c
EES-5656 No need to use `RequiredAttribute` with nullable context ena…
benoutram Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ...tion.ExploreEducationStatistics.Admin.Tests/Services/PublicationServicePermissionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,12 @@ await PermissionTestUtils.PolicyCheckBuilder<SecurityPolicies>()
var service = BuildPublicationService(
context: contentDbContext,
userService: userService.Object);
return await service.AddReleaseSeriesLegacyLink(publication.Id, new ReleaseSeriesLegacyLinkAddRequest());
return await service.AddReleaseSeriesLegacyLink(publication.Id,
new ReleaseSeriesLegacyLinkAddRequest
{
Description = "Test description",
Url = "https://test.url"
});
});
}

Expand Down
468 changes: 183 additions & 285 deletions ...ovUk.Education.ExploreEducationStatistics.Admin.Tests/Services/PublicationServiceTests.cs
View file
Open in desktop

Large diffs are not rendered by default.

18 changes: 8 additions & 10 deletions src/GovUk.Education.ExploreEducationStatistics.Admin/Requests/ReleaseSeriesItemRequests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,19 @@
#nullable enable

using System;

namespace GovUk.Education.ExploreEducationStatistics.Admin.Requests;

public class ReleaseSeriesLegacyLinkAddRequest
public record ReleaseSeriesLegacyLinkAddRequest
{
public string Description { get; set; } = string.Empty;
public string Url { get; set; } = string.Empty;
public required string Description { get; init; }

public required string Url { get; init; }
}

public class ReleaseSeriesItemUpdateRequest
public record ReleaseSeriesItemUpdateRequest
{
public Guid Id { get; set; }
public Guid? ReleaseId { get; set; }
public Guid? ReleaseId { get; init; }

public string? LegacyLinkDescription { get; set; }
public string? LegacyLinkUrl { get; set; }
public string? LegacyLinkDescription { get; init; }
public string? LegacyLinkUrl { get; init; }
}

5 changes: 0 additions & 5 deletions ...ation.ExploreEducationStatistics.Admin/Services/ManageContent/ManageContentPageService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,6 @@ await _releaseVersionRepository
{
return new ReleaseSeriesItemViewModel
{
Id = rsi.Id,
IsLegacyLink = rsi.IsLegacyLink,
Description = rsi.LegacyLinkDescription!,
LegacyLinkUrl = rsi.LegacyLinkUrl,
};
Expand All @@ -130,10 +128,7 @@ await _releaseVersionRepository

return new ReleaseSeriesItemViewModel
{
Id = rsi.Id,
IsLegacyLink = rsi.IsLegacyLink,
Description = latestReleaseVersion.Title,

ReleaseId = latestReleaseVersion.ReleaseId,
ReleaseSlug = latestReleaseVersion.Slug,
};
Expand Down
85 changes: 29 additions & 56 deletions src/GovUk.Education.ExploreEducationStatistics.Admin/Services/PublicationService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,12 @@
using Microsoft.EntityFrameworkCore;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Threading.Tasks;
using GovUk.Education.ExploreEducationStatistics.Admin.Services.Interfaces;
using static GovUk.Education.ExploreEducationStatistics.Admin.Validators.ValidationErrorMessages;
using static GovUk.Education.ExploreEducationStatistics.Admin.Validators.ValidationUtils;
using ExternalMethodologyViewModel = GovUk.Education.ExploreEducationStatistics.Admin.ViewModels.ExternalMethodologyViewModel;
using IPublicationRepository = GovUk.Education.ExploreEducationStatistics.Admin.Services.Interfaces.IPublicationRepository;
using IPublicationService = GovUk.Education.ExploreEducationStatistics.Admin.Services.Interfaces.IPublicationService;
using IReleaseVersionRepository = GovUk.Education.ExploreEducationStatistics.Content.Model.Repository.Interfaces.IReleaseVersionRepository;
using PublicationViewModel = GovUk.Education.ExploreEducationStatistics.Admin.ViewModels.PublicationViewModel;
using ReleaseSummaryViewModel = GovUk.Education.ExploreEducationStatistics.Admin.ViewModels.ReleaseSummaryViewModel;
Expand Down Expand Up @@ -435,11 +433,12 @@ public async Task<Either<ActionResult, List<ReleaseSummaryViewModel>>> ListLates
});
}

public async Task<Either<ActionResult, List<ReleaseSeriesTableEntryViewModel>>> GetReleaseSeries(Guid publicationId)
public async Task<Either<ActionResult, List<ReleaseSeriesTableEntryViewModel>>> GetReleaseSeries(
Guid publicationId)
{
return await _persistenceHelper
.CheckEntityExists<Publication>(publicationId)
.OnSuccess(publication => _userService.CheckCanViewPublication(publication))
return await _context.Publications
.FirstOrNotFoundAsync(p => p.Id == publicationId)
.OnSuccess(_userService.CheckCanViewPublication)
.OnSuccess(async publication =>
{
var result = new List<ReleaseSeriesTableEntryViewModel>();
Expand All @@ -450,43 +449,28 @@ public async Task<Either<ActionResult, List<ReleaseSeriesTableEntryViewModel>>>
result.Add(new ReleaseSeriesTableEntryViewModel
{
Id = seriesItem.Id,
IsLegacyLink = true,
Description = seriesItem.LegacyLinkDescription!,
LegacyLinkUrl = seriesItem.LegacyLinkUrl,
});
}
else
{
// prefer getting the latest published version over an unpublished amendment
var latestVersion = await _context.ReleaseVersions
.LatestReleaseVersion(seriesItem.ReleaseId!.Value, publishedOnly: true)
.SingleOrDefaultAsync();
var release = await _context.Releases
.SingleAsync(r => r.Id == seriesItem.ReleaseId);

if (latestVersion == null)
{
// if the release has no published version, then use its original unpublished version
latestVersion = await _context.ReleaseVersions
.LatestReleaseVersion(seriesItem.ReleaseId!.Value)
.SingleOrDefaultAsync();

if (latestVersion == null)
{
throw new InvalidDataException(
"ReleaseSeriesItem with ReleaseId set should have an associated " +
$"ReleaseVersion. Release: {seriesItem.ReleaseId} " +
$"ReleaseSeriesItem: {seriesItem.Id}");
}
}
var latestPublishedReleaseVersion = await _context.ReleaseVersions
.LatestReleaseVersion(releaseId: seriesItem.ReleaseId!.Value, publishedOnly: true)
.SingleOrDefaultAsync();

result.Add(new ReleaseSeriesTableEntryViewModel
{
Id = seriesItem.Id,
IsLegacyLink = false,
Description = latestVersion.Title,
ReleaseId = latestVersion.ReleaseId,
ReleaseSlug = latestVersion.Slug,
IsLatest = latestVersion.Id == publication.LatestPublishedReleaseVersionId,
IsPublished = latestVersion.Live,
ReleaseId = release.Id,
Description = release.Title,
ReleaseSlug = release.Slug,
IsLatest = publication.LatestPublishedReleaseVersionId != null &&
latestPublishedReleaseVersion?.Id == publication.LatestPublishedReleaseVersionId,
IsPublished = latestPublishedReleaseVersion != null
});
}
}
Expand All @@ -507,7 +491,6 @@ public async Task<Either<ActionResult, List<ReleaseSeriesTableEntryViewModel>>>
publication.ReleaseSeries.Add(new ReleaseSeriesItem
{
Id = Guid.NewGuid(),
ReleaseId = null,
LegacyLinkDescription = newLegacyLink.Description,
LegacyLinkUrl = newLegacyLink.Url,
});
Expand All @@ -517,8 +500,7 @@ public async Task<Either<ActionResult, List<ReleaseSeriesTableEntryViewModel>>>

await _publicationCacheService.UpdatePublication(publication.Slug);

return await GetReleaseSeries(publication.Id)
.OnSuccess(releaseSeries => releaseSeries);
return await GetReleaseSeries(publication.Id);
});
}

Expand All @@ -527,7 +509,6 @@ public async Task<Either<ActionResult, List<ReleaseSeriesTableEntryViewModel>>>
List<ReleaseSeriesItemUpdateRequest> updatedReleaseSeriesItems)
{
return await _context.Publications
.Include(p => p.ReleaseVersions)
.FirstOrNotFoundAsync(p => p.Id == publicationId)
.OnSuccess(_userService.CheckCanManageReleaseSeries)
.OnSuccess(async publication =>
Expand All @@ -538,56 +519,48 @@ public async Task<Either<ActionResult, List<ReleaseSeriesTableEntryViewModel>>>
if (seriesItem.ReleaseId != null && (
seriesItem.LegacyLinkDescription != null || seriesItem.LegacyLinkUrl != null))
{
throw new ArgumentException(
$"LegacyLink details shouldn't be set if ReleaseId is set. ReleaseSeriesItem: {seriesItem.Id}");
throw new ArgumentException("LegacyLink details shouldn't be set if ReleaseId is set.");
}

if (seriesItem.ReleaseId == null && (
seriesItem.LegacyLinkDescription == null || seriesItem.LegacyLinkUrl == null))
{
throw new ArgumentException(
$"LegacyLink details should be set if ReleaseId is null. ReleaseSeriesItem: {seriesItem.Id}");
throw new ArgumentException("LegacyLink details should be set if ReleaseId is null.");
}
}

// Check all publication releases are included in updatedReleaseSeriesItems
var publicationReleaseIds = publication.ReleaseVersions
.Select(rv => rv.ReleaseId)
.Distinct()
.ToList();
var publicationReleaseIds = await _context.Releases
.Where(r => r.PublicationId == publicationId)
.Select(r => r.Id)
.ToListAsync();
mmyoungman marked this conversation as resolved.
Show resolved Hide resolved

var updatedSeriesReleaseIds = updatedReleaseSeriesItems
.Where(rsi => rsi.ReleaseId != null)
.Where(rsi => rsi.ReleaseId.HasValue)
.Select(rsi => rsi.ReleaseId!.Value)
.ToList();

if (!ComparerUtils.SequencesAreEqualIgnoringOrder(publicationReleaseIds, updatedSeriesReleaseIds))
{
throw new ArgumentException(
"Missing or duplicate release in new release series. Expected ReleaseIds: " +
publicationReleaseIds.Select(id => id.ToString()).JoinToString(","));
publicationReleaseIds.JoinToString(","));
}

// NOTE: A malicious user could change the release series items' Ids, but we don't care

var newReleaseSeries = updatedReleaseSeriesItems
publication.ReleaseSeries = updatedReleaseSeriesItems
.Select(request => new ReleaseSeriesItem
{
Id = request.Id,
Id = Guid.NewGuid(),
ReleaseId = request.ReleaseId,
LegacyLinkDescription = request.LegacyLinkDescription,
LegacyLinkUrl = request.LegacyLinkUrl,
}).ToList();

publication.ReleaseSeries = newReleaseSeries;
_context.Publications.Update(publication);

await _context.SaveChangesAsync();

await _publicationCacheService.UpdatePublication(publication.Slug);

return await GetReleaseSeries(publication.Id)
.OnSuccess(releaseSeries => releaseSeries);
return await GetReleaseSeries(publication.Id);
});
}

Expand Down
2 changes: 1 addition & 1 deletion src/GovUk.Education.ExploreEducationStatistics.Admin/Services/ReleaseService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ await CreateGenericContentFromTemplate(releaseCreate.TemplateReleaseId.Value,
publication.ReleaseSeries.Insert(0, new ReleaseSeriesItem
{
Id = Guid.NewGuid(),
ReleaseId = newReleaseVersion.ReleaseId,
ReleaseId = release.Id
});
_context.Publications.Update(publication);

Expand Down
18 changes: 9 additions & 9 deletions ...Education.ExploreEducationStatistics.Admin/ViewModels/ReleaseSeriesTableEntryViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,17 @@ namespace GovUk.Education.ExploreEducationStatistics.Admin.ViewModels;

public record ReleaseSeriesTableEntryViewModel
{
public Guid Id { get; set; }
public bool IsLegacyLink { get; set; }
public string Description { get; set; } = string.Empty;
public required Guid Id { get; init; }
public required string Description { get; init; }

// used by EES release series item
public Guid? ReleaseId { get; set; }
public string? ReleaseSlug { get; set; }
public bool? IsLatest { get; set; }
public bool? IsPublished { get; set; }
public Guid? ReleaseId { get; init; }
public string? ReleaseSlug { get; init; }
public bool? IsLatest { get; init; }
public bool? IsPublished { get; init; }

// used by legacy link series item
public string? LegacyLinkUrl { get; set; }
}
public string? LegacyLinkUrl { get; init; }

public bool IsLegacyLink => ReleaseId == null;
}
12 changes: 6 additions & 6 deletions ...ovUk.Education.ExploreEducationStatistics.Common/ViewModels/ReleaseSeriesItemViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ namespace GovUk.Education.ExploreEducationStatistics.Common.ViewModels;

public record ReleaseSeriesItemViewModel
{
public Guid Id { get; set; }
public bool IsLegacyLink { get; set; }
public string Description { get; set; } = string.Empty;
public required string Description { get; init; }

// used by EES release series item
public Guid? ReleaseId { get; set; }
public string? ReleaseSlug { get; set; }
public Guid? ReleaseId { get; init; }
public string? ReleaseSlug { get; init; }

// used by legacy link series item
public string? LegacyLinkUrl { get; set; }
public string? LegacyLinkUrl { get; init; }

public bool IsLegacyLink => ReleaseId == null;
}
3 changes: 3 additions & 0 deletions src/GovUk.Education.ExploreEducationStatistics.Content.Model/ReleaseSeriesItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ namespace GovUk.Education.ExploreEducationStatistics.Content.Model;

public record ReleaseSeriesItem
{
/// <summary>
/// Unique identifier for the ReleaseSeriesItem which exists to allow safely managing legacy links in the UI.
/// </summary>
public Guid Id { get; set; }

public Guid? ReleaseId { get; set; }
Expand Down
10 changes: 4 additions & 6 deletions ...n.ExploreEducationStatistics.Content.Services.Tests/Cache/PublicationCacheServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,14 @@ public class PublicationCacheServiceTests : CacheServiceTestFixture
Title = ""
}
},
ReleaseSeries = new()
{
new()
ReleaseSeries =
[
new ReleaseSeriesItemViewModel
{
Id = Guid.NewGuid(),
IsLegacyLink = true,
Description = "legacy link description",
LegacyLinkUrl = "http://test.com/",
}
},
],
Theme = new ThemeViewModel(
Guid.NewGuid(),
Slug: "",
Expand Down
1 change: 0 additions & 1 deletion ...Uk.Education.ExploreEducationStatistics.Content.Services.Tests/PublicationServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,6 @@ public async Task Success()
Assert.Null(releaseSeriesItem2.LegacyLinkUrl);

var releaseSeriesItem3 = publicationViewModel.ReleaseSeries[2];
Assert.Equal(_legacyLinks[0].Id, releaseSeriesItem3.Id);
Assert.True(releaseSeriesItem3.IsLegacyLink);
Assert.Null(releaseSeriesItem3.ReleaseId);
Assert.Equal(_legacyLinks[0].LegacyLinkDescription, releaseSeriesItem3.Description);
Expand Down
5 changes: 0 additions & 5 deletions src/GovUk.Education.ExploreEducationStatistics.Content.Services/PublicationService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,6 @@ public async Task<Either<ActionResult, PublicationCacheViewModel>> Get(string pu
{
return new ReleaseSeriesItemViewModel
{
Id = rsi.Id,
IsLegacyLink = rsi.IsLegacyLink,
Description = rsi.LegacyLinkDescription!,
LegacyLinkUrl = rsi.LegacyLinkUrl,
};
Expand All @@ -112,10 +110,7 @@ public async Task<Either<ActionResult, PublicationCacheViewModel>> Get(string pu

return new ReleaseSeriesItemViewModel
{
Id = rsi.Id,
IsLegacyLink = rsi.IsLegacyLink,
Description = latestReleaseVersion.Title,

ReleaseId = latestReleaseVersion.ReleaseId,
ReleaseSlug = latestReleaseVersion.Slug,
};
Expand Down
1 change: 0 additions & 1 deletion ...ion-statistics-admin/src/pages/publication/PublicationEditReleaseSeriesLegacyLinkPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ export const mapToReleaseSeriesItemUpdateRequest = (
releaseSeries: ReleaseSeriesTableEntry[],
): ReleaseSeriesItemUpdateRequest[] => {
return releaseSeries.map(seriesItem => ({
id: seriesItem.id,
releaseId: seriesItem.releaseId,
legacyLinkDescription: seriesItem.isLegacyLink
? seriesItem.description
Expand Down
Loading
Loading