From 6aaef1d07f1cee3e9dc765ca476fc1e0ea150a00 Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Mon, 27 Apr 2026 10:07:08 -0400 Subject: [PATCH] Expand PlanCollaborator roles: add approver and highlighted Phase 2 Item 6: Collaborator Roles Expansion - Add 'approver' and 'highlighted' to PlanCollaborator::ROLES - Migration: add approved_at (datetime) and highlighted_reason (text) columns - Model: scopes (.authors, .reviewers, .approvers, .highlighted), approve!/approved? helpers, highlighted_reason validation, before_validation callback to clear stale role-specific data on role change - API: collaborator_json conditionally includes approved_at and highlighted_reason - Factory: add :approver and :highlighted traits - Spec: 13 examples covering roles, validations, scopes, approval flow, cleanup Amp-Thread-ID: https://ampcode.com/threads/T-019db6dc-e2ac-72de-a66f-e7f513adadf9 Co-authored-by: Amp --- ..._expand_plan_collaborator_roles.co_plan.rb | 7 ++ db/schema.rb | 18 ++-- .../coplan/api/v1/plans_controller.rb | 5 +- engine/app/models/coplan/plan_collaborator.rb | 27 ++++- ...22000000_expand_plan_collaborator_roles.rb | 6 ++ spec/factories/plan_collaborators.rb | 9 ++ spec/models/plan_collaborator_spec.rb | 101 ++++++++++++++++++ 7 files changed, 163 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20260422202551_expand_plan_collaborator_roles.co_plan.rb create mode 100644 engine/db/migrate/20260422000000_expand_plan_collaborator_roles.rb create mode 100644 spec/models/plan_collaborator_spec.rb diff --git a/db/migrate/20260422202551_expand_plan_collaborator_roles.co_plan.rb b/db/migrate/20260422202551_expand_plan_collaborator_roles.co_plan.rb new file mode 100644 index 0000000..346d2bb --- /dev/null +++ b/db/migrate/20260422202551_expand_plan_collaborator_roles.co_plan.rb @@ -0,0 +1,7 @@ +# This migration comes from co_plan (originally 20260422000000) +class ExpandPlanCollaboratorRoles < ActiveRecord::Migration[8.0] + def change + add_column :coplan_plan_collaborators, :approved_at, :datetime + add_column :coplan_plan_collaborators, :highlighted_reason, :text + end +end diff --git a/db/schema.rb b/db/schema.rb index 0850e46..13038f8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_04_10_182145) do +ActiveRecord::Schema[8.1].define(version: 2026_04_22_202551) do create_table "active_admin_comments", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.bigint "author_id" t.string "author_type" @@ -70,13 +70,13 @@ t.integer "start_line" t.string "status", default: "pending", null: false t.datetime "updated_at", null: false - t.index ["addressed_in_plan_version_id"], name: "fk_rails_a77cc69a6e" - t.index ["created_by_user_id"], name: "fk_rails_34dfdd2aac" - t.index ["out_of_date_since_version_id"], name: "fk_rails_60a8d49098" + t.index ["addressed_in_plan_version_id"], name: "fk_rails_e7003e0df7" + t.index ["created_by_user_id"], name: "fk_rails_88fb5e06ca" + t.index ["out_of_date_since_version_id"], name: "fk_rails_be37c1499d" t.index ["plan_id", "out_of_date"], name: "index_coplan_comment_threads_on_plan_id_and_out_of_date" t.index ["plan_id", "status"], name: "index_coplan_comment_threads_on_plan_id_and_status" - t.index ["plan_version_id"], name: "fk_rails_514df5a253" - t.index ["resolved_by_user_id"], name: "fk_rails_e5ed569cf1" + t.index ["plan_version_id"], name: "fk_rails_676660f283" + t.index ["resolved_by_user_id"], name: "fk_rails_8625e1eb43" end create_table "coplan_comments", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| @@ -117,7 +117,7 @@ t.string "status", default: "open", null: false t.datetime "updated_at", null: false t.index ["plan_id", "status"], name: "index_coplan_edit_sessions_on_plan_id_and_status" - t.index ["plan_version_id"], name: "fk_rails_55d7ec476a" + t.index ["plan_version_id"], name: "fk_rails_14c3f0737b" end create_table "coplan_notifications", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| @@ -137,7 +137,9 @@ create_table "coplan_plan_collaborators", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.string "added_by_user_id", limit: 36 + t.datetime "approved_at" t.datetime "created_at", null: false + t.text "highlighted_reason" t.string "plan_id", limit: 36, null: false t.string "role", null: false t.datetime "updated_at", null: false @@ -211,7 +213,7 @@ t.string "title", null: false t.datetime "updated_at", null: false t.index ["created_by_user_id"], name: "index_coplan_plans_on_created_by_user_id" - t.index ["current_plan_version_id"], name: "fk_rails_4193983681" + t.index ["current_plan_version_id"], name: "fk_rails_c401577583" t.index ["plan_type_id"], name: "index_coplan_plans_on_plan_type_id" t.index ["status"], name: "index_coplan_plans_on_status" t.index ["updated_at"], name: "index_coplan_plans_on_updated_at" diff --git a/engine/app/controllers/coplan/api/v1/plans_controller.rb b/engine/app/controllers/coplan/api/v1/plans_controller.rb index 8092cc4..6b2dd2c 100644 --- a/engine/app/controllers/coplan/api/v1/plans_controller.rb +++ b/engine/app/controllers/coplan/api/v1/plans_controller.rb @@ -167,11 +167,14 @@ def reference_json(ref) end def collaborator_json(collaborator) - { + json = { id: collaborator.id, user: user_json(collaborator.user), role: collaborator.role } + json[:approved_at] = collaborator.approved_at if collaborator.role == "approver" + json[:highlighted_reason] = collaborator.highlighted_reason if collaborator.role == "highlighted" + json end def snapshot_threads_json(threads) diff --git a/engine/app/models/coplan/plan_collaborator.rb b/engine/app/models/coplan/plan_collaborator.rb index 7aef677..f4ed501 100644 --- a/engine/app/models/coplan/plan_collaborator.rb +++ b/engine/app/models/coplan/plan_collaborator.rb @@ -1,6 +1,6 @@ module CoPlan class PlanCollaborator < ApplicationRecord - ROLES = %w[author reviewer viewer].freeze + ROLES = %w[author reviewer viewer approver highlighted].freeze belongs_to :plan belongs_to :user, class_name: "CoPlan::User" @@ -8,5 +8,30 @@ class PlanCollaborator < ApplicationRecord validates :role, presence: true, inclusion: { in: ROLES } validates :user_id, uniqueness: { scope: :plan_id } + validates :highlighted_reason, presence: true, if: -> { role == "highlighted" } + + before_validation :clear_irrelevant_role_data + + private + + def clear_irrelevant_role_data + self.approved_at = nil unless role == "approver" + self.highlighted_reason = nil unless role == "highlighted" + end + + public + + scope :authors, -> { where(role: "author") } + scope :reviewers, -> { where(role: "reviewer") } + scope :approvers, -> { where(role: "approver") } + scope :highlighted, -> { where(role: "highlighted") } + + def approve! + update!(approved_at: Time.current) + end + + def approved? + approved_at.present? + end end end diff --git a/engine/db/migrate/20260422000000_expand_plan_collaborator_roles.rb b/engine/db/migrate/20260422000000_expand_plan_collaborator_roles.rb new file mode 100644 index 0000000..4ee1cbb --- /dev/null +++ b/engine/db/migrate/20260422000000_expand_plan_collaborator_roles.rb @@ -0,0 +1,6 @@ +class ExpandPlanCollaboratorRoles < ActiveRecord::Migration[8.0] + def change + add_column :coplan_plan_collaborators, :approved_at, :datetime + add_column :coplan_plan_collaborators, :highlighted_reason, :text + end +end diff --git a/spec/factories/plan_collaborators.rb b/spec/factories/plan_collaborators.rb index 2a534cf..d59ae47 100644 --- a/spec/factories/plan_collaborators.rb +++ b/spec/factories/plan_collaborators.rb @@ -3,5 +3,14 @@ plan user { association(:coplan_user) } role { "reviewer" } + + trait :approver do + role { "approver" } + end + + trait :highlighted do + role { "highlighted" } + highlighted_reason { "Domain expert" } + end end end diff --git a/spec/models/plan_collaborator_spec.rb b/spec/models/plan_collaborator_spec.rb new file mode 100644 index 0000000..c7ad466 --- /dev/null +++ b/spec/models/plan_collaborator_spec.rb @@ -0,0 +1,101 @@ +require "rails_helper" + +RSpec.describe CoPlan::PlanCollaborator, type: :model do + it "is valid with valid attributes" do + collaborator = create(:plan_collaborator) + expect(collaborator).to be_valid + end + + it "requires role" do + collaborator = build(:plan_collaborator, role: nil) + expect(collaborator).not_to be_valid + end + + it "validates role inclusion" do + collaborator = build(:plan_collaborator, role: "admin") + expect(collaborator).not_to be_valid + end + + it "allows all valid roles" do + plan = create(:plan) + CoPlan::PlanCollaborator::ROLES.each do |role| + user = create(:coplan_user) + attrs = { plan: plan, user: user, role: role } + attrs[:highlighted_reason] = "Top expert" if role == "highlighted" + collaborator = build(:plan_collaborator, **attrs) + expect(collaborator).to be_valid, "Expected role '#{role}' to be valid" + end + end + + it "requires unique user per plan" do + collaborator = create(:plan_collaborator) + duplicate = build(:plan_collaborator, plan: collaborator.plan, user: collaborator.user) + expect(duplicate).not_to be_valid + expect(duplicate.errors[:user_id]).to include("has already been taken") + end + + describe "highlighted role" do + it "requires highlighted_reason" do + collaborator = build(:plan_collaborator, role: "highlighted", highlighted_reason: nil) + expect(collaborator).not_to be_valid + expect(collaborator.errors[:highlighted_reason]).to include("can't be blank") + end + + it "is valid with highlighted_reason" do + collaborator = build(:plan_collaborator, role: "highlighted", highlighted_reason: "Domain expert in payments") + expect(collaborator).to be_valid + end + end + + describe "approver role" do + it "tracks approval via approve!" do + collaborator = create(:plan_collaborator, :approver) + expect(collaborator.approved?).to be false + + collaborator.approve! + expect(collaborator.approved?).to be true + expect(collaborator.approved_at).to be_present + end + end + + describe "role data cleanup" do + it "clears approved_at when role changes from approver" do + collaborator = create(:plan_collaborator, :approver) + collaborator.approve! + expect(collaborator.approved_at).to be_present + + collaborator.update!(role: "reviewer") + expect(collaborator.approved_at).to be_nil + end + + it "clears highlighted_reason when role changes from highlighted" do + collaborator = create(:plan_collaborator, :highlighted) + expect(collaborator.highlighted_reason).to be_present + + collaborator.update!(role: "viewer") + expect(collaborator.highlighted_reason).to be_nil + end + end + + describe "scopes" do + let(:plan) { create(:plan) } + + it ".authors returns only author collaborators" do + author = create(:plan_collaborator, plan: plan, role: "author") + create(:plan_collaborator, plan: plan, role: "reviewer") + expect(plan.plan_collaborators.authors).to eq([author]) + end + + it ".approvers returns only approver collaborators" do + approver = create(:plan_collaborator, plan: plan, role: "approver") + create(:plan_collaborator, plan: plan, role: "viewer") + expect(plan.plan_collaborators.approvers).to eq([approver]) + end + + it ".highlighted returns only highlighted collaborators" do + highlighted = create(:plan_collaborator, plan: plan, role: "highlighted", highlighted_reason: "Expert") + create(:plan_collaborator, plan: plan, role: "author") + expect(plan.plan_collaborators.highlighted).to eq([highlighted]) + end + end +end