Move the external homepage onto the draft

12 files changed, 69 insertions, 60 deletions

diff --git a/app/controllers/nodes_controller.rb b/app/controllers/nodes_controller.rb
index 3009e25a..ef47f258 100644
--- a/app/controllers/nodes_controller.rb
+++ b/app/controllers/nodes_controller.rb
@@ -281,7 +281,8 @@ class NodesController < ApplicationController
 
     def page_params
       params.fetch(:page, {}).permit(:title, :abstract, :body, :template_name,
-                                 :published_at, :user_id, :slug, :parent_node_id)
+                                 :published_at, :user_id, :slug, :parent_node_id,
+                                 :external_url)
     end
 
     def find_node
diff --git a/app/helpers/node_actions_helper.rb b/app/helpers/node_actions_helper.rb
index 19385543..f21fee61 100644
--- a/app/helpers/node_actions_helper.rb
+++ b/app/helpers/node_actions_helper.rb
@@ -28,8 +28,7 @@ module NodeActionsHelper
     "admin_revoke"       => "shield-minus",
     "event_create"       => "calendar-plus",
     "event_update"       => "calendar-event",
-    "event_destroy"      => "calendar-x",
-    "node_external_url"  => "world"
+    "event_destroy"      => "calendar-x"
   }.freeze
 
   def verb_icon action
@@ -67,7 +66,7 @@ module NodeActionsHelper
     return true if m["title"].is_a?(Hash) && m.dig("title", "from") != m.dig("title", "to")
     return true if m["external_url"].present?
     %w[author tags template_changed assets assets_changed assets_reordered
-       abstract_changed body_changed].any? { |key| m[key].present? }
+       abstract_changed body_changed external_url].any? { |key| m[key].present? }
   end
 
   def default_locale_changes action
@@ -95,6 +94,11 @@ module NodeActionsHelper
                     :names => linked_asset_names(action, names)).html_safe
       end
     end
