FIX: public sidebar sections belong to system user (#20972)

Before, public sidebar sections were belonging to admin. However, a better choice is system user.

app/controllers/sidebar_sections_controller.rb

@@ -16,9 +16,7 @@ class SidebarSectionsController < ApplicationController
 
   def create
     sidebar_section =
-      SidebarSection.create!(
-        section_params.merge(user: current_user, sidebar_urls_attributes: links_params),
-      )
+      SidebarSection.create!(section_params.merge(sidebar_urls_attributes: links_params))
 
     if sidebar_section.public?
       StaffActionLogger.new(current_user).log_create_public_sidebar_section(sidebar_section)
@@ -38,7 +36,10 @@ class SidebarSectionsController < ApplicationController
     sidebar_section = SidebarSection.find_by(id: section_params["id"])
     @guardian.ensure_can_edit!(sidebar_section)
 
-    sidebar_section.update!(section_params.merge(sidebar_urls_attributes: links_params))
+    ActiveRecord::Base.transaction do
+      sidebar_section.update!(section_params.merge(sidebar_urls_attributes: links_params))
+      sidebar_section.sidebar_section_links.update_all(user_id: sidebar_section.user_id)
+    end
 
     if sidebar_section.public?
       StaffActionLogger.new(current_user).log_update_public_sidebar_section(sidebar_section)
@@ -95,7 +96,9 @@ class SidebarSectionsController < ApplicationController
   end
 
   def section_params
-    params.permit(:id, :title, :public)
+    section_params = params.permit(:id, :title, :public)
+    section_params.merge!(user: current_user) if !params[:public]
+    section_params
   end
 
   def links_params

app/models/sidebar_section.rb

@@ -12,6 +12,8 @@ class SidebarSection < ActiveRecord::Base
 
   accepts_nested_attributes_for :sidebar_urls, allow_destroy: true
 
+  before_save :set_system_user_for_public_section
+
   validates :title,
             presence: true,
             uniqueness: {
@@ -20,6 +22,12 @@ class SidebarSection < ActiveRecord::Base
             length: {
               maximum: MAX_TITLE_LENGTH,
             }
+
+  private
+
+  def set_system_user_for_public_section
+    self.user_id = Discourse.system_user.id if self.public
+  end
 end
 
 # == Schema Information

app/models/sidebar_section_link.rb

@@ -12,7 +12,7 @@ class SidebarSectionLink < ActiveRecord::Base
 
   SUPPORTED_LINKABLE_TYPES = %w[Category Tag SidebarUrl]
 
-  before_validation { self.user_id ||= self.sidebar_section&.user_id }
+  before_validation :inherit_user_id
   before_create do
     if self.user_id && self.sidebar_section
       self.position = self.sidebar_section.sidebar_section_links.maximum(:position).to_i + 1
@@ -21,7 +21,13 @@ class SidebarSectionLink < ActiveRecord::Base
 
   after_destroy { self.linkable.destroy! if self.linkable_type == "SidebarUrl" }
 
-  private def ensure_supported_linkable_type
+  private
+
+  def inherit_user_id
+    self.user_id = sidebar_section.user_id if sidebar_section
+  end
+
+  def ensure_supported_linkable_type
     if (!SUPPORTED_LINKABLE_TYPES.include?(self.linkable_type)) ||
          (self.linkable_type == "Tag" && !SiteSetting.tagging_enabled)
       self.errors.add(

db/migrate/20230404064728_system_user_for_public_sections.rb

@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+class SystemUserForPublicSections < ActiveRecord::Migration[7.0]
+  def up
+    execute(<<-SQL)
+      UPDATE sidebar_sections
+      SET user_id = -1
+      WHERE public IS TRUE
+    SQL
+    execute(<<-SQL)
+      UPDATE sidebar_section_links
+      SET user_id = -1
+      FROM sidebar_sections
+      WHERE sidebar_sections.public IS TRUE
+        AND sidebar_section_links.sidebar_section_id = sidebar_sections.id
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/models/sidebar_section_link_spec.rb

@@ -47,4 +47,28 @@ RSpec.describe SidebarSectionLink do
       end
     end
   end
+
+  it "uses section user for links belonging to private sections" do
+    private_section = Fabricate(:sidebar_section, public: false)
+    sidebar_section_link =
+      Fabricate(
+        :sidebar_section_link,
+        sidebar_section: private_section,
+        linkable_id: 1,
+        linkable_type: "Tag",
+      )
+    expect(sidebar_section_link.user_id).to eq(private_section.user_id)
+  end
+
+  it "uses system user for links belonging to public sections" do
+    public_section = Fabricate(:sidebar_section, public: true)
+    sidebar_section_link =
+      Fabricate(
+        :sidebar_section_link,
+        sidebar_section: public_section,
+        linkable_id: 1,
+        linkable_type: "Tag",
+      )
+    expect(sidebar_section_link.user_id).to eq(Discourse.system_user.id)
+  end
 end

spec/models/sidebar_section_spec.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+RSpec.describe SidebarSection do
+  fab!(:user) { Fabricate(:user) }
+  fab!(:sidebar_section) { Fabricate(:sidebar_section, user: user) }
+
+  it "uses system user for public sections" do
+    expect(sidebar_section.user_id).to eq(user.id)
+    sidebar_section.update!(public: true)
+    expect(sidebar_section.user_id).to eq(Discourse.system_user.id)
+  end
+end

spec/requests/sidebar_sections_controller_spec.rb

@@ -15,14 +15,13 @@ RSpec.describe SidebarSectionsController do
   describe "#index" do
     fab!(:sidebar_section) { Fabricate(:sidebar_section, title: "private section", user: user) }
     fab!(:sidebar_url_1) { Fabricate(:sidebar_url, name: "tags", value: "/tags") }
+    fab!(:sidebar_url_2) { Fabricate(:sidebar_url, name: "categories", value: "/categories") }
     fab!(:section_link_1) do
       Fabricate(:sidebar_section_link, sidebar_section: sidebar_section, linkable: sidebar_url_1)
     end
-    fab!(:sidebar_section_2) do
-      Fabricate(:sidebar_section, title: "public section", user: admin, public: true)
-    end
+    fab!(:sidebar_section_2) { Fabricate(:sidebar_section, title: "public section", public: true) }
     fab!(:section_link_2) do
-      Fabricate(:sidebar_section_link, sidebar_section: sidebar_section, linkable: sidebar_url_1)
+      Fabricate(:sidebar_section_link, sidebar_section: sidebar_section, linkable: sidebar_url_2)
     end
 
     it "returns public and private sections" do
@@ -123,6 +122,7 @@ RSpec.describe SidebarSectionsController do
       sidebar_section = SidebarSection.last
       expect(sidebar_section.title).to eq("custom section")
       expect(sidebar_section.public).to be true
+      expect(sidebar_section.user_id).to be Discourse.system_user.id
 
       user_history = UserHistory.last
       expect(user_history.action).to eq(UserHistory.actions[:create_public_sidebar_section])
@@ -165,7 +165,7 @@ RSpec.describe SidebarSectionsController do
 
     it "allows admin to update public section and links" do
       sign_in(admin)
-      sidebar_section.update!(user: admin, public: true)
+      sidebar_section.update!(public: true)
       put "/sidebar_sections/#{sidebar_section.id}.json",
           params: {
             title: "custom section edited",
@@ -313,7 +313,7 @@ RSpec.describe SidebarSectionsController do
 
     it "allows admin to delete public section" do
       sign_in(admin)
-      sidebar_section.update!(user: admin, public: true)
+      sidebar_section.update!(public: true)
       delete "/sidebar_sections/#{sidebar_section.id}.json"
 
       expect(response.status).to eq(200)