From f122f24b35a8116e47649ae139910d44e3d7488e Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 20 Jan 2023 09:50:24 +0800 Subject: [PATCH] SECURITY: Default tags to show count of topics in unrestricted categories (#19916) Currently, `Tag#topic_count` is a count of all regular topics regardless of whether the topic is in a read restricted category or not. As a result, any users can technically poll a sensitive tag to determine if a new topic is created in a category which the user has not excess to. We classify this as a minor leak in sensitive information. The following changes are introduced in this commit: 1. Introduce `Tag#public_topic_count` which only count topics which have been tagged with a given tag in public categories. 2. Rename `Tag#topic_count` to `Tag#staff_topic_count` which counts the same way as `Tag#topic_count`. In other words, it counts all topics tagged with a given tag regardless of the category the topic is in. The rename is also done so that we indicate that this column contains sensitive information. 3. Change all previous spots which relied on `Topic#topic_count` to rely on `Tag.topic_column_count(guardian)` which will return the right "topic count" column to use based on the current scope. 4. Introduce `SiteSetting.include_secure_categories_in_tag_counts` site setting to allow site administrators to always display the tag topics count using `Tag#staff_topic_count` instead. --- app/controllers/tags_controller.rb | 27 ++--- app/models/tag.rb | 69 +++++++++--- app/models/topic.rb | 13 +++ app/models/topic_tag.rb | 31 +++-- app/serializers/concerns/topic_tags_mixin.rb | 3 +- .../concerns/user_sidebar_mixin.rb | 4 +- app/serializers/detailed_tag_serializer.rb | 2 +- app/serializers/tag_serializer.rb | 4 + app/services/tag_hashtag_data_source.rb | 24 ++-- config/locales/server.en.yml | 1 + config/site_settings.yml | 2 + ...18020114_add_public_topic_count_to_tags.rb | 28 +++++ ...119000943_add_staff_topic_count_to_tags.rb | 27 +++++ ...0119024157_remove_topic_count_from_tags.rb | 15 +++ lib/discourse_tagging.rb | 12 +- lib/topic_view.rb | 3 +- spec/integration/tag_counts_spec.rb | 104 +++++++++++++++++ spec/lib/discourse_tagging_spec.rb | 14 ++- spec/lib/topic_view_spec.rb | 4 +- spec/models/tag_spec.rb | 106 ++++++++++++++++-- spec/models/topic_tag_spec.rb | 39 +++++-- spec/requests/tags_controller_spec.rb | 98 +++++++++++----- spec/serializers/tag_serializer_spec.rb | 25 +++++ .../serializers/topic_view_serializer_spec.rb | 30 ++++- .../hashtag_autocomplete_service_spec.rb | 20 ++-- spec/services/tag_hashtag_data_source_spec.rb | 16 +-- .../user_sidebar_serializer_attributes.rb | 4 +- spec/system/hashtag_autocomplete_spec.rb | 4 +- 28 files changed, 602 insertions(+), 127 deletions(-) create mode 100644 db/migrate/20230118020114_add_public_topic_count_to_tags.rb create mode 100644 db/migrate/20230119000943_add_staff_topic_count_to_tags.rb create mode 100644 db/post_migrate/20230119024157_remove_topic_count_from_tags.rb create mode 100644 spec/integration/tag_counts_spec.rb create mode 100644 spec/serializers/tag_serializer_spec.rb diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 5a8a8137b78..1108b618cc7 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -40,7 +40,7 @@ class TagsController < ::ApplicationController if SiteSetting.tags_listed_by_group ungrouped_tags = Tag.where("tags.id NOT IN (SELECT tag_id FROM tag_group_memberships)") - ungrouped_tags = ungrouped_tags.where("tags.topic_count > 0") unless show_all_tags + ungrouped_tags = ungrouped_tags.used_tags_in_regular_topics(guardian) unless show_all_tags grouped_tag_counts = TagGroup @@ -51,18 +51,14 @@ class TagsController < ::ApplicationController { id: tag_group.id, name: tag_group.name, - tags: - self.class.tag_counts_json( - tag_group.none_synonym_tags, - show_pm_tags: guardian.can_tag_pms?, - ), + tags: self.class.tag_counts_json(tag_group.none_synonym_tags, guardian), } end - @tags = self.class.tag_counts_json(ungrouped_tags, show_pm_tags: guardian.can_tag_pms?) + @tags = self.class.tag_counts_json(ungrouped_tags, guardian) @extras = { tag_groups: grouped_tag_counts } else - tags = show_all_tags ? Tag.all : Tag.where("tags.topic_count > 0") + tags = show_all_tags ? Tag.all : Tag.used_tags_in_regular_topics(guardian) unrestricted_tags = DiscourseTagging.filter_visible(tags.where(target_tag_id: nil), guardian) categories = @@ -77,13 +73,14 @@ class TagsController < ::ApplicationController category_tags = self.class.tag_counts_json( DiscourseTagging.filter_visible(c.tags.where(target_tag_id: nil), guardian), + guardian, ) next if category_tags.empty? { id: c.id, tags: category_tags } end .compact - @tags = self.class.tag_counts_json(unrestricted_tags, show_pm_tags: guardian.can_tag_pms?) + @tags = self.class.tag_counts_json(unrestricted_tags, guardian) @extras = { categories: category_tag_counts } end @@ -264,7 +261,7 @@ class TagsController < ::ApplicationController tags_with_counts, filter_result_context = DiscourseTagging.filter_allowed_tags(guardian, **filter_params, with_context: true) - tags = self.class.tag_counts_json(tags_with_counts, show_pm_tags: guardian.can_tag_pms?) + tags = self.class.tag_counts_json(tags_with_counts, guardian) json_response = { results: tags } @@ -388,18 +385,22 @@ class TagsController < ::ApplicationController end end - def self.tag_counts_json(tags, show_pm_tags: true) + def self.tag_counts_json(tags, guardian) + show_pm_tags = guardian.can_tag_pms? target_tags = Tag.where(id: tags.map(&:target_tag_id).compact.uniq).select(:id, :name) + tags .map do |t| - next if t.topic_count == 0 && t.pm_topic_count > 0 && !show_pm_tags + topic_count = t.public_send(Tag.topic_count_column(guardian)) + + next if topic_count == 0 && t.pm_topic_count > 0 && !show_pm_tags { id: t.name, text: t.name, name: t.name, description: t.description, - count: t.topic_count, + count: topic_count, pm_count: show_pm_tags ? t.pm_topic_count : 0, target_tag: t.target_tag_id ? target_tags.find { |x| x.id == t.target_tag_id }&.name : nil, diff --git a/app/models/tag.rb b/app/models/tag.rb index 64b932d02ed..71435ef046f 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -4,6 +4,10 @@ class Tag < ActiveRecord::Base include Searchable include HasDestroyedWebHook + self.ignored_columns = [ + "topic_count", # TODO(tgxworld): Remove on 1 July 2023 + ] + RESERVED_TAGS = [ "none", "constructor", # prevents issues with javascript's constructor of objects @@ -25,11 +29,14 @@ class Tag < ActiveRecord::Base # tags that have never been used and don't belong to a tag group scope :unused, -> { - where(topic_count: 0, pm_topic_count: 0).joins( + where(staff_topic_count: 0, pm_topic_count: 0).joins( "LEFT JOIN tag_group_memberships tgm ON tags.id = tgm.tag_id", ).where("tgm.tag_id IS NULL") } + scope :used_tags_in_regular_topics, + ->(guardian) { where("tags.#{Tag.topic_count_column(guardian)} > 0") } + scope :base_tags, -> { where(target_tag_id: nil) } has_many :tag_users, dependent: :destroy # notification settings @@ -62,7 +69,7 @@ class Tag < ActiveRecord::Base def self.update_topic_counts DB.exec <<~SQL UPDATE tags t - SET topic_count = x.topic_count + SET staff_topic_count = x.topic_count FROM ( SELECT COUNT(topics.id) AS topic_count, tags.id AS tag_id FROM tags @@ -73,7 +80,31 @@ class Tag < ActiveRecord::Base GROUP BY tags.id ) x WHERE x.tag_id = t.id - AND x.topic_count <> t.topic_count + AND x.topic_count <> t.staff_topic_count + SQL + + DB.exec <<~SQL + UPDATE tags t + SET public_topic_count = x.topic_count + FROM ( + WITH tags_with_public_topics AS ( + SELECT + COUNT(topics.id) AS topic_count, + tags.id AS tag_id + FROM tags + INNER JOIN topic_tags ON tags.id = topic_tags.tag_id + INNER JOIN topics ON topics.id = topic_tags.topic_id AND topics.deleted_at IS NULL AND topics.archetype != 'private_message' + INNER JOIN categories ON categories.id = topics.category_id AND NOT categories.read_restricted + GROUP BY tags.id + ) + SELECT + COALESCE(tags_with_public_topics.topic_count, 0 ) AS topic_count, + tags.id AS tag_id + FROM tags + LEFT JOIN tags_with_public_topics ON tags_with_public_topics.tag_id = tags.id + ) x + WHERE x.tag_id = t.id + AND x.topic_count <> t.public_topic_count; SQL DB.exec <<~SQL @@ -97,19 +128,18 @@ class Tag < ActiveRecord::Base self.find_by("lower(name) = ?", name.downcase) end - def self.top_tags(limit_arg: nil, category: nil, guardian: nil) + def self.top_tags(limit_arg: nil, category: nil, guardian: Guardian.new) # we add 1 to max_tags_in_filter_list to efficiently know we have more tags # than the limit. Frontend is responsible to enforce limit. limit = limit_arg || (SiteSetting.max_tags_in_filter_list + 1) - scope_category_ids = (guardian || Guardian.new).allowed_category_ids - + scope_category_ids = guardian.allowed_category_ids scope_category_ids &= ([category.id] + category.subcategories.pluck(:id)) if category return [] if scope_category_ids.empty? filter_sql = ( - if guardian&.is_staff? + if guardian.is_staff? "" else " AND tags.id IN (#{DiscourseTagging.visible_tags(guardian).select(:id).to_sql})" @@ -130,6 +160,14 @@ class Tag < ActiveRecord::Base tag_names_with_counts.map { |row| row.tag_name } end + def self.topic_count_column(guardian) + if guardian&.is_staff? || SiteSetting.include_secure_categories_in_tag_counts + "staff_topic_count" + else + "public_topic_count" + end + end + def self.pm_tags(limit: 1000, guardian: nil, allowed_user: nil) return [] if allowed_user.blank? || !(guardian || Guardian.new).can_tag_pms? user_id = allowed_user.id @@ -214,14 +252,15 @@ end # # Table name: tags # -# id :integer not null, primary key -# name :string not null -# topic_count :integer default(0), not null -# created_at :datetime not null -# updated_at :datetime not null -# pm_topic_count :integer default(0), not null -# target_tag_id :integer -# description :string +# id :integer not null, primary key +# name :string not null +# created_at :datetime not null +# updated_at :datetime not null +# pm_topic_count :integer default(0), not null +# target_tag_id :integer +# description :string +# public_topic_count :integer default(0), not null +# staff_topic_count :integer default(0), not null # # Indexes # diff --git a/app/models/topic.rb b/app/models/topic.rb index b15bbc519f6..10a0e7a5386 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -958,6 +958,7 @@ class Topic < ActiveRecord::Base def changed_to_category(new_category) return true if new_category.blank? || Category.exists?(topic_id: id) + if new_category.id == SiteSetting.uncategorized_category_id && !SiteSetting.allow_uncategorized_topics return false @@ -971,6 +972,15 @@ class Topic < ActiveRecord::Base if old_category Category.where(id: old_category.id).update_all("topic_count = topic_count - 1") + + count = + if old_category.read_restricted && !new_category.read_restricted + 1 + elsif !old_category.read_restricted && new_category.read_restricted + -1 + end + + Tag.update_counters(self.tags, { public_topic_count: count }) if count end # when a topic changes category we may have to start watching it @@ -1781,12 +1791,15 @@ class Topic < ActiveRecord::Base def convert_to_public_topic(user, category_id: nil) public_topic = TopicConverter.new(self, user).convert_to_public_topic(category_id) + Tag.update_counters(public_topic.tags, { public_topic_count: 1 }) if !category.read_restricted add_small_action(user, "public_topic") if public_topic public_topic end def convert_to_private_message(user) + read_restricted = category.read_restricted private_topic = TopicConverter.new(self, user).convert_to_private_message + Tag.update_counters(private_topic.tags, { public_topic_count: -1 }) if !read_restricted add_small_action(user, "private_topic") if private_topic private_topic end diff --git a/app/models/topic_tag.rb b/app/models/topic_tag.rb index 67a8218d94a..b2ed69791a2 100644 --- a/app/models/topic_tag.rb +++ b/app/models/topic_tag.rb @@ -9,14 +9,18 @@ class TopicTag < ActiveRecord::Base if topic.archetype == Archetype.private_message tag.increment!(:pm_topic_count) else - tag.increment!(:topic_count) + counters_to_update = { staff_topic_count: 1 } - if topic.category_id - if stat = CategoryTagStat.find_by(tag_id: tag_id, category_id: topic.category_id) - stat.increment!(:topic_count) - else - CategoryTagStat.create(tag_id: tag_id, category_id: topic.category_id, topic_count: 1) - end + if Category.exists?(id: topic.category_id, read_restricted: false) + counters_to_update[:public_topic_count] = 1 + end + + Tag.update_counters(tag.id, counters_to_update) + + if stat = CategoryTagStat.find_by(tag_id: tag_id, category_id: topic.category_id) + stat.increment!(:topic_count) + else + CategoryTagStat.create!(tag_id: tag_id, category_id: topic.category_id, topic_count: 1) end end end @@ -27,12 +31,17 @@ class TopicTag < ActiveRecord::Base if topic.archetype == Archetype.private_message tag.decrement!(:pm_topic_count) else - if topic.category_id && - stat = CategoryTagStat.find_by(tag_id: tag_id, category: topic.category_id) - stat.topic_count == 1 ? stat.destroy : stat.decrement!(:topic_count) + if stat = CategoryTagStat.find_by(tag_id: tag_id, category: topic.category_id) + stat.topic_count == 1 ? stat.destroy! : stat.decrement!(:topic_count) end - tag.decrement!(:topic_count) + counters_to_update = { staff_topic_count: -1 } + + if Category.exists?(id: topic.category_id, read_restricted: false) + counters_to_update[:public_topic_count] = -1 + end + + Tag.update_counters(tag.id, counters_to_update) end end end diff --git a/app/serializers/concerns/topic_tags_mixin.rb b/app/serializers/concerns/topic_tags_mixin.rb index 2704ba33611..0c001d46334 100644 --- a/app/serializers/concerns/topic_tags_mixin.rb +++ b/app/serializers/concerns/topic_tags_mixin.rb @@ -32,7 +32,8 @@ module TopicTagsMixin if SiteSetting.tags_sort_alphabetically topic.tags.sort_by(&:name) else - topic.tags.sort_by(&:topic_count).reverse + topic_count_column = Tag.topic_count_column(scope) + topic.tags.sort_by { |tag| tag.public_send(topic_count_column) }.reverse end ) tags = tags.reject { |tag| scope.hidden_tag_names.include?(tag[:name]) } if !scope.is_staff? diff --git a/app/serializers/concerns/user_sidebar_mixin.rb b/app/serializers/concerns/user_sidebar_mixin.rb index f70b97bec28..5f8c5add0a1 100644 --- a/app/serializers/concerns/user_sidebar_mixin.rb +++ b/app/serializers/concerns/user_sidebar_mixin.rb @@ -2,9 +2,11 @@ module UserSidebarMixin def sidebar_tags + topic_count_column = Tag.topic_count_column(scope) + object .visible_sidebar_tags(scope) - .pluck(:name, :topic_count, :pm_topic_count) + .pluck(:name, topic_count_column, :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 diff --git a/app/serializers/detailed_tag_serializer.rb b/app/serializers/detailed_tag_serializer.rb index 83570c3d5e0..7fd7a3943b0 100644 --- a/app/serializers/detailed_tag_serializer.rb +++ b/app/serializers/detailed_tag_serializer.rb @@ -6,7 +6,7 @@ class DetailedTagSerializer < TagSerializer has_many :categories, serializer: BasicCategorySerializer def synonyms - TagsController.tag_counts_json(object.synonyms) + TagsController.tag_counts_json(object.synonyms, scope) end def categories diff --git a/app/serializers/tag_serializer.rb b/app/serializers/tag_serializer.rb index 6fa46a80bdf..81e55156965 100644 --- a/app/serializers/tag_serializer.rb +++ b/app/serializers/tag_serializer.rb @@ -3,6 +3,10 @@ class TagSerializer < ApplicationSerializer attributes :id, :name, :topic_count, :staff, :description + def topic_count + object.public_send(Tag.topic_count_column(scope)) + end + def staff DiscourseTagging.staff_tag_names.include?(name) end diff --git a/app/services/tag_hashtag_data_source.rb b/app/services/tag_hashtag_data_source.rb index 11b089c84f5..b186ca12c42 100644 --- a/app/services/tag_hashtag_data_source.rb +++ b/app/services/tag_hashtag_data_source.rb @@ -12,26 +12,30 @@ class TagHashtagDataSource "tag" end - def self.tag_to_hashtag_item(tag) - tag = Tag.new(tag.slice(:id, :name, :description).merge(topic_count: tag[:count])) if tag.is_a?( - Hash, - ) + def self.tag_to_hashtag_item(tag, guardian) + topic_count_column = Tag.topic_count_column(guardian) + + tag = + Tag.new( + tag.slice(:id, :name, :description).merge(topic_count_column => tag[:count]), + ) if tag.is_a?(Hash) HashtagAutocompleteService::HashtagItem.new.tap do |item| item.text = tag.name - item.secondary_text = "x#{tag.topic_count}" + item.secondary_text = "x#{tag.public_send(topic_count_column)}" item.description = tag.description item.slug = tag.name item.relative_url = tag.url item.icon = icon end end + private_class_method :tag_to_hashtag_item def self.lookup(guardian, slugs) return [] if !SiteSetting.tagging_enabled DiscourseTagging .filter_visible(Tag.where_name(slugs), guardian) - .map { |tag| tag_to_hashtag_item(tag) } + .map { |tag| tag_to_hashtag_item(tag, guardian) } end def self.search( @@ -60,9 +64,9 @@ class TagHashtagDataSource ) TagsController - .tag_counts_json(tags_with_counts) + .tag_counts_json(tags_with_counts, guardian) .take(limit) - .map { |tag| tag_to_hashtag_item(tag) } + .map { |tag| tag_to_hashtag_item(tag, guardian) } end def self.search_sort(search_results, _) @@ -82,8 +86,8 @@ class TagHashtagDataSource ) TagsController - .tag_counts_json(tags_with_counts) + .tag_counts_json(tags_with_counts, guardian) .take(limit) - .map { |tag| tag_to_hashtag_item(tag) } + .map { |tag| tag_to_hashtag_item(tag, guardian) } end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 88121e6d877..6fa88945fa8 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1651,6 +1651,7 @@ en: content_security_policy_frame_ancestors: "Restrict who can embed this site in iframes via CSP. Control allowed hosts on Embedding" content_security_policy_script_src: "Additional allowlisted script sources. The current host and CDN are included by default. See Mitigate XSS Attacks with Content Security Policy." invalidate_inactive_admin_email_after_days: "Admin accounts that have not visited the site in this number of days will need to re-validate their email address before logging in. Set to 0 to disable." + include_secure_categories_in_tag_counts: "When enabled, count of topics for a tag will include topics that are in read restricted categories for all users. When disabled, normal users are only shown a count of topics for a tag where all the topics are in public categories." top_menu: "Determine which items appear in the homepage navigation, and in what order. Example latest|new|unread|categories|top|read|posted|bookmarks" post_menu: "Determine which items appear on the post menu, and in what order. Example like|edit|flag|delete|share|bookmark|reply" post_menu_hidden_items: "The menu items to hide by default in the post menu unless an expansion ellipsis is clicked on." diff --git a/config/site_settings.yml b/config/site_settings.yml index e565dc1725d..5967fe135b5 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1798,6 +1798,8 @@ security: suppress_secured_categories_from_admin: default: false hidden: true + include_secure_categories_in_tag_counts: + default: false onebox: post_onebox_maxlength: diff --git a/db/migrate/20230118020114_add_public_topic_count_to_tags.rb b/db/migrate/20230118020114_add_public_topic_count_to_tags.rb new file mode 100644 index 00000000000..73b9064635a --- /dev/null +++ b/db/migrate/20230118020114_add_public_topic_count_to_tags.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +class AddPublicTopicCountToTags < ActiveRecord::Migration[7.0] + def up + add_column :tags, :public_topic_count, :integer, default: 0, null: false + + execute <<~SQL + UPDATE tags t + SET public_topic_count = x.topic_count + FROM ( + SELECT + COUNT(topics.id) AS topic_count, + tags.id AS tag_id + FROM tags + INNER JOIN topic_tags ON tags.id = topic_tags.tag_id + INNER JOIN topics ON topics.id = topic_tags.topic_id AND topics.deleted_at IS NULL AND topics.archetype != 'private_message' + INNER JOIN categories ON categories.id = topics.category_id AND NOT categories.read_restricted + GROUP BY tags.id + ) x + WHERE x.tag_id = t.id + AND x.topic_count <> t.public_topic_count; + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20230119000943_add_staff_topic_count_to_tags.rb b/db/migrate/20230119000943_add_staff_topic_count_to_tags.rb new file mode 100644 index 00000000000..0154c126246 --- /dev/null +++ b/db/migrate/20230119000943_add_staff_topic_count_to_tags.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class AddStaffTopicCountToTags < ActiveRecord::Migration[7.0] + def up + add_column :tags, :staff_topic_count, :integer, default: 0, null: false + + execute <<~SQL + UPDATE tags t + SET staff_topic_count = x.topic_count + FROM ( + SELECT COUNT(topics.id) AS topic_count, tags.id AS tag_id + FROM tags + LEFT JOIN topic_tags ON tags.id = topic_tags.tag_id + LEFT JOIN topics ON topics.id = topic_tags.topic_id + AND topics.deleted_at IS NULL + AND topics.archetype != 'private_message' + GROUP BY tags.id + ) x + WHERE x.tag_id = t.id + AND x.topic_count <> t.staff_topic_count + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/post_migrate/20230119024157_remove_topic_count_from_tags.rb b/db/post_migrate/20230119024157_remove_topic_count_from_tags.rb new file mode 100644 index 00000000000..f51e3a1bcd5 --- /dev/null +++ b/db/post_migrate/20230119024157_remove_topic_count_from_tags.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require "migration/column_dropper" + +class RemoveTopicCountFromTags < ActiveRecord::Migration[7.0] + DROPPED_COLUMNS ||= { tags: %i[topic_count] } + + def up + DROPPED_COLUMNS.each { |table, columns| Migration::ColumnDropper.execute_drop(table, columns) } + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 8a8c2d7f878..0fd1583be1a 100644 --- a/lib/discourse_tagging.rb +++ b/lib/discourse_tagging.rb @@ -323,17 +323,19 @@ module DiscourseTagging outer_join = category.nil? || category.allow_global_tags || !category_has_restricted_tags + topic_count_column = Tag.topic_count_column(guardian) + distinct_clause = if opts[:order_popularity] - "DISTINCT ON (topic_count, name)" + "DISTINCT ON (#{topic_count_column}, name)" elsif opts[:order_search_results] && opts[:term].present? - "DISTINCT ON (lower(name) = lower(:cleaned_term), topic_count, name)" + "DISTINCT ON (lower(name) = lower(:cleaned_term), #{topic_count_column}, name)" else "" end sql << <<~SQL - SELECT #{distinct_clause} t.id, t.name, t.topic_count, t.pm_topic_count, t.description, + SELECT #{distinct_clause} t.id, t.name, t.#{topic_count_column}, t.pm_topic_count, t.description, tgr.tgm_id as tgm_id, tgr.tag_group_id as tag_group_id, tgr.parent_tag_id as parent_tag_id, tgr.one_per_topic as one_per_topic, t.target_tag_id FROM tags t @@ -479,9 +481,9 @@ module DiscourseTagging end if opts[:order_popularity] - builder.order_by("topic_count DESC, name") + builder.order_by("#{topic_count_column} DESC, name") elsif opts[:order_search_results] && !term.blank? - builder.order_by("lower(name) = lower(:cleaned_term) DESC, topic_count DESC, name") + builder.order_by("lower(name) = lower(:cleaned_term) DESC, #{topic_count_column} DESC, name") end result = builder.query(builder_params).uniq { |t| t.id } diff --git a/lib/topic_view.rb b/lib/topic_view.rb index f6b9c19259e..576b6d3c5c3 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -245,7 +245,8 @@ class TopicView @topic.category title += " - #{@topic.category.name}" elsif SiteSetting.tagging_enabled && @topic.tags.exists? - title += " - #{@topic.tags.order("tags.topic_count DESC").first.name}" + title += + " - #{@topic.tags.order("tags.#{Tag.topic_count_column(@guardian)} DESC").first.name}" end end title diff --git a/spec/integration/tag_counts_spec.rb b/spec/integration/tag_counts_spec.rb new file mode 100644 index 00000000000..82ec44d7b30 --- /dev/null +++ b/spec/integration/tag_counts_spec.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +RSpec.describe "Updating tag counts" do + fab!(:tag1) { Fabricate(:tag) } + fab!(:tag2) { Fabricate(:tag) } + fab!(:group) { Fabricate(:group) } + fab!(:public_category) { Fabricate(:category) } + fab!(:public_category2) { Fabricate(:category) } + fab!(:private_category) { Fabricate(:private_category, group: group) } + fab!(:private_category2) { Fabricate(:private_category, group: group) } + + fab!(:topic_in_public_category) do + Fabricate(:topic, category: public_category, tags: [tag1, tag2]).tap do |topic| + Fabricate(:post, topic: topic) + end + end + + fab!(:topic_in_private_category) do + Fabricate(:topic, category: private_category, tags: [tag1, tag2]).tap do |topic| + Fabricate(:post, topic: topic) + end + end + + fab!(:private_message) do + topic = Fabricate(:private_message_post).topic + topic.update!(tags: [tag1, tag2]) + topic + end + + before do + expect(tag1.public_topic_count).to eq(1) + expect(tag1.staff_topic_count).to eq(2) + expect(tag1.pm_topic_count).to eq(1) + expect(tag2.reload.public_topic_count).to eq(1) + expect(tag2.staff_topic_count).to eq(2) + expect(tag2.pm_topic_count).to eq(1) + end + + it "should decrease Tag#public_topic_count for all tags when topic's category is changed from a public category to a read restricted category" do + expect { topic_in_public_category.change_category_to_id(private_category.id) }.to change { + tag1.reload.public_topic_count + }.by(-1).and change { tag2.reload.public_topic_count }.by(-1) + end + + it "should increase Tag#public_topic_count for all tags when topic's category is changed from a read restricted category to a public category" do + expect { topic_in_private_category.change_category_to_id(public_category.id) }.to change { + tag1.reload.public_topic_count + }.by(1).and change { tag2.reload.public_topic_count }.by(1) + end + + it "should not change Tag#public_topic_count for all tags when topic's category is changed from a public category to another public category" do + expect do + topic_in_public_category.change_category_to_id(public_category2.id) + end.to not_change { tag1.reload.public_topic_count }.and not_change { + tag2.reload.public_topic_count + } + end + + it "should not change Tag#public_topic_count for all tags when topic's category is changed from a read restricted category to another read restricted category" do + expect do + topic_in_private_category.change_category_to_id(private_category2.id) + end.to not_change { tag1.reload.public_topic_count }.and not_change { + tag2.reload.public_topic_count + } + end + + it "increases Tag#public_topic_count for all tags when topic is converted from private message to a regular topic in a public category" do + expect do + private_message.convert_to_public_topic( + Discourse.system_user, + category_id: public_category.id, + ) + end.to change { tag1.reload.public_topic_count }.by(1).and change { + tag2.reload.public_topic_count + }.by(1) + end + + it "should not change Tag#public_topic_count for all tags when topic is converted from private message to a regular topic in a read restricted category" do + expect do + private_message.convert_to_public_topic( + Discourse.system_user, + category_id: private_category.id, + ) + end.to not_change { tag1.reload.public_topic_count }.and not_change { + tag2.reload.public_topic_count + } + end + + it "should decrease Tag#public_topic_count for all tags when regular topic in public category is converted to a private message" do + expect do + topic_in_public_category.convert_to_private_message(Discourse.system_user) + end.to change { tag1.reload.public_topic_count }.by(-1).and change { + tag2.reload.public_topic_count + }.by(-1) + end + + it "should not change Tag#public_topic_count for all tags when regular topic in read restricted category is converted to a private message" do + expect do + topic_in_private_category.convert_to_private_message(Discourse.system_user) + end.to not_change { tag1.reload.public_topic_count }.and not_change { + tag2.reload.public_topic_count + } + end +end diff --git a/spec/lib/discourse_tagging_spec.rb b/spec/lib/discourse_tagging_spec.rb index 1188243c57e..02782eefd5d 100644 --- a/spec/lib/discourse_tagging_spec.rb +++ b/spec/lib/discourse_tagging_spec.rb @@ -641,9 +641,11 @@ RSpec.describe DiscourseTagging do it "user does not get an error when editing their topic with a hidden tag" do PostRevisor.new(post).revise!(admin, raw: post.raw, tags: [hidden_tag.name]) + expect( PostRevisor.new(post).revise!(topic.user, raw: post.raw + " edit", tags: []), ).to be_truthy + expect(topic.reload.tags).to eq([hidden_tag]) end end @@ -967,8 +969,16 @@ RSpec.describe DiscourseTagging do topic = Fabricate(:topic, tags: [tag2]) expect(DiscourseTagging.add_or_create_synonyms_by_name(tag1, [tag2.name])).to eq(true) expect_same_tag_names(topic.reload.tags, [tag1]) - expect(tag1.reload.topic_count).to eq(1) - expect(tag2.reload.topic_count).to eq(0) + + tag1.reload + + expect(tag1.public_topic_count).to eq(1) + expect(tag1.staff_topic_count).to eq(1) + + tag2.reload + + expect(tag2.public_topic_count).to eq(0) + expect(tag2.staff_topic_count).to eq(0) end end end diff --git a/spec/lib/topic_view_spec.rb b/spec/lib/topic_view_spec.rb index 7dc2ff115fc..50328464889 100644 --- a/spec/lib/topic_view_spec.rb +++ b/spec/lib/topic_view_spec.rb @@ -750,8 +750,8 @@ RSpec.describe TopicView do end describe "page_title" do - fab!(:tag1) { Fabricate(:tag) } - fab!(:tag2) { Fabricate(:tag, topic_count: 2) } + fab!(:tag1) { Fabricate(:tag, staff_topic_count: 0, public_topic_count: 0) } + fab!(:tag2) { Fabricate(:tag, staff_topic_count: 2, public_topic_count: 2) } fab!(:op_post) { Fabricate(:post, topic: topic) } fab!(:post1) { Fabricate(:post, topic: topic) } fab!(:whisper) { Fabricate(:post, topic: topic, post_type: Post.types[:whisper]) } diff --git a/spec/models/tag_spec.rb b/spec/models/tag_spec.rb index 7aff07ab795..402c6c73349 100644 --- a/spec/models/tag_spec.rb +++ b/spec/models/tag_spec.rb @@ -202,29 +202,101 @@ RSpec.describe Tag do end end - describe "topic counts" do + describe ".ensure_consistency!" do it "should exclude private message topics" do topic Fabricate(:private_message_topic, tags: [tag]) Tag.ensure_consistency! tag.reload - expect(tag.topic_count).to eq(1) + expect(tag.staff_topic_count).to eq(1) + expect(tag.public_topic_count).to eq(1) + end + + it "should update Tag#topic_count and Tag#public_topic_count correctly" do + tag = Fabricate(:tag, name: "tag1") + tag2 = Fabricate(:tag, name: "tag2") + tag3 = Fabricate(:tag, name: "tag3") + group = Fabricate(:group) + category = Fabricate(:category) + private_category = Fabricate(:private_category, group: group) + private_category2 = Fabricate(:private_category, group: group) + + _topic_with_tag = Fabricate(:topic, category: category, tags: [tag]) + + _topic_with_tag_in_private_category = + Fabricate(:topic, category: private_category, tags: [tag]) + + _topic_with_tag2_in_private_category2 = + Fabricate(:topic, category: private_category2, tags: [tag2]) + + tag.update!(staff_topic_count: 123, public_topic_count: 456) + tag2.update!(staff_topic_count: 123, public_topic_count: 456) + tag3.update!(staff_topic_count: 123, public_topic_count: 456) + + Tag.ensure_consistency! + + tag.reload + tag2.reload + tag3.reload + + expect(tag.staff_topic_count).to eq(2) + expect(tag.public_topic_count).to eq(1) + expect(tag2.staff_topic_count).to eq(1) + expect(tag2.public_topic_count).to eq(0) + expect(tag3.staff_topic_count).to eq(0) + expect(tag3.public_topic_count).to eq(0) end end describe "unused tags scope" do let!(:tags) do [ - Fabricate(:tag, name: "used_publically", topic_count: 2, pm_topic_count: 0), - Fabricate(:tag, name: "used_privately", topic_count: 0, pm_topic_count: 3), - Fabricate(:tag, name: "used_everywhere", topic_count: 0, pm_topic_count: 3), - Fabricate(:tag, name: "unused1", topic_count: 0, pm_topic_count: 0), - Fabricate(:tag, name: "unused2", topic_count: 0, pm_topic_count: 0), + Fabricate( + :tag, + name: "used_publically", + staff_topic_count: 2, + public_topic_count: 2, + pm_topic_count: 0, + ), + Fabricate( + :tag, + name: "used_privately", + staff_topic_count: 0, + public_topic_count: 0, + pm_topic_count: 3, + ), + Fabricate( + :tag, + name: "used_everywhere", + staff_topic_count: 0, + public_topic_count: 0, + pm_topic_count: 3, + ), + Fabricate( + :tag, + name: "unused1", + staff_topic_count: 0, + public_topic_count: 0, + pm_topic_count: 0, + ), + Fabricate( + :tag, + name: "unused2", + staff_topic_count: 0, + public_topic_count: 0, + pm_topic_count: 0, + ), ] end let(:tag_in_group) do - Fabricate(:tag, name: "unused_in_group", topic_count: 0, pm_topic_count: 0) + Fabricate( + :tag, + name: "unused_in_group", + public_topic_count: 0, + staff_topic_count: 0, + pm_topic_count: 0, + ) end let!(:tag_group) { Fabricate(:tag_group, tag_names: [tag_in_group.name]) } @@ -292,4 +364,22 @@ RSpec.describe Tag do expect(category.reload.tags).to include(tag2) end end + + describe ".topic_count_column" do + fab!(:admin) { Fabricate(:admin) } + + it "returns 'staff_topic_count' when user is staff" do + expect(Tag.topic_count_column(Guardian.new(admin))).to eq("staff_topic_count") + end + + it "returns 'public_topic_count' when user is not staff" do + expect(Tag.topic_count_column(Guardian.new(user))).to eq("public_topic_count") + end + + it "returns 'staff_topic_count' when user is not staff but `include_secure_categories_in_tag_counts` site setting is enabled" do + SiteSetting.include_secure_categories_in_tag_counts = true + + expect(Tag.topic_count_column(Guardian.new(user))).to eq("staff_topic_count") + end + end end diff --git a/spec/models/topic_tag_spec.rb b/spec/models/topic_tag_spec.rb index 33779f1e34e..b4a8e231cef 100644 --- a/spec/models/topic_tag_spec.rb +++ b/spec/models/topic_tag_spec.rb @@ -1,33 +1,56 @@ # frozen_string_literal: true RSpec.describe TopicTag do + fab!(:group) { Fabricate(:group) } + fab!(:private_category) { Fabricate(:private_category, group: group) } fab!(:topic) { Fabricate(:topic) } + fab!(:topic_in_private_category) { Fabricate(:topic, category: private_category) } fab!(:tag) { Fabricate(:tag) } let(:topic_tag) { Fabricate(:topic_tag, topic: topic, tag: tag) } describe "#after_create" do - it "tag topic_count should be increased" do - expect { topic_tag }.to change(tag, :topic_count).by(1) + it "should increase Tag#staff_topic_count and Tag#public_topic_count for a regular topic in a public category" do + expect { topic_tag }.to change { tag.reload.staff_topic_count }.by(1).and change { + tag.reload.public_topic_count + }.by(1) end - it "tag topic_count should not be increased" do + it "should only increase Tag#staff_topic_count for a regular topic in a read restricted category" do + expect { Fabricate(:topic_tag, topic: topic_in_private_category, tag: tag) }.to change { + tag.reload.staff_topic_count + }.by(1) + + expect(tag.reload.public_topic_count).to eq(0) + end + + it "should increase Tag#pm_topic_count for a private message topic" do topic.archetype = Archetype.private_message - expect { topic_tag }.not_to change(tag, :topic_count) + expect { topic_tag }.to change { tag.reload.pm_topic_count }.by(1) end end describe "#after_destroy" do - it "tag topic_count should be decreased" do + it "should decrease Tag#staff_topic_count and Tag#public_topic_count for a regular topic in a public category" do topic_tag - expect { topic_tag.destroy }.to change(tag, :topic_count).by(-1) + + expect { topic_tag.destroy! }.to change { tag.reload.staff_topic_count }.by(-1).and change { + tag.reload.public_topic_count + }.by(-1) end - it "tag topic_count should not be decreased" do + it "should only decrease Topic#topic_count for a regular topic in a read restricted category" do + topic_tag = Fabricate(:topic_tag, topic: topic_in_private_category, tag: tag) + + expect { topic_tag.destroy! }.to change { tag.reload.staff_topic_count }.by(-1) + expect(tag.reload.public_topic_count).to eq(0) + end + + it "should decrease Tag#pm_topic_count for a private message topic" do topic.archetype = Archetype.private_message topic_tag - expect { topic_tag.destroy }.not_to change(tag, :topic_count) + expect { topic_tag.destroy! }.to change { tag.reload.pm_topic_count }.by(-1) end end end diff --git a/spec/requests/tags_controller_spec.rb b/spec/requests/tags_controller_spec.rb index 8664605a4c3..834589618be 100644 --- a/spec/requests/tags_controller_spec.rb +++ b/spec/requests/tags_controller_spec.rb @@ -12,18 +12,43 @@ RSpec.describe TagsController do describe "#index" do fab!(:test_tag) { Fabricate(:tag, name: "test") } - fab!(:topic_tag) { Fabricate(:tag, name: "topic-test", topic_count: 1) } + fab!(:topic_tag) do + Fabricate(:tag, name: "topic-test", public_topic_count: 1, staff_topic_count: 1) + end fab!(:synonym) { Fabricate(:tag, name: "synonym", target_tag: topic_tag) } - shared_examples "successfully retrieve tags with topic_count > 0" do - it "should return the right response" do + shared_examples "retrieves the right tags" do + it "retrieves all tags as a staff user" do + sign_in(admin) + + get "/tags.json" + + expect(response.status).to eq(200) + + tags = response.parsed_body["tags"] + + expect(tags[0]["name"]).to eq(test_tag.name) + expect(tags[0]["count"]).to eq(0) + expect(tags[0]["pm_count"]).to eq(0) + + expect(tags[1]["name"]).to eq(topic_tag.name) + expect(tags[1]["count"]).to eq(1) + expect(tags[1]["pm_count"]).to eq(0) + end + + it "only retrieve tags that have been used in public topics for non-staff user" do + sign_in(user) + get "/tags.json" expect(response.status).to eq(200) tags = response.parsed_body["tags"] expect(tags.length).to eq(1) - expect(tags[0]["text"]).to eq("topic-test") + + expect(tags[0]["name"]).to eq(topic_tag.name) + expect(tags[0]["count"]).to eq(1) + expect(tags[0]["pm_count"]).to eq(0) end end @@ -76,7 +101,7 @@ RSpec.describe TagsController do context "with tags_listed_by_group enabled" do before { SiteSetting.tags_listed_by_group = true } - include_examples "successfully retrieve tags with topic_count > 0" + include_examples "retrieves the right tags" it "works for tags in groups" do tag_group = Fabricate(:tag_group, tags: [test_tag, topic_tag, synonym]) @@ -136,20 +161,7 @@ RSpec.describe TagsController do context "with tags_listed_by_group disabled" do before { SiteSetting.tags_listed_by_group = false } - include_examples "successfully retrieve tags with topic_count > 0" - end - - context "when user can admin tags" do - it "successfully retrieve all tags" do - sign_in(admin) - - get "/tags.json" - - expect(response.status).to eq(200) - - tags = response.parsed_body["tags"] - expect(tags.length).to eq(2) - end + include_examples "retrieves the right tags" end context "with hidden tags" do @@ -763,10 +775,10 @@ RSpec.describe TagsController do expect(response.parsed_body["results"].map { |j| j["id"] }.sort).to eq(%w[stuff stumped]) end - it "returns tags ordered by topic_count, and prioritises exact matches" do - Fabricate(:tag, name: "tag1", topic_count: 10) - Fabricate(:tag, name: "tag2", topic_count: 100) - Fabricate(:tag, name: "tag", topic_count: 1) + it "returns tags ordered by public_topic_count, and prioritises exact matches" do + Fabricate(:tag, name: "tag1", public_topic_count: 10, staff_topic_count: 10) + Fabricate(:tag, name: "tag2", public_topic_count: 100, staff_topic_count: 100) + Fabricate(:tag, name: "tag", public_topic_count: 1, staff_topic_count: 1) get "/tags/filter/search.json", params: { q: "tag", limit: 2 } expect(response.status).to eq(200) @@ -976,11 +988,41 @@ RSpec.describe TagsController do context "with some tags" do let!(:tags) do [ - Fabricate(:tag, name: "used_publically", topic_count: 2, pm_topic_count: 0), - Fabricate(:tag, name: "used_privately", topic_count: 0, pm_topic_count: 3), - Fabricate(:tag, name: "used_everywhere", topic_count: 0, pm_topic_count: 3), - Fabricate(:tag, name: "unused1", topic_count: 0, pm_topic_count: 0), - Fabricate(:tag, name: "unused2", topic_count: 0, pm_topic_count: 0), + Fabricate( + :tag, + name: "used_publically", + public_topic_count: 2, + staff_topic_count: 2, + pm_topic_count: 0, + ), + Fabricate( + :tag, + name: "used_privately", + public_topic_count: 0, + staff_topic_count: 0, + pm_topic_count: 3, + ), + Fabricate( + :tag, + name: "used_everywhere", + public_topic_count: 0, + staff_topic_count: 0, + pm_topic_count: 3, + ), + Fabricate( + :tag, + name: "unused1", + public_topic_count: 0, + staff_topic_count: 0, + pm_topic_count: 0, + ), + Fabricate( + :tag, + name: "unused2", + public_topic_count: 0, + staff_topic_count: 0, + pm_topic_count: 0, + ), ] end diff --git a/spec/serializers/tag_serializer_spec.rb b/spec/serializers/tag_serializer_spec.rb new file mode 100644 index 00000000000..c9eab114fc7 --- /dev/null +++ b/spec/serializers/tag_serializer_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +RSpec.describe TagSerializer do + fab!(:user) { Fabricate(:user) } + fab!(:admin) { Fabricate(:admin) } + fab!(:tag) { Fabricate(:tag) } + fab!(:group) { Fabricate(:group) } + fab!(:private_category) { Fabricate(:private_category, group: group) } + fab!(:topic_in_public_category) { Fabricate(:topic, tags: [tag]) } + fab!(:topic_in_private_category) { Fabricate(:topic, category: private_category, tags: [tag]) } + + describe "#topic_count" do + it "should return the value of `Tag#public_topic_count` for a non-staff user" do + serialized = described_class.new(tag, scope: Guardian.new(user), root: false).as_json + + expect(serialized[:topic_count]).to eq(1) + end + + it "should return the vavlue of `Tag#topic_count` for a staff user" do + serialized = described_class.new(tag, scope: Guardian.new(admin), root: false).as_json + + expect(serialized[:topic_count]).to eq(2) + end + end +end diff --git a/spec/serializers/topic_view_serializer_spec.rb b/spec/serializers/topic_view_serializer_spec.rb index 1d064811c80..015ef3d1f3c 100644 --- a/spec/serializers/topic_view_serializer_spec.rb +++ b/spec/serializers/topic_view_serializer_spec.rb @@ -279,9 +279,33 @@ RSpec.describe TopicViewSerializer do end describe "tags order" do - fab!(:tag1) { Fabricate(:tag, name: "ctag", description: "c description", topic_count: 5) } - fab!(:tag2) { Fabricate(:tag, name: "btag", description: "b description", topic_count: 9) } - fab!(:tag3) { Fabricate(:tag, name: "atag", description: "a description", topic_count: 3) } + fab!(:tag1) do + Fabricate( + :tag, + name: "ctag", + description: "c description", + staff_topic_count: 5, + public_topic_count: 5, + ) + end + fab!(:tag2) do + Fabricate( + :tag, + name: "btag", + description: "b description", + staff_topic_count: 9, + public_topic_count: 9, + ) + end + fab!(:tag3) do + Fabricate( + :tag, + name: "atag", + description: "a description", + staff_topic_count: 3, + public_topic_count: 3, + ) + end before do topic.tags << tag1 diff --git a/spec/services/hashtag_autocomplete_service_spec.rb b/spec/services/hashtag_autocomplete_service_spec.rb index f37eb5810c9..986124548dd 100644 --- a/spec/services/hashtag_autocomplete_service_spec.rb +++ b/spec/services/hashtag_autocomplete_service_spec.rb @@ -3,7 +3,9 @@ RSpec.describe HashtagAutocompleteService do fab!(:user) { Fabricate(:user) } fab!(:category1) { Fabricate(:category, name: "The Book Club", slug: "the-book-club") } - fab!(:tag1) { Fabricate(:tag, name: "great-books", topic_count: 22) } + fab!(:tag1) do + Fabricate(:tag, name: "great-books", staff_topic_count: 22, public_topic_count: 22) + end fab!(:topic1) { Fabricate(:topic) } let(:guardian) { Guardian.new(user) } @@ -68,7 +70,7 @@ RSpec.describe HashtagAutocompleteService do end it "includes the tag count" do - tag1.update!(topic_count: 78) + tag1.update!(staff_topic_count: 78, public_topic_count: 78) expect(subject.search("book", %w[tag category]).map(&:text)).to eq( ["great-books", "The Book Club"], ) @@ -149,8 +151,8 @@ RSpec.describe HashtagAutocompleteService do category6 = Fabricate(:category, name: "Book Reviews", slug: "book-reviews") Fabricate(:category, name: "Good Books", slug: "book", parent_category: category6) - Fabricate(:tag, name: "bookmania", topic_count: 15) - Fabricate(:tag, name: "awful-books", topic_count: 56) + Fabricate(:tag, name: "bookmania", staff_topic_count: 15, public_topic_count: 15) + Fabricate(:tag, name: "awful-books", staff_topic_count: 56, public_topic_count: 56) expect(subject.search("book", %w[category tag]).map(&:ref)).to eq( [ @@ -220,9 +222,13 @@ RSpec.describe HashtagAutocompleteService do end fab!(:category4) { Fabricate(:category, name: "Bookworld", slug: "book", topic_count: 56) } fab!(:category5) { Fabricate(:category, name: "Media", slug: "media", topic_count: 446) } - fab!(:tag2) { Fabricate(:tag, name: "mid-books", topic_count: 33) } - fab!(:tag3) { Fabricate(:tag, name: "terrible-books", topic_count: 2) } - fab!(:tag4) { Fabricate(:tag, name: "book", topic_count: 1) } + fab!(:tag2) do + Fabricate(:tag, name: "mid-books", staff_topic_count: 33, public_topic_count: 33) + end + fab!(:tag3) do + Fabricate(:tag, name: "terrible-books", staff_topic_count: 2, public_topic_count: 2) + end + fab!(:tag4) { Fabricate(:tag, name: "book", staff_topic_count: 1, public_topic_count: 1) } it "returns the 'most polular' categories and tags (based on topic_count) that the user can access" do category1.update!(read_restricted: true) diff --git a/spec/services/tag_hashtag_data_source_spec.rb b/spec/services/tag_hashtag_data_source_spec.rb index 3598c664f95..f1475973ac8 100644 --- a/spec/services/tag_hashtag_data_source_spec.rb +++ b/spec/services/tag_hashtag_data_source_spec.rb @@ -1,16 +1,16 @@ # frozen_string_literal: true RSpec.describe TagHashtagDataSource do - fab!(:tag1) { Fabricate(:tag, name: "fact", topic_count: 0) } - fab!(:tag2) { Fabricate(:tag, name: "factor", topic_count: 5) } - fab!(:tag3) { Fabricate(:tag, name: "factory", topic_count: 4) } - fab!(:tag4) { Fabricate(:tag, name: "factorio", topic_count: 3) } - fab!(:tag5) { Fabricate(:tag, name: "factz", topic_count: 1) } + fab!(:tag1) { Fabricate(:tag, name: "fact", public_topic_count: 0) } + fab!(:tag2) { Fabricate(:tag, name: "factor", public_topic_count: 5) } + fab!(:tag3) { Fabricate(:tag, name: "factory", public_topic_count: 4) } + fab!(:tag4) { Fabricate(:tag, name: "factorio", public_topic_count: 3) } + fab!(:tag5) { Fabricate(:tag, name: "factz", public_topic_count: 1) } fab!(:user) { Fabricate(:user) } let(:guardian) { Guardian.new(user) } describe "#search" do - it "orders tag results by exact search match, then topic count, then name" do + it "orders tag results by exact search match, then public topic count, then name" do expect(described_class.search(guardian, "fact", 5).map(&:slug)).to eq( %w[fact factor factory factorio factz], ) @@ -31,7 +31,7 @@ RSpec.describe TagHashtagDataSource do ) end - it "includes the topic count for the text of the tag in secondary text" do + it "includes the public topic count for the text of the tag in secondary text" do expect(described_class.search(guardian, "fact", 5).map(&:secondary_text)).to eq( %w[x0 x5 x4 x3 x1], ) @@ -52,7 +52,7 @@ RSpec.describe TagHashtagDataSource do end describe "#search_without_term" do - it "returns distinct tags sorted by topic_count" do + it "returns distinct tags sorted by public topic count" do expect(described_class.search_without_term(guardian, 5).map(&:slug)).to eq( %w[factor factory factorio factz fact], ) diff --git a/spec/support/user_sidebar_serializer_attributes.rb b/spec/support/user_sidebar_serializer_attributes.rb index f7cea951b34..9bf6eaa5d5c 100644 --- a/spec/support/user_sidebar_serializer_attributes.rb +++ b/spec/support/user_sidebar_serializer_attributes.rb @@ -65,7 +65,9 @@ RSpec.shared_examples "User Sidebar Serializer Attributes" do |serializer_klass| describe "#sidebar_tags" do fab!(:tag) { Fabricate(:tag, name: "foo") } - fab!(:pm_tag) { Fabricate(:tag, name: "bar", pm_topic_count: 5, topic_count: 0) } + 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"]) diff --git a/spec/system/hashtag_autocomplete_spec.rb b/spec/system/hashtag_autocomplete_spec.rb index 131c19d32fc..254f6a686c7 100644 --- a/spec/system/hashtag_autocomplete_spec.rb +++ b/spec/system/hashtag_autocomplete_spec.rb @@ -10,8 +10,8 @@ describe "Using #hashtag autocompletion to search for and lookup categories and fab!(:category2) do Fabricate(:category, name: "Other Category", slug: "other-cat", topic_count: 23) end - fab!(:tag) { Fabricate(:tag, name: "cooltag", topic_count: 324) } - fab!(:tag2) { Fabricate(:tag, name: "othertag", topic_count: 66) } + fab!(:tag) { Fabricate(:tag, name: "cooltag", staff_topic_count: 324, public_topic_count: 324) } + fab!(:tag2) { Fabricate(:tag, name: "othertag", staff_topic_count: 66, public_topic_count: 66) } fab!(:topic) { Fabricate(:topic, category: category, tags: [tag]) } fab!(:post) { Fabricate(:post, topic: topic) } let(:uncategorized_category) { Category.find(SiteSetting.uncategorized_category_id) }