From e0be2f482ef3ebee6d0ea077206b626d30db69e5 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Thu, 22 Sep 2016 15:23:10 -0400 Subject: [PATCH] FEATURE: tag filter dropdown menu is scoped to user and category --- .../routes/build-category-route.js.es6 | 3 -- .../discourse/routes/build-topic-route.js.es6 | 3 ++ .../discourse/routes/tags-show.js.es6 | 3 -- app/models/tag.rb | 13 ++++++- app/models/topic_list.rb | 1 + app/serializers/site_serializer.rb | 2 +- spec/models/tag_spec.rb | 37 +++++++++++++++++++ spec/models/topic_list_spec.rb | 4 +- 8 files changed, 55 insertions(+), 11 deletions(-) diff --git a/app/assets/javascripts/discourse/routes/build-category-route.js.es6 b/app/assets/javascripts/discourse/routes/build-category-route.js.es6 index f9792401565..14bc77f8a5f 100644 --- a/app/assets/javascripts/discourse/routes/build-category-route.js.es6 +++ b/app/assets/javascripts/discourse/routes/build-category-route.js.es6 @@ -67,9 +67,6 @@ export default (filter, params) => { return findTopicList(this.store, this.topicTrackingState, listFilter, findOpts, extras).then(list => { TopicList.hideUniformCategory(list, category); this.set('topics', list); - if (list.topic_list.tags) { - Discourse.Site.currentProp('top_tags', list.topic_list.tags); - } return list; }); }, diff --git a/app/assets/javascripts/discourse/routes/build-topic-route.js.es6 b/app/assets/javascripts/discourse/routes/build-topic-route.js.es6 index 43c920535ee..d7a2116bee6 100644 --- a/app/assets/javascripts/discourse/routes/build-topic-route.js.es6 +++ b/app/assets/javascripts/discourse/routes/build-topic-route.js.es6 @@ -54,6 +54,9 @@ function findTopicList(store, tracking, filter, filterParams, extras) { tracking.trackIncoming(list.filter); } Discourse.Session.currentProp('topicList', list); + if (list.topic_list && list.topic_list.tags) { + Discourse.Site.currentProp('top_tags', list.topic_list.tags); + } return list; }); } diff --git a/app/assets/javascripts/discourse/routes/tags-show.js.es6 b/app/assets/javascripts/discourse/routes/tags-show.js.es6 index bed2d5b36c9..c82acb9eabb 100644 --- a/app/assets/javascripts/discourse/routes/tags-show.js.es6 +++ b/app/assets/javascripts/discourse/routes/tags-show.js.es6 @@ -75,9 +75,6 @@ export default Discourse.Route.extend({ return findTopicList(this.store, this.topicTrackingState, params.filter, params, {}).then(function(list) { controller.set('list', list); controller.set('canCreateTopic', list.get('can_create_topic')); - if (list.topic_list.tags) { - Discourse.Site.currentProp('top_tags', list.topic_list.tags); - } controller.set('loading', false); }); }, diff --git a/app/models/tag.rb b/app/models/tag.rb index 78652a1871a..f6d4661b2fe 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -23,10 +23,19 @@ class Tag < ActiveRecord::Base .where("topics.category_id = ?", category.id) end - def self.top_tags(limit_arg: nil, category: nil) + def self.top_tags(limit_arg: nil, category: nil, guardian: nil) limit = limit_arg || SiteSetting.max_tags_in_filter_list + scope_category_ids = (guardian||Guardian.new).allowed_category_ids - tags = DiscourseTagging.filter_allowed_tags(tags_by_count_query(limit: limit), nil, category: category) + if category + scope_category_ids &= ([category.id] + category.subcategories.pluck(:id)) + end + + tags = DiscourseTagging.filter_allowed_tags( + tags_by_count_query(limit: limit).where("topics.category_id in (?)", scope_category_ids), + nil, # Don't pass guardian. You might not be able to use some tags, but should still be able to see where they've been used. + category: category + ) tags.count.map {|name, _| name} end diff --git a/app/models/topic_list.rb b/app/models/topic_list.rb index 8fda6ede868..e0b3fd8353b 100644 --- a/app/models/topic_list.rb +++ b/app/models/topic_list.rb @@ -31,6 +31,7 @@ class TopicList def tags opts = @category ? { category: @category } : {} + opts[:guardian] = Guardian.new(@current_user) Tag.top_tags(opts) end diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb index c9e3b87cee1..217f7885438 100644 --- a/app/serializers/site_serializer.rb +++ b/app/serializers/site_serializer.rb @@ -111,7 +111,7 @@ class SiteSerializer < ApplicationSerializer end def top_tags - Tag.top_tags + Tag.top_tags(guardian: scope) end def wizard_required diff --git a/spec/models/tag_spec.rb b/spec/models/tag_spec.rb index 1a42a27cea0..d09cd732448 100644 --- a/spec/models/tag_spec.rb +++ b/spec/models/tag_spec.rb @@ -54,6 +54,43 @@ describe Tag do expect(described_class.top_tags.sort).to eq(@tags.map(&:name).sort) end + context "with categories" do + before do + make_some_tags(count: 4) # one tag that isn't used + @category1 = Fabricate(:category) + @private_category = Fabricate(:category) + @private_category.set_permissions(:admins => :full) + @private_category.save! + @topics = [] + @topics << Fabricate(:topic, category: @category1, tags: [@tags[0]]) + @topics << Fabricate(:topic, tags: [@tags[1]]) + @topics << Fabricate(:topic, category: @private_category, tags: [@tags[2]]) + end + + it "doesn't return tags that have only been used in private category to anon" do + expect(described_class.top_tags.sort).to eq([@tags[0].name, @tags[1].name].sort) + end + + it "returns tags used in private category to those who can see that category" do + expect(described_class.top_tags(guardian: Guardian.new(Fabricate(:admin))).sort).to eq([@tags[0].name, @tags[1].name, @tags[2].name].sort) + end + + it "returns tags scoped to a given category" do + expect(described_class.top_tags(category: @category1).sort).to eq([@tags[0].name].sort) + expect(described_class.top_tags(category: @private_category, guardian: Guardian.new(Fabricate(:admin))).sort).to eq([@tags[2].name].sort) + end + + it "returns tags from sub-categories too" do + sub_category = Fabricate(:category, parent_category_id: @category1.id) + Fabricate(:topic, category: sub_category, tags: [@tags[1]]) + expect(described_class.top_tags(category: @category1).sort).to eq([@tags[0].name, @tags[1].name].sort) + end + + it "returns nothing if category arg is private to you" do + expect(described_class.top_tags(category: @private_category)).to be_empty + end + end + context "with category-specific tags" do before do make_some_tags(count: 3) diff --git a/spec/models/topic_list_spec.rb b/spec/models/topic_list_spec.rb index 38a13515d97..f8fbe55c56a 100644 --- a/spec/models/topic_list_spec.rb +++ b/spec/models/topic_list_spec.rb @@ -61,11 +61,11 @@ describe TopicList do expect(TopicList.new('latest', other_topic.user, [other_topic]).tags.sort).to eq([tag.name, other_tag.name].sort) end - it "with another category with no tags, should return exclude tags restricted to other categories" do + it "with another category with no tags, should return no tags" do other_category = Fabricate(:category) topic3 = Fabricate(:topic, category: other_category) list = TopicList.new('latest', topic3.user, [topic3], { category: other_category.id, category_id: other_category.id }) - expect(list.tags).to eq([other_tag.name]) + expect(list.tags).to be_empty end end end