Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FIX: Seed all categories and tags configured as defaults for nav menu (#22793) 

Context of this change:

There are two site settings which an admin can configured to set the
default categories and tags that are shown for a new user. `default_navigation_menu_categories`
is used to determine the default categories while
`default_navigation_menu_tags` is used to determine the default tags.

Prior to this change when seeding the defaults, we will filter out the
categories/tags that the user do not have permission to see. However,
this means that when the user does eventually gain permission down the
line, the default categories and tags do not appear.

What does this change do?

With this commit, we have changed it such that all the categories and tags
configured in the `default_navigation_menu_categories` and
`default_navigation_menu_tags` site settings are seeded regardless of
whether the user's visibility of the categories or tags. During
serialization, we will then filter out the categories and tags which the
user does not have visibility of.
This commit is contained in:
Alan Guo Xiang Tan 2023-07-27 10:52:33 +08:00 committed by GitHub
parent f3db8579d6
commit 0a56274596
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 117 additions and 160 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

73
app/models/user.rb
View File

@ -137,13 +137,6 @@ class User < ActiveRecord::Base
belongs_to :uploaded_avatar, class_name: "Upload"
has_many :sidebar_section_links, dependent: :delete_all
has_many :category_sidebar_section_links,
-> { where(linkable_type: "Category") },
class_name: "SidebarSectionLink"
has_many :custom_sidebar_tags,
through: :sidebar_section_links,
source: :linkable,
source_type: "Tag"
delegate :last_sent_email_address, to: :email_logs
@ -170,11 +163,8 @@ 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 :add_default_sidebar_section_links
after_update :update_default_sidebar_section_links, if: Proc.new { self.saved_change_to_admin? }
after_update :add_default_sidebar_section_links, if: Proc.new { self.saved_change_to_staged? }
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? }
@ -360,9 +350,22 @@ class User < ActiveRecord::Base
)
end
def secured_sidebar_category_ids(user_guardian = nil)
user_guardian ||= guardian
SidebarSectionLink.where(user_id: self.id, linkable_type: "Category").pluck(:linkable_id) &
user_guardian.allowed_category_ids
end
def visible_sidebar_tags(user_guardian = nil)
user_guardian ||= guardian
DiscourseTagging.filter_visible(custom_sidebar_tags, user_guardian)
DiscourseTagging.filter_visible(
Tag.where(
id: SidebarSectionLink.where(user_id: self.id, linkable_type: "Tag").select(:linkable_id),
),
user_guardian,
)
end
def self.max_password_length
@ -2057,16 +2060,6 @@ class User < ActiveRecord::Base
if SiteSetting.default_navigation_menu_categories.present?
categories_to_update = SiteSetting.default_navigation_menu_categories.split("|")
if update
filtered_default_category_ids =
Category.secured(self.guardian).where(id: categories_to_update).pluck(:id)
existing_category_ids =
SidebarSectionLink.where(user: self, linkable_type: "Category").pluck(:linkable_id)
categories_to_update =
existing_category_ids + (filtered_default_category_ids & self.secure_category_ids)
end
SidebarSectionLinksUpdater.update_category_section_links(
self,
category_ids: categories_to_update,
@ -2074,37 +2067,11 @@ class User < ActiveRecord::Base
end
if SiteSetting.tagging_enabled && SiteSetting.default_navigation_menu_tags.present?
tags_to_update = SiteSetting.default_navigation_menu_tags.split("|")
if update
default_tag_ids = Tag.where(name: tags_to_update).pluck(:id)
filtered_default_tags =
DiscourseTagging
.filter_visible(Tag, self.guardian)
.where(id: default_tag_ids)
.pluck(:name)
existing_tag_ids =
SidebarSectionLink.where(user: self, linkable_type: "Tag").pluck(:linkable_id)
existing_tags =
DiscourseTagging
.filter_visible(Tag, self.guardian)
.where(id: existing_tag_ids)
.pluck(:name)
tags_to_update = existing_tags + (filtered_default_tags & DiscourseTagging.hidden_tag_names)
SidebarSectionLinksUpdater.update_tag_section_links(
self,
tag_ids: Tag.where(name: SiteSetting.default_navigation_menu_tags.split("|")).pluck(:id),
)
end
SidebarSectionLinksUpdater.update_tag_section_links(self, tag_names: tags_to_update)
end
end
def add_default_sidebar_section_links
set_default_sidebar_section_links
end
def update_default_sidebar_section_links
set_default_sidebar_section_links(update: true)
end
def stat

2
app/serializers/concerns/user_sidebar_mixin.rb
View File

@ -20,7 +20,7 @@ module UserSidebarMixin
end
def sidebar_category_ids
object.category_sidebar_section_links.pluck(:linkable_id) & scope.allowed_category_ids
object.secured_sidebar_category_ids(scope)
end
def include_sidebar_category_ids?

10
app/services/sidebar_section_links_updater.rb
View File

@ -5,18 +5,15 @@ class SidebarSectionLinksUpdater
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)
category_ids = Category.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?
def self.update_tag_section_links(user, tag_ids:)
if tag_ids.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
@ -46,6 +43,7 @@ class SidebarSectionLinksUpdater
linkable_id: to_delete,
).delete_all
end
SidebarSectionLink.insert_all(to_insert_attributes) if to_insert_attributes.present?
end
end

