From 66173941541a9c75e7601ad9e97576af331e20d1 Mon Sep 17 00:00:00 2001 From: Dmitry Kuleshov Date: Fri, 29 May 2026 12:55:27 +0300 Subject: [PATCH 1/2] feat(server): merge documentReview into approval question type [#445] - Drop `documentReview` from QuestionTypes.AllowedTypes - Allow optional `attachments` on `approval`; render attachment review checklist before decision buttons in Pages/Respond.cshtml - Replace verb-form decisions with past-tense `approved` / `rejected` (drops abstain / request-changes / comment-only) - Require `comment` when decision = `rejected` - Update validator + form handler tests; update Playwright spec and Mothership QA fixture to drive the merged type - Park CheckDeliverableSummary out of the validator pending outpost support for preparing attachment summaries; tests covering it marked Skip so the intent stays documented Closes #445. Co-Authored-By: Claude Opus 4.7 (1M context) --- .gitignore | 3 + server/docs/MOTHERSHIP-E2E-SETUP.md | 2 +- .../Dotbot.Server/Models/ApprovalDecisions.cs | 18 ++-- .../src/Dotbot.Server/Models/QuestionTypes.cs | 2 - server/src/Dotbot.Server/Pages/Respond.cshtml | 37 +++------ .../Delivery/NotificationSummaryBuilder.cs | 4 +- .../Validation/QuestionTemplateValidator.cs | 11 ++- .../Validation/RespondFormHandler.cs | 50 ++++------- .../Integration/MagicLinkMiddlewareTests.cs | 2 +- .../Unit/QuestionTemplateValidatorTests.cs | 50 ++++++----- .../Unit/RespondFormHandlerTests.cs | 83 +++++++------------ tests/Test-E2E-Mothership-QA.ps1 | 24 ++---- .../specs/mothership-question-flow.spec.ts | 22 ++--- 13 files changed, 125 insertions(+), 183 deletions(-) diff --git a/.gitignore b/.gitignore index 0b37d8ce..a3e299cb 100644 --- a/.gitignore +++ b/.gitignore @@ -52,6 +52,9 @@ server/publish.zip server/teams-app/manifest.json server/teams-app/*.zip +# Visual Studio user-local project settings +*.csproj.user + # Enterprise proposal analysis (proprietary) docs/proposal-v4/ diff --git a/server/docs/MOTHERSHIP-E2E-SETUP.md b/server/docs/MOTHERSHIP-E2E-SETUP.md index 6e02e14f..e4b6fc77 100644 --- a/server/docs/MOTHERSHIP-E2E-SETUP.md +++ b/server/docs/MOTHERSHIP-E2E-SETUP.md @@ -1,6 +1,6 @@ # Mothership E2E Test Setup (Playwright + Azurite) -How to run the Mothership web UI end-to-end tests locally. Tests cover the magic-link respond flow for all six question types: `singleChoice`, `multiChoice`, `approval`, `documentReview`, `freeText`, `priorityRanking`. +How to run the Mothership web UI end-to-end tests locally. Tests cover the magic-link respond flow for all question types: `singleChoice`, `multiChoice`, `approval`, `freeText`, `priorityRanking`. --- diff --git a/server/src/Dotbot.Server/Models/ApprovalDecisions.cs b/server/src/Dotbot.Server/Models/ApprovalDecisions.cs index d9756b5c..bf24a0b3 100644 --- a/server/src/Dotbot.Server/Models/ApprovalDecisions.cs +++ b/server/src/Dotbot.Server/Models/ApprovalDecisions.cs @@ -2,23 +2,15 @@ namespace Dotbot.Server.Models; public static class ApprovalDecisions { - public const string Approve = "approve"; - public const string Reject = "reject"; - public const string Abstain = "abstain"; + public const string Approved = "approved"; + public const string Rejected = "rejected"; - public const string RequestChanges = "request-changes"; - public const string CommentOnly = "comment-only"; - - public static readonly string[] ApprovalAllowed = [Approve, Reject, Abstain]; - public static readonly string[] DocumentReviewAllowed = [Approve, RequestChanges, CommentOnly]; + public static readonly string[] ApprovalAllowed = [Approved, Rejected]; public static string Label(string decision) => decision switch { - Approve => "Approve", - Reject => "Reject", - Abstain => "Abstain", - RequestChanges => "Request Changes", - CommentOnly => "Comment Only", + Approved => "Approve", + Rejected => "Reject", _ => decision, }; } diff --git a/server/src/Dotbot.Server/Models/QuestionTypes.cs b/server/src/Dotbot.Server/Models/QuestionTypes.cs index bb3b27c1..19f98095 100644 --- a/server/src/Dotbot.Server/Models/QuestionTypes.cs +++ b/server/src/Dotbot.Server/Models/QuestionTypes.cs @@ -5,7 +5,6 @@ public static class QuestionTypes public const string SingleChoice = "singleChoice"; public const string MultiChoice = "multiChoice"; public const string Approval = "approval"; - public const string DocumentReview = "documentReview"; public const string FreeText = "freeText"; public const string PriorityRanking = "priorityRanking"; @@ -14,7 +13,6 @@ public static class QuestionTypes SingleChoice, MultiChoice, Approval, - DocumentReview, FreeText, PriorityRanking, ]; diff --git a/server/src/Dotbot.Server/Pages/Respond.cshtml b/server/src/Dotbot.Server/Pages/Respond.cshtml index e102e3fe..bf6249c8 100644 --- a/server/src/Dotbot.Server/Pages/Respond.cshtml +++ b/server/src/Dotbot.Server/Pages/Respond.cshtml @@ -52,23 +52,6 @@ @switch (Model.Template.Type) { case QuestionTypes.Approval: - @Html.Raw(ApprovalRadio(ApprovalDecisions.Approve, "Approve", "A")) - @Html.Raw(ApprovalRadio(ApprovalDecisions.Reject, "Reject", "R", requireComment: true)) - @Html.Raw(ApprovalRadio(ApprovalDecisions.Abstain, "Abstain", "X")) -
- - -
- break; - - case QuestionTypes.FreeText: -
- - -
- break; - - case QuestionTypes.DocumentReview: @if (Model.Template.Attachments is { Count: > 0 } docs) {
@@ -104,15 +87,21 @@
} - @Html.Raw(ApprovalRadio(ApprovalDecisions.Approve, "Approve", "AP")) - @Html.Raw(ApprovalRadio(ApprovalDecisions.RequestChanges, "Request Changes", "RC", requireComment: true)) - @Html.Raw(ApprovalRadio(ApprovalDecisions.CommentOnly, "Comment Only", "CO")) + @Html.Raw(ApprovalRadio(ApprovalDecisions.Approved, "Approve", "A")) + @Html.Raw(ApprovalRadio(ApprovalDecisions.Rejected, "Reject", "R", requireComment: true))
- +
break; + case QuestionTypes.FreeText: +
+ + +
+ break; + case QuestionTypes.PriorityRanking:

Drag to reorder. Position 1 = highest priority.

    @@ -182,7 +171,7 @@ if (!form) return; const type = form.dataset.questionType; - // ── Approval / DocumentReview: comment required when destructive option selected ── + // ── Approval: comment required when destructive option selected ── (function wireApproval() { const radios = form.querySelectorAll('input[type="radio"][name="approvalDecision"]'); if (radios.length === 0) return; @@ -202,9 +191,9 @@ }); })(); - // ── DocumentReview: at least one attachment confirmation ── + // ── Approval with attachments: at least one attachment confirmation ── (function wireDocReview() { - if (type !== 'documentReview') return; + if (type !== 'approval') return; const checkboxes = form.querySelectorAll('input[type="checkbox"][name="reviewedAttachmentIds"]'); if (checkboxes.length === 0) return; form.addEventListener('submit', function (e) { diff --git a/server/src/Dotbot.Server/Services/Delivery/NotificationSummaryBuilder.cs b/server/src/Dotbot.Server/Services/Delivery/NotificationSummaryBuilder.cs index 8483d01b..ea117084 100644 --- a/server/src/Dotbot.Server/Services/Delivery/NotificationSummaryBuilder.cs +++ b/server/src/Dotbot.Server/Services/Delivery/NotificationSummaryBuilder.cs @@ -29,8 +29,8 @@ public NotificationSummary Build( ? template.Project.ProjectId : template.Project.Name, // Legacy singleChoice templates carry no DeliverableSummary; providers render - // Title + Context as before. Validator enforces non-empty for approval / - // documentReview, so this stays null only when intentional (PRD §8 line 651). + // Title + Context as before. Validator enforces non-empty for approval with + // attachments, so this stays null only when intentional. DeliverableSummary = template.DeliverableSummary, Context = template.Context, Attachments = template.Attachments? diff --git a/server/src/Dotbot.Server/Validation/QuestionTemplateValidator.cs b/server/src/Dotbot.Server/Validation/QuestionTemplateValidator.cs index f2bc1648..ca273bbd 100644 --- a/server/src/Dotbot.Server/Validation/QuestionTemplateValidator.cs +++ b/server/src/Dotbot.Server/Validation/QuestionTemplateValidator.cs @@ -19,7 +19,7 @@ public QuestionTemplateValidator(IOptions se CheckQuestionId, CheckProjectId, CheckType, - CheckDeliverableSummary, + // CheckDeliverableSummary, CheckOptionUniqueness, CheckAttachments, CheckReferenceLinks, @@ -48,11 +48,16 @@ private IEnumerable CheckType(QuestionTemplate t) yield return $"Unknown type '{t.Type}'. Allowed types: {string.Join(", ", QuestionTypes.AllowedTypes)}"; } + // TODO: unskip when outpost supports preparing summary of attachments private IEnumerable CheckDeliverableSummary(QuestionTemplate t) { - if ((t.Type == QuestionTypes.Approval || t.Type == QuestionTypes.DocumentReview) + // Required only when an approval question carries attachments — that is the + // doc-review case where a reviewer needs a 1-3 line summary of what they're + // approving. Plain approvals (no doc) don't need it. + if (t.Type == QuestionTypes.Approval + && t.Attachments is { Count: > 0 } && string.IsNullOrWhiteSpace(t.DeliverableSummary)) - yield return $"deliverableSummary is required when type is '{t.Type}'"; + yield return "deliverableSummary is required when type is 'approval' and attachments are present"; } private IEnumerable CheckOptionUniqueness(QuestionTemplate t) diff --git a/server/src/Dotbot.Server/Validation/RespondFormHandler.cs b/server/src/Dotbot.Server/Validation/RespondFormHandler.cs index 80c1550b..e86c6ccb 100644 --- a/server/src/Dotbot.Server/Validation/RespondFormHandler.cs +++ b/server/src/Dotbot.Server/Validation/RespondFormHandler.cs @@ -39,9 +39,8 @@ public static RespondFormResult Validate(QuestionTemplate template, RespondFormI return template.Type switch { - QuestionTypes.Approval => ValidateApproval(input), + QuestionTypes.Approval => ValidateApproval(template, input), QuestionTypes.FreeText => ValidateFreeText(input), - QuestionTypes.DocumentReview => ValidateDocumentReview(template, input), QuestionTypes.PriorityRanking => ValidatePriorityRanking(template, input), QuestionTypes.SingleChoice or QuestionTypes.MultiChoice => ValidateSingleOrMultiChoice(template, input), @@ -49,44 +48,15 @@ QuestionTypes.SingleChoice or QuestionTypes.MultiChoice }; } - private static RespondFormResult ValidateApproval(RespondFormInput input) + private static RespondFormResult ValidateApproval(QuestionTemplate template, RespondFormInput input) { var decision = input.ApprovalDecision?.Trim().ToLowerInvariant(); if (string.IsNullOrEmpty(decision) || !ApprovalDecisions.ApprovalAllowed.Contains(decision)) - return RespondFormResult.Fail("Please choose Approve, Reject, or Abstain."); + return RespondFormResult.Fail("Please choose Approve or Reject."); - if (decision == ApprovalDecisions.Reject && string.IsNullOrWhiteSpace(input.Comment)) + if (decision == ApprovalDecisions.Rejected && string.IsNullOrWhiteSpace(input.Comment)) return RespondFormResult.Fail("A comment is required when rejecting."); - return new RespondFormResult - { - ApprovalDecision = decision, - Comment = NullIfBlank(input.Comment), - SelectionLabel = ApprovalDecisions.Label(decision), - }; - } - - private static RespondFormResult ValidateFreeText(RespondFormInput input) - { - if (string.IsNullOrWhiteSpace(input.FreeText)) - return RespondFormResult.Fail("Please type a response."); - - return new RespondFormResult - { - FreeText = input.FreeText.Trim(), - SelectionLabel = "Free-text response", - }; - } - - private static RespondFormResult ValidateDocumentReview(QuestionTemplate template, RespondFormInput input) - { - var decision = input.ApprovalDecision?.Trim().ToLowerInvariant(); - if (string.IsNullOrEmpty(decision) || !ApprovalDecisions.DocumentReviewAllowed.Contains(decision)) - return RespondFormResult.Fail("Please choose Approve, Request Changes, or Comment Only."); - - if (decision == ApprovalDecisions.RequestChanges && string.IsNullOrWhiteSpace(input.Comment)) - return RespondFormResult.Fail("A comment is required when requesting changes."); - var templateAttachments = template.Attachments ?? new List(); if (templateAttachments.Count > 0) { @@ -120,6 +90,18 @@ private static RespondFormResult ValidateDocumentReview(QuestionTemplate templat }; } + private static RespondFormResult ValidateFreeText(RespondFormInput input) + { + if (string.IsNullOrWhiteSpace(input.FreeText)) + return RespondFormResult.Fail("Please type a response."); + + return new RespondFormResult + { + FreeText = input.FreeText.Trim(), + SelectionLabel = "Free-text response", + }; + } + private static RespondFormResult ValidatePriorityRanking(QuestionTemplate template, RespondFormInput input) { var ranked = input.RankedItems?.Where(r => r is not null).ToList() ?? new List(); diff --git a/server/tests/Dotbot.Server.Tests/Integration/MagicLinkMiddlewareTests.cs b/server/tests/Dotbot.Server.Tests/Integration/MagicLinkMiddlewareTests.cs index f5ee32c7..89efd283 100644 --- a/server/tests/Dotbot.Server.Tests/Integration/MagicLinkMiddlewareTests.cs +++ b/server/tests/Dotbot.Server.Tests/Integration/MagicLinkMiddlewareTests.cs @@ -41,7 +41,7 @@ public sealed class MagicLinkMiddlewareTests(DotbotApiFactory factory) QuestionId = questionId, Version = 1, Title = $"Review {suffix}", - Type = QuestionTypes.DocumentReview, + Type = QuestionTypes.Approval, DeliverableSummary = "review", Options = new(), Project = new ProjectRef { ProjectId = ProjectId, Name = "MW" }, diff --git a/server/tests/Dotbot.Server.Tests/Unit/QuestionTemplateValidatorTests.cs b/server/tests/Dotbot.Server.Tests/Unit/QuestionTemplateValidatorTests.cs index e1de9a28..2d924ab8 100644 --- a/server/tests/Dotbot.Server.Tests/Unit/QuestionTemplateValidatorTests.cs +++ b/server/tests/Dotbot.Server.Tests/Unit/QuestionTemplateValidatorTests.cs @@ -73,30 +73,39 @@ public void UnknownType_OneErrorListingAllowedValues() [InlineData(QuestionTypes.MultiChoice)] [InlineData(QuestionTypes.FreeText)] [InlineData(QuestionTypes.PriorityRanking)] + [InlineData(QuestionTypes.Approval)] public void TypeWithoutDeliverableSummaryRequirement_NoErrorWhenSummaryMissing(string type) => Assert.Empty(Validate(Template(type: type))); - [Theory] - [InlineData(QuestionTypes.Approval, null)] - [InlineData(QuestionTypes.Approval, "")] - [InlineData(QuestionTypes.Approval, " ")] - [InlineData(QuestionTypes.DocumentReview, null)] - [InlineData(QuestionTypes.DocumentReview, "")] - [InlineData(QuestionTypes.DocumentReview, " ")] - public void ApprovalOrDocumentReviewWithoutDeliverableSummary_OneError(string type, string? summary) - { - var errors = Validate(Template(type: type, deliverableSummary: summary)); + // CheckDeliverableSummary is currently parked out of the validator + // rules array pending outpost support for preparing the summary of + // attachments. The skipped tests below document the intended behaviour + // and should be re-enabled once the rule is restored. + [Theory(Skip = "CheckDeliverableSummary parked; restore when outpost emits the summary")] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void ApprovalWithAttachmentsAndNoDeliverableSummary_OneError(string? summary) + { + var errors = Validate(Template( + type: QuestionTypes.Approval, + deliverableSummary: summary, + attachments: [new QuestionAttachment { AttachmentId = Guid.NewGuid(), Name = "n", Url = "https://x" }])); Assert.Single(errors); Assert.Contains("deliverableSummary", errors[0]); - Assert.Contains(type, errors[0]); + Assert.Contains("approval", errors[0]); } - [Theory] - [InlineData(QuestionTypes.Approval)] - [InlineData(QuestionTypes.DocumentReview)] - public void ApprovalOrDocumentReviewWithDeliverableSummary_NoErrors(string type) - => Assert.Empty(Validate( - Template(type: type, deliverableSummary: "ship plan v1"))); + [Fact] + public void ApprovalWithAttachmentsAndDeliverableSummary_NoErrors() + => Assert.Empty(Validate(Template( + type: QuestionTypes.Approval, + deliverableSummary: "ship plan v1", + attachments: [new QuestionAttachment { AttachmentId = Guid.NewGuid(), Name = "n", Url = "https://x" }]))); + + [Fact] + public void ApprovalWithoutAttachments_NoDeliverableSummaryRequired() + => Assert.Empty(Validate(Template(type: QuestionTypes.Approval))); [Fact] public void NullAttachments_NoErrors() @@ -172,12 +181,13 @@ public void MultipleRulesFail_AllErrorsReturned() Assert.Contains("bogus", errors[1]); } - [Fact] - public void ProjectIdEmptyAndApprovalWithoutSummary_TwoErrors() + [Fact(Skip = "CheckDeliverableSummary parked; restore when outpost emits the summary")] + public void ProjectIdEmptyAndApprovalWithAttachmentsWithoutSummary_TwoErrors() { var errors = Validate(Template( projectId: "", - type: QuestionTypes.Approval)); + type: QuestionTypes.Approval, + attachments: [new QuestionAttachment { AttachmentId = Guid.NewGuid(), Name = "n", Url = "https://x" }])); Assert.Equal(2, errors.Count); Assert.Contains("project.projectId", errors[0]); Assert.Contains("deliverableSummary", errors[1]); diff --git a/server/tests/Dotbot.Server.Tests/Unit/RespondFormHandlerTests.cs b/server/tests/Dotbot.Server.Tests/Unit/RespondFormHandlerTests.cs index 34b03293..a999ad22 100644 --- a/server/tests/Dotbot.Server.Tests/Unit/RespondFormHandlerTests.cs +++ b/server/tests/Dotbot.Server.Tests/Unit/RespondFormHandlerTests.cs @@ -13,20 +13,20 @@ public class RespondFormHandlerTests Type = type, Options = options.ToList(), Project = new ProjectRef { ProjectId = "p1" }, - DeliverableSummary = type is QuestionTypes.Approval or QuestionTypes.DocumentReview ? "deliverable" : null, + DeliverableSummary = type == QuestionTypes.Approval ? "deliverable" : null, }; private static TemplateOption Option(string key = "A", string title = "Alpha") => new() { OptionId = Guid.NewGuid(), Key = key, Title = title }; - // ── Approval ─────────────────────────────────────────────────────────── + // ── Approval (no attachments) ────────────────────────────────────────── [Fact] public void Approval_NoDecision_Fails() { var result = RespondFormHandler.Validate(Template(QuestionTypes.Approval), new RespondFormInput()); Assert.False(result.IsValid); - Assert.Contains("Approve, Reject, or Abstain", result.Error); + Assert.Contains("Approve or Reject", result.Error); } [Fact] @@ -39,43 +39,33 @@ public void Approval_UnknownDecision_Fails() } [Fact] - public void Approval_Reject_RequiresComment() + public void Approval_Rejected_RequiresComment() { var result = RespondFormHandler.Validate( Template(QuestionTypes.Approval), - new RespondFormInput(ApprovalDecision: ApprovalDecisions.Reject, Comment: " ")); + new RespondFormInput(ApprovalDecision: ApprovalDecisions.Rejected, Comment: " ")); Assert.False(result.IsValid); Assert.Contains("comment is required", result.Error, StringComparison.OrdinalIgnoreCase); } [Fact] - public void Approval_Reject_WithComment_Succeeds() + public void Approval_Rejected_WithComment_Succeeds() { var result = RespondFormHandler.Validate( Template(QuestionTypes.Approval), - new RespondFormInput(ApprovalDecision: ApprovalDecisions.Reject, Comment: "needs work")); + new RespondFormInput(ApprovalDecision: ApprovalDecisions.Rejected, Comment: "needs work")); Assert.True(result.IsValid); - Assert.Equal(ApprovalDecisions.Reject, result.ApprovalDecision); + Assert.Equal(ApprovalDecisions.Rejected, result.ApprovalDecision); Assert.Equal("needs work", result.Comment); Assert.Equal("Reject", result.SelectionLabel); } [Fact] - public void Approval_Approve_NoCommentNeeded() + public void Approval_Approved_NoCommentNeeded() { var result = RespondFormHandler.Validate( Template(QuestionTypes.Approval), - new RespondFormInput(ApprovalDecision: ApprovalDecisions.Approve)); - Assert.True(result.IsValid); - Assert.Null(result.Comment); - } - - [Fact] - public void Approval_Abstain_TrimsCommentNullIfBlank() - { - var result = RespondFormHandler.Validate( - Template(QuestionTypes.Approval), - new RespondFormInput(ApprovalDecision: ApprovalDecisions.Abstain, Comment: " ")); + new RespondFormInput(ApprovalDecision: ApprovalDecisions.Approved)); Assert.True(result.IsValid); Assert.Null(result.Comment); } @@ -85,9 +75,9 @@ public void Approval_DecisionCaseInsensitive_Normalised() { var result = RespondFormHandler.Validate( Template(QuestionTypes.Approval), - new RespondFormInput(ApprovalDecision: "APPROVE")); + new RespondFormInput(ApprovalDecision: "APPROVED")); Assert.True(result.IsValid); - Assert.Equal(ApprovalDecisions.Approve, result.ApprovalDecision); + Assert.Equal(ApprovalDecisions.Approved, result.ApprovalDecision); } // ── FreeText ─────────────────────────────────────────────────────────── @@ -112,56 +102,48 @@ public void FreeText_Trimmed_Succeeds() Assert.Equal("hello", result.FreeText); } - // ── DocumentReview ───────────────────────────────────────────────────── + // ── Approval with attachments (formerly DocumentReview) ──────────────── [Fact] - public void DocumentReview_NoDecision_Fails() + public void ApprovalWithAttachments_NoDecision_Fails() { - var template = Template(QuestionTypes.DocumentReview); + var template = Template(QuestionTypes.Approval); + template.Attachments = [new QuestionAttachment { AttachmentId = Guid.NewGuid(), Name = "spec", BlobPath = "x" }]; var result = RespondFormHandler.Validate(template, new RespondFormInput()); Assert.False(result.IsValid); } [Fact] - public void DocumentReview_RequestChanges_RequiresComment() + public void ApprovalWithAttachments_Rejected_RequiresComment() { - var template = Template(QuestionTypes.DocumentReview); + var template = Template(QuestionTypes.Approval); + template.Attachments = [new QuestionAttachment { AttachmentId = Guid.NewGuid(), Name = "spec", BlobPath = "x" }]; var result = RespondFormHandler.Validate(template, new RespondFormInput( - ApprovalDecision: ApprovalDecisions.RequestChanges)); + ApprovalDecision: ApprovalDecisions.Rejected)); Assert.False(result.IsValid); Assert.Contains("comment is required", result.Error, StringComparison.OrdinalIgnoreCase); } [Fact] - public void DocumentReview_NoTemplateAttachments_AcceptsApproveWithoutReviewedIds() - { - var template = Template(QuestionTypes.DocumentReview); - var result = RespondFormHandler.Validate(template, new RespondFormInput( - ApprovalDecision: ApprovalDecisions.Approve)); - Assert.True(result.IsValid); - Assert.Null(result.ReviewedAttachmentIds); - } - - [Fact] - public void DocumentReview_WithAttachments_RequiresAtLeastOneReviewed() + public void ApprovalWithAttachments_RequiresAtLeastOneReviewed() { - var template = Template(QuestionTypes.DocumentReview); + var template = Template(QuestionTypes.Approval); template.Attachments = [new QuestionAttachment { AttachmentId = Guid.NewGuid(), Name = "spec", BlobPath = "x" }]; var result = RespondFormHandler.Validate(template, new RespondFormInput( - ApprovalDecision: ApprovalDecisions.Approve, + ApprovalDecision: ApprovalDecisions.Approved, ReviewedAttachmentIds: Array.Empty())); Assert.False(result.IsValid); Assert.Contains("reviewed at least one", result.Error, StringComparison.OrdinalIgnoreCase); } [Fact] - public void DocumentReview_WithAttachments_FiltersUnknownIds() + public void ApprovalWithAttachments_FiltersUnknownIds() { var aid = Guid.NewGuid(); - var template = Template(QuestionTypes.DocumentReview); + var template = Template(QuestionTypes.Approval); template.Attachments = [new QuestionAttachment { AttachmentId = aid, Name = "spec", BlobPath = "x" }]; var result = RespondFormHandler.Validate(template, new RespondFormInput( - ApprovalDecision: ApprovalDecisions.Approve, + ApprovalDecision: ApprovalDecisions.Approved, ReviewedAttachmentIds: new[] { aid, Guid.NewGuid() })); Assert.True(result.IsValid); Assert.Single(result.ReviewedAttachmentIds!); @@ -169,25 +151,24 @@ public void DocumentReview_WithAttachments_FiltersUnknownIds() } [Fact] - public void DocumentReview_OnlyUnknownIds_Fails() + public void ApprovalWithAttachments_OnlyUnknownIds_Fails() { - var template = Template(QuestionTypes.DocumentReview); + var template = Template(QuestionTypes.Approval); template.Attachments = [new QuestionAttachment { AttachmentId = Guid.NewGuid(), Name = "spec", BlobPath = "x" }]; var result = RespondFormHandler.Validate(template, new RespondFormInput( - ApprovalDecision: ApprovalDecisions.CommentOnly, - Comment: "ok", + ApprovalDecision: ApprovalDecisions.Approved, ReviewedAttachmentIds: new[] { Guid.NewGuid() })); Assert.False(result.IsValid); } [Fact] - public void DocumentReview_DuplicatesDeduped() + public void ApprovalWithAttachments_DuplicatesDeduped() { var aid = Guid.NewGuid(); - var template = Template(QuestionTypes.DocumentReview); + var template = Template(QuestionTypes.Approval); template.Attachments = [new QuestionAttachment { AttachmentId = aid, Name = "spec", BlobPath = "x" }]; var result = RespondFormHandler.Validate(template, new RespondFormInput( - ApprovalDecision: ApprovalDecisions.Approve, + ApprovalDecision: ApprovalDecisions.Approved, ReviewedAttachmentIds: new[] { aid, aid, aid })); Assert.True(result.IsValid); Assert.Single(result.ReviewedAttachmentIds!); diff --git a/tests/Test-E2E-Mothership-QA.ps1 b/tests/Test-E2E-Mothership-QA.ps1 index 3444eafa..4312ab01 100644 --- a/tests/Test-E2E-Mothership-QA.ps1 +++ b/tests/Test-E2E-Mothership-QA.ps1 @@ -3,9 +3,9 @@ .SYNOPSIS Layer 5: Mothership web UI E2E — Playwright against live server + Azurite. .DESCRIPTION - Runs Playwright specs that navigate the magic-link respond flow for all six - question types: singleChoice, multiChoice, freeText, approval, - documentReview, priorityRanking. + Runs Playwright specs that navigate the magic-link respond flow for all + question types: singleChoice, multiChoice, freeText, approval (with and + without attachments), priorityRanking. For each question type the script: 1. POST /api/templates — creates a template @@ -173,25 +173,13 @@ $questionTypes = @( Submit = @{ selectedKey = "opt_a" } } @{ - Type = "approval" - Title = "Playwright E2E: approval" - DeliverableSummary = "Playwright E2E test deliverable for approval" + Type = "approval" + Title = "Playwright E2E: approval" Options = @( @{ key = "approve"; label = "Approve" } @{ key = "reject"; label = "Reject" } - @{ key = "abstain"; label = "Abstain" } ) - Submit = @{ approvalDecision = "approve" } - } - @{ - Type = "documentReview" - Title = "Playwright E2E: documentReview" - DeliverableSummary = "Playwright E2E test deliverable for documentReview" - Options = @( - @{ key = "approve"; label = "Approve" } - @{ key = "reject"; label = "Reject" } - ) - Submit = @{ approvalDecision = "approve" } + Submit = @{ approvalDecision = "approved" } } @{ Type = "freeText" diff --git a/tests/e2e-server/specs/mothership-question-flow.spec.ts b/tests/e2e-server/specs/mothership-question-flow.spec.ts index 22e1bf4c..d9e139c3 100644 --- a/tests/e2e-server/specs/mothership-question-flow.spec.ts +++ b/tests/e2e-server/specs/mothership-question-flow.spec.ts @@ -58,16 +58,10 @@ for (const scenario of scenarios) { if (scenario.type === "approval") { await expect( - page.locator('[value="approve"], [data-key="approve"]').first(), + page.locator('[value="approved"], [data-key="approve"]').first(), ).toBeVisible(); await expect( - page.locator('[value="reject"], [data-key="reject"]').first(), - ).toBeVisible(); - } - - if (scenario.type === "documentReview") { - await expect( - page.locator('[value="approve"], [data-key="approve"]').first(), + page.locator('[value="rejected"], [data-key="reject"]').first(), ).toBeVisible(); } @@ -98,8 +92,8 @@ for (const scenario of scenarios) { } } - if (scenario.type === "approval" || scenario.type === "documentReview") { - const decision = scenario.submit.approvalDecision ?? "approve"; + if (scenario.type === "approval") { + const decision = scenario.submit.approvalDecision ?? "approved"; const radio = page .locator(`input[type="radio"][value="${decision}"]`) .first(); @@ -151,8 +145,8 @@ for (const scenario of scenarios) { injectBody.freeText = scenario.submit.freeText ?? "test answer"; } else if (scenario.type === "priorityRanking") { injectBody.rankedItems = scenario.submit.rankedItems; - } else if (scenario.type === "approval" || scenario.type === "documentReview") { - injectBody.approvalDecision = scenario.submit.approvalDecision ?? "approve"; + } else if (scenario.type === "approval") { + injectBody.approvalDecision = scenario.submit.approvalDecision ?? "approved"; } else { injectBody.selectedKey = scenario.submit.selectedKey; } @@ -173,8 +167,8 @@ for (const scenario of scenarios) { } else if (scenario.type === "priorityRanking") { expect(Array.isArray(last.rankedItems)).toBeTruthy(); expect(last.rankedItems.length).toBeGreaterThan(0); - } else if (scenario.type === "approval" || scenario.type === "documentReview") { - expect(last.approvalDecision).toBe(scenario.submit.approvalDecision ?? "approve"); + } else if (scenario.type === "approval") { + expect(last.approvalDecision).toBe(scenario.submit.approvalDecision ?? "approved"); } else if (scenario.submit.selectedKey) { expect(last.selectedKey).toBe(scenario.submit.selectedKey); } From a94e76e7d59e09f1290c4f8f3b65827d87a79f79 Mon Sep 17 00:00:00 2001 From: Dmitry Kuleshov Date: Fri, 29 May 2026 12:56:07 +0300 Subject: [PATCH 2/2] feat(mcp,outpost): merge documentReview into approval question type [#445] MCP tools (`task_mark_needs_input`, `task_answer_question`): - Drop `documentReview` from the allowed `type` enum. - Restrict `approval` decisions to `approved` / `rejected`; drop `abstained`, `changes_requested`, `comment_only`. - Require `comment` only when decision = `rejected`. NotificationClient.psm1: - Drop the `documentReview` switch branch in ConvertTo-TypedResponse; approval handling now covers both attachment-bearing and no-document cases. Outpost Decisions tab (core/ui/static/modules/actions.js): - Drop the Abstain button on `approval` cards. - When the question carries attachments (`question.attachments` / `question.attachmentList`), render the "I reviewed this" checklist and block submission until at least one attachment is acknowledged; reviewed IDs flow through `reviewed_attachment_ids` on the `/api/task/answer` payload. Test-Components.ps1: - Update ConvertTo-TypedResponse fixture to use the merged `approval` type with attachments. - Add `documentReview` rejection assertion and lock the deprecated `changes_requested` decision out of approval-typed answers. Refs #445. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/mcp/modules/NotificationClient.psm1 | 10 ++-- .../tools/task-answer-question/metadata.yaml | 5 +- .../mcp/tools/task-answer-question/script.ps1 | 19 ++++--- .../tools/task-mark-needs-input/metadata.yaml | 2 +- .../tools/task-mark-needs-input/script.ps1 | 2 +- core/ui/static/modules/actions.js | 51 ++++++++++++++++--- tests/Test-Components.ps1 | 27 +++++----- 7 files changed, 73 insertions(+), 43 deletions(-) diff --git a/core/mcp/modules/NotificationClient.psm1 b/core/mcp/modules/NotificationClient.psm1 index 950b10c2..18599225 100644 --- a/core/mcp/modules/NotificationClient.psm1 +++ b/core/mcp/modules/NotificationClient.psm1 @@ -265,8 +265,8 @@ function Send-TaskNotification { Optional notification settings. If not provided, reads from config. .PARAMETER Type - PRD §4.6 question type — singleChoice (default) | approval | documentReview | - freeText | priorityRanking. Drives card rendering and response parsing. + PRD sec. 4.6 question type — singleChoice (default) | approval | freeText | + priorityRanking. Drives card rendering and response parsing. .PARAMETER DeliverableSummary Optional 1-3 line summary shown in channel notifications (PRD §5.2). @@ -802,7 +802,7 @@ function ConvertTo-TypedResponse { Hashtable. Keys (only set when applicable): answer_type - the type string echoed back answer - resolved string for singleChoice (key) / freeText (string) - approval_decision - approval / documentReview decision value + approval_decision - approval decision value comment - free-text comment ranked_items - array for priorityRanking attachment_refs - array of @{ name; size_bytes; storage_ref; description } @@ -860,10 +860,6 @@ function ConvertTo-TypedResponse { if ($decision) { $out['approval_decision'] = $decision } if ($comment) { $out['comment'] = $comment } } - 'documentReview' { - if ($decision) { $out['approval_decision'] = $decision } - if ($comment) { $out['comment'] = $comment } - } 'priorityRanking' { if ($rankedItems) { # Server emits RankedItem[] = [{ optionId: Guid, rank: int }] diff --git a/core/mcp/tools/task-answer-question/metadata.yaml b/core/mcp/tools/task-answer-question/metadata.yaml index 5e881dae..3dada51f 100644 --- a/core/mcp/tools/task-answer-question/metadata.yaml +++ b/core/mcp/tools/task-answer-question/metadata.yaml @@ -11,11 +11,12 @@ inputSchema: description: "The answer - either an option key (A, B, C, D, E) or a custom freeform answer. Required for singleChoice/freeText types." type: type: string - enum: [singleChoice, approval, documentReview, freeText, priorityRanking] + enum: [singleChoice, approval, freeText, priorityRanking] description: "Question type. Determines which fields are required/validated." decision: type: string - description: "Required for approval/documentReview. Allowed values vary per type (PRD Section 4.1)." + enum: [approved, rejected] + description: "Required for approval. Allowed values: approved, rejected." comment: type: string description: "Required when decision=rejected on an approval question." diff --git a/core/mcp/tools/task-answer-question/script.ps1 b/core/mcp/tools/task-answer-question/script.ps1 index 9eb8a29f..06f9fefe 100644 --- a/core/mcp/tools/task-answer-question/script.ps1 +++ b/core/mcp/tools/task-answer-question/script.ps1 @@ -43,10 +43,9 @@ function Invoke-TaskAnswerQuestion { # Type-specific validation (PRD §4.1, §4.6) $validDecisions = @{ - approval = @('approved', 'rejected', 'abstained') - documentReview = @('approved', 'changes_requested', 'comment_only') + approval = @('approved', 'rejected') } - $validTypes = @('singleChoice', 'approval', 'documentReview', 'freeText', 'priorityRanking') + $validTypes = @('singleChoice', 'approval', 'freeText', 'priorityRanking') if ($questionType -and $questionType -notin $validTypes) { throw "Invalid 'type' value '$questionType'. Allowed: $($validTypes -join ', ')" } @@ -58,8 +57,8 @@ function Invoke-TaskAnswerQuestion { if ($decision -notin $validDecisions[$questionType]) { throw "Invalid 'decision' value '$decision' for type '$questionType'. Allowed: $($validDecisions[$questionType] -join ', ')" } - if ($decision -in @('rejected', 'changes_requested') -and -not $comment) { - throw "'comment' is required when decision='$decision'" + if ($decision -eq 'rejected' -and -not $comment) { + throw "'comment' is required when decision='rejected'" } } elseif ($questionType -eq 'priorityRanking') { if (-not $rankedItems -or @($rankedItems).Count -eq 0) { @@ -78,11 +77,11 @@ function Invoke-TaskAnswerQuestion { # 'type' is omitted — legacy callers default to singleChoice for this check # so decision/comment/ranked_items can't sneak in alongside a plain answer. $effectiveType = if ($questionType) { $questionType } else { 'singleChoice' } - if ($decision -and $effectiveType -notin @('approval', 'documentReview')) { - throw "'decision' is only valid for type 'approval' or 'documentReview', got type='$effectiveType'" + if ($decision -and $effectiveType -ne 'approval') { + throw "'decision' is only valid for type 'approval', got type='$effectiveType'" } - if ($comment -and $effectiveType -notin @('approval', 'documentReview')) { - throw "'comment' is only valid for type 'approval' or 'documentReview', got type='$effectiveType'" + if ($comment -and $effectiveType -ne 'approval') { + throw "'comment' is only valid for type 'approval', got type='$effectiveType'" } if ($rankedItems -and $effectiveType -ne 'priorityRanking') { throw "'ranked_items' is only valid for type 'priorityRanking', got type='$effectiveType'" @@ -90,7 +89,7 @@ function Invoke-TaskAnswerQuestion { # Reject 'answer' when caller passes it alongside a typed payload — would # produce inconsistent resolvedEntry (e.g., answer='A' + approval_decision='approved'). if ($answer -and $effectiveType -notin @('singleChoice', 'freeText')) { - throw "'answer' is only valid for type 'singleChoice' or 'freeText', got type='$effectiveType'. Use 'decision' (approval/documentReview) or 'ranked_items' (priorityRanking)." + throw "'answer' is only valid for type 'singleChoice' or 'freeText', got type='$effectiveType'. Use 'decision' (approval) or 'ranked_items' (priorityRanking)." } # Synthesize an answer string for non-question types so downstream diff --git a/core/mcp/tools/task-mark-needs-input/metadata.yaml b/core/mcp/tools/task-mark-needs-input/metadata.yaml index 46633eb4..c789e52f 100644 --- a/core/mcp/tools/task-mark-needs-input/metadata.yaml +++ b/core/mcp/tools/task-mark-needs-input/metadata.yaml @@ -114,7 +114,7 @@ inputSchema: - sub_tasks type: type: string - enum: [singleChoice, approval, documentReview, freeText, priorityRanking] + enum: [singleChoice, approval, freeText, priorityRanking] default: singleChoice description: "Question type (PRD Section 4.6). Drives card rendering and response parsing." deliverable_summary: diff --git a/core/mcp/tools/task-mark-needs-input/script.ps1 b/core/mcp/tools/task-mark-needs-input/script.ps1 index 9dee8f7e..6794a111 100644 --- a/core/mcp/tools/task-mark-needs-input/script.ps1 +++ b/core/mcp/tools/task-mark-needs-input/script.ps1 @@ -20,7 +20,7 @@ function Invoke-TaskMarkNeedsInput { if (-not $question -and -not $questionsArg -and -not $splitProposal) { throw "Either 'questions' array, 'question' object, or 'split_proposal' is required" } if (($question -or $questionsArg) -and $splitProposal) { throw "Cannot provide both questions and split_proposal - use one at a time" } - $validTypes = @('singleChoice', 'approval', 'documentReview', 'freeText', 'priorityRanking') + $validTypes = @('singleChoice', 'approval', 'freeText', 'priorityRanking') if ($questionType -notin $validTypes) { throw "Invalid 'type' value '$questionType'. Allowed: $($validTypes -join ', ')" } diff --git a/core/ui/static/modules/actions.js b/core/ui/static/modules/actions.js index e417a73b..18f43b40 100644 --- a/core/ui/static/modules/actions.js +++ b/core/ui/static/modules/actions.js @@ -306,8 +306,30 @@ function renderQuestionItem(item) { } if (questionType === 'approval') { + const attachments = Array.isArray(question.attachments) ? question.attachments + : Array.isArray(question.attachmentList) ? question.attachmentList + : []; + const hasAttachments = attachments.length > 0; + const attachmentListHtml = hasAttachments ? ` +
    +
    Confirm the attachments you reviewed:
    +
      + ${attachments.map(att => { + const id = att.attachmentId || att.id || att.attachment_id || ''; + const name = att.name || 'attachment'; + return ` +
    • + +
      • + `; + }).join('')} +
    +
    ` : ''; return ` -
    +
    Approval ${escapeHtml(item.task_name)} @@ -315,9 +337,9 @@ function renderQuestionItem(item) {
    ${escapeHtml(question.question || 'No question text')}
    ${question.context ? `
    ${escapeHtml(question.context)}
    ` : ''} + ${attachmentListHtml}
    -