-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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 Publishing change #5474
EES-5656 Publishing change #5474
Conversation
src/GovUk.Education.ExploreEducationStatistics.Publisher/PublisherHostBuilderExtensions.cs
Show resolved
Hide resolved
...n.ExploreEducationStatistics.Content.Model.Tests/Repository/ReleaseVersionRepositoryTests.cs
Outdated
Show resolved
Hide resolved
...n.ExploreEducationStatistics.Content.Model.Tests/Repository/ReleaseVersionRepositoryTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Can't spot anything too out of place. Only jotted a few things down. I've not ran the tests or checked if it builds or anything.
...GovUk.Education.ExploreEducationStatistics.Content.Services.Tests/PublicationServiceTests.cs
Show resolved
Hide resolved
@@ -469,11 +471,11 @@ public async Task Success() | |||
|
|||
var result = await repository.ListLatestReleaseVersionIds(publication.Id, publishedOnly: PublishedOnly); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be better to create a private method ListLatestReleaseVersionIds
, that feeds a value of false
for the publishedOnly
parameter into the method call, rather than storing the publishedOnly
parameter in a class constant.
so you call it like var result = await ListLatestReleaseVersionIds(repository, publication.Id)
, and you can forget about the publishedOnly
flag.
Similar for the other test classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good suggestion to reduce the number of mentions of the flag, so I made this change!
I did notice though that it's really easy to accidently do something like this, using the default publishedOnly = false
await repository.ListLatestReleaseVersionIds, publication.Id)
when we intend to always do something like this now
await ListLatestReleaseVersionIds(repository, publication.Id)
43eab57
to
5adfaca
Compare
e6a4d96
to
1fb1808
Compare
1fb1808
to
ca6b0f7
Compare
2634134
to
9787a3d
Compare
…s dependent on the value of `publishedOnly`
…test release page
…l to not use method ListLatestPublishedReleaseVersions
…blishedReleaseVersions
…blishedReleaseVersionIds and ListLatestReleaseVersions/ListLatestPublishedReleaseVersions with new publishedOnly flag.
…leaseVersion(Guid publicationId)
…ReleaseVersion(Guid publicationId)
…hedReleaseVersion(Guid publicationId)
…lishedReleaseVersion(Guid publicationId)
…ersion for publication based on the latest release with a published version
…ublishedReleaseVersionByReleaseSlug
…ods to use release series when working out the latest published release version of publication
ca6b0f7
to
9412715
Compare
This PR follows up the changes already made for EES-5656 in #5455.
It also builds upon #5426 which removed
IReleaseVersionRepository.ListLatestReleaseVersions(CancellationToken cancellationToken)
which returned release versions in reverse chronological order for User Management pages.Now on publishing a release version, we change the
LatestPublishedReleaseVersionId
of a publication based on the publication's release series. If the release version being published is within the same release as the latest release in the series then we will replace theLatestPublishedReleaseVersionId
with the current one being published. If the release version being published is not within the same release then it's left unchanged.This replaces the previous behaviour which replaced the value if the release version belonged to the publication's latest release in reverse chronological order according to time period and year.
It updates methods
IReleaseVersionRepository.GetLatestReleaseVersion
to use the latest release in the release series when determining the latest version, andIReleaseVersionRepository.ListLatestReleaseVersions
to use the release series to order the release versions by.Other changes
Remove
IReleaseVersionRepository.GetLatestPublishedRelease(Guid publicationId)
- This method which previously calculated the latest release version of a publication's latest release in reverse chronological order according to time period and year is removed in favour of usingPublication.LatestPublishedReleaseVersion
directly.Consolidated methods
IReleaseVersionRepository.ListLatestReleaseVersionIds/ListLatestPublishedReleaseVersionIds
andIReleaseVersionRepository.ListLatestReleaseVersions/ListLatestPublishedReleaseVersions
with newpublishedOnly
flag.UI test report