17
app/services/user_updater.rb
View File

@ -65,6 +65,7 @@ class UserUpdater
def initialize(actor, user)
@user = user
@user_guardian = Guardian.new(user)
@guardian = Guardian.new(actor)
@actor = actor
end
@ -153,10 +154,10 @@ class UserUpdater
# special handling for theme_id cause we need to bump a sequence number
if attributes.key?(:theme_ids)
user_guardian = Guardian.new(user)
attributes[:theme_ids].reject!(&:blank?)
attributes[:theme_ids].map!(&:to_i)
if user_guardian.allow_themes?(attributes[:theme_ids])
if @user_guardian.allow_themes?(attributes[:theme_ids])
user.user_option.theme_key_seq += 1 if user.user_option.theme_ids != attributes[:theme_ids]
else
attributes.delete(:theme_ids)
@ -211,14 +212,22 @@ class UserUpdater
if attributes.key?(:sidebar_category_ids)
SidebarSectionLinksUpdater.update_category_section_links(
user,
category_ids: attributes[:sidebar_category_ids],
category_ids:
Category
.secured(@user_guardian)
.where(id: attributes[:sidebar_category_ids])
.pluck(:id),
)
end
if attributes.key?(:sidebar_tag_names) && SiteSetting.tagging_enabled
SidebarSectionLinksUpdater.update_tag_section_links(
user,
tag_names: attributes[:sidebar_tag_names],
tag_ids:
DiscourseTagging
.filter_visible(Tag, @user_guardian)
.where(name: attributes[:sidebar_tag_names])
.pluck(:id),
)
end

41
spec/lib/sidebar_section_links_updater_spec.rb
View File

@ -7,12 +7,11 @@ RSpec.describe SidebarSectionLinksUpdater do
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) do
Fabricate(:category_sidebar_section_link, linkable: public_category, user: user)
end
fab!(:user2_category_section_link) do
Fabricate(:category_sidebar_section_link, linkable: public_category, user: user2)
end
@ -24,71 +23,49 @@ RSpec.describe SidebarSectionLinksUpdater do
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
it "updates user's sidebar category section link records to given category ids" 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],
category_ids: [public_category.id, public_category2.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)
).to contain_exactly(public_category.id, public_category2.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) do
Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name])
end
fab!(:user_tag_section_link) { Fabricate(:tag_sidebar_section_link, linkable: tag, user: user) }
fab!(:user2_tag_section_link) do
Fabricate(:tag_sidebar_section_link, linkable: tag, user: user2)
end
it "deletes all sidebar tag section links when tag names provided is blank" do
described_class.update_tag_section_links(user, tag_names: [])
described_class.update_tag_section_links(user, tag_ids: [])
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
it "updates user's sidebar tag section link records to given tag names" 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])
described_class.update_tag_section_links(user, tag_ids: [tag.id, tag2.id])
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)
).to contain_exactly(tag.id, tag2.id)
end
end
end

