From d56155231814633e04f856d22646fea24ef97011 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Wed, 15 Jul 2026 01:41:34 +0200 Subject: Add NodeAction: an append-only log of who did what to a node node_id/page_id/user_id are lookup and ordering only -- all three nullify on delete, so an entry outlives its actor and its subject. Everything that must survive those deletions lives in a mandatory metadata jsonb written once at creation: the actor's username, the node's human-readable name (pinned to the default locale), and action-specific extras such as publish's title from/to. NodeAction.record! is the single constructor, so every entry gets the same baseline metadata without each call site re-implementing it. occurred_at is one field for live and backfilled entries alike; inferred_from distinguishes them -- nil means witnessed at the moment it happened, populated names how a backfilled entry was estimated. Instrumented so far: publish (crediting the actual publisher, threaded through from the controller -- previously nobody had the act of publishing recorded anywhere), revert's discard_autosave and destroy_draft branches, and translation destroy. publish_draft! now runs in a transaction so the promotion and its log entry land together. The remaining verbs follow once this mechanism has proven itself. --- app/controllers/nodes_controller.rb | 2 +- app/controllers/page_translations_controller.rb | 3 + app/models/node.rb | 65 +++++++++-------- app/models/node_action.rb | 33 +++++++++ db/migrate/20260714233414_create_node_actions.rb | 19 +++++ .../page_translations_controller_test.rb | 15 ++++ test/models/node_test.rb | 83 ++++++++++++++++++++++ 7 files changed, 191 insertions(+), 29 deletions(-) create mode 100644 app/models/node_action.rb create mode 100644 db/migrate/20260714233414_create_node_actions.rb diff --git a/app/controllers/nodes_controller.rb b/app/controllers/nodes_controller.rb index bff1733..9c5d228 100644 --- a/app/controllers/nodes_controller.rb +++ b/app/controllers/nodes_controller.rb @@ -147,7 +147,7 @@ class NodesController < ApplicationController end def publish - @node.publish_draft! + @node.publish_draft!(current_user) flash[:notice] = "Draft has been published" redirect_to node_path(@node) end diff --git a/app/controllers/page_translations_controller.rb b/app/controllers/page_translations_controller.rb index 38a7c4f..be4f488 100644 --- a/app/controllers/page_translations_controller.rb +++ b/app/controllers/page_translations_controller.rb @@ -63,6 +63,9 @@ class PageTranslationsController < ApplicationController draft.translations.where(:locale => @locale).delete_all draft.reload + NodeAction.record!(:node => @node, :page => draft, :user => current_user, + :action => "translation_destroy", :locale => @locale) + flash[:notice] = "#{@locale.upcase} translation removed from the draft. Publish to make this permanent." redirect_to node_path(@node) rescue LockedByAnotherUser => e diff --git a/app/models/node.rb b/app/models/node.rb index 311c5c0..b41df06 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -170,10 +170,12 @@ class Node < ApplicationRecord self.autosave.destroy self.autosave_id = nil self.save! + NodeAction.record!(:node => self, :user => current_user, :action => "discard_autosave") elsif self.draft && self.head self.draft.destroy self.draft_id = nil self.save! + NodeAction.record!(:node => self, :user => current_user, :action => "destroy_draft") end self.unlock! unless self.draft @@ -188,43 +190,50 @@ class Node < ApplicationRecord end end - def publish_draft! + def publish_draft! current_user = nil # Return nil if nothing to publish and no staged changes return nil unless self.draft || staged_slug || staged_parent_id - if self.draft - self.head = self.draft - self.head.published_at ||= Time.now - self.head.save! - self.draft = nil - end + ActiveRecord::Base.transaction do + if self.draft + previous_title = self.head ? Globalize.with_locale(I18n.default_locale) { self.head.title } : nil + self.head = self.draft + self.head.published_at ||= Time.now + self.head.save! + self.draft = nil + + new_title = Globalize.with_locale(I18n.default_locale) { self.head.title } + NodeAction.record!(:node => self, :page => self.head, :user => current_user, + :action => "publish", :from => previous_title, :to => new_title) + end - if staged_slug && (staged_slug != slug) - self.slug = staged_slug - self.staged_slug = nil - end + if staged_slug && (staged_slug != slug) + self.slug = staged_slug + self.staged_slug = nil + end - if staged_parent_id && (staged_parent_id != parent_id) - new_parent = Node.find(staged_parent_id) + if staged_parent_id && (staged_parent_id != parent_id) + new_parent = Node.find(staged_parent_id) - if new_parent == self || self.descendants.include?(new_parent) - raise ActiveRecord::RecordInvalid.new(self), "Cannot move a node under itself or one of its own descendants" - end + if new_parent == self || self.descendants.include?(new_parent) + raise ActiveRecord::RecordInvalid.new(self), "Cannot move a node under itself or one of its own descendants" + end - self.staged_parent_id = nil - self.save! - self.move_to_child_of(new_parent) - else - unless self.save - raise ActiveRecord::RecordInvalid.new(self) + self.staged_parent_id = nil + self.save! + self.move_to_child_of(new_parent) + else + unless self.save + raise ActiveRecord::RecordInvalid.new(self) + end end - end - self.reload - self.update_unique_name - self.send(:update_unique_names_of_children) - self.unlock! - self + self.reload + self.update_unique_name + self.send(:update_unique_names_of_children) + self.unlock! + self + end end def restore_revision! revision diff --git a/app/models/node_action.rb b/app/models/node_action.rb new file mode 100644 index 0000000..f32b5b7 --- /dev/null +++ b/app/models/node_action.rb @@ -0,0 +1,33 @@ +class NodeAction < ApplicationRecord + belongs_to :node, optional: true + belongs_to :page, optional: true + belongs_to :user, optional: true + + validates :action, presence: true + validates :occurred_at, presence: true + + def self.record!(node:, action:, user: nil, page: nil, locale: nil, **extra) + create!( + :node => node, + :page => page, + :user => user, + :action => action, + :locale => locale, + :occurred_at => Time.now, + :metadata => { + "username" => user&.login, + "human_readable_node_name" => Globalize.with_locale(I18n.default_locale) { + node&.head&.title || node&.draft&.title + }, + }.merge(extra.stringify_keys) + ) + end + + def actor_name + metadata["username"] || "unknown" + end + + def subject_name + metadata["human_readable_node_name"] || node&.unique_name || "deleted node" + end +end diff --git a/db/migrate/20260714233414_create_node_actions.rb b/db/migrate/20260714233414_create_node_actions.rb new file mode 100644 index 0000000..6c18285 --- /dev/null +++ b/db/migrate/20260714233414_create_node_actions.rb @@ -0,0 +1,19 @@ +class CreateNodeActions < ActiveRecord::Migration[8.1] + def change + create_table :node_actions do |t| + t.references :node, foreign_key: { on_delete: :nullify } + t.references :page, foreign_key: { on_delete: :nullify } + t.references :user, foreign_key: { on_delete: :nullify } + t.string :action, null: false + t.string :locale + t.string :inferred_from + t.jsonb :metadata, null: false + t.datetime :occurred_at, null: false + + t.timestamps + end + + add_index :node_actions, [:node_id, :occurred_at] + add_index :node_actions, :action + end +end diff --git a/test/controllers/page_translations_controller_test.rb b/test/controllers/page_translations_controller_test.rb index 8fa732f..82f8bce 100644 --- a/test/controllers/page_translations_controller_test.rb +++ b/test/controllers/page_translations_controller_test.rb @@ -82,4 +82,19 @@ class PageTranslationsControllerTest < ActionController::TestCase assert_equal page_count_before, node.pages.count assert_equal "in progress", Globalize.with_locale(:en) { node.autosave.title } end + + test "destroy logs a translation_destroy NodeAction" do + login_as :quentin + node = Node.root.children.create!(:slug => "translation_destroy_log_test") + Globalize.with_locale(:en) { node.draft.update!(:title => "English title") } + + delete :destroy, params: { :node_id => node.id, :translation_locale => "en" } + + action = NodeAction.last + assert_equal node, action.node + assert_equal "en", action.locale + assert_equal "translation_destroy", action.action + assert_equal users(:quentin), action.user + assert_redirected_to node_path(node) + end end diff --git a/test/models/node_test.rb b/test/models/node_test.rb index 959387d..022fff6 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -621,4 +621,87 @@ class NodeTest < ActiveSupport::TestCase assert_equal "Edited directly on autosave", Globalize.with_locale(:en) { autosave.title } assert_equal "v2", Globalize.with_locale(:de) { autosave.title } end + + test "publish_draft! logs a NodeAction crediting the actual publisher" do + node = Node.root.children.create!(:slug => "publish_log_test") + node.draft.update!(:title => "Version one") + + node.publish_draft!(@user1) + + action = NodeAction.last + assert_equal node, action.node + assert_equal node.head, action.page + assert_equal @user1, action.user + assert_equal "publish", action.action + end + + test "publish_draft! called with no user logs no actor, not a guessed one" do + node = Node.root.children.create!(:slug => "publish_log_no_user_test") + node.draft.update!(:title => "Version one") + + node.publish_draft! + + action = NodeAction.last + assert_nil action.user + assert_nil action.metadata["username"] + end + + test "publish_draft! with nothing pending creates no NodeAction" do + node = Node.root.children.create!(:slug => "publish_log_noop_test") + node.publish_draft! + count_before = NodeAction.count + + result = node.publish_draft! + + assert_nil result + assert_equal count_before, NodeAction.count + end + + test "revert! logs discard_autosave for an in-progress autosave" do + node = create_node_with_published_page + node.lock_for_editing!(@user1) + node.autosave!({:title => "in progress"}, @user1) + + node.revert!(@user1) + + action = NodeAction.last + assert_equal node, action.node + assert_equal @user1, action.user + assert_equal "discard_autosave", action.action + end + + test "revert! logs destroy_draft for a draft with a head behind it" do + node = create_node_with_published_page + find_or_create_draft(node, @user1) + + node.revert!(@user1) + + action = NodeAction.last + assert_equal node, action.node + assert_equal @user1, action.user + assert_equal "destroy_draft", action.action + end + + test "revert! with nothing to revert logs nothing" do + node = create_node_with_published_page + node.lock_for_editing!(@user1) + count_before = NodeAction.count + + node.revert!(@user1) + + assert_equal count_before, NodeAction.count + end + + test "publish_draft! records the title's from/to in metadata" do + node = create_node_with_published_page + Globalize.with_locale(:de) { node.head.update!(:title => "Original Title") } + find_or_create_draft(node, @user1) + Globalize.with_locale(:de) { node.draft.update!(:title => "New Title") } + + node.publish_draft!(@user1) + + action = NodeAction.last + assert_equal "Original Title", action.metadata["from"] + assert_equal "New Title", action.metadata["to"] + end end -- cgit v1.3