+    if m["external_url"]
+      items << t("node_actions.detail_external_url",
+                  :from => m.dig("external_url", "from").presence || t("node_actions.event_none"),
+                  :to   => m.dig("external_url", "to").presence   || t("node_actions.event_none"))
+    end
     items << t("node_actions.assets_reordered") if m["assets_reordered"]
     items << t("node_actions.assets_changed")   if m["assets_changed"]
     items << t("node_actions.detail_external_url",
@@ -405,11 +409,6 @@ module NodeActionsHelper
     event_sentence(action, "event_destroy")
   end
 
-  def summarize_node_external_url action
-    t("node_actions.node_external_url", :actor => actor_ref(action),
-       :subject => subject_ref(action)).html_safe
-  end
-
   def event_sentence action, key
     if action.node
       t("node_actions.#{key}_on", :actor => actor_ref(action),
diff --git a/app/models/node.rb b/app/models/node.rb
index 5c28a786..f5b76b45 100644
--- a/app/models/node.rb
+++ b/app/models/node.rb
@@ -42,9 +42,6 @@ class Node < ApplicationRecord
             :inclusion   => { :in => ->(_) { Page.custom_templates } },
             :allow_blank => true,
             :if          => :default_template_name_changed?
-  validates :external_url, :format => { :with => %r{\Ahttps?://}i,
-                                        :allow_blank => true,
-                                        :message => :must_be_http }
 
   # Everything outside the Trash subtree, the Trash node included.
   # Relies on unique_name being authoritative for tree position --
@@ -260,6 +257,8 @@ class Node < ApplicationRecord
         self.slug = self.head.slug
       end
 
+      self.external_url = self.head.external_url
+
       if self.head.parent_node_id && self.head.parent_node_id != parent_id
         new_parent = Node.find_by(:id => self.head.parent_node_id)
 
@@ -604,23 +603,6 @@ class Node < ApplicationRecord
     self.created_at < new_id_format_date ? unique_path : id
   end
 
-  def update_external_url!(url, current_user = nil)
-    normalised = url.presence
-    return false if normalised == external_url
-
-    guard_live_change!(current_user)
-    previous = external_url
-
-    transaction do
-      update!(:external_url => normalised)
-      NodeAction.record!(:node => self, :user => current_user,
-                         :action => "node_external_url",
-                         :path => unique_name,
-                         :external_url => { "from" => previous, "to" => normalised })
-    end
-    true
-  end
-
   # Full-text search across all locale translations using PostgreSQL tsvector.
   # Uses 'simple' dictionary (no stemming, no stopwords) so queries work
   # across German and English content without language detection.
diff --git a/app/models/node_action.rb b/app/models/node_action.rb
index f1e4eaea..1e64861e 100644
--- a/app/models/node_action.rb
+++ b/app/models/node_action.rb
@@ -35,6 +35,7 @@ class NodeAction < ApplicationRecord
   #   "title"  -- pair, always; "from" null on first publish
   #   "author" -- pair, when the byline changed (incl. first publish)
   #   "tags"   -- pair of arrays, when changed
+  #   "external_url" -- pair, when a chapter's homepage changed
   #   "assets" -- {"added" => [asset names], "removed" => [asset names]},
   #               keys only when any; a delta, not a pair. The event IS
   #               the delta, full sets would bloat every entry. Changed
@@ -134,7 +135,6 @@ class NodeAction < ApplicationRecord
   #                    from the node verbs' "tags", which is a pair,
   #                    so one renderer cannot mistake the other.
   #   "path"        -- the node's unique_name, when it has a node
-  #   "external_url" -- pair
   #
   # On "event_update" only, and only when something changed -- an
   # update that changes nothing records no entry at all:
@@ -199,12 +199,16 @@ class NodeAction < ApplicationRecord
                          "to"   => title_of.call(new_page) } }
     unless old_page
       diff[:author] = { "from" => nil, "to" => new_page.user&.login } if new_page.user
+      diff[:external_url] = { "from" => nil, "to" => new_page.external_url } if new_page.external_url.present?
       return diff
     end
 
     old_author, new_author = old_page.user&.login, new_page.user&.login
     diff[:author] = { "from" => old_author, "to" => new_author } if old_author != new_author
 
+    old_url, new_url = old_page.external_url, new_page.external_url
+    diff[:external_url] = { "from" => old_url, "to" => new_url } if old_url != new_url
+
     old_tags, new_tags = old_page.tag_list.sort, new_page.tag_list.sort
     diff[:tags] = { "from" => old_tags, "to" => new_tags } if old_tags != new_tags
 
diff --git a/app/models/page.rb b/app/models/page.rb
index bba7e6bc..3240057f 100644
--- a/app/models/page.rb
+++ b/app/models/page.rb
@@ -20,6 +20,9 @@ class Page < ApplicationRecord
             :inclusion   => { :in => ->(_) { Page.custom_templates } },
             :allow_blank => true,
             :if          => :template_name_changed?
+  validates :external_url, :format => { :with => %r{\Ahttps?://}i,
+                                      :allow_blank => true,
+                                      :message => :must_be_http }
   validates_format_of   :slug, :with => /\A[A-Za-z0-9][A-Za-z0-9_-]*\z/,
                         :unless => -> { slug.blank? }
   validate :page_slug_not_reserved
@@ -208,6 +211,7 @@ class Page < ApplicationRecord
     # Clone untranslated attributes
     self.slug             = page.slug
     self.parent_node_id   = page.parent_node_id
+    self.external_url     = page.external_url
     self.tag_list         = page.tag_list
     self.template_name  ||= page.template_name
     self.published_at     = page.published_at
diff --git a/app/views/custom/page_templates/public/chapter_detail.html.erb b/app/views/custom/page_templates/public/chapter_detail.html.erb
index 794be823..105ec21a 100644
--- a/app/views/custom/page_templates/public/chapter_detail.html.erb
+++ b/app/views/custom/page_templates/public/chapter_detail.html.erb
@@ -1,8 +1,8 @@
 <div class="article" lang="<%= @page.effective_lang %>">
   <h1 class="headline"><%= @page.title %></h1>
   <%= headline_image %>
-  <% if @page.node.external_url.present? %>
-    <div class="chapter_url"><%= link_to @page.node.external_url, @page.node.external_url, target: '_blank', rel: 'noopener' %></div>
+  <% if @page.external_url.present? %>
+    <div class="chapter_url"><%= link_to @page.external_url, @page.external_url, target: '_blank', rel: 'noopener' %></div>
   <% end %>
   <p><%= sanitize(@page.abstract) %></p>
   <%= aggregate?(@page.body) %>
diff --git a/app/views/custom/partials/_chapter.html.erb b/app/views/custom/partials/_chapter.html.erb
index 8a92b46f..2065560a 100644
--- a/app/views/custom/partials/_chapter.html.erb
+++ b/app/views/custom/partials/_chapter.html.erb
@@ -7,8 +7,8 @@
     <% if page.abstract.present? %>
       <div class="chapter_location"><%= page.abstract %></div>
     <% end %>
-    <% if page.node.external_url.present? %>
-      <div class="chapter_url"><%= link_to page.node.external_url, page.node.external_url, target: '_blank', rel: 'noopener' %></div>
+    <% if page.external_url.present? %>
+      <div class="chapter_url"><%= link_to page.external_url, page.external_url, target: '_blank', rel: 'noopener' %></div>
     <% end %>
     <% open_days = page.node.events.tagged_with('open-day').order(:start_time) %>
     <% if open_days.any? %>
diff --git a/app/views/nodes/edit.html.erb b/app/views/nodes/edit.html.erb
index ae206578..6b5a1234 100644
--- a/app/views/nodes/edit.html.erb
+++ b/app/views/nodes/edit.html.erb
@@ -116,8 +116,7 @@
 
         <div class="layout_row_label"><%= t(".external_url") %></div>
         <div class="layout_row_content">
-          <%= f.text_field :external_url %>
-          <p class="field_hint"><%= t(".external_url_hint") %></p>
+          <%= d.text_field :external_url %>
         </div>
 
         <div class="layout_row_label"><%= t(".tags") %></div>
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 032d7647..c6dc17b7 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -120,17 +120,12 @@ de:
     errors:
       models:
         node:
-          # Attribute messages: full_messages prefixes the human attribute
-          # name, so these read as fragments. Defined at model level rather
-          # than per attribute, because several attributes share a message.
           reserved_for_trash: "ist für den Papierkorb reserviert"
           trash_immutable: "des Papierkorb-Nodes kann nicht geändert werden"
           trash_must_be_empty: "muss auf dem Papierkorb-Node leer bleiben"
           inside_trash: "kann im Papierkorb nicht existieren"
           attributes:
             base:
-              # :base messages stand alone -- no attribute name is prefixed,
-              # so these are whole sentences.
               has_children: "Ein Node mit Kindern kann nicht gelöscht werden"
               trash_undeletable: "Der Papierkorb-Node kann nicht gelöscht werden"
               publish_in_trash: "Ein Node im Papierkorb kann nicht veröffentlicht werden"
@@ -145,6 +140,8 @@ de:
           attributes:
             slug:
               reserved_for_trash: "ist für den Papierkorb reserviert"
+            external_url:
+              must_be_http: "muss mit http:// oder https:// beginnen"
         user:
           attributes:
             roles:
@@ -163,7 +160,7 @@ de:
         event:
           attributes:
             url:
-              must_be_http: "must start with http:// or https://"
+              must_be_http: "muss mit http:// oder https:// beginnen"
 
   tags:
     index:
@@ -253,7 +250,6 @@ de:
     detail_event_allday: "ganztägig %{from} → %{to}"
     detail_event_end: "Ende %{from} → %{to}"
     event_none: "—"
-    node_external_url: "%{actor} hat die externe Homepage von %{subject} geändert"
     unknown_event: "ein Termin"
 
   open_gallery: "Gallerie anzeigen"
@@ -456,7 +452,6 @@ de:
       set_headline_title: "Dieses Foto als Aufmacherbild der Seite verwenden"
       remove_image_label: "Bild entfernen"
       external_url: "Externe Homepage"
-      external_url_hint: "Die eigene Website einer Dezentrale, wird auf dessen Seite angezeigt. Vorsicht, Änderung wird ohne Draft sofort veröffentlicht."
     show:
       status: "Status"
       head: "Head"
diff --git a/config/locales/en.yml b/config/locales/en.yml
index e75000bc..bd339cc9 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -91,6 +91,8 @@ en:
           attributes:
             slug:
               reserved_for_trash: "is reserved for the Trash"
+            external_url:
+              must_be_http: "must start with http:// or https://"
         user:
           attributes:
             roles:
@@ -199,7 +201,6 @@ en:
     detail_event_coordinates: "coordinates changed"
     detail_event_allday: "all-day %{from} → %{to}"
     detail_event_end: "end %{from} → %{to}"
-    node_external_url: "%{actor} changed the external homepage of %{subject}"
     event_none: "—"
     unknown_event: "an event"
 
@@ -403,7 +404,6 @@ en:
       set_headline_title: "Use this photo as the page's headline image"
       remove_image_label: "Remove image"
       external_url: "External homepage"
-      external_url_hint: "A chapter's own website, shown on its page. N.B.: Changing takes effect immediately."
     show:
       status: "Status"
       head: "Head"
diff --git a/lib/tasks/pages.rake b/lib/tasks/pages.rake
index 0cfbc5dc..ea0776b7 100644
--- a/lib/tasks/pages.rake
+++ b/lib/tasks/pages.rake
@@ -30,4 +30,32 @@ namespace :pages do
 
     puts "#{write ? "updated" : "would update"} #{touched} pages"
   end
+
+  desc "Backfill pages.external_url from each page's node."
+  task :backfill_external_url => :environment do
+    write = ENV["WRITE"] == "1"
+    puts "DRY RUN -- nothing written. Re-run with WRITE=1." unless write
+
+    touched = 0
+    Node.where.not(:external_url => [nil, ""]).find_each do |node|
+      scope = node.pages.where("external_url IS DISTINCT FROM :u", :u => node.external_url)
+      count = scope.count
+      next if count.zero?
+
+      scope.update_all(:external_url => node.external_url) if write
+      touched += count
+    end
+
+    # Autosaves carry no node_id, so has_many :pages does not cover them.
+    Node.where.not(:autosave_id => nil).where.not(:external_url => [nil, ""])
+        .includes(:autosave).find_each do |node|
+      a = node.autosave
+      next if a.nil? || a.external_url == node.external_url
+
+      a.update_columns(:external_url => node.external_url) if write
+      touched += 1
+    end
+
+    puts "#{write ? "updated" : "would update"} #{touched} pages"
+  end
 end
diff --git a/test/models/node_test.rb b/test/models/node_test.rb
index df1e96cb..735d2abe 100644
--- a/test/models/node_test.rb
+++ b/test/models/node_test.rb
@@ -1072,32 +1072,29 @@ class NodeTest < ActiveSupport::TestCase
     assert_equal club.id, node.parent_id
   end
 
-  test "setting an external url is gated and witnessed" do
+  test "an external url is carried by the draft and applied on publish" do
     node = Node.root.children.create!(:slug => "chapter_url_test")
-
-    assert_difference -> { NodeAction.where(:action => "node_external_url").count }, 1 do
-      assert node.update_external_url!("https://example.org", users(:aaron))
-    end
+    node.draft.update!(:external_url => "https://example.org")
+    node.publish_draft!(users(:aaron))
 
     assert_equal "https://example.org", node.reload.external_url
-    entry = NodeAction.where(:action => "node_external_url").last
-    assert_equal({ "from" => nil, "to" => "https://example.org" },
-                 entry.metadata["external_url"])
+    assert_equal "https://example.org", node.head.external_url
   end
 
-  test "a javascript url is refused" do
+  test "a javascript url is refused on the draft" do
     node = Node.root.children.create!(:slug => "chapter_url_reject")
     assert_raises(ActiveRecord::RecordInvalid) do
-      node.update_external_url!("javascript:alert(1)", users(:aaron))
+      node.draft.update!(:external_url => "javascript:alert(1)")
     end
   end
 
-  test "setting an external url on a restricted node needs redaktion" do
-    node = Node.find_by(:unique_name => "updates") ||
-           Node.root.children.create!(:slug => "updates")
+  test "publishing an external url change on a restricted node needs redaktion" do
+    updates = Node.root.children.create!(:slug => "updates")
+    node = updates.children.create!(:slug => "chapter_url_gated")
+    node.draft.update!(:external_url => "https://example.org")
 
     assert_raises(ActiveRecord::RecordInvalid) do
-      node.update_external_url!("https://example.org", users(:quentin))
+      node.publish_draft!(users(:quentin))
     end
   end