85
spec/models/user_spec.rb
View File

@ -52,76 +52,18 @@ RSpec.describe User do
SiteSetting.default_navigation_menu_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
it "creates sidebar section link records for categories and tags that have been configured as defaults" 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)
admin = Fabricate(:admin)
expect(
SidebarSectionLink.where(linkable_type: "Category", user_id: admin.id).pluck(
:linkable_id,
),
).to contain_exactly(category.id, secured_category.id)
expect(
SidebarSectionLink.where(linkable_type: "Tag", user_id: admin.id).pluck(:linkable_id),
SidebarSectionLink.where(linkable_type: "Tag", user_id: user.id).pluck(:linkable_id),
).to contain_exactly(tag.id, hidden_tag.id)
end
it "should create and remove the right sidebar section link records when user is promoted/demoted as an admin" do
user = Fabricate(:user)
another_category = Fabricate(:category)
another_tag = Fabricate(:tag)
# User has customized their sidebar categories and tags
SidebarSectionLink.where(user: user).delete_all
SidebarSectionLinksUpdater.update_category_section_links(
user,
category_ids: [another_category.id],
)
SidebarSectionLinksUpdater.update_tag_section_links(user, tag_names: [another_tag.name])
# A user promoted to admin now has any default categories/tags they didn't previously have access to
user.update(admin: true)
expect(
SidebarSectionLink.where(linkable_type: "Category", user_id: user.id).pluck(:linkable_id),
).to contain_exactly(another_category.id, secured_category.id)
expect(
SidebarSectionLink.where(linkable_type: "Tag", user_id: user.id).pluck(:linkable_id),
).to contain_exactly(another_tag.id, hidden_tag.id)
# User still has their customized sidebar categories and tags after demotion
user.update(admin: false)
expect(
SidebarSectionLink.where(linkable_type: "Category", user_id: user.id).pluck(:linkable_id),
).to contain_exactly(another_category.id)
expect(
SidebarSectionLink.where(linkable_type: "Tag", user_id: user.id).pluck(:linkable_id),
).to contain_exactly(another_tag.id)
end
it "should not receive any new categories w/ suppress secured categories from admin enabled" do
SiteSetting.suppress_secured_categories_from_admin = true
user = Fabricate(:user)
SidebarSectionLink.where(user: user).delete_all # User has customized their sidebar categories
user.update(admin: true)
expect(
SidebarSectionLink.where(linkable_type: "Category", user_id: user.id).pluck(:linkable_id),
).to be_empty
user.update(admin: false)
expect(
SidebarSectionLink.where(linkable_type: "Category", user_id: user.id).pluck(:linkable_id),
).to be_empty
end
it "should not create any sidebar section link records when navigation_menu site setting is still legacy" do
SiteSetting.navigation_menu = "legacy"
@ -3372,6 +3314,29 @@ RSpec.describe User do
end
end
describe "#secured_sidebar_category_ids" do
fab!(:user) { Fabricate(:user) }
fab!(:category) { Fabricate(:category) }
fab!(:group) { Fabricate(:group) }
fab!(:secured_category) { Fabricate(:private_category, group: group) }
fab!(:category_sidebar_section_link) do
Fabricate(:category_sidebar_section_link, user: user, linkable: category)
end
fab!(:secured_category_sidebar_section_link) do
Fabricate(:category_sidebar_section_link, user: user, linkable: secured_category)
end
it "should only return the category ids of category sidebar section link records that the user is allowed to see" do
expect(user.secured_sidebar_category_ids).to contain_exactly(category.id)
user.update!(admin: true)
expect(user.secured_sidebar_category_ids).to contain_exactly(category.id, secured_category.id)
end
end
describe "#visible_sidebar_tags" do
fab!(:user) { Fabricate(:user) }
fab!(:tag) { Fabricate(:tag) }

40
spec/requests/users_controller_spec.rb
View File

