DEV: Centralise logic for updating sidebar section links (#19275)

The centralization helps in reducing code duplication in our code base
and more importantly, centralizing logic for guardian checks into a
single spot.

app/models/user.rb

@@ -1950,39 +1950,19 @@ class User < ActiveRecord::Base
     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("|")
+      SidebarSectionLinksUpdater.update_category_section_links(
-
+        self,
-      # Filters out categories that user does not have access to or do not exist anymore
+        category_ids: SiteSetting.default_sidebar_categories.split("|")
-      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("|")
+      SidebarSectionLinksUpdater.update_tag_section_links(
-
+        self,
-      # Filters out tags that user cannot see or do not exist anymore
+        tag_names: SiteSetting.default_sidebar_tags.split("|")
-      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

app/services/sidebar_section_links_updater.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+class SidebarSectionLinksUpdater
+  def self.update_category_section_links(user, category_ids:)
+    if category_ids.blank?
+      delete_section_links(user: user, linkable_type: 'Category')
+    else
+      category_ids = Category.secured(Guardian.new(user)).where(id: category_ids).pluck(:id)
+      update_section_links(user: user, linkable_type: 'Category', new_linkable_ids: category_ids)
+    end
+  end
+
+  def self.update_tag_section_links(user, tag_names:)
+    if tag_names.blank?
+      delete_section_links(user: user, linkable_type: 'Tag')
+    else
+      tag_ids = DiscourseTagging
+        .filter_visible(Tag, Guardian.new(user))
+        .where(name: tag_names)
+        .pluck(:id)
+
+      update_section_links(user: user, linkable_type: 'Tag', new_linkable_ids: tag_ids)
+    end
+  end
+
+  def self.delete_section_links(user:, linkable_type:)
+    SidebarSectionLink.where(user: user, linkable_type: linkable_type).delete_all
+  end
+  private_class_method :delete_section_links
+
+  def self.update_section_links(user:, linkable_type:, new_linkable_ids:)
+    SidebarSectionLink.transaction do
+      existing_linkable_ids = SidebarSectionLink.where(user: user, linkable_type: linkable_type).pluck(:linkable_id)
+
+      to_delete = existing_linkable_ids - new_linkable_ids
+      to_insert = new_linkable_ids - existing_linkable_ids
+
+      to_insert_attributes = to_insert.map do |linkable_id|
+        {
+          linkable_type: linkable_type,
+          linkable_id: linkable_id,
+          user_id: user.id
+        }
+      end
+
+      SidebarSectionLink.where(user: user, linkable_type: linkable_type, linkable_id: to_delete).delete_all if to_delete.present?
+      SidebarSectionLink.insert_all(to_insert_attributes) if to_insert_attributes.present?
+    end
+  end
+  private_class_method :update_section_links
+end

app/services/user_updater.rb

@@ -212,11 +212,11 @@ class UserUpdater
       end
 
       if attributes.key?(:sidebar_category_ids)
-        update_sidebar_category_section_links(attributes[:sidebar_category_ids])
+        SidebarSectionLinksUpdater.update_category_section_links(user, category_ids: attributes[:sidebar_category_ids])
       end
 
       if attributes.key?(:sidebar_tag_names) && SiteSetting.tagging_enabled
-        update_sidebar_tag_section_links(attributes[:sidebar_tag_names])
+        SidebarSectionLinksUpdater.update_tag_section_links(user, tag_names: attributes[:sidebar_tag_names])
       end
 
       if SiteSetting.enable_user_status?
@@ -315,48 +315,6 @@ class UserUpdater
 
   private
 
-  def delete_all_sidebar_section_links(linkable_type)
-    SidebarSectionLink.where(user: user, linkable_type: linkable_type).delete_all
-  end
-
-  def update_sidebar_section_links(linkable_type, new_linkable_ids)
-    if new_linkable_ids.blank?
-      SidebarSectionLink.where(user: user, linkable_type: linkable_type).delete_all
-    else
-      existing_linkable_ids = SidebarSectionLink.where(user: user, linkable_type: linkable_type).pluck(:linkable_id)
-
-      to_delete = existing_linkable_ids - new_linkable_ids
-      to_insert = new_linkable_ids - existing_linkable_ids
-
-      to_insert_attributes = to_insert.map do |linkable_id|
-        {
-          linkable_type: linkable_type,
-          linkable_id: linkable_id,
-          user_id: user.id
-        }
-      end
-
-      SidebarSectionLink.where(user: user, linkable_type: linkable_type, linkable_id: to_delete).delete_all if to_delete.present?
-      SidebarSectionLink.insert_all(to_insert_attributes) if to_insert_attributes.present?
-    end
-  end
-
-  def update_sidebar_tag_section_links(tag_names)
-    if tag_names.blank?
-      delete_all_sidebar_section_links('Tag')
-    else
-      update_sidebar_section_links('Tag', Tag.where(name: tag_names).pluck(:id))
-    end
-  end
-
-  def update_sidebar_category_section_links(category_ids)
-    if category_ids.blank?
-      delete_all_sidebar_section_links('Category')
-    else
-      update_sidebar_section_links('Category', Category.secured(guardian).where(id: category_ids).pluck(:id))
-    end
-  end
-
   def update_user_status(status)
     if status.blank?
       @user.clear_status!

spec/lib/sidebar_section_links_updater_spec.rb

@@ -0,0 +1,82 @@
+# frozen_string_literal: true
+
+RSpec.describe SidebarSectionLinksUpdater do
+  fab!(:user) { Fabricate(:user) }
+  fab!(:user2) { Fabricate(:user) }
+
+  describe '.update_category_section_links' do
+    fab!(:public_category) { Fabricate(:category) }
+    fab!(:public_category2) { Fabricate(:category) }
+    fab!(:group) { Fabricate(:group) }
+    fab!(:secured_category) { Fabricate(:private_category, group: group) }
+
+    fab!(:user_category_section_link) { Fabricate(:category_sidebar_section_link, linkable: public_category, user: user) }
+    fab!(:user2_category_section_link) { Fabricate(:category_sidebar_section_link, linkable: public_category, user: user2) }
+
+    it 'deletes all sidebar category section links when category ids provided is blank' do
+      described_class.update_category_section_links(user, category_ids: [])
+
+      expect(SidebarSectionLink.exists?(linkable: public_category, user: user)).to eq(false)
+      expect(SidebarSectionLink.exists?(linkable: public_category, user: user2)).to eq(true)
+    end
+
+    it "updates user's sidebar category section link records to given category ids except for category restricted to user" do
+      expect(SidebarSectionLink.where(linkable_type: 'Category', user: user).pluck(:linkable_id)).to contain_exactly(
+        public_category.id
+      )
+
+      described_class.update_category_section_links(user, category_ids: [public_category2.id, secured_category.id])
+
+      expect(SidebarSectionLink.where(linkable_type: 'Category', user: user).pluck(:linkable_id)).to contain_exactly(
+        public_category2.id
+      )
+
+      group.add(user)
+
+      described_class.update_category_section_links(user, category_ids: [public_category2.id, secured_category.id])
+
+      expect(SidebarSectionLink.where(linkable_type: 'Category', user: user).pluck(:linkable_id)).to contain_exactly(
+        public_category2.id,
+        secured_category.id
+      )
+    end
+  end
+
+  describe '.update_tag_section_links' do
+    fab!(:tag) { Fabricate(:tag) }
+    fab!(:tag2) { Fabricate(:tag) }
+    fab!(:hidden_tag) { Fabricate(:tag) }
+    fab!(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) }
+
+    fab!(:user_tag_section_link) { Fabricate(:tag_sidebar_section_link, linkable: tag, user: user) }
+    fab!(:user2_tag_section_link) { Fabricate(:tag_sidebar_section_link, linkable: tag, user: user2) }
+
+    it 'deletes all sidebar tag section links when tag names provided is blank' do
+      described_class.update_tag_section_links(user, tag_names: [])
+
+      expect(SidebarSectionLink.exists?(linkable: tag, user: user)).to eq(false)
+      expect(SidebarSectionLink.exists?(linkable: tag, user: user2)).to eq(true)
+    end
+
+    it "updates user's sidebar tag section link records to given tag names except for tags not visible to user" do
+      expect(SidebarSectionLink.where(linkable_type: 'Tag', user: user).pluck(:linkable_id)).to contain_exactly(
+        tag.id
+      )
+
+      described_class.update_tag_section_links(user, tag_names: [tag2.name, hidden_tag.name])
+
+      expect(SidebarSectionLink.where(linkable_type: 'Tag', user: user).pluck(:linkable_id)).to contain_exactly(
+        tag2.id
+      )
+
+      user.update!(admin: true)
+
+      described_class.update_tag_section_links(user, tag_names: [tag2.name, hidden_tag.name])
+
+      expect(SidebarSectionLink.where(linkable_type: 'Tag', user: user).pluck(:linkable_id)).to contain_exactly(
+        tag2.id,
+        hidden_tag.id
+      )
+    end
+  end
+end