From 1b56a55f5087091a8573849a42c78f63992e1142 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 27 Oct 2022 06:38:50 +0800 Subject: [PATCH] DEV: Sidebar default tags and categories are determined at user creation (#18620) The previous sidebar default tags and categories implementation did not allow for a user to configure their sidebar to have no categories or tags. This commit changes how the defaults are applied. When a user is being created, we create the SidebarSectionLink records based on the `default_sidebar_categories` and `default_sidebar_tags` site settings. SidebarSectionLink records are only created for categories and tags which the user has visibility on at the point of user creation. With this change, we're also adding the ability for admins to apply changes to the `default_sidebar_categories` and `default_sidebar_tags` site settings historically when changing their site setting. When a new category/tag has been added to the default, the new category/tag will be added to the sidebar for all users if the admin elects to apply the changes historically. Like wise when a tag/category is removed, the tag/category will be removed from the sidebar for all users if the admin elects to apply the changes historically. Internal Ref: /t/73500 --- .../admin/addon/mixins/setting-component.js | 3 + .../admin/site_settings_controller.rb | 10 + .../regular/backfill_sidebar_site_settings.rb | 9 + app/models/user.rb | 70 ++++-- .../concerns/user_sidebar_tags_mixin.rb | 14 +- app/serializers/current_user_serializer.rb | 2 +- app/serializers/sidebar/tag_serializer.rb | 11 - app/serializers/site_serializer.rb | 4 +- .../sidebar_site_settings_backfiller.rb | 93 +++++++ spec/models/user_spec.rb | 101 ++++++++ .../admin/site_settings_controller_spec.rb | 62 +++++ .../current_user_serializer_spec.rb | 81 +++--- spec/serializers/site_serializer_spec.rb | 49 ++-- .../sidebar_site_settings_backfiller_spec.rb | 235 ++++++++++++++++++ 14 files changed, 637 insertions(+), 107 deletions(-) create mode 100644 app/jobs/regular/backfill_sidebar_site_settings.rb delete mode 100644 app/serializers/sidebar/tag_serializer.rb create mode 100644 app/services/sidebar_site_settings_backfiller.rb create mode 100644 spec/services/sidebar_site_settings_backfiller_spec.rb 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