From 7e73b933c7feca450f89e32d541f346141c844cf Mon Sep 17 00:00:00 2001 From: James Kiesel Date: Thu, 11 Aug 2016 01:38:16 -0400 Subject: [PATCH 1/8] First pass --- .../discourse/routes/app-route-map.js.es6 | 1 + .../discourse/routes/tags-show.js.es6 | 6 ++++++ app/controllers/tags_controller.rb | 6 ++++-- config/routes.rb | 1 + lib/topic_query.rb | 19 +++++++++++++++---- spec/components/topic_query_spec.rb | 4 ++++ spec/controllers/tags_controller_spec.rb | 12 ++++++++++++ 7 files changed, 43 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/discourse/routes/app-route-map.js.es6 b/app/assets/javascripts/discourse/routes/app-route-map.js.es6 index 2f26a0248e..6ce4265544 100644 --- a/app/assets/javascripts/discourse/routes/app-route-map.js.es6 +++ b/app/assets/javascripts/discourse/routes/app-route-map.js.es6 @@ -132,6 +132,7 @@ export default function() { this.route('showCategory' + filter.capitalize(), {path: '/c/:category/:tag_id/l/' + filter}); this.route('showParentCategory' + filter.capitalize(), {path: '/c/:parent_category/:category/:tag_id/l/' + filter}); }); + this.route('show', {path: 'intersection/:tag_id/:secondary_tag_id'}); }); this.resource('tagGroups', {path: '/tag_groups'}, function() { diff --git a/app/assets/javascripts/discourse/routes/tags-show.js.es6 b/app/assets/javascripts/discourse/routes/tags-show.js.es6 index a828e39c68..961e28b80a 100644 --- a/app/assets/javascripts/discourse/routes/tags-show.js.es6 +++ b/app/assets/javascripts/discourse/routes/tags-show.js.es6 @@ -14,6 +14,8 @@ export default Discourse.Route.extend({ var tag = this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(params.tag_id) }), f = ''; + this.set("secondaryTagId", params.secondary_tag_id) + if (params.category) { f = 'c/'; if (params.parent_category) { f += params.parent_category + '/'; } @@ -43,6 +45,7 @@ export default Discourse.Route.extend({ const params = controller.getProperties('order', 'ascending'); const categorySlug = this.get('categorySlug'); + const secondaryTagId = this.get('secondaryTagId') const parentCategorySlug = this.get('parentCategorySlug'); const filter = this.get('navMode'); const tag_id = (tag ? tag.id : 'none'); @@ -56,6 +59,9 @@ export default Discourse.Route.extend({ } this.set('category', category); + } else if (secondaryTagId) { + params.filter = `tags/intersection/${tag_id}/${secondaryTagId}`; + this.set('category', null); } else { params.filter = `tags/${tag_id}/l/${filter}`; this.set('category', null); diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index d4290eaebe..0989f5715e 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -44,6 +44,7 @@ class TagsController < ::ApplicationController Discourse.filters.each do |filter| define_method("show_#{filter}") do @tag_id = DiscourseTagging.clean_tag(params[:tag_id]) + @secondary_tag_id = DiscourseTagging.clean_tag(params[:secondary_tag_id]) if params[:secondary_tag_id] page = params[:page].to_i list_opts = build_topic_list_options @@ -71,7 +72,7 @@ class TagsController < ::ApplicationController @list.more_topics_url = construct_url_with(:next, list_opts) @list.prev_topics_url = construct_url_with(:prev, list_opts) @rss = "tag" - @description_meta = I18n.t("rss_by_tag", tag: @tag_id) + @description_meta = I18n.t("rss_by_tag", tag: [@tag_id, @secondary_tag_id].compact.join(' & ')) @title = @description_meta canonical_url "#{Discourse.base_url_no_prefix}#{public_send(url_method(params.slice(:category, :parent_category)))}" @@ -297,7 +298,8 @@ class TagsController < ::ApplicationController if params[:tag_id] == 'none' options[:no_tags] = true else - options[:tags] = [params[:tag_id]] + options[:tags] = [params[:tag_id], params[:secondary_tag_id]].compact + options[:match_all_tags] = true end options diff --git a/config/routes.rb b/config/routes.rb index 37a93cd269..d38ad905f4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -630,6 +630,7 @@ Discourse::Application.routes.draw do get '/:tag_id' => 'tags#show', as: 'tag_show' get '/c/:category/:tag_id' => 'tags#show', as: 'tag_category_show' get '/c/:parent_category/:category/:tag_id' => 'tags#show', as: 'tag_parent_category_category_show' + get '/intersection/:tag_id/:secondary_tag_id' => 'tags#show', as: 'tag_intersection' get '/:tag_id/notifications' => 'tags#notifications' put '/:tag_id/notifications' => 'tags#update_notifications' put '/:tag_id' => 'tags#update' diff --git a/lib/topic_query.rb b/lib/topic_query.rb index a8e1d77e69..85cd7cf9fc 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -20,6 +20,7 @@ class TopicQuery visible category tags + match_all_tags no_tags order ascending @@ -460,11 +461,21 @@ class TopicQuery if @options[:tags] && @options[:tags].size > 0 result = result.joins(:tags) - # ANY of the given tags: - if @options[:tags][0].is_a?(Integer) - result = result.where("tags.id in (?)", @options[:tags]) + + # ALL of the given tags: + if @options[:match_all_tags] + @options[:tags] = Tag.where(name: @options[:tags]).pluck(:id) unless @options[:tags][0].is_a?(Integer) + @options[:tags].each_with_index do |tag, index| + sql_alias = ['t', index].join + result = result.joins("INNER JOIN topic_tags #{sql_alias} ON #{sql_alias}.topic_id = topics.id AND #{sql_alias}.tag_id = #{tag}") + end else - result = result.where("tags.name in (?)", @options[:tags]) + # ANY of the given tags: + if @options[:tags][0].is_a?(Integer) + result = result.where("tags.id in (?)", @options[:tags]) + else + result = result.where("tags.name in (?)", @options[:tags]) + end end elsif @options[:no_tags] # the following will do: ("topics"."id" NOT IN (SELECT DISTINCT "topic_tags"."topic_id" FROM "topic_tags")) diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index 3461a5d275..32b8d81442 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -145,6 +145,10 @@ describe TopicQuery do # expect(TopicQuery.new(moderator, tags: [tag.id, other_tag.id]).list_latest.topics.map(&:id)).to eq([tagged_topic3.id].sort) end + it "can return topics with all specified tags" do + expect(TopicQuery.new(moderator, tags: [tag.name, other_tag.name], match_all_tags: true).list_latest.topics.map(&:id)).to eq([tagged_topic3.id]) + end + it "can return topics with no tags" do expect(TopicQuery.new(moderator, no_tags: true).list_latest.topics.map(&:id)).to eq([no_tags_topic.id]) end diff --git a/spec/controllers/tags_controller_spec.rb b/spec/controllers/tags_controller_spec.rb index 82ea6c13a5..fa814fef60 100644 --- a/spec/controllers/tags_controller_spec.rb +++ b/spec/controllers/tags_controller_spec.rb @@ -3,9 +3,13 @@ require 'rails_helper' describe TagsController do describe 'show_latest' do let(:tag) { Fabricate(:tag) } + let(:other_tag) { Fabricate(:tag) } let(:category) { Fabricate(:category) } let(:subcategory) { Fabricate(:category, parent_category_id: category.id) } + let(:single_tag_topic) { Fabricate(:topic, tags: [tag]) } + let(:multi_tag_topic) { Fabricate(:topic, tags: [tag, other_tag]) } + context 'tagging disabled' do it "returns 404" do xhr :get, :show_latest, tag_id: tag.name @@ -23,6 +27,14 @@ describe TagsController do expect(response).to be_success end + it "can filter by multiple tags" do + single_tag_topic; multi_tag_topic + xhr :get, :show_latest, tag_id: tag.name, secondary_tag_id: other_tag.name + expect(response).to be_success + expect(assigns(:list).topics).to include multi_tag_topic + expect(assigns(:list).topics).to_not include single_tag_topic + end + it "can filter by category and tag" do xhr :get, :show_latest, tag_id: tag.name, category: category.slug expect(response).to be_success From 3471499613120b91d7e8dc488ed1bab6bd57fe8a Mon Sep 17 00:00:00 2001 From: James Kiesel Date: Fri, 12 Aug 2016 15:51:09 -0400 Subject: [PATCH 2/8] Tighten up tags intersection page --- .../discourse/controllers/tags-show.js.es6 | 3 ++- .../javascripts/discourse/routes/tags-show.js.es6 | 14 +++++++++----- .../javascripts/discourse/templates/tags/show.hbs | 10 ++++++++-- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/tags-show.js.es6 b/app/assets/javascripts/discourse/controllers/tags-show.js.es6 index 34c4729228..809c0623e8 100644 --- a/app/assets/javascripts/discourse/controllers/tags-show.js.es6 +++ b/app/assets/javascripts/discourse/controllers/tags-show.js.es6 @@ -43,6 +43,7 @@ export default Ember.Controller.extend(BulkTopicSelection, { needs: ["application"], tag: null, + secondaryTag: null, list: null, canAdminTag: Ember.computed.alias("currentUser.staff"), filterMode: null, @@ -72,7 +73,7 @@ export default Ember.Controller.extend(BulkTopicSelection, { }.property(), showAdminControls: function() { - return this.get('canAdminTag') && !this.get('category'); + return !this.get('secondaryTag') && this.get('canAdminTag') && !this.get('category'); }.property('canAdminTag', 'category'), loadMoreTopics() { diff --git a/app/assets/javascripts/discourse/routes/tags-show.js.es6 b/app/assets/javascripts/discourse/routes/tags-show.js.es6 index 961e28b80a..69b2007c89 100644 --- a/app/assets/javascripts/discourse/routes/tags-show.js.es6 +++ b/app/assets/javascripts/discourse/routes/tags-show.js.es6 @@ -12,9 +12,12 @@ export default Discourse.Route.extend({ model(params) { var tag = this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(params.tag_id) }), + secondaryTag = null, f = ''; - this.set("secondaryTagId", params.secondary_tag_id) + if (params.secondary_tag_id) { + this.set("secondaryTag", this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(params.secondary_tag_id) })) + } if (params.category) { f = 'c/'; @@ -45,10 +48,10 @@ export default Discourse.Route.extend({ const params = controller.getProperties('order', 'ascending'); const categorySlug = this.get('categorySlug'); - const secondaryTagId = this.get('secondaryTagId') const parentCategorySlug = this.get('parentCategorySlug'); const filter = this.get('navMode'); const tag_id = (tag ? tag.id : 'none'); + const secondary_tag_id = this.get('secondaryTag.id') if (categorySlug) { var category = Discourse.Category.findBySlug(categorySlug, parentCategorySlug); @@ -59,8 +62,8 @@ export default Discourse.Route.extend({ } this.set('category', category); - } else if (secondaryTagId) { - params.filter = `tags/intersection/${tag_id}/${secondaryTagId}`; + } else if (secondary_tag_id) { + params.filter = `tags/intersection/${tag_id}/${secondary_tag_id}`; this.set('category', null); } else { params.filter = `tags/${tag_id}/l/${filter}`; @@ -100,6 +103,7 @@ export default Discourse.Route.extend({ this.controllerFor('tags.show').setProperties({ model, tag: model, + secondaryTag: this.get('secondaryTag'), category: this.get('category'), filterMode: this.get('filterMode'), navMode: this.get('navMode'), @@ -129,7 +133,7 @@ export default Discourse.Route.extend({ // Pre-fill the tags input field if (controller.get('model.id')) { var c = self.controllerFor('composer').get('model'); - c.set('tags', [controller.get('model.id')]); + c.set('tags', _.compact([controller.get('model.id'), controller.get('secondaryTag.id')])); } }); }, diff --git a/app/assets/javascripts/discourse/templates/tags/show.hbs b/app/assets/javascripts/discourse/templates/tags/show.hbs index db3ecce1e4..ef54c1a1b8 100644 --- a/app/assets/javascripts/discourse/templates/tags/show.hbs +++ b/app/assets/javascripts/discourse/templates/tags/show.hbs @@ -5,8 +5,10 @@
{{#if tagNotification}} - {{tag-notifications-button action="changeTagNotification" - notificationLevel=tagNotification.notification_level}} + {{#unless secondaryTag}} + {{tag-notifications-button action="changeTagNotification" + notificationLevel=tagNotification.notification_level}} + {{/unless}} {{/if}} {{#if showAdminControls}} @@ -31,6 +33,10 @@ {{#link-to 'tags'}}{{i18n "tagging.tags"}}{{/link-to}} {{fa-icon "angle-right"}} {{discourse-tag-bound tagRecord=tag style="simple"}} + {{#if secondaryTag}} + & + {{discourse-tag-bound tagRecord=secondaryTag style="simple"}} + {{/if}} {{/if}}
From e14f3c802b3877af97f6e32f4e943c86d30930b2 Mon Sep 17 00:00:00 2001 From: James Kiesel Date: Fri, 12 Aug 2016 15:56:56 -0400 Subject: [PATCH 3/8] Cleanup --- app/assets/javascripts/discourse/controllers/tags-show.js.es6 | 2 +- app/assets/javascripts/discourse/templates/tags/show.hbs | 2 +- lib/topic_query.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/tags-show.js.es6 b/app/assets/javascripts/discourse/controllers/tags-show.js.es6 index 809c0623e8..cac14abaa4 100644 --- a/app/assets/javascripts/discourse/controllers/tags-show.js.es6 +++ b/app/assets/javascripts/discourse/controllers/tags-show.js.es6 @@ -74,7 +74,7 @@ export default Ember.Controller.extend(BulkTopicSelection, { showAdminControls: function() { return !this.get('secondaryTag') && this.get('canAdminTag') && !this.get('category'); - }.property('canAdminTag', 'category'), + }.property('secondaryTag', 'canAdminTag', 'category'), loadMoreTopics() { return this.get("list").loadMore(); diff --git a/app/assets/javascripts/discourse/templates/tags/show.hbs b/app/assets/javascripts/discourse/templates/tags/show.hbs index ef54c1a1b8..b652898b11 100644 --- a/app/assets/javascripts/discourse/templates/tags/show.hbs +++ b/app/assets/javascripts/discourse/templates/tags/show.hbs @@ -34,7 +34,7 @@ {{fa-icon "angle-right"}} {{discourse-tag-bound tagRecord=tag style="simple"}} {{#if secondaryTag}} - & + & {{discourse-tag-bound tagRecord=secondaryTag style="simple"}} {{/if}} diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 85cd7cf9fc..902b42c4a3 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -462,8 +462,8 @@ class TopicQuery if @options[:tags] && @options[:tags].size > 0 result = result.joins(:tags) - # ALL of the given tags: if @options[:match_all_tags] + # ALL of the given tags: @options[:tags] = Tag.where(name: @options[:tags]).pluck(:id) unless @options[:tags][0].is_a?(Integer) @options[:tags].each_with_index do |tag, index| sql_alias = ['t', index].join From d3792c0149ed124979ea5f3a344a0855a5b9cea6 Mon Sep 17 00:00:00 2001 From: James Kiesel Date: Fri, 12 Aug 2016 16:03:52 -0400 Subject: [PATCH 4/8] Fix linting errors --- app/assets/javascripts/discourse/routes/tags-show.js.es6 | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/discourse/routes/tags-show.js.es6 b/app/assets/javascripts/discourse/routes/tags-show.js.es6 index 69b2007c89..a699654432 100644 --- a/app/assets/javascripts/discourse/routes/tags-show.js.es6 +++ b/app/assets/javascripts/discourse/routes/tags-show.js.es6 @@ -12,11 +12,10 @@ export default Discourse.Route.extend({ model(params) { var tag = this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(params.tag_id) }), - secondaryTag = null, f = ''; if (params.secondary_tag_id) { - this.set("secondaryTag", this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(params.secondary_tag_id) })) + this.set("secondaryTag", this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(params.secondary_tag_id) })); } if (params.category) { @@ -51,7 +50,7 @@ export default Discourse.Route.extend({ const parentCategorySlug = this.get('parentCategorySlug'); const filter = this.get('navMode'); const tag_id = (tag ? tag.id : 'none'); - const secondary_tag_id = this.get('secondaryTag.id') + const secondary_tag_id = this.get('secondaryTag.id'); if (categorySlug) { var category = Discourse.Category.findBySlug(categorySlug, parentCategorySlug); From 037e9bb7b8ed465e7eead030a931c2d2c78279dd Mon Sep 17 00:00:00 2001 From: James Kiesel Date: Mon, 15 Aug 2016 15:30:17 -0400 Subject: [PATCH 5/8] Support any number of tag intersections --- .../discourse/controllers/tags-show.js.es6 | 6 ++--- .../discourse/routes/app-route-map.js.es6 | 2 +- .../discourse/routes/tags-show.js.es6 | 16 ++++++------ .../discourse/templates/tags/show.hbs | 8 +++--- app/controllers/tags_controller.rb | 10 +++++--- config/routes.rb | 2 +- lib/topic_query.rb | 12 ++++++--- spec/controllers/tags_controller_spec.rb | 25 ++++++++++++++++--- 8 files changed, 56 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/tags-show.js.es6 b/app/assets/javascripts/discourse/controllers/tags-show.js.es6 index cac14abaa4..6170f2142b 100644 --- a/app/assets/javascripts/discourse/controllers/tags-show.js.es6 +++ b/app/assets/javascripts/discourse/controllers/tags-show.js.es6 @@ -43,7 +43,7 @@ export default Ember.Controller.extend(BulkTopicSelection, { needs: ["application"], tag: null, - secondaryTag: null, + additionalTags: null, list: null, canAdminTag: Ember.computed.alias("currentUser.staff"), filterMode: null, @@ -73,8 +73,8 @@ export default Ember.Controller.extend(BulkTopicSelection, { }.property(), showAdminControls: function() { - return !this.get('secondaryTag') && this.get('canAdminTag') && !this.get('category'); - }.property('secondaryTag', 'canAdminTag', 'category'), + return this.get('additionalTags') && this.get('canAdminTag') && !this.get('category'); + }.property('additionalTags', 'canAdminTag', 'category'), loadMoreTopics() { return this.get("list").loadMore(); diff --git a/app/assets/javascripts/discourse/routes/app-route-map.js.es6 b/app/assets/javascripts/discourse/routes/app-route-map.js.es6 index 6ce4265544..002f998c35 100644 --- a/app/assets/javascripts/discourse/routes/app-route-map.js.es6 +++ b/app/assets/javascripts/discourse/routes/app-route-map.js.es6 @@ -132,7 +132,7 @@ export default function() { this.route('showCategory' + filter.capitalize(), {path: '/c/:category/:tag_id/l/' + filter}); this.route('showParentCategory' + filter.capitalize(), {path: '/c/:parent_category/:category/:tag_id/l/' + filter}); }); - this.route('show', {path: 'intersection/:tag_id/:secondary_tag_id'}); + this.route('show', {path: 'intersection/:tag_id/*additional_tags'}); }); this.resource('tagGroups', {path: '/tag_groups'}, function() { diff --git a/app/assets/javascripts/discourse/routes/tags-show.js.es6 b/app/assets/javascripts/discourse/routes/tags-show.js.es6 index a699654432..4fab00b935 100644 --- a/app/assets/javascripts/discourse/routes/tags-show.js.es6 +++ b/app/assets/javascripts/discourse/routes/tags-show.js.es6 @@ -14,8 +14,11 @@ export default Discourse.Route.extend({ var tag = this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(params.tag_id) }), f = ''; - if (params.secondary_tag_id) { - this.set("secondaryTag", this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(params.secondary_tag_id) })); + if (params.additional_tags) { + this.set("additionalTags", _.compact(params.additional_tags.split('/')).map((tag) => { + return this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(tag) }); + })); + this.set("additionalTagIds", _.pluck(this.get("additionalTags"), 'id')); } if (params.category) { @@ -50,7 +53,6 @@ export default Discourse.Route.extend({ const parentCategorySlug = this.get('parentCategorySlug'); const filter = this.get('navMode'); const tag_id = (tag ? tag.id : 'none'); - const secondary_tag_id = this.get('secondaryTag.id'); if (categorySlug) { var category = Discourse.Category.findBySlug(categorySlug, parentCategorySlug); @@ -61,8 +63,8 @@ export default Discourse.Route.extend({ } this.set('category', category); - } else if (secondary_tag_id) { - params.filter = `tags/intersection/${tag_id}/${secondary_tag_id}`; + } else if (_.any(this.get("additionalTags"))) { + params.filter = `tags/intersection/${tag_id}/${this.get('additionalTagIds').join('/')}`; this.set('category', null); } else { params.filter = `tags/${tag_id}/l/${filter}`; @@ -102,7 +104,7 @@ export default Discourse.Route.extend({ this.controllerFor('tags.show').setProperties({ model, tag: model, - secondaryTag: this.get('secondaryTag'), + additionalTags: this.get('additionalTags'), category: this.get('category'), filterMode: this.get('filterMode'), navMode: this.get('navMode'), @@ -132,7 +134,7 @@ export default Discourse.Route.extend({ // Pre-fill the tags input field if (controller.get('model.id')) { var c = self.controllerFor('composer').get('model'); - c.set('tags', _.compact([controller.get('model.id'), controller.get('secondaryTag.id')])); + c.set('tags', _.flatten([controller.get('model.id')], controller.get('additionalTagIds'))); } }); }, diff --git a/app/assets/javascripts/discourse/templates/tags/show.hbs b/app/assets/javascripts/discourse/templates/tags/show.hbs index b652898b11..3c35139e5a 100644 --- a/app/assets/javascripts/discourse/templates/tags/show.hbs +++ b/app/assets/javascripts/discourse/templates/tags/show.hbs @@ -5,7 +5,7 @@
{{#if tagNotification}} - {{#unless secondaryTag}} + {{#unless additionalTags}} {{tag-notifications-button action="changeTagNotification" notificationLevel=tagNotification.notification_level}} {{/unless}} @@ -33,10 +33,10 @@ {{#link-to 'tags'}}{{i18n "tagging.tags"}}{{/link-to}} {{fa-icon "angle-right"}} {{discourse-tag-bound tagRecord=tag style="simple"}} - {{#if secondaryTag}} + {{#each additionalTags as |tag|}} & - {{discourse-tag-bound tagRecord=secondaryTag style="simple"}} - {{/if}} + {{discourse-tag-bound tagRecord=tag style="simple"}} + {{/each}} {{/if}}
diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 0989f5715e..87412084de 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -44,7 +44,7 @@ class TagsController < ::ApplicationController Discourse.filters.each do |filter| define_method("show_#{filter}") do @tag_id = DiscourseTagging.clean_tag(params[:tag_id]) - @secondary_tag_id = DiscourseTagging.clean_tag(params[:secondary_tag_id]) if params[:secondary_tag_id] + @additional_tags = params[:additional_tag_ids].to_s.split('/').map { |tag| DiscourseTagging.clean_tag(tag) } page = params[:page].to_i list_opts = build_topic_list_options @@ -72,7 +72,7 @@ class TagsController < ::ApplicationController @list.more_topics_url = construct_url_with(:next, list_opts) @list.prev_topics_url = construct_url_with(:prev, list_opts) @rss = "tag" - @description_meta = I18n.t("rss_by_tag", tag: [@tag_id, @secondary_tag_id].compact.join(' & ')) + @description_meta = I18n.t("rss_by_tag", tag: tag_params.join(' & ')) @title = @description_meta canonical_url "#{Discourse.base_url_no_prefix}#{public_send(url_method(params.slice(:category, :parent_category)))}" @@ -298,7 +298,7 @@ class TagsController < ::ApplicationController if params[:tag_id] == 'none' options[:no_tags] = true else - options[:tags] = [params[:tag_id], params[:secondary_tag_id]].compact + options[:tags] = tag_params options[:match_all_tags] = true end @@ -317,4 +317,8 @@ class TagsController < ::ApplicationController raise Discourse::NotFound end end + + def tag_params + [@tag_id].concat(Array(@additional_tags)) + end end diff --git a/config/routes.rb b/config/routes.rb index d38ad905f4..05b39f1ed2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -630,7 +630,7 @@ Discourse::Application.routes.draw do get '/:tag_id' => 'tags#show', as: 'tag_show' get '/c/:category/:tag_id' => 'tags#show', as: 'tag_category_show' get '/c/:parent_category/:category/:tag_id' => 'tags#show', as: 'tag_parent_category_category_show' - get '/intersection/:tag_id/:secondary_tag_id' => 'tags#show', as: 'tag_intersection' + get '/intersection/:tag_id/*additional_tag_ids' => 'tags#show', as: 'tag_intersection' get '/:tag_id/notifications' => 'tags#notifications' put '/:tag_id/notifications' => 'tags#update_notifications' put '/:tag_id' => 'tags#update' diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 902b42c4a3..bdae2033b8 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -464,10 +464,16 @@ class TopicQuery if @options[:match_all_tags] # ALL of the given tags: + tags_count = @options[:tags].length @options[:tags] = Tag.where(name: @options[:tags]).pluck(:id) unless @options[:tags][0].is_a?(Integer) - @options[:tags].each_with_index do |tag, index| - sql_alias = ['t', index].join - result = result.joins("INNER JOIN topic_tags #{sql_alias} ON #{sql_alias}.topic_id = topics.id AND #{sql_alias}.tag_id = #{tag}") + + if tags_count == @options[:tags].length + @options[:tags].each_with_index do |tag, index| + sql_alias = ['t', index].join + result = result.joins("INNER JOIN topic_tags #{sql_alias} ON #{sql_alias}.topic_id = topics.id AND #{sql_alias}.tag_id = #{tag}") + end + else + result = result.none # don't return any results unless all tags exist in the database end else # ANY of the given tags: diff --git a/spec/controllers/tags_controller_spec.rb b/spec/controllers/tags_controller_spec.rb index fa814fef60..cc0ad376cd 100644 --- a/spec/controllers/tags_controller_spec.rb +++ b/spec/controllers/tags_controller_spec.rb @@ -4,11 +4,13 @@ describe TagsController do describe 'show_latest' do let(:tag) { Fabricate(:tag) } let(:other_tag) { Fabricate(:tag) } + let(:third_tag) { Fabricate(:tag) } let(:category) { Fabricate(:category) } let(:subcategory) { Fabricate(:category, parent_category_id: category.id) } let(:single_tag_topic) { Fabricate(:topic, tags: [tag]) } let(:multi_tag_topic) { Fabricate(:topic, tags: [tag, other_tag]) } + let(:all_tag_topic) { Fabricate(:topic, tags: [tag, other_tag, third_tag]) } context 'tagging disabled' do it "returns 404" do @@ -27,14 +29,31 @@ describe TagsController do expect(response).to be_success end - it "can filter by multiple tags" do - single_tag_topic; multi_tag_topic - xhr :get, :show_latest, tag_id: tag.name, secondary_tag_id: other_tag.name + it "can filter by two tags" do + single_tag_topic; multi_tag_topic; all_tag_topic + xhr :get, :show_latest, tag_id: tag.name, additional_tag_ids: other_tag.name expect(response).to be_success + expect(assigns(:list).topics).to include all_tag_topic expect(assigns(:list).topics).to include multi_tag_topic expect(assigns(:list).topics).to_not include single_tag_topic end + it "can filter by multiple tags" do + single_tag_topic; multi_tag_topic; all_tag_topic + xhr :get, :show_latest, tag_id: tag.name, additional_tag_ids: "#{other_tag.name}/#{third_tag.name}" + expect(response).to be_success + expect(assigns(:list).topics).to include all_tag_topic + expect(assigns(:list).topics).to_not include multi_tag_topic + expect(assigns(:list).topics).to_not include single_tag_topic + end + + it "does not find any tags when a tag which doesn't exist is passed" do + single_tag_topic + xhr :get, :show_latest, tag_id: tag.name, additional_tag_ids: "notatag" + expect(response).to be_success + expect(assigns(:list).topics).to_not include single_tag_topic + end + it "can filter by category and tag" do xhr :get, :show_latest, tag_id: tag.name, category: category.slug expect(response).to be_success From 5dd9009718b2216c746cd8f869fd4901e94d71f7 Mon Sep 17 00:00:00 2001 From: James Kiesel Date: Mon, 15 Aug 2016 15:36:28 -0400 Subject: [PATCH 6/8] Clean up additionalTags impl --- .../javascripts/discourse/routes/tags-show.js.es6 | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/discourse/routes/tags-show.js.es6 b/app/assets/javascripts/discourse/routes/tags-show.js.es6 index 4fab00b935..79e6ea987a 100644 --- a/app/assets/javascripts/discourse/routes/tags-show.js.es6 +++ b/app/assets/javascripts/discourse/routes/tags-show.js.es6 @@ -15,10 +15,9 @@ export default Discourse.Route.extend({ f = ''; if (params.additional_tags) { - this.set("additionalTags", _.compact(params.additional_tags.split('/')).map((tag) => { - return this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(tag) }); + this.set("additionalTags", params.additional_tags.split('/').map((tag) => { + return this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(tag) }).id; })); - this.set("additionalTagIds", _.pluck(this.get("additionalTags"), 'id')); } if (params.category) { @@ -63,8 +62,8 @@ export default Discourse.Route.extend({ } this.set('category', category); - } else if (_.any(this.get("additionalTags"))) { - params.filter = `tags/intersection/${tag_id}/${this.get('additionalTagIds').join('/')}`; + } else if (this.get("additionalTags")) { + params.filter = `tags/intersection/${tag_id}/${this.get('additionalTags').join('/')}`; this.set('category', null); } else { params.filter = `tags/${tag_id}/l/${filter}`; @@ -134,7 +133,7 @@ export default Discourse.Route.extend({ // Pre-fill the tags input field if (controller.get('model.id')) { var c = self.controllerFor('composer').get('model'); - c.set('tags', _.flatten([controller.get('model.id')], controller.get('additionalTagIds'))); + c.set('tags', _.flatten([controller.get('model.id')], controller.get('additionalTags'))); } }); }, From 554d15fdd406c77b1865abe8012d52dce579d146 Mon Sep 17 00:00:00 2001 From: James Kiesel Date: Mon, 15 Aug 2016 15:42:06 -0400 Subject: [PATCH 7/8] Add extra spec for topic_query --- spec/components/topic_query_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index 32b8d81442..6506377d49 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -149,6 +149,10 @@ describe TopicQuery do expect(TopicQuery.new(moderator, tags: [tag.name, other_tag.name], match_all_tags: true).list_latest.topics.map(&:id)).to eq([tagged_topic3.id]) end + it "returns an empty relation when an invalid tag is passed" do + expect(TopicQuery.new(moderator, tags: [tag.name, 'notatag'], match_all_tags: true).list_latest.topics).to be_empty + end + it "can return topics with no tags" do expect(TopicQuery.new(moderator, no_tags: true).list_latest.topics.map(&:id)).to eq([no_tags_topic.id]) end From 98d300e79c2d2d51a97bc84b0af3beb366a992c0 Mon Sep 17 00:00:00 2001 From: James Kiesel Date: Mon, 15 Aug 2016 15:45:23 -0400 Subject: [PATCH 8/8] Fix linting error --- app/assets/javascripts/discourse/routes/tags-show.js.es6 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/discourse/routes/tags-show.js.es6 b/app/assets/javascripts/discourse/routes/tags-show.js.es6 index 79e6ea987a..d5e27ad405 100644 --- a/app/assets/javascripts/discourse/routes/tags-show.js.es6 +++ b/app/assets/javascripts/discourse/routes/tags-show.js.es6 @@ -15,8 +15,8 @@ export default Discourse.Route.extend({ f = ''; if (params.additional_tags) { - this.set("additionalTags", params.additional_tags.split('/').map((tag) => { - return this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(tag) }).id; + this.set("additionalTags", params.additional_tags.split('/').map((t) => { + return this.store.createRecord("tag", { id: Handlebars.Utils.escapeExpression(t) }).id; })); }