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..35b75c4f 100644
--- a/engine/app/controllers/coplan/api/v1/review_requests_controller.rb
+++ b/engine/app/controllers/coplan/api/v1/review_requests_controller.rb
@@ -35,9 +35,9 @@ 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)
+ requirement: params[:requirement].presence || @review_request.requirement, api_token: @api_token, request_id: @review_request.id)
else
- @plan.with_lock { @review_request.update!(requirement: params.require(:requirement)) }
+ @plan.with_lock { @review_request.reload.update!(requirement: params.require(:requirement)) }
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..4417b0f4 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
@@ -29,9 +30,9 @@ def 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)
+ requirement: attributes[:requirement].presence || @review_request.requirement, request_id: @review_request.id)
else
- @plan.with_lock { @review_request.update!(requirement: attributes.fetch(:requirement)) }
+ @plan.with_lock { @review_request.reload.update!(requirement: attributes.fetch(:requirement)) }
Reviews::Changed.call(request: @review_request, event_type: "review.requested",
payload: { "action" => "requirement_changed", "requirement" => @review_request.requirement })
end
@@ -40,7 +41,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..4adabcdf 100644
--- a/engine/app/javascript/controllers/coplan/review_tray_controller.js
+++ b/engine/app/javascript/controllers/coplan/review_tray_controller.js
@@ -267,7 +267,7 @@ 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
@@ -354,7 +354,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
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..5f028d60 100644
--- a/engine/app/services/coplan/reviews/request.rb
+++ b/engine/app/services/coplan/reviews/request.rb
@@ -1,27 +1,41 @@
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, request_id: nil, replace_withdrawn: false)
+ new(plan:, reviewer:, actor:, requirement:, api_token:, create_only:, request_id:, replace_withdrawn:).call
end
- def initialize(plan:, reviewer:, actor:, requirement:, api_token:, create_only:)
+ def initialize(plan:, reviewer:, actor:, requirement:, api_token:, create_only:, request_id:, replace_withdrawn:)
@plan, @reviewer, @actor, @requirement, @api_token = plan, reviewer, actor, requirement, api_token
@create_only = create_only
+ @request_id = request_id
+ @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_id
+ @plan.review_requests.where(reviewer: @reviewer).find(@request_id)
+ 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 +53,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..201b9d9d
--- /dev/null
+++ b/spec/services/coplan/reviews/remove_spec.rb
@@ -0,0 +1,118 @@
+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.call(plan: stale.plan, reviewer: stale.reviewer,
+ actor: stale.requested_by_user, request_id: old_id)
+ 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/system/plan_reviews_spec.rb b/spec/system/plan_reviews_spec.rb
index 47fa5804..dcfbbb05 100644
--- a/spec/system/plan_reviews_spec.rb
+++ b/spec/system/plan_reviews_spec.rb
@@ -40,15 +40,9 @@ def lose_next_review_result
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 +60,95 @@ 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
+
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 +175,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"