diff --git a/app/controllers/sidebar_sections_controller.rb b/app/controllers/sidebar_sections_controller.rb index 0d811f554ba..e0607d4ffd6 100644 --- a/app/controllers/sidebar_sections_controller.rb +++ b/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 diff --git a/app/models/sidebar_section.rb b/app/models/sidebar_section.rb index 1fc3d13dab3..22549ed6b06 100644 --- a/app/models/sidebar_section.rb +++ b/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 diff --git a/app/models/sidebar_section_link.rb b/app/models/sidebar_section_link.rb index 0112f5eac88..68be317e82f 100644 --- a/app/models/sidebar_section_link.rb +++ b/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( diff --git a/db/migrate/20230404064728_system_user_for_public_sections.rb b/db/migrate/20230404064728_system_user_for_public_sections.rb new file mode 100644 index 00000000000..6b81b91351a --- /dev/null +++ b/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 diff --git a/spec/models/sidebar_section_link_spec.rb b/spec/models/sidebar_section_link_spec.rb index bc7db7703e4..54128b332c8 100644 --- a/spec/models/sidebar_section_link_spec.rb +++ b/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 diff --git a/spec/models/sidebar_section_spec.rb b/spec/models/sidebar_section_spec.rb new file mode 100644 index 00000000000..5db3cef5c28 --- /dev/null +++ b/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 diff --git a/spec/requests/sidebar_sections_controller_spec.rb b/spec/requests/sidebar_sections_controller_spec.rb index bbba5431b12..8e97ccdff9b 100644 --- a/spec/requests/sidebar_sections_controller_spec.rb +++ b/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)