diff --git a/app/admin/plan_events.rb b/app/admin/plan_events.rb new file mode 100644 index 0000000..f4ccc7c --- /dev/null +++ b/app/admin/plan_events.rb @@ -0,0 +1,37 @@ +ActiveAdmin.register CoPlan::PlanEvent, as: "PlanEvent" do + actions :index, :show + + index do + selectable_column + id_column + column :plan + column :event_type + column :field + column :before_value + column :after_value + column :actor_type + column :actor_user + column :created_at + actions + end + + filter :plan + filter :event_type, as: :select, collection: CoPlan::PlanEvent::EVENT_TYPES + filter :actor_type, as: :select, collection: CoPlan::PlanEvent::ACTOR_TYPES + filter :created_at + + show do + attributes_table do + row :id + row :plan + row :event_type + row :field + row :before_value + row :after_value + row :actor_type + row :actor_user + row :metadata + row :created_at + end + end +end diff --git a/db/migrate/20260519011926_create_coplan_plan_events.co_plan.rb b/db/migrate/20260519011926_create_coplan_plan_events.co_plan.rb new file mode 100644 index 0000000..e618da7 --- /dev/null +++ b/db/migrate/20260519011926_create_coplan_plan_events.co_plan.rb @@ -0,0 +1,20 @@ +# This migration comes from co_plan (originally 20260519000000) +class CreateCoplanPlanEvents < ActiveRecord::Migration[8.1] + def change + create_table :coplan_plan_events, id: { type: :string, limit: 36 } do |t| + t.string :plan_id, limit: 36, null: false + t.string :actor_id, limit: 36 + t.string :actor_type, null: false + t.string :event_type, null: false + t.string :field + t.text :before_value + t.text :after_value + t.json :metadata + t.datetime :created_at, null: false + + t.index :plan_id + t.index [:plan_id, :created_at] + t.index :event_type + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 05614bf..bf99854 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_05_08_203305) do +ActiveRecord::Schema[8.1].define(version: 2026_05_19_011926) 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" @@ -150,6 +150,21 @@ t.index ["user_id"], name: "index_coplan_plan_collaborators_on_user_id" end + create_table "coplan_plan_events", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| + t.string "actor_id", limit: 36 + t.string "actor_type", null: false + t.text "after_value" + t.text "before_value" + t.datetime "created_at", null: false + t.string "event_type", null: false + t.string "field" + t.json "metadata" + t.string "plan_id", limit: 36, null: false + t.index ["event_type"], name: "index_coplan_plan_events_on_event_type" + t.index ["plan_id", "created_at"], name: "index_coplan_plan_events_on_plan_id_and_created_at" + t.index ["plan_id"], name: "index_coplan_plan_events_on_plan_id" + end + create_table "coplan_plan_tags", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.datetime "created_at", null: false t.string "plan_id", limit: 36, null: false diff --git a/engine/app/assets/stylesheets/coplan/application.css b/engine/app/assets/stylesheets/coplan/application.css index b8807c1..88fca03 100644 --- a/engine/app/assets/stylesheets/coplan/application.css +++ b/engine/app/assets/stylesheets/coplan/application.css @@ -880,6 +880,35 @@ img.avatar { margin-top: 2px; } +/* Metadata events in the history feed (status changes, tag adds, etc.). + Reuses .history-split__item layout but is non-interactive — clicking does + not load a diff because the event payload *is* the diff. */ +.history-split__item--event { + cursor: default; +} + +.history-split__item--event:hover { + background: inherit; +} + +.history-split__event-chip { + display: inline-block; + padding: 1px 6px; + border-radius: var(--radius); + background: var(--color-bg-muted); + border: 1px solid var(--color-border); + font-size: 0.7rem; + font-weight: 600; + text-transform: uppercase; + letter-spacing: 0.03em; + color: var(--color-text-muted); +} + +.history-split__event-link { + color: var(--color-primary); + text-decoration: underline; +} + .history-split__detail { padding: var(--space-md) var(--space-lg); overflow-y: auto; diff --git a/engine/app/controllers/coplan/api/v1/plans_controller.rb b/engine/app/controllers/coplan/api/v1/plans_controller.rb index 6b2dd2c..017da5a 100644 --- a/engine/app/controllers/coplan/api/v1/plans_controller.rb +++ b/engine/app/controllers/coplan/api/v1/plans_controller.rb @@ -72,6 +72,11 @@ def update permitted[:title] = params[:title] if params.key?(:title) permitted[:status] = params[:status] if params.key?(:status) + # Snapshot before-state so LogEvent can record meaningful diffs. + old_title = @plan.title + old_status = @plan.status + old_tag_names = @plan.tag_names + @plan.tag_names = params[:tags] if params.key?(:tags) @plan.update!(permitted) @@ -79,17 +84,47 @@ def update Broadcaster.replace_to(@plan, target: "plan-header", partial: "coplan/plans/header", locals: { plan: @plan }) end + if permitted.key?(:title) && @plan.saved_change_to_title? + Plans::LogEvent.call( + plan: @plan, actor: current_user, event_type: "title_changed", + before: old_title, after: @plan.title + ) + end + if permitted.key?(:status) && @plan.saved_change_to_status? + Plans::LogEvent.call( + plan: @plan, actor: current_user, event_type: "status_changed", + before: old_status, after: @plan.status + ) Plans::TriggerAutomatedReviews.call(plan: @plan, new_status: permitted[:status], triggered_by: current_user) end + if params.key?(:tags) + new_tag_names = @plan.tag_names + (new_tag_names - old_tag_names).each do |added| + Plans::LogEvent.call(plan: @plan, actor: current_user, event_type: "tag_added", after: added) + end + (old_tag_names - new_tag_names).each do |removed| + Plans::LogEvent.call(plan: @plan, actor: current_user, event_type: "tag_removed", before: removed) + end + end + if params[:references].is_a?(Array) params[:references].each do |ref_params| next unless ref_params[:url].present? ref_type = ref_params[:reference_type].presence || Reference.classify_url(ref_params[:url]) ref = @plan.references.find_or_initialize_by(url: ref_params[:url]) + # Only emit a "reference_added" event for genuinely new references; + # existing-reference updates fall through silently for now. + was_new = ref.new_record? ref.assign_attributes(key: ref_params[:key], title: ref_params[:title], reference_type: ref_type, source: "explicit") ref.save! + if was_new + Plans::LogEvent.call( + plan: @plan, actor: current_user, event_type: "reference_added", + after: ref.url, metadata: { title: ref.title, reference_type: ref.reference_type } + ) + end end end diff --git a/engine/app/controllers/coplan/application_controller.rb b/engine/app/controllers/coplan/application_controller.rb index b2419f9..e12a8be 100644 --- a/engine/app/controllers/coplan/application_controller.rb +++ b/engine/app/controllers/coplan/application_controller.rb @@ -11,6 +11,7 @@ def self.controller_path helper CoPlan::MarkdownHelper helper CoPlan::CommentsHelper helper CoPlan::ReferencesHelper + helper CoPlan::PlanEventsHelper # Skip host auth — CoPlan handles authentication internally via config.authenticate skip_before_action :authenticate_user!, raise: false diff --git a/engine/app/controllers/coplan/plans_controller.rb b/engine/app/controllers/coplan/plans_controller.rb index 54e02c5..a7037d8 100644 --- a/engine/app/controllers/coplan/plans_controller.rb +++ b/engine/app/controllers/coplan/plans_controller.rb @@ -42,7 +42,7 @@ def show def history authorize!(@plan, :show?) - @versions = @plan.plan_versions.includes(:actor_user).order(revision: :desc) + @history_items = @plan.history_items render layout: false end @@ -52,7 +52,16 @@ def edit def update authorize!(@plan, :update?) - @plan.update!(title: params[:plan][:title]) + old_title = @plan.title + new_title = params[:plan][:title] + @plan.update!(title: new_title) + Plans::LogEvent.call( + plan: @plan, + actor: current_user, + event_type: "title_changed", + before: old_title, + after: new_title + ) broadcast_plan_update(@plan) redirect_to plan_path(@plan), notice: "Plan updated." end @@ -60,9 +69,17 @@ def update def update_status authorize!(@plan, :update_status?) new_status = params[:status] + old_status = @plan.status if Plan::STATUSES.include?(new_status) && @plan.update(status: new_status) broadcast_plan_update(@plan) if @plan.saved_change_to_status? + Plans::LogEvent.call( + plan: @plan, + actor: current_user, + event_type: "status_changed", + before: old_status, + after: new_status + ) Plans::TriggerAutomatedReviews.call(plan: @plan, new_status: new_status, triggered_by: current_user) end redirect_to plan_path(@plan), notice: "Status updated to #{new_status}." diff --git a/engine/app/controllers/coplan/references_controller.rb b/engine/app/controllers/coplan/references_controller.rb index d4db257..988a487 100644 --- a/engine/app/controllers/coplan/references_controller.rb +++ b/engine/app/controllers/coplan/references_controller.rb @@ -14,6 +14,7 @@ def create end ref = @plan.references.find_or_initialize_by(url: url) + was_new = ref.new_record? ref.assign_attributes( key: params[:reference][:key].presence || ref.key, title: params[:reference][:title].presence || ref.title, @@ -23,6 +24,16 @@ def create ) ref.save! + if was_new + Plans::LogEvent.call( + plan: @plan, + actor: current_user, + event_type: "reference_added", + after: ref.url, + metadata: { title: ref.title, reference_type: ref.reference_type } + ) + end + respond_to do |format| format.turbo_stream { render_references_stream } format.html { redirect_to plan_path(@plan, tab: "references"), notice: "Reference added." } @@ -38,8 +49,19 @@ def destroy authorize!(@plan, :update?) ref = @plan.references.find(params[:id]) + removed_url = ref.url + removed_title = ref.title + removed_type = ref.reference_type ref.destroy! + Plans::LogEvent.call( + plan: @plan, + actor: current_user, + event_type: "reference_removed", + before: removed_url, + metadata: { title: removed_title, reference_type: removed_type } + ) + respond_to do |format| format.turbo_stream { render_references_stream } format.html { redirect_to plan_path(@plan, tab: "references"), notice: "Reference removed." } diff --git a/engine/app/helpers/coplan/plan_events_helper.rb b/engine/app/helpers/coplan/plan_events_helper.rb new file mode 100644 index 0000000..5ebc520 --- /dev/null +++ b/engine/app/helpers/coplan/plan_events_helper.rb @@ -0,0 +1,51 @@ +module CoPlan + module PlanEventsHelper + # Render a one-line, human-readable summary of a PlanEvent for the + # history feed. Each event type gets a tailored "X → Y" or "added X" / + # "removed X" phrasing instead of a generic field/before/after dump, + # because the same shape doesn't read well for status changes and tag + # adds and reference removals. + def render_event_summary(event) + case event.event_type + when "status_changed" + safe_join([ + "Status: ", + content_tag(:strong, event.before_value || "—"), + " → ", + content_tag(:strong, event.after_value || "—") + ]) + when "title_changed" + safe_join([ + "Renamed: ", + content_tag(:em, event.before_value.to_s.presence || "—"), + " → ", + content_tag(:em, event.after_value.to_s.presence || "—") + ]) + when "plan_type_changed" + safe_join([ + "Plan type: ", + content_tag(:strong, event.before_value || "—"), + " → ", + content_tag(:strong, event.after_value || "—") + ]) + when "tag_added" + safe_join(["Added tag ", content_tag(:code, event.after_value.to_s)]) + when "tag_removed" + safe_join(["Removed tag ", content_tag(:code, event.before_value.to_s)]) + when "reference_added" + title = event.metadata.is_a?(Hash) ? event.metadata["title"].presence : nil + url = event.after_value.to_s + label = title || url + safe_join(["Added reference ", link_to(label, url, class: "history-split__event-link", target: "_blank", rel: "noopener")]) + when "reference_removed" + title = event.metadata.is_a?(Hash) ? event.metadata["title"].presence : nil + url = event.before_value.to_s + label = title || url + safe_join(["Removed reference ", content_tag(:span, label, class: "history-split__event-link")]) + else + # Fallback for unknown / future event types — still useful, never blank. + "#{event.event_type}: #{event.before_value || "—"} → #{event.after_value || "—"}" + end + end + end +end diff --git a/engine/app/models/coplan/plan.rb b/engine/app/models/coplan/plan.rb index bb02aac..8265f47 100644 --- a/engine/app/models/coplan/plan.rb +++ b/engine/app/models/coplan/plan.rb @@ -6,6 +6,7 @@ class Plan < ApplicationRecord belongs_to :current_plan_version, class_name: "PlanVersion", optional: true belongs_to :plan_type, optional: true has_many :plan_versions, -> { order(revision: :asc) }, dependent: :destroy + has_many :plan_events, dependent: :destroy has_many :plan_collaborators, dependent: :destroy has_many :collaborators, through: :plan_collaborators, source: :user has_many :comment_threads, dependent: :destroy @@ -58,5 +59,18 @@ def tag_names=(names) desired = Array(names).map(&:strip).reject(&:blank?).uniq self.tags = desired.map { |name| Tag.find_or_create_by!(name: name) } end + + # Unified, time-sorted feed of everything that has happened to this plan: + # both content versions (PlanVersion) and metadata events (PlanEvent). + # Used by the history tab. Newest first. + # + # PlanVersions and PlanEvents both expose a `created_at` and a + # `#history_kind` so the view can render each appropriately without + # branching on class. Eager-loads actors for both to avoid N+1. + def history_items + versions = plan_versions.includes(:actor_user).order(revision: :desc).to_a + events = plan_events.includes(:actor_user).order(created_at: :desc).to_a + (versions + events).sort_by { |item| -item.created_at.to_f } + end end end diff --git a/engine/app/models/coplan/plan_event.rb b/engine/app/models/coplan/plan_event.rb new file mode 100644 index 0000000..648cec5 --- /dev/null +++ b/engine/app/models/coplan/plan_event.rb @@ -0,0 +1,74 @@ +module CoPlan + # A first-class log entry for metadata mutations on a plan — status changes, + # title changes, tag adds/removes, reference adds/removes, plan_type changes. + # + # Lives alongside PlanVersion (which captures content snapshots) so the + # history tab can present a unified, time-sorted view of "everything that + # happened to this plan" without overloading PlanVersion (which has hard + # requirements like content_markdown / content_sha256 that don't apply to + # metadata-only changes). + # + # Records are append-only — never updated, never destroyed except through + # the parent plan's cascade. + class PlanEvent < ApplicationRecord + # Mirrors PlanVersion::ACTOR_TYPES so history items from either source can + # be rendered uniformly. + ACTOR_TYPES = %w[human local_agent cloud_persona system].freeze + + EVENT_TYPES = %w[ + status_changed + title_changed + plan_type_changed + tag_added + tag_removed + reference_added + reference_removed + ].freeze + + belongs_to :plan + belongs_to :actor_user, class_name: "CoPlan::User", foreign_key: "actor_id", optional: true + + after_initialize { self.metadata ||= {} } + + validates :actor_type, presence: true, inclusion: { in: ACTOR_TYPES } + validates :event_type, presence: true, inclusion: { in: EVENT_TYPES } + + scope :for_history, -> { order(created_at: :desc) } + + def self.ransackable_attributes(_auth_object = nil) + %w[id plan_id actor_id actor_type event_type field before_value after_value created_at] + end + + def self.ransackable_associations(_auth_object = nil) + %w[plan actor_user] + end + + # Marker so history rendering can branch on the kind of item without + # introspecting class names. + def history_kind + :event + end + + after_create_commit :broadcast_history_update + + private + + # Prepend the new event into the open history tab and bump the count + # badge so anyone watching the plan sees metadata changes appear live + # alongside content versions. + def broadcast_history_update + Broadcaster.prepend_to( + plan, + target: "plan-history-list", + partial: "coplan/plans/event_item", + locals: { event: self, plan: plan } + ) + count = plan.plan_versions.count + plan.plan_events.count + Broadcaster.replace_to( + plan, + target: "history-count", + html: ApplicationController.helpers.content_tag(:span, count, class: "plan-tabs__count", id: "history-count") + ) + end + end +end diff --git a/engine/app/models/coplan/plan_version.rb b/engine/app/models/coplan/plan_version.rb index 6384f21..d84c911 100644 --- a/engine/app/models/coplan/plan_version.rb +++ b/engine/app/models/coplan/plan_version.rb @@ -14,6 +14,13 @@ class PlanVersion < ApplicationRecord validates :actor_type, presence: true, inclusion: { in: ACTOR_TYPES } before_validation :compute_sha256, if: -> { content_markdown.present? && content_sha256.blank? } + + # Marker so a mixed history feed (PlanVersion + PlanEvent) can render each + # item appropriately without introspecting class names. + def history_kind + :version + end + after_create_commit :extract_references after_create_commit :broadcast_history_update @@ -31,7 +38,7 @@ def broadcast_history_update partial: "coplan/plans/version_item", locals: { version: self, plan: plan } ) - count = plan.plan_versions.count + count = plan.plan_versions.count + plan.plan_events.count Broadcaster.replace_to( plan, target: "history-count", diff --git a/engine/app/services/coplan/plans/log_event.rb b/engine/app/services/coplan/plans/log_event.rb new file mode 100644 index 0000000..16590a7 --- /dev/null +++ b/engine/app/services/coplan/plans/log_event.rb @@ -0,0 +1,86 @@ +module CoPlan + module Plans + # Single entry point for recording a metadata mutation on a plan. + # + # Centralizing this means every mutation path uses the same shape (actor, + # event_type, before/after, optional field name + metadata) and we test + # the contract once instead of per call site. Returns the persisted + # PlanEvent record, or nil if `before` and `after` are equal (which is a + # common case for "save without actually changing anything"). + # + # Usage: + # + # Plans::LogEvent.call( + # plan: plan, + # actor: current_user, + # event_type: "status_changed", + # field: "status", + # before: "considering", + # after: "developing" + # ) + # + # For events without a meaningful before/after (e.g. a reference being + # added), pass nil for the side that doesn't apply. + class LogEvent + def self.call(**kwargs) + new(**kwargs).call + end + + def initialize(plan:, actor:, event_type:, field: nil, before: nil, after: nil, metadata: {}) + @plan = plan + @actor = actor + @event_type = event_type.to_s + @field = field&.to_s + @before = stringify(before) + @after = stringify(after) + @metadata = metadata || {} + end + + def call + # No-op when nothing changed. Callers can fire on every save without + # worrying about whether the value actually moved — keeps call sites + # short and predictable. + return nil if @before == @after && %w[status_changed title_changed plan_type_changed].include?(@event_type) + + PlanEvent.create!( + plan: @plan, + actor_id: actor_id, + actor_type: actor_type, + event_type: @event_type, + field: @field || default_field_for(@event_type), + before_value: @before, + after_value: @after, + metadata: @metadata + ) + end + + private + + def actor_id + @actor.respond_to?(:id) ? @actor.id : nil + end + + # Mirror PlanVersion's actor model: prefer "human" when we have a user, + # otherwise treat as a system event (e.g. backfill jobs). Callers that + # need to record agent activity should pass an actor with the right + # bookkeeping in place. + def actor_type + @actor.present? ? "human" : "system" + end + + def default_field_for(event_type) + case event_type + when "status_changed" then "status" + when "title_changed" then "title" + when "plan_type_changed" then "plan_type" + when "tag_added", "tag_removed" then "tags" + when "reference_added", "reference_removed" then "references" + end + end + + def stringify(value) + value.nil? ? nil : value.to_s + end + end + end +end diff --git a/engine/app/views/coplan/plans/_event_item.html.erb b/engine/app/views/coplan/plans/_event_item.html.erb new file mode 100644 index 0000000..b1d252f --- /dev/null +++ b/engine/app/views/coplan/plans/_event_item.html.erb @@ -0,0 +1,18 @@ +<%# A metadata event in the unified history feed. Rendered alongside + _version_item.html.erb. Non-interactive (no turbo-frame target) since + there's no separate diff to display — the event payload itself is the + diff. %> +
+
<%= event.field || event.event_type %>
+
+ <% if event.actor_user %> + <%= user_avatar(event.actor_user) %> <%= event.actor_user.name %> + <% else %> + <%= event.actor_type %> + <% end %> +
+
+ <%= render_event_summary(event) %> +
+
<%= time_ago_in_words(event.created_at) %> ago
+
diff --git a/engine/app/views/coplan/plans/_history.html.erb b/engine/app/views/coplan/plans/_history.html.erb index c492f8f..4aad031 100644 --- a/engine/app/views/coplan/plans/_history.html.erb +++ b/engine/app/views/coplan/plans/_history.html.erb @@ -1,12 +1,21 @@ +<%# Unified history feed: content versions (PlanVersion) and metadata events + (PlanEvent) interleaved by created_at. Each item exposes `history_kind` + so we render the right partial without introspecting class names. %> +<% first_version = history_items.find { |item| item.history_kind == :version } %>
- <% versions.each_with_index do |version, i| %> - <%= render partial: "coplan/plans/version_item", locals: { version: version, plan: plan } %> + <% history_items.each do |item| %> + <% case item.history_kind %> + <% when :version %> + <%= render partial: "coplan/plans/version_item", locals: { version: item, plan: plan } %> + <% when :event %> + <%= render partial: "coplan/plans/event_item", locals: { event: item, plan: plan } %> + <% end %> <% end %>
- +

