diff --git a/app/assets/javascripts/admin/addon/mixins/setting-component.js b/app/assets/javascripts/admin/addon/mixins/setting-component.js index 3e2fd8f0645..d651d1690c6 100644 --- a/app/assets/javascripts/admin/addon/mixins/setting-component.js +++ b/app/assets/javascripts/admin/addon/mixins/setting-component.js @@ -157,7 +157,10 @@ export default Mixin.create({ "default_tags_watching_first_post", "default_text_size", "default_title_count_mode", + "default_sidebar_categories", + "default_sidebar_tags", ]; + const key = this.buffered.get("setting"); if (defaultUserPreferences.includes(key)) { diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb index 846f4953a53..a0a6ad9b869 100644 --- a/app/controllers/admin/site_settings_controller.rb +++ b/app/controllers/admin/site_settings_controller.rb @@ -23,9 +23,11 @@ class Admin::SiteSettingsController < Admin::AdminController if !override raise Discourse::InvalidParameters, "You cannot change this site setting because it is deprecated, use #{new_name} instead." end + break new_name end end + id = new_setting_name if new_setting_name raise_access_hidden_setting(id) @@ -101,6 +103,8 @@ class Admin::SiteSettingsController < Admin::AdminController TagUser.insert_all!(tag_users) end end + elsif is_sidebar_default_setting?(id) + Jobs.enqueue(:backfill_sidebar_site_settings, setting_name: id, previous_value: previous_value, new_value: new_value) end end @@ -159,6 +163,8 @@ class Admin::SiteSettingsController < Admin::AdminController .pluck("users.id") json[:user_count] = user_ids.uniq.count + elsif is_sidebar_default_setting?(id) + json[:user_count] = SidebarSiteSettingsBackfiller.new(id, previous_value: previous_value, new_value: new_value).number_of_users_to_backfill end render json: json @@ -166,6 +172,10 @@ class Admin::SiteSettingsController < Admin::AdminController private + def is_sidebar_default_setting?(setting_name) + %w{default_sidebar_categories default_sidebar_tags}.include?(setting_name.to_s) + end + def user_options { default_email_mailing_list_mode: "mailing_list_mode", diff --git a/app/jobs/regular/backfill_sidebar_site_settings.rb b/app/jobs/regular/backfill_sidebar_site_settings.rb new file mode 100644 index 00000000000..8e07427e83d --- /dev/null +++ b/app/jobs/regular/backfill_sidebar_site_settings.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class Jobs::BackfillSidebarSiteSettings < Jobs::Base + def execute(args) + SidebarSiteSettingsBackfiller + .new(args[:setting_name], previous_value: args[:previous_value], new_value: args[:new_value]) + .backfill! + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 3539bd3739e..d92780a13b4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -135,6 +135,11 @@ class User < ActiveRecord::Base after_create :ensure_in_trust_level_group after_create :set_default_categories_preferences after_create :set_default_tags_preferences + after_create :set_default_sidebar_section_links + + after_update :set_default_sidebar_section_links, if: Proc.new { + self.saved_change_to_staged? + } after_update :trigger_user_updated_event, if: Proc.new { self.human? && self.saved_change_to_uploaded_avatar_id? @@ -279,6 +284,11 @@ class User < ActiveRecord::Base MAX_STAFF_DELETE_POST_COUNT ||= 5 + def visible_sidebar_tags(user_guardian = nil) + user_guardian ||= guardian + DiscourseTagging.filter_visible(custom_sidebar_tags, user_guardian) + end + def self.max_password_length 200 end @@ -1670,27 +1680,6 @@ class User < ActiveRecord::Base SiteSetting.enable_experimental_sidebar_hamburger end - def sidebar_categories_ids - categories_ids = category_sidebar_section_links.pluck(:linkable_id) - - if categories_ids.blank? && SiteSetting.default_sidebar_categories.present? - return SiteSetting.default_sidebar_categories.split("|").map(&:to_i) & guardian.allowed_category_ids - end - - categories_ids - end - - def sidebar_tags - return custom_sidebar_tags if custom_sidebar_tags.present? - - if SiteSetting.default_sidebar_tags.present? - tag_names = SiteSetting.default_sidebar_tags.split("|") - DiscourseTagging.hidden_tag_names(guardian) - return Tag.where(name: tag_names) - end - - Tag.none - end - protected def badge_grant @@ -1919,6 +1908,45 @@ class User < ActiveRecord::Base private + def set_default_sidebar_section_links + return if !SiteSetting.enable_experimental_sidebar_hamburger + return if staged? || bot? + + records = [] + + if SiteSetting.default_sidebar_categories.present? + category_ids = SiteSetting.default_sidebar_categories.split("|") + + # Filters out categories that user does not have access to or do not exist anymore + category_ids = Category.secured(self.guardian).where(id: category_ids).pluck(:id) + + category_ids.each do |category_id| + records.push( + linkable_type: 'Category', + linkable_id: category_id, + user_id: self.id + ) + end + end + + if SiteSetting.tagging_enabled && SiteSetting.default_sidebar_tags.present? + tag_names = SiteSetting.default_sidebar_tags.split("|") + + # Filters out tags that user cannot see or do not exist anymore + tag_ids = DiscourseTagging.filter_visible(Tag, self.guardian).where(name: tag_names).pluck(:id) + + tag_ids.each do |tag_id| + records.push( + linkable_type: 'Tag', + linkable_id: tag_id, + user_id: self.id + ) + end + end + + SidebarSectionLink.insert_all!(records) if records.present? + end + def stat user_stat || create_user_stat end diff --git a/app/serializers/concerns/user_sidebar_tags_mixin.rb b/app/serializers/concerns/user_sidebar_tags_mixin.rb index e387f01c310..05fa60afdab 100644 --- a/app/serializers/concerns/user_sidebar_tags_mixin.rb +++ b/app/serializers/concerns/user_sidebar_tags_mixin.rb @@ -2,9 +2,19 @@ module UserSidebarTagsMixin def self.included(base) - base.attributes :display_sidebar_tags + base.attributes :display_sidebar_tags, + :sidebar_tags + end - base.has_many :sidebar_tags, serializer: Sidebar::TagSerializer, embed: :objects + def sidebar_tags + object.visible_sidebar_tags(scope) + .pluck(:name, :topic_count, :pm_topic_count) + .reduce([]) do |tags, sidebar_tag| + tags.push( + name: sidebar_tag[0], + pm_only: sidebar_tag[1] == 0 && sidebar_tag[2] > 0 + ) + end end def include_sidebar_tags? diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 62b358e1652..0a607648e75 100644 --- a/app/serializers/current_user_serializer.rb +++ b/app/serializers/current_user_serializer.rb @@ -339,7 +339,7 @@ class CurrentUserSerializer < BasicUserSerializer end def sidebar_category_ids - object.sidebar_categories_ids + object.category_sidebar_section_links.pluck(:linkable_id) & scope.allowed_category_ids end def include_sidebar_category_ids? diff --git a/app/serializers/sidebar/tag_serializer.rb b/app/serializers/sidebar/tag_serializer.rb deleted file mode 100644 index 3e576cd2474..00000000000 --- a/app/serializers/sidebar/tag_serializer.rb +++ /dev/null @@ -1,11 +0,0 @@ -# frozen_string_literal: true - -module Sidebar - class TagSerializer < ::ApplicationSerializer - attributes :name, :pm_only - - def pm_only - object.topic_count == 0 && object.pm_topic_count > 0 - end - end -end diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb index 0ac35515c19..de82619c9fe 100644 --- a/app/serializers/site_serializer.rb +++ b/app/serializers/site_serializer.rb @@ -229,11 +229,11 @@ class SiteSerializer < ApplicationSerializer end def anonymous_default_sidebar_tags - User.new.sidebar_tags.pluck(:name) + SiteSetting.default_sidebar_tags.split("|") - DiscourseTagging.hidden_tag_names(scope) end def include_anonymous_default_sidebar_tags? - scope.anonymous? && SiteSetting.tagging_enabled && SiteSetting.default_sidebar_tags.present? + scope.anonymous? && SiteSetting.enable_experimental_sidebar_hamburger && SiteSetting.tagging_enabled && SiteSetting.default_sidebar_tags.present? end private diff --git a/app/services/sidebar_site_settings_backfiller.rb b/app/services/sidebar_site_settings_backfiller.rb new file mode 100644 index 00000000000..3154e6a663e --- /dev/null +++ b/app/services/sidebar_site_settings_backfiller.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +# A service class that backfills the changes to the default sidebar categories and tags site settings. +# +# When a category/tag is removed from the site settings, the `SidebarSectionLink` records associated with the category/tag +# are deleted. +# +# When a category/tag is added to the site settings, a `SidebarSectionLink` record for the associated category/tag are +# created for all users that do not already have a `SidebarSectionLink` record for the category/tag. +class SidebarSiteSettingsBackfiller + def initialize(setting_name, previous_value:, new_value:) + @setting_name = setting_name + + @linkable_klass, previous_ids, new_ids = + case setting_name + when "default_sidebar_categories" + [ + Category, + previous_value.split("|"), + new_value.split("|") + ] + when "default_sidebar_tags" + klass = Tag + + [ + klass, + klass.where(name: previous_value.split("|")).pluck(:id), + klass.where(name: new_value.split("|")).pluck(:id) + ] + else + raise 'Invalid setting_name' + end + + @added_ids = new_ids - previous_ids + @removed_ids = previous_ids - new_ids + end + + def backfill! + DistributedMutex.synchronize("backfill_sidebar_site_settings_#{@setting_name}") do + SidebarSectionLink.where(linkable_type: @linkable_klass.to_s, linkable_id: @removed_ids).delete_all + + User.real.where(staged: false).select(:id).find_in_batches do |users| + rows = [] + + users.each do |user| + @added_ids.each do |linkable_id| + rows << { user_id: user[:id], linkable_type: @linkable_klass.to_s, linkable_id: linkable_id } + end + end + + SidebarSectionLink.insert_all!(rows) if rows.present? + end + end + end + + def number_of_users_to_backfill + select_statements = [] + + if @removed_ids.present? + select_statements.push(<<~SQL) + SELECT + sidebar_section_links.user_id + FROM sidebar_section_links + WHERE sidebar_section_links.linkable_type = '#{@linkable_klass.to_s}' + AND sidebar_section_links.linkable_id IN (#{@removed_ids.join(",")}) + SQL + end + + if @added_ids.present? + # Returns the ids of users that will receive the new additions by excluding the users that already have the additions + # Note that we want to avoid doing a left outer join against the "sidebar_section_links" table as PG will end up having + # to do a full table join for both tables first which is less efficient and can be slow on large sites. + select_statements.push(<<~SQL) + SELECT + users.id + FROM users + WHERE users.id NOT IN ( + SELECT + sidebar_section_links.user_id + FROM sidebar_section_links + WHERE sidebar_section_links.linkable_type = '#{@linkable_klass.to_s}' + AND sidebar_section_links.linkable_id IN (#{@added_ids.join(",")}) + ) AND users.id > 0 AND NOT users.staged + SQL + end + + DB.query_single(<<~SQL)[0] + SELECT + COUNT(*) + FROM (#{select_statements.join("\nUNION DISTINCT\n")}) AS user_ids + SQL + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 60d6a9e202c..ca58071bb1b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -20,6 +20,90 @@ RSpec.describe User do end end + describe 'Callbacks' do + describe 'default sidebar section links' do + fab!(:category) { Fabricate(:category) } + + fab!(:secured_category) do |category| + category = Fabricate(:category) + category.permissions = { "staff" => :full } + category.save! + category + end + + fab!(:tag) { Fabricate(:tag) } + fab!(:hidden_tag) { Fabricate(:tag) } + fab!(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) } + + before do + SiteSetting.enable_experimental_sidebar_hamburger = true + SiteSetting.tagging_enabled = true + SiteSetting.default_sidebar_categories = "#{category.id}|#{secured_category.id}" + SiteSetting.default_sidebar_tags = "#{tag.name}|#{hidden_tag.name}" + end + + it 'creates the right sidebar section link records for categories and tags that a user can see' do + user = Fabricate(:user) + + expect(SidebarSectionLink.where(linkable_type: 'Category', user_id: user.id).pluck(:linkable_id)).to contain_exactly( + category.id + ) + + expect(SidebarSectionLink.where(linkable_type: 'Tag', user_id: user.id).pluck(:linkable_id)).to contain_exactly( + tag.id + ) + + user = Fabricate(:admin) + + expect(SidebarSectionLink.where(linkable_type: 'Category', user_id: user.id).pluck(:linkable_id)).to contain_exactly( + category.id, + secured_category.id + ) + + expect(SidebarSectionLink.where(linkable_type: 'Tag', user_id: user.id).pluck(:linkable_id)).to contain_exactly( + tag.id, + hidden_tag.id + ) + end + + it 'should not create any sidebar section link records when experimental sidebar is disabled' do + SiteSetting.enable_experimental_sidebar_hamburger = false + + user = Fabricate(:user) + + expect(SidebarSectionLink.exists?(user_id: user.id)).to eq(false) + end + + it 'should not create any sidebar section link records for staged users' do + user = Fabricate(:user, staged: true) + + expect(SidebarSectionLink.exists?).to eq(false) + end + + it 'should create sidebar section link records when user has been unstaged' do + user = Fabricate(:user, staged: true) + user.unstage! + + expect(SidebarSectionLink.exists?(user: user)).to eq(true) + end + + it 'should not create any sidebar section link records for non human users' do + user = Fabricate(:user, id: -Time.now.to_i) + + expect(SidebarSectionLink.exists?).to eq(false) + end + + it 'should not create any tag sidebar section link records when tagging is disabled' do + SiteSetting.tagging_enabled = false + + user = Fabricate(:user) + + expect(SidebarSectionLink.exists?(linkable_type: 'Category', user_id: user.id)).to eq(true) + expect(SidebarSectionLink.exists?(linkable_type: 'Tag', user_id: user.id)).to eq(false) + end + end + end + describe 'Validations' do describe '#username' do it { is_expected.to validate_presence_of :username } @@ -2984,4 +3068,21 @@ RSpec.describe User do expect(user.reload.seen_notification_id).to eq(last_notification.id) end end + + describe '#visible_sidebar_tags' do + fab!(:user) { Fabricate(:user) } + fab!(:tag) { Fabricate(:tag) } + fab!(:hidden_tag) { Fabricate(:tag, name: "secret") } + fab!(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["secret"]) } + fab!(:tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user, linkable: tag) } + fab!(:tag_sidebar_section_link_2) { Fabricate(:tag_sidebar_section_link, user: user, linkable: hidden_tag) } + + it 'should only return tag sidebar section link records of tags that the user is allowed to see' do + expect(user.visible_sidebar_tags).to contain_exactly(tag) + + user.update!(admin: true) + + expect(user.visible_sidebar_tags).to contain_exactly(tag, hidden_tag) + end + end end diff --git a/spec/requests/admin/site_settings_controller_spec.rb b/spec/requests/admin/site_settings_controller_spec.rb index c573a89701c..8837fadf492 100644 --- a/spec/requests/admin/site_settings_controller_spec.rb +++ b/spec/requests/admin/site_settings_controller_spec.rb @@ -116,6 +116,44 @@ RSpec.describe Admin::SiteSettingsController do end end + context 'when updating default sidebar categories and tags' do + it 'does not enqueue the backfilling job if update_existing_user param is not present' do + expect_not_enqueued_with(job: :backfill_sidebar_site_settings) do + put "/admin/site_settings/default_sidebar_categories.json", params: { + default_sidebar_categories: "1|2", + } + + expect(response.status).to eq(200) + end + end + + it 'enqueus the backfilling job if update_existing_user param is present when updating default sidebar tags' do + SiteSetting.default_sidebar_tags = "tag3" + + expect_enqueued_with(job: :backfill_sidebar_site_settings, args: { setting_name: 'default_sidebar_tags', new_value: 'tag1|tag2', previous_value: 'tag3' }) do + put "/admin/site_settings/default_sidebar_tags.json", params: { + default_sidebar_tags: "tag1|tag2", + update_existing_user: true + } + + expect(response.status).to eq(200) + end + end + + it 'enqueus the backfilling job if update_existing_user param is present when updating default sidebar categories' do + SiteSetting.default_sidebar_categories = "3|4" + + expect_enqueued_with(job: :backfill_sidebar_site_settings, args: { setting_name: 'default_sidebar_categories', new_value: '1|2', previous_value: '3|4' }) do + put "/admin/site_settings/default_sidebar_categories.json", params: { + default_sidebar_categories: "1|2", + update_existing_user: true + } + + expect(response.status).to eq(200) + end + end + end + describe 'default categories' do fab!(:user1) { Fabricate(:user) } fab!(:user2) { Fabricate(:user) } @@ -265,6 +303,30 @@ RSpec.describe Admin::SiteSettingsController do SiteSetting.setting(:default_tags_watching, "") end + context "for sidebar defaults" do + it 'returns the right count for the default_sidebar_categories site setting' do + category = Fabricate(:category) + + put "/admin/site_settings/default_sidebar_categories/user_count.json", params: { + default_sidebar_categories: "#{category.id}" + } + + expect(response.status).to eq(200) + expect(response.parsed_body["user_count"]).to eq(User.real.not_staged.count) + end + + it 'returns the right count for the default_sidebar_tags site setting' do + tag = Fabricate(:tag) + + put "/admin/site_settings/default_sidebar_tags/user_count.json", params: { + default_sidebar_tags: "#{tag.name}" + } + + expect(response.status).to eq(200) + expect(response.parsed_body["user_count"]).to eq(User.real.not_staged.count) + end + end + context "with user options" do def expect_user_count(site_setting_name:, user_setting_name:, current_site_setting_value:, new_site_setting_value:, current_user_setting_value: nil, new_user_setting_value: nil) diff --git a/spec/serializers/current_user_serializer_spec.rb b/spec/serializers/current_user_serializer_spec.rb index 68479524d84..3b6ce305831 100644 --- a/spec/serializers/current_user_serializer_spec.rb +++ b/spec/serializers/current_user_serializer_spec.rb @@ -221,15 +221,15 @@ RSpec.describe CurrentUserSerializer do end describe '#sidebar_tags' do - fab!(:tag_1) { Fabricate(:tag, name: "foo") } - fab!(:tag_2) { Fabricate(:tag, name: "bar") } + fab!(:tag) { Fabricate(:tag, name: "foo") } + fab!(:pm_tag) { Fabricate(:tag, name: "bar", pm_topic_count: 5, topic_count: 0) } fab!(:hidden_tag) { Fabricate(:tag, name: "secret") } fab!(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["secret"]) } - let(:tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user) } - let(:tag_sidebar_section_link_2) { Fabricate(:tag_sidebar_section_link, user: user) } + fab!(:tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user, linkable: tag) } + fab!(:tag_sidebar_section_link_2) { Fabricate(:tag_sidebar_section_link, user: user, linkable: pm_tag) } + fab!(:tag_sidebar_section_link_3) { Fabricate(:tag_sidebar_section_link, user: user, linkable: hidden_tag) } it "is not included when experimental sidebar has not been enabled" do - tag_sidebar_section_link SiteSetting.enable_experimental_sidebar_hamburger = false SiteSetting.tagging_enabled = true @@ -239,7 +239,6 @@ RSpec.describe CurrentUserSerializer do end it "is not included when tagging has not been enabled" do - tag_sidebar_section_link SiteSetting.enable_experimental_sidebar_hamburger = true SiteSetting.tagging_enabled = false @@ -248,54 +247,37 @@ RSpec.describe CurrentUserSerializer do expect(json[:sidebar_tags]).to eq(nil) end - it "is present when experimental sidebar and tagging has been enabled" do - tag_sidebar_section_link + it "serializes only the tags that the user can see when experimental sidebar and tagging has been enabled" do SiteSetting.enable_experimental_sidebar_hamburger = true SiteSetting.tagging_enabled = true - tag_sidebar_section_link_2.linkable.update!(pm_topic_count: 5, topic_count: 0) - json = serializer.as_json expect(json[:sidebar_tags]).to contain_exactly( - { name: tag_sidebar_section_link.linkable.name, pm_only: false }, - { name: tag_sidebar_section_link_2.linkable.name, pm_only: true } + { name: tag.name, pm_only: false }, + { name: pm_tag.name, pm_only: true } ) - end - it 'includes visible default sidebar tags' do - SiteSetting.enable_experimental_sidebar_hamburger = true - SiteSetting.tagging_enabled = true - SiteSetting.default_sidebar_tags = "foo|bar|secret" + user.update!(admin: true) json = serializer.as_json - expect(json[:sidebar_tags]).to eq([ - { name: "foo", pm_only: false }, - { name: "bar", pm_only: false } - ]) - end - - it 'includes tags choosen by user' do - SiteSetting.enable_experimental_sidebar_hamburger = true - SiteSetting.tagging_enabled = true - SiteSetting.default_sidebar_tags = "foo|bar|secret" - tag_sidebar_section_link = Fabricate(:tag_sidebar_section_link, user: user) - - json = serializer.as_json - - expect(json[:sidebar_tags]).to eq([ - { name: tag_sidebar_section_link.linkable.name, pm_only: false } - ]) + expect(json[:sidebar_tags]).to contain_exactly( + { name: tag.name, pm_only: false }, + { name: pm_tag.name, pm_only: true }, + { name: hidden_tag.name, pm_only: false } + ) end end describe '#sidebar_category_ids' do + fab!(:group) { Fabricate(:group) } fab!(:category) { Fabricate(:category) } fab!(:category_2) { Fabricate(:category) } - fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) } - let(:category_sidebar_section_link) { Fabricate(:category_sidebar_section_link, user: user) } - let(:category_sidebar_section_link_2) { Fabricate(:category_sidebar_section_link, user: user) } + fab!(:private_category) { Fabricate(:private_category, group: group) } + fab!(:category_sidebar_section_link) { Fabricate(:category_sidebar_section_link, user: user, linkable: category) } + fab!(:category_sidebar_section_link_2) { Fabricate(:category_sidebar_section_link, user: user, linkable: category_2) } + fab!(:category_sidebar_section_link_3) { Fabricate(:category_sidebar_section_link, user: user, linkable: private_category) } it "is not included when SiteSetting.enable_experimental_sidebar_hamburger is false" do category_sidebar_section_link @@ -307,7 +289,6 @@ RSpec.describe CurrentUserSerializer do end it "is not included when experimental sidebar has not been enabled" do - category_sidebar_section_link SiteSetting.enable_experimental_sidebar_hamburger = false json = serializer.as_json @@ -315,23 +296,25 @@ RSpec.describe CurrentUserSerializer do expect(json[:sidebar_category_ids]).to eq(nil) end - it 'includes visible default sidebar categories' do + it 'serializes only the categories that the user can see when experimental sidebar and tagging has been enabled"' do SiteSetting.enable_experimental_sidebar_hamburger = true - SiteSetting.default_sidebar_categories = "#{category.id}|#{category_2.id}|#{private_category.id}" json = serializer.as_json - expect(json[:sidebar_category_ids]).to eq([category.id, category_2.id]) - end - it 'includes categories choosen by user' do - SiteSetting.enable_experimental_sidebar_hamburger = true - SiteSetting.default_sidebar_categories = "#{category.id}|#{category_2.id}|#{private_category.id}" - - category_sidebar_section_link - category_sidebar_section_link_2 + expect(json[:sidebar_category_ids]).to eq([ + category.id, + category_2.id + ]) + group.add(user) + serializer = described_class.new(user, scope: Guardian.new(user), root: false) json = serializer.as_json - expect(json[:sidebar_category_ids]).to eq([category_sidebar_section_link.linkable.id, category_sidebar_section_link_2.linkable.id]) + + expect(json[:sidebar_category_ids]).to eq([ + category.id, + category_2.id, + private_category.id + ]) end end diff --git a/spec/serializers/site_serializer_spec.rb b/spec/serializers/site_serializer_spec.rb index 8f367ec7e82..15bef392a76 100644 --- a/spec/serializers/site_serializer_spec.rb +++ b/spec/serializers/site_serializer_spec.rb @@ -141,6 +141,16 @@ RSpec.describe SiteSerializer do describe '#anonymous_default_sidebar_tags' do fab!(:user) { Fabricate(:user) } + fab!(:tag) { Fabricate(:tag, name: 'dev') } + fab!(:tag2) { Fabricate(:tag, name: 'random') } + fab!(:hidden_tag) { Fabricate(:tag, name: "secret") } + fab!(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) } + + before do + SiteSetting.enable_experimental_sidebar_hamburger = true + SiteSetting.tagging_enabled = true + SiteSetting.default_sidebar_tags = "#{tag.name}|#{tag2.name}|#{hidden_tag.name}" + end it 'is not included in the serialised object when tagging is not enabled' do SiteSetting.tagging_enabled = false @@ -150,33 +160,30 @@ RSpec.describe SiteSerializer do expect(serialized[:anonymous_default_sidebar_tags]).to eq(nil) end - describe 'when tagging is enabled and default sidebar tags have been configured' do - fab!(:tag) { Fabricate(:tag, name: 'dev') } - fab!(:tag2) { Fabricate(:tag, name: 'random') } + it 'is not included in the serialised object when experimental sidebar has not been enabled' do + SiteSetting.enable_experimental_sidebar_hamburger = false - before do - SiteSetting.tagging_enabled = true - SiteSetting.default_sidebar_tags = "#{tag.name}|#{tag2.name}" - end + serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json + expect(serialized[:anonymous_default_sidebar_tags]).to eq(nil) + end - it 'is not included in the serialised object when user is not anonymous' do - guardian = Guardian.new(user) + it 'is not included in the serialised object when user is not anonymous' do + guardian = Guardian.new(user) - serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json - expect(serialized[:anonymous_default_sidebar_tags]).to eq(nil) - end + serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json + expect(serialized[:anonymous_default_sidebar_tags]).to eq(nil) + end - it 'is not included in the serialisd object when default sidebar tags have not been configured' do - SiteSetting.default_sidebar_tags = "" + it 'is not included in the serialisd object when default sidebar tags have not been configured' do + SiteSetting.default_sidebar_tags = "" - serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json - expect(serialized[:anonymous_default_sidebar_tags]).to eq(nil) - end + serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json + expect(serialized[:anonymous_default_sidebar_tags]).to eq(nil) + end - it 'is included in the serialised object when user is anonymous' do - serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json - expect(serialized[:anonymous_default_sidebar_tags]).to eq(["dev", "random"]) - end + it 'includes only tags user can see in the serialised object when user is anonymous' do + serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json + expect(serialized[:anonymous_default_sidebar_tags]).to eq(["dev", "random"]) end end diff --git a/spec/services/sidebar_site_settings_backfiller_spec.rb b/spec/services/sidebar_site_settings_backfiller_spec.rb new file mode 100644 index 00000000000..42b614e6aa3 --- /dev/null +++ b/spec/services/sidebar_site_settings_backfiller_spec.rb @@ -0,0 +1,235 @@ +# frozen_string_literal: true + +RSpec.describe SidebarSiteSettingsBackfiller do + fab!(:user) { Fabricate(:user) } + fab!(:user2) { Fabricate(:user) } + fab!(:user3) { Fabricate(:user) } + fab!(:staged_user) { Fabricate(:user, staged: true) } + fab!(:category) { Fabricate(:category) } + fab!(:category2) { Fabricate(:category) } + fab!(:category3) { Fabricate(:category) } + fab!(:user_category_sidebar_section_link) { Fabricate(:category_sidebar_section_link, user: user, linkable: category) } + fab!(:user2_category_sidebar_section_link) { Fabricate(:category_sidebar_section_link, user: user2, linkable: category) } + fab!(:user3_category2_sidebar_section_link) { Fabricate(:category_sidebar_section_link, user: user3, linkable: category2) } + + let!(:category_sidebar_section_link_ids) do + [ + user_category_sidebar_section_link.id, + user2_category_sidebar_section_link.id, + user3_category2_sidebar_section_link.id + ] + end + + fab!(:tag) { Fabricate(:tag) } + fab!(:tag2) { Fabricate(:tag) } + fab!(:tag3) { Fabricate(:tag) } + fab!(:user_tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user, linkable: tag) } + fab!(:user2_tag_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user2, linkable: tag) } + fab!(:user3_tag2_sidebar_section_link) { Fabricate(:tag_sidebar_section_link, user: user3, linkable: tag2) } + + let!(:tag_sidebar_section_link_ids) do + [ + user_tag_sidebar_section_link.id, + user2_tag_sidebar_section_link.id, + user3_tag2_sidebar_section_link.id + ] + end + + before do + # Clean up random users created as part of fabrication to make assertions easier to understand. + User.real.where("id NOT IN (?)", [user.id, user2.id, user3.id, staged_user.id]).delete_all + end + + it 'raises an error when class is initialized with invalid setting name' do + expect do + described_class.new('some_random_setting_name', previous_value: '', new_value: '') + end.to raise_error(RuntimeError, "Invalid setting_name") + end + + describe '#backfill!' do + context 'for default_sidebar_categories setting' do + it 'deletes the right sidebar section link records when categories are removed' do + backfiller = described_class.new( + "default_sidebar_categories", + previous_value: "#{category.id}|#{category2.id}|#{category3.id}", + new_value: "#{category3.id}" + ) + + expect do + backfiller.backfill! + end.to change { SidebarSectionLink.count }.by(-3) + + expect(SidebarSectionLink.exists?(id: category_sidebar_section_link_ids)).to eq(false) + end + + it 'creates the right sidebar section link records when categories are added' do + backfiller = described_class.new( + "default_sidebar_categories", + previous_value: "#{category.id}|#{category2.id}", + new_value: "#{category.id}|#{category2.id}|#{category3.id}" + ) + + expect do + backfiller.backfill! + end.to change { SidebarSectionLink.count }.by(3) + + expect(SidebarSectionLink.where(linkable_type: 'Category', linkable_id: category3.id).pluck(:user_id)).to contain_exactly( + user.id, + user2.id, + user3.id + ) + end + + it 'deletes and creates the right sidebar section link records when categories are added and removed' do + backfiller = described_class.new( + "default_sidebar_categories", + previous_value: "#{category.id}|#{category2.id}", + new_value: "#{category3.id}" + ) + + original_count = SidebarSectionLink.count + + expect do + backfiller.backfill! + end.to change { SidebarSectionLink.where(linkable_type: 'Category', linkable_id: category.id).count }.by(-2) + .and change { SidebarSectionLink.where(linkable_type: 'Category', linkable_id: category2.id).count }.by(-1) + .and change { SidebarSectionLink.where(linkable_type: 'Category', linkable_id: category3.id).count }.by(3) + + expect(SidebarSectionLink.count).to eq(original_count) # Net change of 0 + + expect(SidebarSectionLink.where(linkable_type: 'Category', linkable_id: category3.id).pluck(:user_id)).to contain_exactly( + user.id, + user2.id, + user3.id + ) + end + end + + context 'for default_sidebar_tags setting' do + it 'deletes the right sidebar section link records when tags are removed' do + backfiller = described_class.new( + "default_sidebar_tags", + previous_value: "#{tag.name}|#{tag2.name}|#{tag3.name}", + new_value: "#{tag3.name}" + ) + + expect do + backfiller.backfill! + end.to change { SidebarSectionLink.count }.by(-3) + + expect(SidebarSectionLink.exists?(id: tag_sidebar_section_link_ids)).to eq(false) + end + + it 'creates the right sidebar section link records when tags are added' do + backfiller = described_class.new( + "default_sidebar_tags", + previous_value: "#{tag.name}|#{tag2.name}", + new_value: "#{tag.name}|#{tag2.name}|#{tag3.name}" + ) + + expect do + backfiller.backfill! + end.to change { SidebarSectionLink.count }.by(3) + + expect(SidebarSectionLink.where(linkable_type: 'Tag', linkable_id: tag3.id).pluck(:user_id)).to contain_exactly( + user.id, + user2.id, + user3.id + ) + end + + it 'deletes and creates the right sidebar section link records when tags are added and removed' do + backfiller = described_class.new( + "default_sidebar_tags", + previous_value: "#{tag.name}|#{tag2.name}", + new_value: "#{tag3.name}" + ) + + original_count = SidebarSectionLink.count + + expect do + backfiller.backfill! + end.to change { SidebarSectionLink.where(linkable_type: 'Tag', linkable_id: tag.id).count }.by(-2) + .and change { SidebarSectionLink.where(linkable_type: 'Tag', linkable_id: tag2.id).count }.by(-1) + .and change { SidebarSectionLink.where(linkable_type: 'Tag', linkable_id: tag3.id).count }.by(3) + + expect(SidebarSectionLink.count).to eq(original_count) # net change of 0 + + expect(SidebarSectionLink.where(linkable_type: 'Tag', linkable_id: tag3.id).pluck(:user_id)).to contain_exactly( + user.id, + user2.id, + user3.id + ) + end + end + end + + describe '#number_of_users_to_backfill' do + context 'for default_sidebar_categories setting' do + it "returns 3 for the user count when a new category for all users is added" do + backfiller = described_class.new( + "default_sidebar_categories", + previous_value: "", + new_value: "#{category3.id}" + ) + + expect(backfiller.number_of_users_to_backfill).to eq(3) + end + + it "returns 2 for the user count when category which 2 users have configured in sidebar is removed" do + backfiller = described_class.new( + "default_sidebar_categories", + previous_value: "#{category.id}|#{category2.id}", + new_value: "#{category2.id}" + ) + + expect(backfiller.number_of_users_to_backfill).to eq(2) + end + + # category, category2 => category2, category3 + it "returns 3 for the user count when a new category is added and a category is removed" do + backfiller = described_class.new( + "default_sidebar_categories", + previous_value: "#{category.id}|#{category2.id}", + new_value: "#{category2.id}|#{category3.id}" + ) + + expect(backfiller.number_of_users_to_backfill).to eq(3) + end + end + + context 'for default_sidebar_tags setting' do + it "returns 3 for the user count when a new tag for all users is added" do + backfiller = described_class.new( + "default_sidebar_tags", + previous_value: "", + new_value: "#{tag3.name}" + ) + + expect(backfiller.number_of_users_to_backfill).to eq(3) + end + + # tag, tag2 => tag2 + it "returns 2 for the user count when tag which 2 users have configured in sidebar is removed" do + backfiller = described_class.new( + "default_sidebar_tags", + previous_value: "#{tag.name}|#{tag2.name}", + new_value: "#{tag2.name}" + ) + + expect(backfiller.number_of_users_to_backfill).to eq(2) + end + + # tag, tag2 => tag2, tag3 + it "returns 3 for the user count when a new tag is added and a tag is removed" do + backfiller = described_class.new( + "default_sidebar_tags", + previous_value: "#{tag.name}|#{tag2.name}", + new_value: "#{tag2.name}|#{tag3.name}" + ) + + expect(backfiller.number_of_users_to_backfill).to eq(3) + end + end + end +end