diff --git a/engine/app/controllers/coplan/api/v1/review_requests_controller.rb b/engine/app/controllers/coplan/api/v1/review_requests_controller.rb index c9b9d4da..45ff79e4 100644 --- a/engine/app/controllers/coplan/api/v1/review_requests_controller.rb +++ b/engine/app/controllers/coplan/api/v1/review_requests_controller.rb @@ -34,10 +34,14 @@ def create def update if ActiveModel::Type::Boolean.new.cast(params[:request_again]) - Reviews::Request.call(plan: @plan, reviewer: @review_request.reviewer, actor: current_user, - requirement: params[:requirement].presence || @review_request.requirement, api_token: @api_token) + Reviews::Request.again(request: @review_request, actor: current_user, + requirement: params[:requirement].presence, api_token: @api_token) else - @plan.with_lock { @review_request.update!(requirement: params.require(:requirement)) } + @plan.with_lock do + # The request may have been removed while waiting for the plan lock. + @review_request.reload + @review_request.update!(requirement: params.require(:requirement)) + end Reviews::Changed.call(request: @review_request, event_type: "review.requested", actor_token_id: api_token_id, payload: { "action" => "requirement_changed", "requirement" => @review_request.requirement }) end diff --git a/engine/app/controllers/coplan/review_requests_controller.rb b/engine/app/controllers/coplan/review_requests_controller.rb index f62be26d..ec2bdc51 100644 --- a/engine/app/controllers/coplan/review_requests_controller.rb +++ b/engine/app/controllers/coplan/review_requests_controller.rb @@ -1,7 +1,7 @@ module CoPlan class ReviewRequestsController < ApplicationController before_action :set_plan - before_action :set_review_request, only: [ :update, :destroy, :respond, :compare ] + before_action :set_review_request, only: [ :update, :respond, :compare ] rescue_from Reviews::Respond::Conflict, with: :render_conflict rescue_from ActiveRecord::RecordInvalid, with: :render_invalid @@ -17,10 +17,11 @@ def suggestions def create authorize!(@plan, :update?) - attributes = params.expect(review_request: [ :reviewer_id, :requirement ]) + attributes = params.expect(review_request: [ :reviewer_id, :requirement, :replace_withdrawn ]) reviewer = User.find(attributes[:reviewer_id]) request = Reviews::Request.call(plan: @plan, reviewer: reviewer, actor: current_user, - requirement: attributes[:requirement].presence || "required", create_only: true) + requirement: attributes[:requirement].presence || "required", create_only: true, + replace_withdrawn: ActiveModel::Type::Boolean.new.cast(attributes[:replace_withdrawn])) render json: Reviews::Serialize.call(request), status: :created end @@ -28,10 +29,14 @@ def update authorize!(@plan, :update?) attributes = params.expect(review_request: [ :requirement, :request_again ]) if ActiveModel::Type::Boolean.new.cast(attributes[:request_again]) - Reviews::Request.call(plan: @plan, reviewer: @review_request.reviewer, actor: current_user, - requirement: attributes[:requirement].presence || @review_request.requirement) + Reviews::Request.again(request: @review_request, actor: current_user, + requirement: attributes[:requirement].presence) else - @plan.with_lock { @review_request.update!(requirement: attributes.fetch(:requirement)) } + @plan.with_lock do + # The request may have been removed while waiting for the plan lock. + @review_request.reload + @review_request.update!(requirement: attributes.fetch(:requirement)) + end Reviews::Changed.call(request: @review_request, event_type: "review.requested", payload: { "action" => "requirement_changed", "requirement" => @review_request.requirement }) end @@ -40,7 +45,12 @@ def update def destroy authorize!(@plan, :update?) - Reviews::Withdraw.call(request: @review_request, actor: current_user) + if ActiveModel::Type::Boolean.new.cast(params.dig(:review_request, :remove)) + Reviews::Remove.call(plan: @plan, request_id: params[:id]) + else + set_review_request + Reviews::Withdraw.call(request: @review_request, actor: current_user) + end head :no_content end diff --git a/engine/app/javascript/controllers/coplan/review_tray_controller.js b/engine/app/javascript/controllers/coplan/review_tray_controller.js index e818fb8b..a59b0d26 100644 --- a/engine/app/javascript/controllers/coplan/review_tray_controller.js +++ b/engine/app/javascript/controllers/coplan/review_tray_controller.js @@ -11,7 +11,9 @@ export default class extends Controller { // Shared streams carry an invalidation signal, never another viewer's // forms, tokens or personal task status. window.Turbo.StreamActions["coplan-review-changed"] ||= function () { - document.getElementById(this.getAttribute("target"))?.dispatchEvent(new CustomEvent("coplan:reviews-changed")) + document.getElementById(this.getAttribute("target"))?.dispatchEvent(new CustomEvent("coplan:reviews-changed", { + detail: { removedRequestId: this.getAttribute("data-removed-review-request-id") } + })) } const tray = this.element.querySelector(".review-tray") if (tray) tray.showPopover() @@ -132,6 +134,8 @@ export default class extends Controller { confirmApproval(event) { this.responseUrl = event.currentTarget.dataset.url this.approvalRevision = this.viewedRevision() + this.approvalInvalidated = false + this.approveButtonTarget.disabled = false this.approvalErrorTarget.textContent = "" this.approveButtonTarget.textContent = `Approve v${this.approvalRevision}` this.approvalTitleTarget.textContent = `Approve v${this.approvalRevision}?` @@ -142,6 +146,12 @@ export default class extends Controller { this.approvalTarget.close() } + invalidateApproval() { + this.approvalInvalidated = true + this.approveButtonTarget.disabled = true + this.approvalErrorTarget.textContent = "This review request changed. Close this dialog and reopen approval for the current request. Your note is preserved." + } + responseIntent(url, revision, decision, note) { this.responseIntents ||= {} const fields = JSON.stringify([url, String(revision), note.trim()]) @@ -154,13 +164,26 @@ export default class extends Controller { if (this.responseIntents[decision] === intent) delete this.responseIntents[decision] } + responseIsCurrent(url) { + const parsed = new URL(url, document.baseURI) + return !this.reviewWithdrawn && this.hasCommentFormTarget && this.commentFormTarget.action === parsed.href && + !this.removedRequestIds?.has(parsed.pathname.split("/").at(-2)) + } + async approve(event) { event.preventDefault() + if (this.approvalInvalidated) { this.invalidateApproval(); return } await this.withBusy(event.target, this.approvalErrorTarget, async () => { - const intent = this.responseIntent(this.responseUrl, this.approvalRevision, "approved", this.approvalNoteTarget.value) - const result = await this.write(this.responseUrl, "POST", { review_response: { revision: this.approvalRevision, decision: "approved", note: this.approvalNoteTarget.value, idempotency_key: intent.key } }) + const url = this.responseUrl + const intent = this.responseIntent(url, this.approvalRevision, "approved", this.approvalNoteTarget.value) + const result = await this.write(url, "POST", { review_response: { revision: this.approvalRevision, decision: "approved", note: this.approvalNoteTarget.value, idempotency_key: intent.key } }) this.confirmIntent("approved", intent) - const synced = !result.comment_thread_id || await this.refreshThreads(result.comment_thread_id) + const current = () => !this.approvalInvalidated && this.responseUrl === url && this.responseIsCurrent(url) + if (!current()) return + const synced = !result.comment_thread_id || await this.refreshThreads(result.comment_thread_id, current) + // Removal can also arrive after the server saved the old approval but + // before its result and comments arrive. Keep the dialog and its note. + if (!current()) return this.approvalTarget.close() this.approvalNoteTarget.value = "" if (this.hasNoticeTarget) this.noticeTarget.textContent = `Your approval of v${this.approvalRevision} is recorded.` + (synced ? "" : " Refresh the page to see your comment.") @@ -168,6 +191,9 @@ export default class extends Controller { if (tray) delete tray.dataset.reconsidering await this.refresh() }, "Recording…") + // A refresh can invalidate this dialog while the original POST is in + // flight. Keep it invalid after the busy-state cleanup restores buttons. + if (this.approvalInvalidated && this.approvalTarget.open) this.invalidateApproval() } async post(event) { @@ -177,11 +203,15 @@ export default class extends Controller { if (form.closest('[data-dictation-busy="true"]') || form.querySelector('[data-dictation-busy="true"]')) return const error = form.querySelector('[role="alert"]') await this.withBusy(form, error, async () => { - const intent = this.responseIntent(form.action, this.changesRevision, "changes_requested", input.value) - const result = await this.write(form.action, "POST", { review_response: { revision: this.changesRevision, decision: "changes_requested", note: input.value, idempotency_key: intent.key } }) + const url = form.action + const current = () => form.isConnected && this.responseIsCurrent(url) + const intent = this.responseIntent(url, this.changesRevision, "changes_requested", input.value) + const result = await this.write(url, "POST", { review_response: { revision: this.changesRevision, decision: "changes_requested", note: input.value, idempotency_key: intent.key } }) this.confirmIntent("changes_requested", intent) + if (!current()) return // Feedback lives in the ordinary discussion collection. - const synced = await this.refreshThreads(result.comment_thread_id) + const synced = await this.refreshThreads(result.comment_thread_id, current) + if (!current()) return this.noticeTarget.textContent = "Changes requested. Your feedback is in the plan’s comments." + (synced ? "" : " Refresh the page to see your comment.") input.value = "" this.collapse() @@ -189,13 +219,18 @@ export default class extends Controller { }, "Posting…") } - async refreshThreads(threadId) { + async refreshThreads(threadId, current = () => true) { + if (!this.element.isConnected) return false + // Another reviewer's removal can also make this collection snapshot stale. + const removalSequence = this.removalSequence const editor = document.querySelector('[data-coplan--inline-editor-comments-url-value]') if (!editor) return false try { const response = await fetch(editor.getAttribute("data-coplan--inline-editor-comments-url-value"), { credentials: "same-origin", headers: { Accept: "text/html" } }) if (response.ok) { - const incoming = new DOMParser().parseFromString(await response.text(), "text/html") + const html = await response.text() + if (!this.element.isConnected || !current() || removalSequence !== this.removalSequence) return false + const incoming = new DOMParser().parseFromString(html, "text/html") const id = `comment_thread_${threadId}` const thread = incoming.getElementById(id) if (thread && !document.getElementById(id)) document.getElementById("plan-threads")?.append(thread) @@ -211,7 +246,14 @@ export default class extends Controller { return false } - async refresh() { + async refresh(event) { + const removedId = event?.detail?.removedRequestId + if (removedId) { + this.removalSequence = (this.removalSequence || 0) + 1 + this.removedRequestIds ||= new Set() + this.removedRequestIds.add(removedId) + if (this.approvalTarget.open && this.responseUrl?.endsWith(`/${removedId}/respond`)) this.invalidateApproval() + } const sequence = ++this.refreshSequence try { const response = await fetch(this.urlValue, { credentials: "same-origin", headers: { Accept: "text/html" } }) @@ -220,6 +262,8 @@ export default class extends Controller { if (sequence !== this.refreshSequence || !this.element.isConnected) return const tray = incoming.querySelector(".review-tray") const existingTray = this.element.querySelector(".review-tray") + const responsePath = tray?.querySelector('[data-coplan--review-tray-target="commentForm"]')?.getAttribute("action") + if (this.approvalTarget.open && responsePath !== this.responseUrl) this.invalidateApproval() const wasWithdrawn = this.reviewWithdrawn this.reviewWithdrawn = !tray if (!tray && existingTray) { @@ -234,6 +278,12 @@ export default class extends Controller { } } if (tray && existingTray) existingTray.dataset.completed = tray.dataset.completed + if (tray && existingTray) { + // Keep the draft and dictation state, but submit new feedback to the + // current request. An already-issued fetch retains its original URL. + const form = tray.querySelector('[data-coplan--review-tray-target="commentForm"]') + if (form && this.hasCommentFormTarget) this.commentFormTarget.action = form.action + } if (tray && !this.element.querySelector(".review-tray")) this.element.append(tray) const currentTray = this.element.querySelector(".review-tray") if (tray && currentTray) { @@ -267,12 +317,13 @@ export default class extends Controller { event.preventDefault() if (!this.reviewerIdTarget.value) { this.assignmentErrorTarget.textContent = "Choose a person from the results first."; return } await this.withBusy(event.target, this.assignmentErrorTarget, async () => { - await this.write(this.urlValue, "POST", { review_request: { reviewer_id: this.reviewerIdTarget.value, requirement: event.target.querySelector('[type="checkbox"]').checked ? "required" : "optional" } }) + await this.write(this.urlValue, "POST", { review_request: { reviewer_id: this.reviewerIdTarget.value, requirement: event.target.querySelector('[type="checkbox"]').checked ? "required" : "optional", replace_withdrawn: true } }) this.searchTarget.value = ""; this.reviewerIdTarget.value = ""; this.resultsTarget.replaceChildren() this.closeAssignment() event.target.querySelector('[type="checkbox"]').checked = true this.addReviewerButtonTarget.focus({ preventScroll: true }) await this.refresh() + await this.refreshThreads(null) }, "Requesting…") this.assignButtonTarget.disabled = !this.reviewerIdTarget.value } @@ -354,7 +405,7 @@ export default class extends Controller { requestAgain(event) { this.manage(event, "PATCH", { review_request: { request_again: true } }) } changeRequirement(event) { this.manage(event, "PATCH", { review_request: { requirement: event.currentTarget.dataset.requirement } }) } - withdraw(event) { this.manage(event, "DELETE") } + withdraw(event) { this.manage(event, "DELETE", { review_request: { remove: true } }) } async manage(event, method, body) { if (this.managing) return @@ -372,7 +423,9 @@ export default class extends Controller { try { await this.withBusy(button, this.managementErrorTarget, async () => { await this.write(button.dataset.url, method, body) - await this.refresh() + const removedId = method === "DELETE" && body?.review_request?.remove ? new URL(button.dataset.url, document.baseURI).pathname.split("/").at(-1) : null + await this.refresh(removedId ? { detail: { removedRequestId: removedId } } : undefined) + if (removedId) await this.refreshThreads(null) }, "Updating…") } finally { this.managing = false diff --git a/engine/app/services/coplan/reviews/changed.rb b/engine/app/services/coplan/reviews/changed.rb index 14d76fe7..f3119dda 100644 --- a/engine/app/services/coplan/reviews/changed.rb +++ b/engine/app/services/coplan/reviews/changed.rb @@ -14,9 +14,17 @@ def call AgentEvents::Publish.call(plan: request.plan, event_type: event_type, actor_token_id: actor_token_id, payload: { "review_request_id" => request.id, "reviewer_id" => request.reviewer_id }.merge(payload)) Notifications::BroadcastBadges.call(user_ids: [ request.reviewer_id, request.plan.created_by_user_id ]) - # No viewer-relative HTML or CSRF tokens on a shared stream. Each - # open tab fetches its own controls, preserving its current draft. - Broadcaster.custom_action_to(request.plan, action: "coplan-review-changed", target: "plan-review-controls", html: "") + # No viewer-relative HTML or CSRF tokens on a shared stream. Announce + # removals before replacing comments so late HTTP results cannot put + # deleted decision labels back while the controls fetch is pending. + attrs = payload["removed"] ? { "data-removed-review-request-id" => request.id } : {} + Broadcaster.custom_action_to(request.plan, action: "coplan-review-changed", target: "plan-review-controls", html: "", attrs: attrs) + if payload["removed"] + # Public notes survive removal; their deleted review decisions must + # disappear from the discussion labels in open tabs as well. + Broadcaster.replace_to(request.plan, target: "plan-general-comment-list", partial: "coplan/plans/general_comment_list", + locals: { threads: request.plan.comment_threads.with_kept_comments.includes(:comments).order(:created_at) }) + end end end end diff --git a/engine/app/services/coplan/reviews/remove.rb b/engine/app/services/coplan/reviews/remove.rb new file mode 100644 index 00000000..e269863b --- /dev/null +++ b/engine/app/services/coplan/reviews/remove.rb @@ -0,0 +1,22 @@ +module CoPlan + module Reviews + # A later assignment gets a fresh ID, so old response URLs stay invalid. + class Remove + def self.call(plan:, request_id:) + request = plan.with_lock do + request = plan.review_requests.find_by(id: request_id) + if request + unless plan.plan_events.where(event_type: "review_requested", after_value: request.id).exists? + RecordRequest.call(request: request) + end + request.destroy! + end + request + end + Changed.call(request: request, event_type: "review.requested", + payload: { "action" => "removed", "removed" => true }) if request + request + end + end + end +end diff --git a/engine/app/services/coplan/reviews/request.rb b/engine/app/services/coplan/reviews/request.rb index 5e646a28..2a7d8f21 100644 --- a/engine/app/services/coplan/reviews/request.rb +++ b/engine/app/services/coplan/reviews/request.rb @@ -1,27 +1,50 @@ module CoPlan module Reviews class Request - def self.call(plan:, reviewer:, actor:, requirement: "required", api_token: nil, create_only: false) - new(plan:, reviewer:, actor:, requirement:, api_token:, create_only:).call + def self.call(plan:, reviewer:, actor:, requirement: "required", api_token: nil, create_only: false, replace_withdrawn: false) + new(plan:, reviewer:, actor:, requirement:, api_token:, create_only:, replace_withdrawn:).call end - def initialize(plan:, reviewer:, actor:, requirement:, api_token:, create_only:) + def self.again(request:, actor:, requirement: nil, api_token: nil) + new(plan: request.plan, reviewer: request.reviewer, request:, actor:, requirement:, api_token:, + create_only: false, replace_withdrawn: false).call + end + + def initialize(plan:, reviewer:, actor:, requirement:, api_token:, create_only:, replace_withdrawn:, request: nil) @plan, @reviewer, @actor, @requirement, @api_token = plan, reviewer, actor, requirement, api_token @create_only = create_only + @request = request + @replace_withdrawn = replace_withdrawn end def call changed = false + removed = nil request = @plan.with_lock do - request = @plan.review_requests.find_or_initialize_by(reviewer: @reviewer) - # Retrying a create cannot change a decision, reopen a withdrawal, - # or send another nudge. Only an explicit re-request starts a cycle. + request = if @request + # The plan lock does not reload its children. Reject a request + # removed while this operation was waiting for the lock. + @request.reload + @requirement = @request.requirement if @requirement.nil? + @request + else + @plan.review_requests.find_or_initialize_by(reviewer: @reviewer) + end + # API create retries preserve every existing task. The web add form + # deliberately replaces withdrawn tasks left by earlier removals. if @create_only && request.persisted? unless ReviewRequest::REQUIREMENTS.include?(@requirement) request.errors.add(:requirement, :inclusion) raise ActiveRecord::RecordInvalid, request end - next request + next request unless @replace_withdrawn && request.status == "withdrawn" + + unless @plan.plan_events.where(event_type: "review_requested", after_value: request.id).exists? + RecordRequest.call(request: request) + end + request.destroy! + removed = request + request = @plan.review_requests.new(reviewer: @reviewer) end # Preserve the original request for tasks created before request @@ -39,6 +62,8 @@ def call changed = true request end + Changed.call(request: removed, event_type: "review.requested", actor_token_id: @api_token&.id, + payload: { "action" => "removed", "removed" => true }) if removed Changed.call(request: request, event_type: "review.requested", actor_token_id: @api_token&.id, payload: { "action" => "requested", "requirement" => request.requirement }) if changed request diff --git a/engine/app/views/coplan/agent_instructions/guides/comments.text.erb b/engine/app/views/coplan/agent_instructions/guides/comments.text.erb index 8ae64f43..1a255732 100644 --- a/engine/app/views/coplan/agent_instructions/guides/comments.text.erb +++ b/engine/app/views/coplan/agent_instructions/guides/comments.text.erb @@ -87,4 +87,5 @@ Discussion is the default. An approval means “satisfied with this exact versio - The plan author's credential may `GET /api/v1/plans/:plan_id/review_requests/suggestions` for suggested reviewers. Each entry in `suggestions` has `reviewer` (`id`, `name`, `username`), a `reason`, and a suggested `requirement`. A deployment may base them on the files and repositories the plan links to, so link the files a plan will change before asking. The list leaves out the author and people already reviewing, and is empty when the deployment suggests nobody. Suggestions are not requests. When your principal wants the plan reviewed, `POST` each chosen `reviewer.id` with its `requirement`. - `PATCH /api/v1/plans/:plan_id/review_requests/:id` accepts `requirement` without reopening the task or starting another review cycle. Use `{ "request_again": true }` when feedback is ready for another look. That preserves earlier responses and notifies the reviewer again. Live events distinguish these actions; see the live guide. - `DELETE` the same path withdraws the request and records the withdrawing actor and time in review activity. Earlier request cycles and decisions remain. It clears the reviewer's request nudge, not the author's unread responses. Archiving a plan does not withdraw pending reviews. +- Removing a reviewer in the web management pane is different from API withdrawal: it deletes their request and past review decisions. Adding them again creates a new pending request with a new ID. Requests to the removed ID return 404; stop using that ID and fetch the current requests. Never retry an old response against a replacement request. - Only the requested reviewer's credential may `POST /api/v1/plans/:plan_id/review_requests/:id/respond` with `{ "revision": 4, "decision": "approved", "note": "optional public note", "idempotency_key": "RESPONSE_UUID" }` or `{ "revision": 4, "decision": "changes_requested", "note": "what needs adjustment", "idempotency_key": "RESPONSE_UUID" }`. Change requests create ordinary public comment threads. Agent responses are explicitly attributed to the token's agent acting for its human owner. A stale revision returns 409 with `code: review_version_changed`; re-read before responding. A new response to a withdrawn request returns 409 with `code: review_request_withdrawn`; stop responding unless the author requests another look. Replace `RESPONSE_UUID` with a fresh UUID for each intended response. Retry with the same UUID and identical fields if the HTTP response is lost. Use a new UUID for another review, even for the same version and decision. A keyed retry returns the original result even after a plan edit or withdrawal. It does not complete a newly re-requested task. Without a key, identical fields are treated as a retry. Retries may use a renewed token for the same human principal. The original response keeps its original agent and token attribution. Reusing a key with different fields returns 422. Never manufacture a human sign-off. diff --git a/engine/app/views/coplan/agent_instructions/guides/live.text.erb b/engine/app/views/coplan/agent_instructions/guides/live.text.erb index c682ed2c..7761d69c 100644 --- a/engine/app/views/coplan/agent_instructions/guides/live.text.erb +++ b/engine/app/views/coplan/agent_instructions/guides/live.text.erb @@ -105,6 +105,7 @@ For `review.requested`, inspect `payload.action`: | `requested` | A new request or another look. `requirement` is `required` or `optional`. | | `requirement_changed` | Only `requirement` changed. This does not reopen the task or start another review cycle. Do not treat it as a request for another look. | | `withdrawn` | The request was withdrawn. Stop working on that request; its earlier decisions remain. The payload also has `withdrawn: true`. | +| `removed` | The web management pane deleted this request and its review decisions. Stop using its ID. Reassignment creates a new pending request. The payload also has `removed: true`. | The attach and bridge tools show this review data and point to the review guide. Review events do not authorize content edits or execution. diff --git a/engine/app/views/coplan/reviews/_roster.html.erb b/engine/app/views/coplan/reviews/_roster.html.erb index 2092a34b..898fbca8 100644 --- a/engine/app/views/coplan/reviews/_roster.html.erb +++ b/engine/app/views/coplan/reviews/_roster.html.erb @@ -21,7 +21,7 @@ <% if approval && approval.plan_version.revision != plan.current_revision %><% end %> - + <% elsif approval && approval.plan_version.revision != plan.current_revision %> diff --git a/spec/requests/api/v1/review_requests_spec.rb b/spec/requests/api/v1/review_requests_spec.rb index 3e0a02aa..595bb8b1 100644 --- a/spec/requests/api/v1/review_requests_spec.rb +++ b/spec/requests/api/v1/review_requests_spec.rb @@ -14,6 +14,37 @@ def headers(token) before { author_token; reviewer_token } + [ true, false ].each do |keyed| + it "cannot replay a removed #{keyed ? 'keyed' : 'legacy'} review into a fresh request", :review_removal do + payload = { revision: 1, decision: "changes_requested", note: "Earlier feedback" } + payload[:idempotency_key] = SecureRandom.uuid if keyed + post respond_api_v1_plan_review_request_path(plan, task), params: payload, headers: headers("reviews-reviewer"), as: :json + expect(response).to have_http_status(:created) + old_response_id = response.parsed_body.fetch("id") + sign_in_as(author) + delete plan_review_request_path(plan, task), params: { review_request: { remove: true } }, as: :json + expect(response).to have_http_status(:no_content) + post api_v1_plan_review_requests_path(plan), params: { reviewer_id: reviewer.id }, headers: headers("reviews-owner"), as: :json + expect(response).to have_http_status(:created) + fresh = plan.review_requests.find(response.parsed_body.fetch("id")) + expect(fresh.id).not_to eq(task.id) + expect(response.parsed_body.fetch("responses")).to be_empty + + post respond_api_v1_plan_review_request_path(plan, task), params: payload, headers: headers("reviews-reviewer"), as: :json + expect(response).to have_http_status(:not_found) + patch api_v1_plan_review_request_path(plan, task), params: { request_again: true }, headers: headers("reviews-owner"), as: :json + expect(response).to have_http_status(:not_found) + delete api_v1_plan_review_request_path(plan, task), headers: headers("reviews-owner") + expect(response).to have_http_status(:not_found) + expect(CoPlan::ReviewResponse.exists?(old_response_id)).to be(false) + expect(fresh.reload.status).to eq("pending") + get api_v1_plan_review_requests_path(plan), headers: headers("reviews-owner") + requests = response.parsed_body.fetch("review_requests") + expect(requests.map { |request| request.fetch("id") }).to eq([ fresh.id ]) + expect(requests.first.fetch("responses")).to be_empty + end + end + [ true, false ].each do |keyed| it "replays a #{keyed ? 'keyed' : 'legacy unkeyed'} response through a renewed session token without changing its attribution", :review_retries do first_token, first_secret = reviewer_token.mint_session_token!(agent_name: "Originalbot") diff --git a/spec/requests/review_requests_spec.rb b/spec/requests/review_requests_spec.rb index 4a4c9baa..6ac79639 100644 --- a/spec/requests/review_requests_spec.rb +++ b/spec/requests/review_requests_spec.rb @@ -6,6 +6,15 @@ let(:plan) { create(:plan, :published, created_by_user: author) } let(:task) { create(:review_request, plan: plan, reviewer: reviewer) } + it "allows only the plan author to erase a reviewer's decisions", :review_removal do + previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: "approved") + sign_in_as(reviewer) + delete plan_review_request_path(plan, task), params: { review_request: { remove: true } }, as: :json + expect(response).to have_http_status(:not_found) + expect(previous.reload.decision).to eq("approved") + expect(task.reload.status).to eq("approved") + end + it "marks an unread note-less review decision as attention until its owner reads it", :review_attention do CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: plan.current_revision, decision: "approved") notice = author.notifications.find_by!(review_request: task, reason: "review_response") diff --git a/spec/services/coplan/reviews/remove_spec.rb b/spec/services/coplan/reviews/remove_spec.rb new file mode 100644 index 00000000..7340a19c --- /dev/null +++ b/spec/services/coplan/reviews/remove_spec.rb @@ -0,0 +1,117 @@ +require "rails_helper" + +RSpec.describe CoPlan::Reviews::Remove do + let(:plan) { create(:plan, :published) } + let(:reviewer) { create(:coplan_user) } + let(:task) { CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user) } + + %w[approved changes_requested].each do |decision| + it "erases a removed reviewer's #{decision} decisions and starts a new pending request" do + previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, + decision: decision, note: decision == "changes_requested" ? "Earlier feedback" : nil) + other = CoPlan::Reviews::Request.call(plan: plan, reviewer: create(:coplan_user), actor: plan.created_by_user) + other_response = CoPlan::Reviews::Respond.call(request: other, actor: other.reviewer, revision: 1, decision: "approved") + + described_class.call(plan: plan, request_id: task.id) + + expect(CoPlan::ReviewRequest.exists?(task.id)).to be(false) + expect(CoPlan::ReviewResponse.exists?(previous.id)).to be(false) + expect(plan.notifications.where(review_request_id: task.id)).to be_empty + expect(other_response.reload.decision).to eq("approved") + + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user, create_only: true) + expect(fresh.id).not_to eq(task.id) + expect(fresh.status).to eq("pending") + expect(fresh.review_responses).to be_empty + expect(fresh.latest_approval).to be_nil + end + end + + it "does not remove or reset the replacement when an old removal is retried" do + old_id = task.id + described_class.call(plan: plan, request_id: old_id) + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user, create_only: true) + approval = CoPlan::Reviews::Respond.call(request: fresh, actor: reviewer, revision: 1, decision: "approved") + expect { described_class.call(plan: plan, request_id: old_id) }.not_to change(CoPlan::Notification, :count) + expect(fresh.reload.status).to eq("approved") + expect(fresh.review_responses).to eq([ approval ]) + end + + it "preserves a legacy request's audit before deleting the task" do + legacy = create(:review_request, plan: plan, reviewer: reviewer) + expect(plan.plan_events.where(event_type: "review_requested")).to be_empty + described_class.call(plan: plan, request_id: legacy.id) + event = plan.plan_events.find_by!(event_type: "review_requested", after_value: legacy.id) + expect(event).to have_attributes(actor_id: legacy.requested_by_user_id, created_at: legacy.requested_at) + expect(event.metadata).to include("reviewer_name" => reviewer.name, "revision" => 1) + expect(CoPlan::ReviewRequest.exists?(legacy.id)).to be(false) + end + + it "announces the old identity's removal when replacing a withdrawn legacy request" do + legacy = create(:review_request, plan: plan, reviewer: reviewer, status: "withdrawn") + watcher = create(:api_token, user: reviewer) + CoPlan::AgentSession.create!(plan: plan, api_token: watcher, state: "watching", agent_name: "Watcher") + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user, + create_only: true, replace_withdrawn: true) + payloads = watcher.agent_events.where(event_type: "review.requested").order(:created_at).map(&:payload) + expect(payloads.size).to eq(2) + expect(payloads.first).to include("action" => "removed", "removed" => true, "review_request_id" => legacy.id) + expect(payloads.last).to include("action" => "requested", "review_request_id" => fresh.id) + expect(plan.plan_events.where(event_type: "review_requested").pluck(:after_value)).to contain_exactly(legacy.id, fresh.id) + expect(fresh.status).to eq("pending") + end + + it "keeps the old review if a replacement request is invalid" do + previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: "approved") + CoPlan::Reviews::Withdraw.call(request: task, actor: plan.created_by_user) + expect { + CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: plan.created_by_user, + requirement: "invalid", create_only: true, replace_withdrawn: true) + }.to raise_error(ActiveRecord::RecordInvalid) + expect(previous.reload.decision).to eq("approved") + expect(task.reload.status).to eq("withdrawn") + end + + describe "requests already waiting for the plan lock" do + self.use_transactional_tests = false + after { truncate_tables(*(ActiveRecord::Base.connection.tables - %w[schema_migrations ar_internal_metadata])) } + + %i[respond request_again].each do |action| + it "rejects an old #{action} after removal and reassignment commit" do + old_id = task.id + reviewer_id = reviewer.id + ready = Queue.new + worker = nil + fresh = nil + plan.with_lock do + worker = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + stale = CoPlan::ReviewRequest.find(old_id) + ready << true + begin + if action == :respond + CoPlan::Reviews::Respond.call(request: stale, actor: stale.reviewer, revision: 1, decision: "approved") + else + CoPlan::Reviews::Request.again(request: stale, actor: stale.requested_by_user) + end + rescue ActiveRecord::RecordNotFound => error + error + end + end + end + Timeout.timeout(10) { ready.pop } + described_class.call(plan: plan, request_id: old_id) + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: CoPlan::User.find(reviewer_id), + actor: plan.created_by_user, create_only: true) + end + expect(worker.join(10)).to be_present + expect(worker.value).to be_a(ActiveRecord::RecordNotFound) + expect(fresh.reload.status).to eq("pending") + expect(fresh.review_responses).to be_empty + expect(plan.review_requests.pluck(:id)).to eq([ fresh.id ]) + ensure + worker&.kill if worker&.alive? + end + end + end +end diff --git a/spec/services/coplan/reviews_spec.rb b/spec/services/coplan/reviews_spec.rb index ba55bceb..ee19d1fe 100644 --- a/spec/services/coplan/reviews_spec.rb +++ b/spec/services/coplan/reviews_spec.rb @@ -23,6 +23,18 @@ def respond(decision: "approved", note: nil, **attributes) expect(plan.plan_collaborators).to be_empty end + it "re-requests the existing task using its current requirement unless overridden" do + stale = request + CoPlan::ReviewRequest.find(stale.id).update!(requirement: "optional") + + repeated = CoPlan::Reviews::Request.again(request: stale, actor: plan.created_by_user) + expect(repeated).to have_attributes(id: stale.id, requirement: "optional", status: "pending") + expect(plan.plan_events.where(event_type: "review_requested").last.metadata).to include("requirement" => "optional") + + overridden = CoPlan::Reviews::Request.again(request: stale, actor: plan.created_by_user, requirement: "required") + expect(overridden).to have_attributes(id: stale.id, requirement: "required", status: "pending") + end + it "preserves the original approval after edits and another review request" do response = respond(note: "Looks right") expect(response.comment_thread.comments.first.body_markdown).to eq("Looks right") diff --git a/spec/system/plan_reviews_spec.rb b/spec/system/plan_reviews_spec.rb index b529b4eb..f91bf6d4 100644 --- a/spec/system/plan_reviews_spec.rb +++ b/spec/system/plan_reviews_spec.rb @@ -38,17 +38,56 @@ def lose_next_review_result JS end + # Hold automatic invalidations so both separate and coalesced delivery can + # be exercised deterministically without replacing the open draft. + def pause_review_refreshes + page.execute_script(<<~JS) + const root = document.getElementById("plan-review-controls"); + const controller = window.Stimulus.getControllerForElementAndIdentifier(root, "coplan--review-tray"); + window.refreshReviewControls = controller.refresh.bind(controller); + controller.refresh = async () => {}; + JS + end + + def refresh_review_controls + page.driver.browser.execute_async_script("window.refreshReviewControls().then(arguments[0])") + end + + def hold_review_delivery(stage, controls: false) + page.execute_script(<<~JS, stage, controls) + const stage = arguments[0], blockControls = arguments[1]; + const originalFetch = window.fetch.bind(window); + const controlsUrl = document.getElementById("plan-review-controls").getAttribute("data-coplan--review-tray-url-value"); + const commentsUrl = document.querySelector('[data-coplan--inline-editor-comments-url-value]').getAttribute("data-coplan--inline-editor-comments-url-value"); + let held = false; + const controls = []; + window.blockReviewControls = blockControls; + window.releaseReviewControls = () => { + window.blockReviewControls = false; + controls.splice(0).forEach(resolve => resolve()); + }; + window.fetch = async (...args) => { + if (window.blockReviewControls && String(args[0]) === controlsUrl && !args[1]?.method) { + await new Promise(resolve => controls.push(resolve)); + } + const response = await originalFetch(...args); + const selected = stage === "result" ? args[1]?.method === "POST" && String(args[0]).endsWith("/respond") : String(args[0]) === commentsUrl; + if (!held && selected) { + held = true; + window.heldReviewDelivery = await response.clone().text(); + document.documentElement.dataset.reviewDeliveryHeld = "true"; + await new Promise(resolve => { window.releaseReviewDelivery = resolve; }); + } + return response; + }; + JS + end + before { sign_in_browser(reviewer, open_sections: !RSpec.current_example.metadata[:collapsed_support]) } - it "keeps a withdrawn request between its two request cycles", :review_withdrawals do + it "keeps an API-withdrawn request between its two request cycles", :review_withdrawals do sign_in_browser(author) - click_button "More actions" - within("#plan-menu") { click_button "Manage reviewers" } - within(".review-management[open]") do - click_button "Actions for Plan Reviewer", enable_aria_label: true - click_button "Remove request" - expect(page).to have_no_css("[data-review-assignments] .review-person") - end + CoPlan::Reviews::Withdraw.call(request: task, actor: author) expect(task.reload.status).to eq("withdrawn") CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author) visit plan_page_path(plan) @@ -66,6 +105,434 @@ def lose_next_review_result end end + [ [ "light", "approved" ], [ "dark", "changes_requested" ] ].each do |theme, decision| + it "starts fresh after removing a #{decision} reviewer in #{theme} theme", :review_readdition do + task.destroy! + requested = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author) + previous = CoPlan::Reviews::Respond.call(request: requested, actor: reviewer, revision: 1, decision: decision, + note: decision == "changes_requested" ? "Earlier review feedback" : nil) + sign_in_browser(author) + page.execute_script("document.documentElement.dataset.theme = arguments[0]", theme) + click_button "More actions" + within("#plan-menu") { click_button "Manage reviewers" } + within(".review-management[open]") do + click_button "Actions for Plan Reviewer", enable_aria_label: true + click_button "Remove reviewer" + expect(page).to have_no_css("[data-review-assignments] .review-person") + click_button "Add reviewer", exact: true + end + expect(CoPlan::ReviewRequest.exists?(requested.id)).to be(false) + expect(CoPlan::ReviewResponse.exists?(previous.id)).to be(false) + fill_in "Find a reviewer", with: reviewer.name + find(".review-search-result", text: reviewer.name).click + uncheck "Required" + within(".review-assignment-form") { click_button "Add reviewer", exact: true } + within(".review-management[open] [data-review-assignments]") do + expect(page).to have_css(".review-person", text: "Plan Reviewer") + expect(page).to have_css(".review-person__requirement", text: "Optional") + expect(page).to have_css(".review-state", text: "Pending") + end + fresh = plan.review_requests.find_by!(reviewer: reviewer) + expect(fresh.id).not_to eq(requested.id) + expect(fresh).to have_attributes(status: "pending", requirement: "optional") + expect(plan.review_requests.where(reviewer: reviewer).count).to eq(1) + expect(fresh.review_responses).to be_empty + expect(fresh.notifications.unread.where(user: reviewer, reason: "review_requested").count).to eq(1) + page.save_screenshot(Rails.root.join("tmp/review-preview/readded-reviewer-#{theme}.png")) + within(".review-management[open]") { click_button "Close reviews", enable_aria_label: true } + within("#footnote-approvals") { click_button "View activity", exact: false } + within(".review-activity-dialog[open]") do + expect(all(".review-log__event").map { |entry| entry.text.squish }).to eq([ "Plan Author requested a review from Plan Reviewer" ] * 2) + expect(page).to have_no_text("Earlier review feedback") + expect(plan.plan_events.where(event_type: "review_requested").pluck(:after_value)).to contain_exactly(requested.id, fresh.id) + end + page.save_screenshot(Rails.root.join("tmp/review-preview/fresh-review-history-#{theme}.png")) + sign_in_browser(reviewer) + expect(page).to have_css(".review-tray", text: "Your review is requested") + end + end + + it "re-adds a reviewer removed before permanent removal was supported", :review_readdition do + previous = CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: "approved") + CoPlan::Reviews::Withdraw.call(request: task, actor: author) + sign_in_browser(author) + click_button "More actions" + within("#plan-menu") { click_button "Manage reviewers" } + within(".review-management[open]") { click_button "Add reviewer", exact: true } + fill_in "Find a reviewer", with: reviewer.name + find(".review-search-result", text: reviewer.name).click + within(".review-assignment-form") { click_button "Add reviewer", exact: true } + expect(page).to have_css(".review-person", text: reviewer.name) + expect(page).to have_css(".review-state", text: "Pending") + fresh = plan.review_requests.find_by!(reviewer: reviewer) + expect(fresh.id).not_to eq(task.id) + expect(fresh.review_responses).to be_empty + expect(CoPlan::ReviewResponse.exists?(previous.id)).to be(false) + end + + it "leaves an already active review unchanged when a selected re-add is submitted", :review_readdition do + CoPlan::Reviews::Withdraw.call(request: task, actor: author) + sign_in_browser(author) + click_button "More actions" + within("#plan-menu") { click_button "Manage reviewers" } + within(".review-management[open]") { click_button "Add reviewer", exact: true } + fill_in "Find a reviewer", with: reviewer.name + find(".review-search-result", text: reviewer.name).click + # Another tab restores and completes this request after the person was + # selected. Submitting the stale add form must not reset that decision. + restored = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author, requirement: "optional") + approval = CoPlan::Reviews::Respond.call(request: restored, actor: reviewer, revision: 1, decision: "approved") + counts = [ plan.plan_events.where(event_type: "review_requested").count, CoPlan::Notification.count ] + within(".review-assignment-form") { click_button "Add reviewer", exact: true } + within(".review-management[open] [data-review-assignments]") do + expect(page).to have_css(".review-person", text: reviewer.name) + expect(page).to have_css(".review-state", text: "Approved") + expect(page).to have_css(".review-person__requirement", text: "Optional") + end + expect(task.reload).to have_attributes(status: "approved", requirement: "optional") + expect(task.review_responses.pluck(:id)).to eq([ approval.id ]) + expect([ plan.plan_events.where(event_type: "review_requested").count, CoPlan::Notification.count ]).to eq(counts) + end + + [ false, true ].each do |legacy| + it "updates discussion labels after #{legacy ? 'legacy reassignment' : 'removal'} without ActionCable", :review_offline do + CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: "approved", note: "Retain this public discussion") + CoPlan::Reviews::Withdraw.call(request: task, actor: author) if legacy + sign_in_browser(author) + expect(page).to have_css(".document-comment__type", text: "Review · Approved · v1") + page.execute_script('document.querySelectorAll("turbo-cable-stream-source").forEach(source => source.remove())') + expect(page).to have_no_css("turbo-cable-stream-source", visible: :all) + click_button "More actions" + within("#plan-menu") { click_button "Manage reviewers" } + within(".review-management[open]") do + if legacy + click_button "Add reviewer", exact: true + else + click_button "Actions for Plan Reviewer", enable_aria_label: true + click_button "Remove reviewer" + expect(page).to have_no_css("[data-review-assignments] .review-person") + end + end + if legacy + fill_in "Find a reviewer", with: reviewer.name + find(".review-search-result", text: reviewer.name).click + within(".review-assignment-form") { click_button "Add reviewer", exact: true } + expect(page).to have_css(".review-state", text: "Pending") + end + expect(page).to have_css(".document-comment__type", text: "Plan comment") + expect(page).to have_no_css(".document-comment__type", text: "Review · Approved") + within(".review-management[open]") { click_button "Close reviews", enable_aria_label: true } + expect(page).to have_content("Retain this public discussion") + end + end + + [ false, true ].each do |legacy| + it "does not apply a delayed #{legacy ? 'reassignment' : 'removal'} discussion refresh to another plan", :review_navigation do + CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: "approved", note: "Plan A review discussion") + CoPlan::Reviews::Withdraw.call(request: task, actor: author) if legacy + destination = create(:plan, :published, created_by_user: author, title: "Destination plan") + thread = create(:comment_thread, plan: destination, created_by_user: author) + create(:comment, comment_thread: thread, author_id: author.id, body_markdown: "Plan B discussion stays here") + sign_in_browser(author) + hold_review_delivery("comments") + page.execute_script(<<~JS) + const root = document.getElementById("plan-review-controls"); + const controller = window.Stimulus.getControllerForElementAndIdentifier(root, "coplan--review-tray"); + const refresh = controller.refreshThreads.bind(controller); + controller.refreshThreads = async (...args) => { + const result = await refresh(...args); + document.documentElement.dataset.oldReviewFinished = "true"; + return result; + }; + JS + click_button "More actions" + within("#plan-menu") { click_button "Manage reviewers" } + within(".review-management[open]") do + if legacy + click_button "Add reviewer", exact: true + else + click_button "Actions for Plan Reviewer", enable_aria_label: true + click_button "Remove reviewer" + end + end + if legacy + fill_in "Find a reviewer", with: reviewer.name + find(".review-search-result", text: reviewer.name).click + within(".review-assignment-form") { click_button "Add reviewer", exact: true } + end + expect(page).to have_css('html[data-review-delivery-held="true"]') + expect(page.evaluate_script("window.heldReviewDelivery")).to include("Plan A review discussion") + page.execute_script("window.Turbo.visit(arguments[0])", plan_page_path(destination)) + expect(page).to have_current_path(plan_page_path(destination)) + expect(page).to have_css("#plan-general-comment-list", text: "Plan B discussion stays here", visible: :all) + page.execute_script("window.releaseReviewDelivery()") + expect(page).to have_css('html[data-old-review-finished="true"]') + expect(page).to have_css("#plan-general-comment-list", text: "Plan B discussion stays here", visible: :all) + expect(page).to have_no_css("#plan-general-comment-list", text: "Plan A review discussion", visible: :all) + end + end + + [ [ "light", false ], [ "dark", true ] ].each do |theme, separate_refresh| + it "submits a preserved draft to the replacement after #{separate_refresh ? 'separate' : 'coalesced'} refreshes", :review_readdition do + page.execute_script("document.documentElement.dataset.theme = arguments[0]", theme) + within(".review-tray") { click_button "Request changes", exact: true } + fill_in "Public review comment", with: "Please clarify the retry limits.", enable_aria_label: true + page.execute_script('window.reviewDraft = document.getElementById("review-discussion-body")') + pause_review_refreshes + CoPlan::Reviews::Remove.call(plan: plan, request_id: task.id) + if separate_refresh + refresh_review_controls + expect(page).to have_css('.review-tray__discussion [type="submit"][disabled]') + expect(page).to have_content("Your unsent feedback is still here.") + end + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author, create_only: true) + refresh_review_controls + expect(page).to have_css("[data-review-decision-actions] button[data-url*='#{fresh.id}']", visible: :all) + expect(find('[data-coplan--review-tray-target="commentForm"]')["action"]).to end_with(respond_plan_review_request_path(plan, fresh)) + expect(page).to have_field("Public review comment", with: "Please clarify the retry limits.", enable_aria_label: true) + expect(page.evaluate_script('window.reviewDraft === document.getElementById("review-discussion-body")')).to be(true) + page.save_screenshot(Rails.root.join("tmp/review-preview/preserved-reassigned-draft-#{theme}.png")) + within(".review-tray__discussion") { click_button "Request changes" } + expect(page).to have_content("Changes requested. Your feedback is in the plan’s comments.") + expect(fresh.reload.status).to eq("changes_requested") + expect(fresh.review_responses.last!.note).to eq("Please clarify the retry limits.") + expect(CoPlan::ReviewRequest.exists?(task.id)).to be(false) + end + end + + it "keeps an in-flight submission bound to the removed request", :review_readdition do + within(".review-tray") { click_button "Request changes", exact: true } + fill_in "Public review comment", with: "Feedback submitted before removal", enable_aria_label: true + pause_review_refreshes + page.execute_script(<<~JS) + const originalFetch = window.fetch.bind(window); + window.reviewWrites = []; + window.fetch = async (...args) => { + if (args[1]?.method === "POST" && String(args[0]).endsWith("/respond")) { + window.reviewWrites.push({ url: String(args[0]), key: JSON.parse(args[1].body).review_response.idempotency_key }); + if (window.reviewWrites.length === 1) { + document.documentElement.dataset.reviewWriteHeld = "true"; + await new Promise(resolve => { window.releaseReviewWrite = resolve; }); + } + } + return originalFetch(...args); + }; + JS + within(".review-tray__discussion") { click_button "Request changes" } + expect(page).to have_css('html[data-review-write-held="true"]') + CoPlan::Reviews::Remove.call(plan: plan, request_id: task.id) + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author, create_only: true) + refresh_review_controls + page.execute_script("window.releaseReviewWrite()") + expect(page).to have_content("Couldn't save your response. Your draft is still here.") + expect(fresh.reload.status).to eq("pending") + expect(fresh.review_responses).to be_empty + expect(page).to have_field("Public review comment", with: "Feedback submitted before removal", enable_aria_label: true) + within(".review-tray__discussion") { click_button "Request changes" } + expect(page).to have_content("Changes requested. Your feedback is in the plan’s comments.") + writes = page.evaluate_script("window.reviewWrites") + expect(writes.map { |write| URI(write.fetch("url")).path }).to eq([ + respond_plan_review_request_path(plan, task), respond_plan_review_request_path(plan, fresh) + ]) + expect(writes.map { |write| write.fetch("key") }.uniq.size).to eq(2) + expect(fresh.review_responses.count).to eq(1) + end + + [ [ "light", false ], [ "dark", true ] ].each do |theme, separate_refresh| + it "invalidates an open approval after #{separate_refresh ? 'separate' : 'coalesced'} reassignment refreshes", :approval_reassignment do + page.execute_script("document.documentElement.dataset.theme = arguments[0]", theme) + within(".review-tray") { click_button "Approve", exact: true } + fill_in "Public note", with: "Keep the approval audit trail." + pause_review_refreshes + CoPlan::Reviews::Remove.call(plan: plan, request_id: task.id) + refresh_review_controls if separate_refresh + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author, create_only: true) + refresh_review_controls + within(".review-confirmation[open]") do + expect(page).to have_content("This review request changed.") + expect(page).to have_button("Approve v1", disabled: true) + expect(page).to have_field("Public note", with: "Keep the approval audit trail.") + page.save_screenshot(Rails.root.join("tmp/review-preview/invalidated-approval-#{theme}.png")) + click_button "Close approval", enable_aria_label: true + end + expect(fresh.reload.status).to eq("pending") + expect(fresh.review_responses).to be_empty + within(".review-tray") { click_button "Approve", exact: true } + expect(page).to have_field("Public note", with: "Keep the approval audit trail.") + click_button "Approve v1" + expect(page).to have_no_css(".review-confirmation[open]") + expect(fresh.reload.status).to eq("approved") + expect(fresh.review_responses.last!.note).to eq("Keep the approval audit trail.") + end + end + + it "keeps an approval dialog usable when only its requirement changes", :approval_reassignment do + within(".review-tray") { click_button "Approve", exact: true } + fill_in "Public note", with: "Same review request" + pause_review_refreshes + task.update!(requirement: "optional") + refresh_review_controls + expect(page).to have_button("Approve v1", disabled: false) + expect(page).to have_no_content("This review request changed.") + click_button "Approve v1" + expect(page).to have_no_css(".review-confirmation[open]") + expect(task.reload.status).to eq("approved") + end + + [ false, true ].each do |saved| + it "invalidates an approval removed #{saved ? 'after' : 'before'} its in-flight submission is saved", :approval_reassignment do + within(".review-tray") { click_button "Approve", exact: true } + fill_in "Public note", with: "An approval sent before removal" + pause_review_refreshes + page.execute_script(<<~JS, saved) + const holdAfterSave = arguments[0]; + const originalFetch = window.fetch.bind(window); + window.reviewWrites = []; + window.fetch = async (...args) => { + if (args[1]?.method === "POST" && String(args[0]).endsWith("/respond")) { + window.reviewWrites.push({ url: String(args[0]), key: JSON.parse(args[1].body).review_response.idempotency_key }); + if (window.reviewWrites.length === 1) { + const result = holdAfterSave ? await originalFetch(...args) : null; + document.documentElement.dataset.reviewWriteHeld = "true"; + await new Promise(resolve => { window.releaseReviewWrite = resolve; }); + if (result) return result; + } + } + return originalFetch(...args); + }; + JS + click_button "Approve v1" + expect(page).to have_css('html[data-review-write-held="true"]') + CoPlan::Reviews::Remove.call(plan: plan, request_id: task.id) + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author, create_only: true) + refresh_review_controls + page.execute_script("window.releaseReviewWrite()") + expect(page).to have_no_css('.review-confirmation form[aria-busy="true"]') + expect(page).to have_content("This review request changed.") + expect(page).to have_button("Approve v1", disabled: true) + expect(page).to have_field("Public note", with: "An approval sent before removal") + expect(fresh.reload.status).to eq("pending") + expect(fresh.review_responses).to be_empty + click_button "Close approval", enable_aria_label: true + within(".review-tray") { click_button "Approve", exact: true } + click_button "Approve v1" + expect(page).to have_no_css(".review-confirmation[open]") + writes = page.evaluate_script("window.reviewWrites") + expect(writes.map { |write| URI(write.fetch("url")).path }).to eq([ + respond_plan_review_request_path(plan, task), respond_plan_review_request_path(plan, fresh) + ]) + expect(writes.map { |write| write.fetch("key") }.uniq.size).to eq(2) + expect(fresh.review_responses.count).to eq(1) + expect(fresh.reload.status).to eq("approved") + end + end + + it "preserves feedback when a saved result arrives after reassignment", :review_delivery do + expect(page).to have_css("turbo-cable-stream-source[connected]", visible: :all) + hold_review_delivery("result", controls: true) + within(".review-tray") { click_button "Request changes", exact: true } + fill_in "Public review comment", with: "Keep this feedback for the new request", enable_aria_label: true + within(".review-tray__discussion") { click_button "Request changes" } + expect(page).to have_css('html[data-review-delivery-held="true"]') + expect(task.reload.status).to eq("changes_requested") + CoPlan::Reviews::Remove.call(plan: plan, request_id: task.id) + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author, create_only: true) + expect(page).to have_css(".document-comment__type", text: "Plan comment") + page.execute_script("window.releaseReviewDelivery()") + expect(page).to have_no_css('.review-tray__discussion form[aria-busy="true"]') + expect(page).to have_field("Public review comment", with: "Keep this feedback for the new request", enable_aria_label: true) + expect(page).to have_css(".review-tray__discussion", visible: true) + expect(page).to have_no_content("Changes requested. Your feedback is in the plan’s comments.") + expect(fresh.reload.status).to eq("pending") + page.execute_script("window.releaseReviewControls()") + expect(page).to have_css("[data-review-decision-actions] button[data-url*='#{fresh.id}']", visible: :all) + within(".review-tray__discussion") { click_button "Request changes" } + expect(page).to have_content("Changes requested. Your feedback is in the plan’s comments.") + expect(fresh.reload.status).to eq("changes_requested") + end + + %w[approved changes_requested].each do |decision| + it "discards stale #{decision} comment HTML while replacement controls are delayed", :review_delivery do + expect(page).to have_css("turbo-cable-stream-source[connected]", visible: :all) + hold_review_delivery("comments", controls: true) + if decision == "approved" + within(".review-tray") { click_button "Approve", exact: true } + fill_in "Public note", with: "Keep the public note" + click_button "Approve v1" + else + within(".review-tray") { click_button "Request changes", exact: true } + fill_in "Public review comment", with: "Keep the public note", enable_aria_label: true + within(".review-tray__discussion") { click_button "Request changes" } + end + expect(page).to have_css('html[data-review-delivery-held="true"]') + expect(page.evaluate_script("window.heldReviewDelivery")).to include("Review · #{decision.humanize} · v1") + CoPlan::Reviews::Remove.call(plan: plan, request_id: task.id) + fresh = CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author, create_only: true) + expect(page).to have_css(".document-comment__type", text: "Plan comment") + page.execute_script("window.releaseReviewDelivery()") + expect(page).to have_no_css('#plan-review-controls form[aria-busy="true"]', visible: :all) + expect(page).to have_css(".document-comment__type", text: "Plan comment") + expect(page).to have_no_css(".document-comment__type", text: "Review ·") + if decision == "approved" + expect(page).to have_css(".review-confirmation[open]", text: "This review request changed.") + expect(page).to have_field("Public note", with: "Keep the public note") + expect(page).to have_button("Approve v1", disabled: true) + click_button "Close approval", enable_aria_label: true + else + expect(page).to have_field("Public review comment", with: "Keep the public note", enable_aria_label: true) + expect(page).to have_css(".review-tray__discussion", visible: true) + end + expect(fresh.reload.status).to eq("pending") + expect(fresh.review_responses).to be_empty + page.execute_script("window.releaseReviewControls()") + expect(page).to have_css("[data-review-decision-actions] button[data-url*='#{fresh.id}']", visible: :all) + end + end + + it "does not restore another reviewer's deleted label from a delayed comments response", :review_delivery do + other = create(:coplan_user) + other_task = CoPlan::Reviews::Request.call(plan: plan, reviewer: other, actor: author) + CoPlan::Reviews::Respond.call(request: other_task, actor: other, revision: 1, decision: "approved", note: "Other reviewer approval") + expect(page).to have_css(".document-comment", text: "Other reviewer approval") + hold_review_delivery("comments", controls: true) + within(".review-tray") { click_button "Approve", exact: true } + fill_in "Public note", with: "My current approval" + click_button "Approve v1" + expect(page).to have_css('html[data-review-delivery-held="true"]') + CoPlan::Reviews::Remove.call(plan: plan, request_id: other_task.id) + within(find(".document-comment", text: "Other reviewer approval")) do + expect(page).to have_css(".document-comment__type", text: "Plan comment") + end + page.execute_script("window.releaseReviewDelivery(); window.releaseReviewControls()") + expect(page).to have_no_css(".review-confirmation[open]") + within(find(".document-comment", text: "Other reviewer approval")) do + expect(page).to have_css(".document-comment__type", text: "Plan comment") + end + expect(task.reload.status).to eq("approved") + expect(task.review_responses.last!.note).to eq("My current approval") + end + + [ [ "light", false ], [ "dark", true ] ].each do |theme, legacy| + it "updates a surviving note's label after #{legacy ? 'legacy replacement' : 'removal'} without navigation", :review_readdition do + CoPlan::Reviews::Respond.call(request: task, actor: reviewer, revision: 1, decision: "approved", note: "Keep the audit trail.") + CoPlan::Reviews::Withdraw.call(request: task, actor: author) if legacy + sign_in_browser(author) + page.execute_script("document.documentElement.dataset.theme = arguments[0]", theme) + expect(page).to have_css("turbo-cable-stream-source[connected]", visible: :all) + expect(page).to have_css(".document-comment__type", text: "Review · Approved · v1") + if legacy + CoPlan::Reviews::Request.call(plan: plan, reviewer: reviewer, actor: author, create_only: true, replace_withdrawn: true) + else + CoPlan::Reviews::Remove.call(plan: plan, request_id: task.id) + end + expect(page).to have_css(".document-comment__type", text: "Plan comment") + expect(page).to have_no_css(".document-comment__type", text: "Review · Approved · v1") + expect(page).to have_content("Keep the audit trail.") + page.execute_script('document.getElementById("plan-general-comments").scrollIntoView({block: "center"})') + page.save_screenshot(Rails.root.join("tmp/review-preview/removed-review-note-#{theme}.png")) + end + end + it "offers equal review choices and preserves cancelled feedback without moving the reader" do within(".review-tray__actions") do expect(page).to have_button("Request changes") @@ -92,7 +559,7 @@ def lose_next_review_result expect(page).to have_field("Public review comment", with: "Can we clarify this boundary?", enable_aria_label: true) end - [ [ "Request another review", false ], [ "Remove request", true ] ].each do |action, failed| + [ [ "Request another review", false ], [ "Remove reviewer", true ] ].each do |action, failed| it "keeps reviewer-update progress visible for #{action}", :review_interactions do sign_in_browser(author) click_button "More actions"