Loading…

diff --git a/engine/app/views/coplan/plans/history.html.erb b/engine/app/views/coplan/plans/history.html.erb index aa44a61..172e95a 100644 --- a/engine/app/views/coplan/plans/history.html.erb +++ b/engine/app/views/coplan/plans/history.html.erb @@ -1,3 +1,3 @@ - <%= render partial: "coplan/plans/history", locals: { versions: @versions, plan: @plan } %> + <%= render partial: "coplan/plans/history", locals: { history_items: @history_items, plan: @plan } %> diff --git a/engine/app/views/coplan/plans/show.html.erb b/engine/app/views/coplan/plans/show.html.erb index 9c7e2f7..3abffca 100644 --- a/engine/app/views/coplan/plans/show.html.erb +++ b/engine/app/views/coplan/plans/show.html.erb @@ -20,7 +20,7 @@
diff --git a/engine/db/migrate/20260519000000_create_coplan_plan_events.rb b/engine/db/migrate/20260519000000_create_coplan_plan_events.rb new file mode 100644 index 0000000..e7e6512 --- /dev/null +++ b/engine/db/migrate/20260519000000_create_coplan_plan_events.rb @@ -0,0 +1,19 @@ +class CreateCoplanPlanEvents < ActiveRecord::Migration[8.1] + def change + create_table :coplan_plan_events, id: { type: :string, limit: 36 } do |t| + t.string :plan_id, limit: 36, null: false + t.string :actor_id, limit: 36 + t.string :actor_type, null: false + t.string :event_type, null: false + t.string :field + t.text :before_value + t.text :after_value + t.json :metadata + t.datetime :created_at, null: false + + t.index :plan_id + t.index [:plan_id, :created_at] + t.index :event_type + end + end +end diff --git a/spec/factories/plan_events.rb b/spec/factories/plan_events.rb new file mode 100644 index 0000000..97577e2 --- /dev/null +++ b/spec/factories/plan_events.rb @@ -0,0 +1,38 @@ +FactoryBot.define do + factory :plan_event, class: "CoPlan::PlanEvent" do + plan + actor_type { "human" } + event_type { "status_changed" } + field { "status" } + before_value { "considering" } + after_value { "developing" } + metadata { {} } + + trait :title_changed do + event_type { "title_changed" } + field { "title" } + before_value { "Old title" } + after_value { "New title" } + end + + trait :tag_added do + event_type { "tag_added" } + field { "tags" } + before_value { nil } + after_value { "payments" } + end + + trait :reference_added do + event_type { "reference_added" } + field { "references" } + before_value { nil } + after_value { "https://github.com/squareup/example" } + metadata { { "title" => "Example repo", "reference_type" => "repository" } } + end + + trait :system do + actor_type { "system" } + actor_id { nil } + end + end +end diff --git a/spec/models/plan_event_spec.rb b/spec/models/plan_event_spec.rb new file mode 100644 index 0000000..ac57022 --- /dev/null +++ b/spec/models/plan_event_spec.rb @@ -0,0 +1,51 @@ +require "rails_helper" + +RSpec.describe CoPlan::PlanEvent, type: :model do + let(:plan) { create(:plan) } + + describe "validations" do + it "requires actor_type" do + event = build(:plan_event, actor_type: nil) + expect(event).not_to be_valid + expect(event.errors[:actor_type]).to be_present + end + + it "rejects unknown actor_type values" do + event = build(:plan_event, actor_type: "spider") + expect(event).not_to be_valid + expect(event.errors[:actor_type]).to be_present + end + + it "requires event_type" do + event = build(:plan_event, event_type: nil) + expect(event).not_to be_valid + expect(event.errors[:event_type]).to be_present + end + + it "rejects unknown event_type values" do + event = build(:plan_event, event_type: "exploded") + expect(event).not_to be_valid + expect(event.errors[:event_type]).to be_present + end + end + + describe "#history_kind" do + it "is :event so the history feed can distinguish it from PlanVersion" do + expect(build(:plan_event).history_kind).to eq(:event) + end + end + + describe "after_initialize" do + it "defaults metadata to an empty hash (per AGENTS.md JSON-column rule)" do + expect(CoPlan::PlanEvent.new.metadata).to eq({}) + end + end + + describe "association with plan" do + it "is configured to be destroyed when the plan is destroyed" do + reflection = CoPlan::Plan.reflect_on_association(:plan_events) + expect(reflection).to be_present + expect(reflection.options[:dependent]).to eq(:destroy) + end + end +end diff --git a/spec/requests/plan_events_spec.rb b/spec/requests/plan_events_spec.rb new file mode 100644 index 0000000..122baa9 --- /dev/null +++ b/spec/requests/plan_events_spec.rb @@ -0,0 +1,128 @@ +require "rails_helper" + +# Integration-level verification that every mutation path that *should* record +# a metadata event actually does. The point isn't to retest the LogEvent +# service (that has its own spec) — it's to lock down the contract between +# the mutation surfaces and the event log so future controllers don't quietly +# drop the wiring. +RSpec.describe "Plan metadata event logging", type: :request do + let(:user) { create(:coplan_user) } + let(:plan) { create(:plan, :considering, created_by_user: user, title: "Original") } + + before { sign_in_as(user) } + + describe "PATCH /plans/:id (web)" do + it "records a title_changed event when the title moves" do + expect { + patch plan_path(plan), params: { plan: { title: "Renamed" } } + }.to change { plan.plan_events.where(event_type: "title_changed").count }.by(1) + + event = plan.plan_events.where(event_type: "title_changed").last + expect(event.before_value).to eq("Original") + expect(event.after_value).to eq("Renamed") + expect(event.actor_user).to eq(user) + end + end + + describe "PATCH /plans/:id/status (web)" do + it "records a status_changed event when the status moves" do + expect { + patch update_status_plan_path(plan), params: { status: "developing" } + }.to change { plan.plan_events.where(event_type: "status_changed").count }.by(1) + + event = plan.plan_events.where(event_type: "status_changed").last + expect(event.before_value).to eq("considering") + expect(event.after_value).to eq("developing") + end + + it "does not record an event when the status doesn't actually change" do + expect { + patch update_status_plan_path(plan), params: { status: "considering" } + }.not_to change { plan.plan_events.count } + end + end + + describe "POST /plans/:id/references (web)" do + it "records a reference_added event with the URL and metadata" do + expect { + post plan_references_path(plan), params: { + reference: { url: "https://github.com/squareup/example", title: "Example" } + } + }.to change { plan.plan_events.where(event_type: "reference_added").count }.by(1) + + event = plan.plan_events.where(event_type: "reference_added").last + expect(event.after_value).to eq("https://github.com/squareup/example") + expect(event.metadata).to include("title" => "Example") + end + end + + describe "DELETE /plans/:id/references/:id (web)" do + it "records a reference_removed event capturing the original URL" do + reference = plan.references.create!(url: "https://github.com/squareup/x", title: "X", reference_type: "repository", source: "explicit") + + expect { + delete plan_reference_path(plan, reference) + }.to change { plan.plan_events.where(event_type: "reference_removed").count }.by(1) + + event = plan.plan_events.where(event_type: "reference_removed").last + expect(event.before_value).to eq("https://github.com/squareup/x") + end + end + + describe "PATCH /api/v1/plans/:id (API)" do + let(:raw_token) { CoPlan::ApiToken.generate_token } + let!(:token) { create(:api_token, user: user, raw_token: raw_token) } + let(:auth_headers) { { "Authorization" => "Bearer #{raw_token}" } } + + it "records events for title, status, and tag diffs in a single request" do + plan.tag_names = ["existing"] + + expect { + patch "/api/v1/plans/#{plan.id}", params: { + title: "API-renamed", + status: "developing", + tags: ["existing", "added"] + }, headers: auth_headers, as: :json + }.to change { plan.plan_events.count }.by(3) + + expect(plan.plan_events.pluck(:event_type)).to contain_exactly( + "title_changed", "status_changed", "tag_added" + ) + end + + it "records tag_removed events for tags that disappear from the list" do + plan.tag_names = ["payments", "billing"] + + expect { + patch "/api/v1/plans/#{plan.id}", params: { + tags: ["payments"] + }, headers: auth_headers, as: :json + }.to change { plan.plan_events.where(event_type: "tag_removed").count }.by(1) + + removed = plan.plan_events.where(event_type: "tag_removed").last + expect(removed.before_value).to eq("billing") + end + + it "records reference_added when a new reference is included in the update payload" do + expect { + patch "/api/v1/plans/#{plan.id}", params: { + references: [{ url: "https://docs.example.com/spec", title: "Spec" }] + }, headers: auth_headers, as: :json + }.to change { plan.plan_events.where(event_type: "reference_added").count }.by(1) + end + end + + describe "Plan#history_items" do + it "interleaves content versions and metadata events, newest first" do + # The :plan factory already creates a revision-1 PlanVersion for us. + v1 = plan.plan_versions.first + v1.update_columns(created_at: 3.hours.ago) + e1 = create(:plan_event, plan: plan, event_type: "status_changed", created_at: 2.hours.ago) + v2 = create(:plan_version, plan: plan, revision: 2, created_at: 1.hour.ago) + + items = plan.history_items + expect(items.map(&:id)).to eq([v2.id, e1.id, v1.id]) + expect(items.map(&:history_kind)).to eq([:version, :event, :version]) + end + end +end diff --git a/spec/services/plans/log_event_spec.rb b/spec/services/plans/log_event_spec.rb new file mode 100644 index 0000000..b35daf9 --- /dev/null +++ b/spec/services/plans/log_event_spec.rb @@ -0,0 +1,71 @@ +require "rails_helper" + +RSpec.describe CoPlan::Plans::LogEvent do + let(:user) { create(:coplan_user) } + let(:plan) { create(:plan, created_by_user: user) } + + describe ".call" do + it "creates a PlanEvent attributed to the actor" do + expect { + described_class.call( + plan: plan, + actor: user, + event_type: "status_changed", + before: "considering", + after: "developing" + ) + }.to change { CoPlan::PlanEvent.count }.by(1) + + event = CoPlan::PlanEvent.last + expect(event.plan).to eq(plan) + expect(event.actor_user).to eq(user) + expect(event.actor_type).to eq("human") + expect(event.event_type).to eq("status_changed") + expect(event.before_value).to eq("considering") + expect(event.after_value).to eq("developing") + end + + it "infers a sensible default field per event_type when not provided" do + described_class.call(plan: plan, actor: user, event_type: "tag_added", after: "payments") + expect(CoPlan::PlanEvent.last.field).to eq("tags") + end + + it "treats the actor as 'system' when no user is passed (e.g. backfill jobs)" do + described_class.call(plan: plan, actor: nil, event_type: "status_changed", before: "a", after: "b") + event = CoPlan::PlanEvent.last + expect(event.actor_type).to eq("system") + expect(event.actor_id).to be_nil + end + + it "returns nil and does not persist when before == after for change events" do + # Call sites should be allowed to fire on every save without checking; + # the service no-ops when nothing actually changed. + expect { + described_class.call(plan: plan, actor: user, event_type: "status_changed", before: "considering", after: "considering") + }.not_to change { CoPlan::PlanEvent.count } + end + + it "still records add/remove events even when one side is nil" do + described_class.call(plan: plan, actor: user, event_type: "tag_added", after: "payments") + described_class.call(plan: plan, actor: user, event_type: "reference_removed", before: "https://x") + expect(CoPlan::PlanEvent.count).to eq(2) + end + + it "passes through structured metadata" do + described_class.call( + plan: plan, + actor: user, + event_type: "reference_added", + after: "https://github.com/example/repo", + metadata: { title: "Example", reference_type: "repository" } + ) + event = CoPlan::PlanEvent.last + expect(event.metadata).to include("title" => "Example", "reference_type" => "repository") + end + + it "coerces non-string before/after values to strings for storage" do + described_class.call(plan: plan, actor: user, event_type: "tag_added", after: 42) + expect(CoPlan::PlanEvent.last.after_value).to eq("42") + end + end +end