@ -2581,9 +2581,10 @@ RSpec.describe UsersController do
end.to change { user.sidebar_section_links.count }.from(1).to(0)
end
it "should allow user to modify category sidebar section links" do
it "should allow user to only modify category sidebar section links for categories they have access to" do
category = Fabricate(:category)
restricted_category = Fabricate(:category, read_restricted: true)
group = Fabricate(:group)
restricted_category = Fabricate(:private_category, group: group)
category_sidebar_section_link = Fabricate(:category_sidebar_section_link, user: user)
put "/u/#{user.username}.json",
@ -2598,6 +2599,21 @@ RSpec.describe UsersController do
sidebar_section_link = user.sidebar_section_links.first
expect(sidebar_section_link.linkable).to eq(category)
group.add(user)
expect do
put "/u/#{user.username}.json",
params: {
sidebar_category_ids: [category.id, restricted_category.id],
}
expect(response.status).to eq(200)
end.to change { user.sidebar_section_links.count }.from(1).to(2)
expect(SidebarSectionLink.exists?(user: user, linkable: restricted_category)).to eq(
true,
)
end
it "should allow user to remove all tag sidebar section links" do
@ -2623,15 +2639,18 @@ RSpec.describe UsersController do
expect(user.reload.sidebar_section_links.count).to eq(0)
end
it "should allow user to add tag sidebar section links" do
it "should allow user to add tag sidebar section links only for tags that are visible to the user" do
SiteSetting.tagging_enabled = true
tag = Fabricate(:tag)
tag_sidebar_section_link = Fabricate(:tag_sidebar_section_link, user: user)
hidden_tag = Fabricate(:tag)
Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name])
put "/u/#{user.username}.json",
params: {
sidebar_tag_names: [tag.name, "somerandomtag"],
sidebar_tag_names: [tag.name, "somerandomtag", hidden_tag.name],
}
expect(response.status).to eq(200)
@ -2641,6 +2660,19 @@ RSpec.describe UsersController do
sidebar_section_link = user.sidebar_section_links.first
expect(sidebar_section_link.linkable).to eq(tag)
user.update!(admin: true)
expect do
put "/u/#{user.username}.json",
params: {
sidebar_tag_names: [tag.name, "somerandomtag", hidden_tag.name],
}
expect(response.status).to eq(200)
end.to change { user.sidebar_section_links.count }.from(1).to(2)
expect(SidebarSectionLink.exists?(user: user, linkable: hidden_tag)).to eq(true)
end
end
end

9
spec/support/user_sidebar_serializer_attributes.rb
View File

@ -12,12 +12,15 @@ RSpec.shared_examples "User Sidebar Serializer Attributes" do |serializer_klass|
fab!(:category) { Fabricate(:category) }
fab!(:category_2) { Fabricate(:category) }
fab!(:private_category) { Fabricate(:private_category, group: group) }
fab!(:category_sidebar_section_link) do
Fabricate(:category_sidebar_section_link, user: user, linkable: category)
end
fab!(:category_sidebar_section_link_2) do
Fabricate(:category_sidebar_section_link, user: user, linkable: category_2)
end
fab!(:category_sidebar_section_link_3) do
Fabricate(:category_sidebar_section_link, user: user, linkable: private_category)
end
@ -47,19 +50,25 @@ RSpec.shared_examples "User Sidebar Serializer Attributes" do |serializer_klass|
describe "#sidebar_tags" do
fab!(:tag) { Fabricate(:tag, name: "foo", description: "foo tag") }
fab!(:pm_tag) do
Fabricate(:tag, name: "bar", pm_topic_count: 5, staff_topic_count: 0, public_topic_count: 0)
end
fab!(:hidden_tag) { Fabricate(:tag, name: "secret") }
fab!(:staff_tag_group) do
Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: ["secret"])
end
fab!(:tag_sidebar_section_link) do
Fabricate(:tag_sidebar_section_link, user: user, linkable: tag)
end
fab!(:tag_sidebar_section_link_2) do
Fabricate(:tag_sidebar_section_link, user: user, linkable: pm_tag)
end
fab!(:tag_sidebar_section_link_3) do
Fabricate(:tag_sidebar_section_link, user: user, linkable: hidden_tag)
end