diff --git a/src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor b/src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor index e0ca5c3e8..90bfed11b 100644 --- a/src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor +++ b/src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor @@ -5,6 +5,7 @@ @using FluentCMS.Core.ChangeSets @using FluentCMS.Core.Desk @inject IChangeSetDirectory Directory +@inject IChangeSetLists Lists @inject IDeskQueues Queues @inject FluentCMS.Core.Checks.IChangeSetChecks Checks @inject IAuditReader Audit @@ -461,7 +462,7 @@ private async Task SubmitAsync() { - await ActAsync(Submit, async () => + await ActAsync(Submit, [ChangeSetState.Open], async () => { var submitted = await Checks.SubmitAsync(Id); return submitted.IsSuccess @@ -472,7 +473,7 @@ private async Task ApproveAsync() { - if (await ActAsync(Approve, () => Directory.TransitionAsync(Id, ChangeSetState.Approved), L["Approved. Publish the set to make its items live."]) is not null) + if (await ActAsync(Approve, [ChangeSetState.AwaitingReview], () => Directory.TransitionAsync(Id, ChangeSetState.Approved), L["Approved. Publish the set to make its items live."]) is not null) { MoveOn("approved"); } @@ -480,12 +481,12 @@ private Task PublishAsync() { - return ActAsync(Publish, () => Directory.PublishAsync(Id), L["Published. Its items are live."]); + return ActAsync(Publish, [ChangeSetState.Approved], () => Directory.PublishAsync(Id), L["Published. Its items are live."]); } private async Task RejectAsync() { - if (await ActAsync(Reject, () => Directory.RejectAsync(Id, Reject?.Reason ?? string.Empty), L["Rejected. The reason is stored with the set."]) is not null) + if (await ActAsync(Reject, RejectableStates, () => Directory.RejectAsync(Id, Reject?.Reason ?? string.Empty), L["Rejected. The reason is stored with the set."]) is not null) { MoveOn("rejected"); } @@ -503,24 +504,44 @@ Navigation.NavigateTo(_next is { } next ? $"sets/{next}?queue={slug}&done={done}" : $"?queue={slug}&done={done}"); } + // A revert already in progress is where a second click lands, so a resubmitted page or a later click opens no second set. Two + // posts at the same instant can still both open one, until the directory refuses it (#2646). private async Task RevertAsync() { var rationale = _set is { } set ? L["Revert change set {0}: {1}", set.Id, set.Rationale].Value : string.Empty; - if (await ActAsync(Revert, () => Directory.RevertAsync(Id, rationale), null) is { } opened) + var opened = await ActAsync(Revert, [ChangeSetState.Published], async () => + { + var inProgress = await Lists.ListAsync(new ChangeSetListQuery { States = RevertInProgress, Reverts = Id, Limit = 1 }); + return inProgress is { IsSuccess: true, Value.Items: [var existing, ..] } + ? await Directory.GetAsync(existing.Id) + : await Directory.RevertAsync(Id, rationale); + }, null); + if (opened is not null) { Navigation.NavigateTo($"sets/{opened.Id}"); } } + private static readonly ChangeSetState[] RejectableStates = [ChangeSetState.AwaitingReview, ChangeSetState.ChecksFailed, ChangeSetState.Approved]; + + // A revert set that may still publish; a rejected one leaves room for another. + private static readonly ChangeSetState[] RevertInProgress = + [ChangeSetState.Open, ChangeSetState.ChecksRunning, ChangeSetState.ChecksFailed, ChangeSetState.AwaitingReview, ChangeSetState.Approved]; + // A decision is saved by the call itself; the page then shows the set as it is now, whatever the outcome. - private async Task ActAsync(ReviewForm? form, Func>> action, string? done) + // An action is taken only from a state the panel offers it in, whatever a post claims, so no set skips its checks or review. + private async Task ActAsync( + ReviewForm? form, + IReadOnlyCollection offeredIn, + Func>> action, + string? done) { if (_set is null) { return null; } - if (form?.Seen is null || form.Seen != _seen) + if (form?.Seen is null || form.Seen != _seen || !offeredIn.Contains(_set.State)) { _refusal = L["The set has moved on since this page was shown. Review it as it is now."]; return null; diff --git a/src/FluentCMS.Admin/Components/Review/ChangeSetItems.razor b/src/FluentCMS.Admin/Components/Review/ChangeSetItems.razor index f8ffcaec3..a934aaf53 100644 --- a/src/FluentCMS.Admin/Components/Review/ChangeSetItems.razor +++ b/src/FluentCMS.Admin/Components/Review/ChangeSetItems.razor @@ -1,5 +1,6 @@ @using FluentCMS.Core.ChangeSets -@inject IChangeSetDirectory Directory +@using Microsoft.Extensions.DependencyInjection +@inject IServiceScopeFactory Scopes @inject IStringLocalizer L @* The set view's interactive island: DiffView adds items in batches over the circuit (PERF-04) while the rest of the page stays static. *@ @@ -32,7 +33,10 @@ return; } - var diff = await Directory.DiffAsync(ChangeSetId); + // Prerendered while the set view may still be loading: an async read on PostgreSQL can finish at once, so the view renders the + // island before its own reads are done. Its own scope keeps the two off one DbContext. + await using var scope = Scopes.CreateAsyncScope(); + var diff = await scope.ServiceProvider.GetRequiredService().DiffAsync(ChangeSetId); if (diff.IsSuccess) { (_diff, _error) = (diff.Value, null); diff --git a/src/FluentCMS.Core/ChangeSets/ChangeSetListQuery.cs b/src/FluentCMS.Core/ChangeSets/ChangeSetListQuery.cs index dd42548fb..7b7b657eb 100644 --- a/src/FluentCMS.Core/ChangeSets/ChangeSetListQuery.cs +++ b/src/FluentCMS.Core/ChangeSets/ChangeSetListQuery.cs @@ -15,6 +15,9 @@ public sealed record ChangeSetListQuery // Only the sets this person or agent opened; null for every set. public Initiator? OpenedBy { get; init; } + // Only the sets that revert this set; null for every set. + public Guid? Reverts { get; init; } + // Newest first puts the set that entered its state last first; otherwise the set that has waited longest comes first. public bool NewestFirst { get; init; } diff --git a/src/FluentCMS.Infrastructure/ChangeSets/ChangeSetLists.cs b/src/FluentCMS.Infrastructure/ChangeSets/ChangeSetLists.cs index 063248fae..595663d22 100644 --- a/src/FluentCMS.Infrastructure/ChangeSets/ChangeSetLists.cs +++ b/src/FluentCMS.Infrastructure/ChangeSets/ChangeSetLists.cs @@ -78,7 +78,7 @@ private static (string Sql, List Parameters) Select(ChangeSetListQuery q (SELECT MAX(e."Timestamp") FROM "AuditEvents" e WHERE e."SubjectType" = {movedSubject} AND e."SubjectId" = s."Id" AND e."Operation" IN ({moves})) AS "StateSince" FROM "ChangeSets" s - WHERE s."SiteId" = {site} AND s."State" IN ({states}){OpenedBy(query, parameters)} + WHERE s."SiteId" = {site} AND s."State" IN ({states}){OpenedBy(query, parameters)}{Reverts(query, parameters)} ) AS "Listed" """; @@ -101,6 +101,11 @@ private static string OpenedBy(ChangeSetListQuery query, List parameters return $" AND s.\"InitiatorKind\" = {Parameter(parameters, kind)} AND s.\"InitiatorId\" = {Parameter(parameters, id)}"; } + private static string Reverts(ChangeSetListQuery query, List parameters) + { + return query.Reverts is { } reverted ? $" AND s.\"RevertsChangeSetId\" = {Parameter(parameters, reverted)}" : string.Empty; + } + private static string Parameter(List parameters, object value) { parameters.Add(value); diff --git a/tests/FluentCMS.IntegrationTests/Desk/ReviewActionTests.cs b/tests/FluentCMS.IntegrationTests/Desk/ReviewActionTests.cs index 34cc65bdf..720b49986 100644 --- a/tests/FluentCMS.IntegrationTests/Desk/ReviewActionTests.cs +++ b/tests/FluentCMS.IntegrationTests/Desk/ReviewActionTests.cs @@ -12,17 +12,21 @@ namespace FluentCMS.IntegrationTests.Desk; public sealed partial class ReviewActionTests { - [Fact] - public async Task A_reviewer_approves_and_then_publishes_a_set_and_each_step_names_them() + [Theory] + [InlineData(DatabaseProviders.Sqlite)] + [PostgresData] + public async Task A_reviewer_approves_and_then_publishes_a_set_and_each_step_names_them(string provider) { using var install = new TemporaryInstall(); - await using var host = await AdminHost.StartAsync(install); + await using var database = await TemporaryDatabase.CreateAsync(provider); + await using var host = await AdminHost.StartAsync(install, settings: database.Configuration); var ownerId = await host.CreateFirstAdministratorAsync(); var set = await AwaitingReviewAsync(host, ownerId, "product"); await host.SignInAsync(AdminHost.Owner); var approved = await ActAsync(host, set.Id, "approve"); Assert.Equal(HttpStatusCode.OK, approved.StatusCode); + Assert.Equal(ChangeSetState.Approved, await StateAsync(host, ownerId, set.Id)); var page = await approved.Content.ReadAsStringAsync(); Assert.Contains("data-testid=\"review-message\"", page); Assert.Contains("data-state=\"approved\"", page); @@ -38,11 +42,14 @@ public async Task A_reviewer_approves_and_then_publishes_a_set_and_each_step_nam Assert.Contains($"data-change-set=\"{set.Id:N}\"", await host.Client.GetStringAsync("/?queue=recently-published")); } - [Fact] - public async Task Reviewing_from_a_queue_moves_on_to_its_next_set_and_back_to_the_queue_once_it_is_empty() + [Theory] + [InlineData(DatabaseProviders.Sqlite)] + [PostgresData] + public async Task Reviewing_from_a_queue_moves_on_to_its_next_set_and_back_to_the_queue_once_it_is_empty(string provider) { using var install = new TemporaryInstall(); - await using var host = await AdminHost.StartAsync(install); + await using var database = await TemporaryDatabase.CreateAsync(provider); + await using var host = await AdminHost.StartAsync(install, settings: database.Configuration); var ownerId = await host.CreateFirstAdministratorAsync(); var first = await AwaitingReviewAsync(host, ownerId, "product"); var second = await AwaitingReviewAsync(host, ownerId, "category"); @@ -57,11 +64,14 @@ public async Task Reviewing_from_a_queue_moves_on_to_its_next_set_and_back_to_th Assert.Contains("data-testid=\"desk-done\"", await host.Client.GetStringAsync(host.LocationOf(rejected)!)); } - [Fact] - public async Task Submitting_an_open_set_runs_its_checks_and_the_view_shows_each_result_with_its_evidence() + [Theory] + [InlineData(DatabaseProviders.Sqlite)] + [PostgresData] + public async Task Submitting_an_open_set_runs_its_checks_and_the_view_shows_each_result_with_its_evidence(string provider) { using var install = new TemporaryInstall(); - await using var host = await AdminHost.StartAsync(install); + await using var database = await TemporaryDatabase.CreateAsync(provider); + await using var host = await AdminHost.StartAsync(install, settings: database.Configuration); var ownerId = await host.CreateFirstAdministratorAsync(); await host.AsUserAsync(ownerId, async types => (await types.CreateAsync(new NewContentType( "Product page", "product-page", new ContentTypeDefinition([new FieldDefinition("headline", "text", required: true)])), TestChangeSetTargets.ContentModel)).Value); @@ -99,11 +109,14 @@ await host.AsUserAsync(ownerId, async types Assert.Contains("best in class", result); } - [Fact] - public async Task A_rejection_needs_a_reason_which_the_view_then_shows() + [Theory] + [InlineData(DatabaseProviders.Sqlite)] + [PostgresData] + public async Task A_rejection_needs_a_reason_which_the_view_then_shows(string provider) { using var install = new TemporaryInstall(); - await using var host = await AdminHost.StartAsync(install); + await using var database = await TemporaryDatabase.CreateAsync(provider); + await using var host = await AdminHost.StartAsync(install, settings: database.Configuration); var ownerId = await host.CreateFirstAdministratorAsync(); var set = await AwaitingReviewAsync(host, ownerId, "product"); await host.SignInAsync(AdminHost.Owner); @@ -119,11 +132,14 @@ public async Task A_rejection_needs_a_reason_which_the_view_then_shows() Assert.Equal(ChangeSetState.Rejected, (await host.AsUserAsync(ownerId, async sets => (await sets.GetAsync(set.Id)).Value)).State); } - [Fact] - public async Task Revert_opens_the_set_that_undoes_a_published_one_in_one_click_and_lands_on_it() + [Theory] + [InlineData(DatabaseProviders.Sqlite)] + [PostgresData] + public async Task Revert_opens_the_set_that_undoes_a_published_one_in_one_click_and_lands_on_it(string provider) { using var install = new TemporaryInstall(); - await using var host = await AdminHost.StartAsync(install); + await using var database = await TemporaryDatabase.CreateAsync(provider); + await using var host = await AdminHost.StartAsync(install, settings: database.Configuration); var ownerId = await host.CreateFirstAdministratorAsync(); var set = await AwaitingReviewAsync(host, ownerId, "product"); await host.SignInAsync(AdminHost.Owner); @@ -141,13 +157,63 @@ public async Task Revert_opens_the_set_that_undoes_a_published_one_in_one_click_ Assert.Contains($"Revert change set {set.Id}: Add the product type.", page); var revert = await host.AsUserAsync(ownerId, async sets => (await sets.GetAsync(revertId)).Value); Assert.Equal(set.Id, revert.RevertsChangeSetId); + + // Back and click again lands on the revert already in progress. + var again = await ActAsync(host, set.Id, "revert"); + Assert.Equal(location, host.LocationOf(again)); + Assert.Equal(1, await QueryAsync(host, context => context.ChangeSets.CountAsync(row => row.RevertsChangeSetId == set.Id))); + } + + [Theory] + [InlineData(DatabaseProviders.Sqlite)] + [PostgresData] + public async Task An_action_posted_for_a_state_that_does_not_offer_it_is_refused_and_changes_nothing(string provider) + { + using var install = new TemporaryInstall(); + await using var database = await TemporaryDatabase.CreateAsync(provider); + await using var host = await AdminHost.StartAsync(install, settings: database.Configuration); + var ownerId = await host.CreateFirstAdministratorAsync(); + var open = await OpenAsync(host, ownerId, "product"); + var running = await OpenAsync(host, ownerId, "category", ChangeSetState.ChecksRunning); + var failed = await OpenAsync(host, ownerId, "brand", ChangeSetState.ChecksRunning, ChangeSetState.ChecksFailed); + var waiting = await AwaitingReviewAsync(host, ownerId, "region"); + var approved = await OpenAsync(host, ownerId, "store", ChangeSetState.ChecksRunning, ChangeSetState.AwaitingReview, ChangeSetState.Approved); + var published = await OpenAsync(host, ownerId, "tag", ChangeSetState.ChecksRunning, ChangeSetState.AwaitingReview, ChangeSetState.Approved); + await host.AsUserAsync(ownerId, async sets => (await sets.PublishAsync(published.Id)).Value); + await host.SignInAsync(AdminHost.Owner); + + // Each post carries the set's current Seen, as someone could read it from the audit page. + (ChangeSet Set, string Action)[] refused = + [ + (running, "approve"), (failed, "approve"), (open, "approve"), (published, "approve"), + (waiting, "publish"), (approved, "revert"), (failed, "submit"), (open, "reject"), + ]; + foreach (var (set, action) in refused) + { + var before = await StateAsync(host, ownerId, set.Id); + var form = new Dictionary { [$"{char.ToUpperInvariant(action[0])}{action[1..]}.Seen"] = await LastMoveAsync(host, set.Id) }; + if (action == "reject") + { + form["Reject.Reason"] = "Not now."; + } + + var page = await (await host.SubmitAsync($"/sets/{set.Id}", action, form)).Content.ReadAsStringAsync(); + + Assert.True(page.Contains("data-testid=\"review-refusal\"", StringComparison.Ordinal), $"{action} on a {before} set was not refused."); + Assert.Equal(before, await StateAsync(host, ownerId, set.Id)); + } + + Assert.Equal(0, await QueryAsync(host, context => context.ChangeSets.CountAsync(row => row.RevertsChangeSetId != null))); } - [Fact] - public async Task An_action_on_a_set_that_moved_on_since_the_page_was_shown_is_refused() + [Theory] + [InlineData(DatabaseProviders.Sqlite)] + [PostgresData] + public async Task An_action_on_a_set_that_moved_on_since_the_page_was_shown_is_refused(string provider) { using var install = new TemporaryInstall(); - await using var host = await AdminHost.StartAsync(install); + await using var database = await TemporaryDatabase.CreateAsync(provider); + await using var host = await AdminHost.StartAsync(install, settings: database.Configuration); var ownerId = await host.CreateFirstAdministratorAsync(); var set = await AwaitingReviewAsync(host, ownerId, "product"); await host.SignInAsync(AdminHost.Owner); @@ -164,30 +230,46 @@ public async Task An_action_on_a_set_that_moved_on_since_the_page_was_shown_is_r Assert.Single(await MovesAsync(host, set.Id), row => row.Operation == "change_set.approved"); } - [Fact] - public async Task A_post_without_the_anti_forgery_token_changes_nothing_and_an_anonymous_one_goes_to_sign_in() + [Theory] + [InlineData(DatabaseProviders.Sqlite)] + [PostgresData] + public async Task A_post_without_the_anti_forgery_token_changes_nothing_and_an_anonymous_one_goes_to_sign_in(string provider) { using var install = new TemporaryInstall(); - await using var host = await AdminHost.StartAsync(install); + await using var database = await TemporaryDatabase.CreateAsync(provider); + await using var host = await AdminHost.StartAsync(install, settings: database.Configuration); var ownerId = await host.CreateFirstAdministratorAsync(); var set = await AwaitingReviewAsync(host, ownerId, "product"); - var anonymous = await host.Client.PostAsync($"/sets/{set.Id}", Form(("_handler", "approve"))); + string[] actions = ["submit", "approve", "publish", "reject", "revert"]; + foreach (var action in actions) + { + var anonymous = await host.Client.PostAsync($"/sets/{set.Id}", Form(("_handler", action))); + Assert.Equal(HttpStatusCode.Redirect, anonymous.StatusCode); + Assert.StartsWith("/sign-in?", host.LocationOf(anonymous)); + } + await host.SignInAsync(AdminHost.Owner); - var seen = Seen(await host.Client.GetStringAsync($"/sets/{set.Id}"), "Approve"); - var forged = await host.Client.PostAsync($"/sets/{set.Id}", Form(("_handler", "approve"), ("Approve.Seen", seen))); + var seen = await LastMoveAsync(host, set.Id); + var events = (await MovesAsync(host, set.Id)).Count; + foreach (var action in actions) + { + var name = $"{char.ToUpperInvariant(action[0])}{action[1..]}"; + var forged = await host.Client.PostAsync($"/sets/{set.Id}", Form(("_handler", action), ($"{name}.Seen", seen), ("Reject.Reason", "Forged."))); + Assert.Equal(HttpStatusCode.BadRequest, forged.StatusCode); + } - Assert.Equal(HttpStatusCode.Redirect, anonymous.StatusCode); - Assert.StartsWith("/sign-in?", host.LocationOf(anonymous)); - Assert.Equal(HttpStatusCode.BadRequest, forged.StatusCode); - Assert.DoesNotContain(await MovesAsync(host, set.Id), row => row.Operation == "change_set.approved"); + Assert.Equal(events, (await MovesAsync(host, set.Id)).Count); } - [Fact] - public async Task Only_the_actions_the_state_allows_are_offered() + [Theory] + [InlineData(DatabaseProviders.Sqlite)] + [PostgresData] + public async Task Only_the_actions_the_state_allows_are_offered(string provider) { using var install = new TemporaryInstall(); - await using var host = await AdminHost.StartAsync(install); + await using var database = await TemporaryDatabase.CreateAsync(provider); + await using var host = await AdminHost.StartAsync(install, settings: database.Configuration); var ownerId = await host.CreateFirstAdministratorAsync(); var waiting = await AwaitingReviewAsync(host, ownerId, "product"); var failed = await OpenAsync(host, ownerId, "category", ChangeSetState.ChecksRunning, ChangeSetState.ChecksFailed); @@ -253,6 +335,23 @@ private static async Task OpenAsync(AdminHost host, Guid userId, stri }); } + private static Task StateAsync(AdminHost host, Guid userId, Guid id) + { + return host.AsUserAsync(userId, async sets => (await sets.GetAsync(id)).Value.State); + } + + // The Seen of a set as the page computes it: the id of its last move. + private static async Task LastMoveAsync(AdminHost host, Guid id) + { + return (await MovesAsync(host, id)).Last(row => ChangeSetMoves.Operations.Contains(row.Operation)).Id.ToString("N"); + } + + private static async Task QueryAsync(AdminHost host, Func> query) + { + await using var scope = host.App.Services.CreateAsyncScope(); + return await query(scope.ServiceProvider.GetRequiredService()); + } + private static async Task> MovesAsync(AdminHost host, Guid id) { await using var scope = host.App.Services.CreateAsyncScope(); diff --git a/tests/FluentCMS.Tests/Admin/ChangeSetViewTests.cs b/tests/FluentCMS.Tests/Admin/ChangeSetViewTests.cs index 2dd99e981..e69ccac2e 100644 --- a/tests/FluentCMS.Tests/Admin/ChangeSetViewTests.cs +++ b/tests/FluentCMS.Tests/Admin/ChangeSetViewTests.cs @@ -260,6 +260,7 @@ private static AdminTestContext Context(FakeSets sets) context.Services.AddSingleton(new FakeHistory(sets)); context.Services.AddSingleton(new QueueAfter(sets)); context.Services.AddSingleton(new FakeChecks(sets)); + context.Services.AddSingleton(new NoReverts()); context.Renderer.SetRendererInfo(new RendererInfo("Static", isInteractive: false)); return context; } @@ -412,4 +413,15 @@ private static AuditEvent Event(string operation, Initiator initiator, int minut return new AuditEvent { Id = Guid.NewGuid(), Timestamp = Opened.AddMinutes(minutes), Initiator = initiator, Operation = operation }; } } + + // No revert is in progress, so a revert opens one. + private sealed class NoReverts : IChangeSetLists + { + public Task> ListAsync(ChangeSetListQuery query, CancellationToken cancellationToken = default) + { + return Task.FromResult(Result.Success(new ChangeSetList([], 0))); + } + + public Task> CountAsync(ChangeSetListQuery query, CancellationToken cancellationToken = default) => throw new NotSupportedException(); + } }