diff --git a/app/assets/javascripts/discourse/components/tag-chooser.js.es6 b/app/assets/javascripts/discourse/components/tag-chooser.js.es6 index d9b3863cf..4f33e3b6d 100644 --- a/app/assets/javascripts/discourse/components/tag-chooser.js.es6 +++ b/app/assets/javascripts/discourse/components/tag-chooser.js.es6 @@ -36,12 +36,19 @@ export default Ember.TextField.extend({ const site = this.site, self = this, filterRegexp = new RegExp(this.site.tags_filter_regexp, "g"); + var limit = this.siteSettings.max_tags_per_topic; + + if (this.get('unlimitedTagCount')) { + limit = null; + } else if (this.get('limit')) { + limit = parseInt(this.get('limit')); + } this.$().select2({ tags: true, placeholder: I18n.t(this.get('placeholderKey') || 'tagging.choose_for_topic'), maximumInputLength: this.siteSettings.max_tag_length, - maximumSelectionSize: self.get('unlimitedTagCount') ? null : this.siteSettings.max_tags_per_topic, + maximumSelectionSize: limit, initSelection(element, callback) { const data = []; @@ -92,7 +99,12 @@ export default Ember.TextField.extend({ url: Discourse.getURL("/tags/filter/search"), dataType: 'json', data: function (term) { - const d = { q: term, limit: self.siteSettings.max_tag_search_results, categoryId: self.get('categoryId') }; + const d = { + q: term, + limit: self.siteSettings.max_tag_search_results, + categoryId: self.get('categoryId'), + selected_tags: self.get('tags') + }; if (!self.get('everyTag')) { d.filterForInput = true; } diff --git a/app/assets/javascripts/discourse/models/tag-group.js.es6 b/app/assets/javascripts/discourse/models/tag-group.js.es6 index 6ab079c37..67ee9b63e 100644 --- a/app/assets/javascripts/discourse/models/tag-group.js.es6 +++ b/app/assets/javascripts/discourse/models/tag-group.js.es6 @@ -20,7 +20,9 @@ const TagGroup = RestModel.extend({ return Discourse.ajax(url, { data: { name: this.get('name'), - tag_names: this.get('tag_names') + tag_names: this.get('tag_names'), + parent_tag_name: this.get('parent_tag_name') ? this.get('parent_tag_name') : undefined, + one_per_topic: this.get('one_per_topic') }, type: this.get('id') ? 'PUT' : 'POST' }).then(function(result) { diff --git a/app/assets/javascripts/discourse/templates/tag-groups-show.hbs b/app/assets/javascripts/discourse/templates/tag-groups-show.hbs index aec3ce6e5..58b80d565 100644 --- a/app/assets/javascripts/discourse/templates/tag-groups-show.hbs +++ b/app/assets/javascripts/discourse/templates/tag-groups-show.hbs @@ -1,13 +1,25 @@

{{text-field value=model.name}}


-
- -
- {{tag-chooser tags=model.tag_names everyTag="false" unlimitedTagCount="true"}} -
-
+
+
+ {{tag-chooser tags=model.tag_names everyTag="true" unlimitedTagCount="true"}} +
+ +
+ + {{tag-chooser tags=model.parent_tag_name everyTag="true" limit="1" placeholderKey="tagging.groups.parent_tag_placeholder"}} + {{i18n 'tagging.groups.parent_tag_description'}} +
+ +
+ +
+ - + {{model.savingStatus}}
\ No newline at end of file diff --git a/app/assets/stylesheets/common/base/tagging.scss b/app/assets/stylesheets/common/base/tagging.scss index 3d90c7709..ff4cf495b 100644 --- a/app/assets/stylesheets/common/base/tagging.scss +++ b/app/assets/stylesheets/common/base/tagging.scss @@ -222,6 +222,14 @@ header .discourse-tag {color: $tag-color !important; } .tag-group-content { width: 75%; float: right; + section { + margin-bottom: 20px; + } + label { + font-size: 1em; + display: inline-block; + margin-right: 10px; + } } .group-tags-list .tag-chooser { height: 150px !important; @@ -233,4 +241,10 @@ header .discourse-tag {color: $tag-color !important; } .saving { margin-left: 20px; } + .parent-tag-section { + .tag-chooser { + width: 200px; + margin-right: 10px; + } + } } \ No newline at end of file diff --git a/app/controllers/tag_groups_controller.rb b/app/controllers/tag_groups_controller.rb index 4bfc943a1..36d955268 100644 --- a/app/controllers/tag_groups_controller.rb +++ b/app/controllers/tag_groups_controller.rb @@ -69,7 +69,10 @@ class TagGroupsController < ApplicationController end def tag_groups_params - params[:tag_names] ||= [] - params.permit(:id, :name, :tag_names => []) + result = params.permit(:id, :name, :one_per_topic, :tag_names => [], :parent_tag_name => []) + result[:tag_names] ||= [] + result[:parent_tag_name] ||= [] + result[:one_per_topic] = (params[:one_per_topic] == "true") + result end end diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 7fffed3b5..a476c204d 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -117,7 +117,12 @@ class TagsController < ::ApplicationController tags_with_counts = DiscourseTagging.filter_allowed_tags( self.class.tags_by_count(guardian, params.slice(:limit)), guardian, - { for_input: params[:filterForInput], term: params[:q], category: category } + { + for_input: params[:filterForInput], + term: params[:q], + category: category, + selected_tags: params[:selected_tags] + } ) tags = tags_with_counts.count.map {|t, c| { id: t, text: t, count: c } } @@ -125,7 +130,7 @@ class TagsController < ::ApplicationController unused_tags = DiscourseTagging.filter_allowed_tags( Tag.where(topic_count: 0), guardian, - { for_input: params[:filterForInput], term: params[:q], category: category } + { for_input: params[:filterForInput], term: params[:q], category: category, selected_tags: params[:selected_tags] } ) unused_tags.each do |t| diff --git a/app/models/tag_group.rb b/app/models/tag_group.rb index dea1199a6..484619793 100644 --- a/app/models/tag_group.rb +++ b/app/models/tag_group.rb @@ -6,7 +6,19 @@ class TagGroup < ActiveRecord::Base has_many :category_tag_groups, dependent: :destroy has_many :categories, through: :category_tag_groups + belongs_to :parent_tag, class_name: 'Tag' + def tag_names=(tag_names_arg) DiscourseTagging.add_or_create_tags_by_name(self, tag_names_arg) end + + def parent_tag_name=(tag_names_arg) + if tag_names_arg.empty? + self.parent_tag = nil + else + if tag_name = DiscourseTagging.tags_for_saving(tag_names_arg, Guardian.new(Discourse.system_user)).first + self.parent_tag = Tag.find_by_name(tag_name) || Tag.create(name: tag_name) + end + end + end end diff --git a/app/models/tag_group_membership.rb b/app/models/tag_group_membership.rb index 0fee1fdcf..eaa9c5178 100644 --- a/app/models/tag_group_membership.rb +++ b/app/models/tag_group_membership.rb @@ -1,4 +1,4 @@ class TagGroupMembership < ActiveRecord::Base belongs_to :tag - belongs_to :tag_group, counter_cache: "tag_count" + belongs_to :tag_group, counter_cache: "tag_count" # TODO: remove counter cache end diff --git a/app/serializers/tag_group_serializer.rb b/app/serializers/tag_group_serializer.rb index 0aa0f412d..560eef34b 100644 --- a/app/serializers/tag_group_serializer.rb +++ b/app/serializers/tag_group_serializer.rb @@ -1,7 +1,11 @@ class TagGroupSerializer < ApplicationSerializer - attributes :id, :name, :tag_names + attributes :id, :name, :tag_names, :parent_tag_name, :one_per_topic def tag_names object.tags.pluck(:name).sort end + + def parent_tag_name + [object.parent_tag.try(:name)].compact + end end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index b7647e2b7..433b9c4a1 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3015,6 +3015,10 @@ en: about: "Add tags to groups to manage them more easily." new: "New Group" tags_label: "Tags in this group:" + parent_tag_label: "Parent tag:" + parent_tag_placeholder: "Optional" + parent_tag_description: "Tags from this group can't be used unless the parent tag is present." + one_per_topic_label: "Limit one tag per topic from this group" new_name: "New Tag Group" save: "Save" delete: "Delete" diff --git a/db/migrate/20160607213656_add_tag_group_options.rb b/db/migrate/20160607213656_add_tag_group_options.rb new file mode 100644 index 000000000..54f0c6050 --- /dev/null +++ b/db/migrate/20160607213656_add_tag_group_options.rb @@ -0,0 +1,6 @@ +class AddTagGroupOptions < ActiveRecord::Migration + def change + add_column :tag_groups, :parent_tag_id, :integer + add_column :tag_groups, :one_per_topic, :boolean, default: false + end +end diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 72ef4aecd..f2bb69921 100644 --- a/lib/discourse_tagging.rb +++ b/lib/discourse_tagging.rb @@ -33,7 +33,7 @@ module DiscourseTagging if tag_names.present? category = topic.category - tags = filter_allowed_tags(Tag.where(name: tag_names), guardian, { for_input: true, category: category }).to_a + tags = filter_allowed_tags(Tag.where(name: tag_names), guardian, { for_input: true, category: category, selected_tags: tag_names }).to_a if tags.size < tag_names.size && (category.nil? || category.tags.count == 0) tag_names.each do |name| @@ -56,8 +56,9 @@ module DiscourseTagging # Options: # term: a search term to filter tags by name - # for_input: result is for an input field, so only show permitted tags # category: a Category to which the object being tagged belongs + # for_input: result is for an input field, so only show permitted tags + # selected_tags: an array of tag names that are in the current selection def self.filter_allowed_tags(query, guardian, opts={}) term = opts[:term] if term.present? @@ -67,6 +68,8 @@ module DiscourseTagging end if opts[:for_input] + selected_tag_ids = opts[:selected_tags] ? Tag.where(name: opts[:selected_tags]).pluck(:id) : [] + unless guardian.is_staff? staff_tag_names = SiteSetting.staff_tags.split("|") query = query.where('tags.name NOT IN (?)', staff_tag_names) if staff_tag_names.present? @@ -74,20 +77,24 @@ module DiscourseTagging # Filters for category-specific tags: - if opts[:category] && (opts[:category].tags.count > 0 || opts[:category].tag_groups.count > 0) - if opts[:category].tags.count > 0 && opts[:category].tag_groups.count > 0 - tag_group_ids = opts[:category].tag_groups.pluck(:id) + category = opts[:category] + + if category && (category.tags.count > 0 || category.tag_groups.count > 0) + if category.tags.count > 0 && category.tag_groups.count > 0 + tag_group_ids = category.tag_groups.pluck(:id) + query = query.where( "tags.id IN (SELECT tag_id FROM category_tags WHERE category_id = ? UNION - SELECT tag_id FROM tag_group_memberships WHERE tag_group_id = ?)", - opts[:category].id, tag_group_ids + SELECT tag_id FROM tag_group_memberships WHERE tag_group_id IN (?))", + category.id, tag_group_ids ) - elsif opts[:category].tags.count > 0 - query = query.where("tags.id IN (SELECT tag_id FROM category_tags WHERE category_id = ?)", opts[:category].id) - else # opts[:category].tag_groups.count > 0 - tag_group_ids = opts[:category].tag_groups.pluck(:id) - query = query.where("tags.id IN (SELECT tag_id FROM tag_group_memberships WHERE tag_group_id = ?)", tag_group_ids) + elsif category.tags.count > 0 + query = query.where("tags.id IN (SELECT tag_id FROM category_tags WHERE category_id = ?)", category.id) + else # category.tag_groups.count > 0 + tag_group_ids = category.tag_groups.pluck(:id) + + query = query.where("tags.id IN (SELECT tag_id FROM tag_group_memberships WHERE tag_group_id IN (?))", tag_group_ids) end else # exclude tags that are restricted to other categories @@ -97,7 +104,41 @@ module DiscourseTagging if CategoryTagGroup.exists? tag_group_ids = CategoryTagGroup.pluck(:tag_group_id).uniq - query = query.where("tags.id NOT IN (SELECT tag_id FROM tag_group_memberships WHERE tag_group_id = ?)", tag_group_ids) + query = query.where("tags.id NOT IN (SELECT tag_id FROM tag_group_memberships WHERE tag_group_id IN (?))", tag_group_ids) + end + end + + # exclude tag groups that have a parent tag which is missing from selected_tags + + select_sql = <<-SQL + SELECT tag_id + FROM tag_group_memberships tgm + INNER JOIN tag_groups tg + ON tgm.tag_group_id = tg.id + SQL + + if selected_tag_ids.empty? + sql = "tags.id NOT IN (#{select_sql} WHERE tg.parent_tag_id IS NOT NULL)" + query = query.where(sql) + else + # One tag per group restriction + exclude_group_ids = TagGroup.where(one_per_topic: true) + .joins(:tag_group_memberships) + .where('tag_group_memberships.tag_id in (?)', selected_tag_ids) + .pluck(:id) + + if exclude_group_ids.empty? + sql = "tags.id NOT IN (#{select_sql} WHERE tg.parent_tag_id NOT IN (?))" + query = query.where(sql, selected_tag_ids) + else + # It's possible that the selected tags violate some one-tag-per-group restrictions, + # so filter them out by picking one from each group. + limit_tag_ids = TagGroupMembership.select('distinct on (tag_group_id) tag_id') + .where(tag_id: selected_tag_ids) + .where(tag_group_id: exclude_group_ids) + .map(&:tag_id) + sql = "(tags.id NOT IN (#{select_sql} WHERE (tg.parent_tag_id NOT IN (?) OR tg.id in (?))) OR tags.id IN (?))" + query = query.where(sql, selected_tag_ids, exclude_group_ids, limit_tag_ids) end end end diff --git a/spec/fabricators/tag_fabricator.rb b/spec/fabricators/tag_fabricator.rb index 45940081a..5c61060c8 100644 --- a/spec/fabricators/tag_fabricator.rb +++ b/spec/fabricators/tag_fabricator.rb @@ -1,3 +1,3 @@ Fabricator(:tag) do - name { sequence(:name) { |i| "tag#{i}" } } + name { sequence(:name) { |i| "tag#{i+1}" } } end diff --git a/spec/integration/category_tag_spec.rb b/spec/integration/category_tag_spec.rb index 9b2f0c3a9..047056ced 100644 --- a/spec/integration/category_tag_spec.rb +++ b/spec/integration/category_tag_spec.rb @@ -9,6 +9,10 @@ describe "category tag restrictions" do tag_records.map(&:name).sort end + def filter_allowed_tags(opts={}) + DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), opts) + end + let!(:tag1) { Fabricate(:tag) } let!(:tag2) { Fabricate(:tag) } let!(:tag3) { Fabricate(:tag) } @@ -40,9 +44,9 @@ describe "category tag restrictions" do end it "search can show only permitted tags" do - expect(DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user)).count).to eq(Tag.count) - expect(DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), {for_input: true, category: category_with_tags}).pluck(:name).sort).to eq([tag1.name, tag2.name].sort) - expect(DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), {for_input: true}).pluck(:name).sort).to eq([tag3.name, tag4.name].sort) + expect(filter_allowed_tags.count).to eq(Tag.count) + expect(filter_allowed_tags({for_input: true, category: category_with_tags}).pluck(:name).sort).to eq([tag1.name, tag2.name].sort) + expect(filter_allowed_tags({for_input: true}).pluck(:name).sort).to eq([tag3.name, tag4.name].sort) end it "can't create new tags in a restricted category" do @@ -76,13 +80,13 @@ describe "category tag restrictions" do category.allowed_tag_groups = [tag_group1.name] category.reload - expect(sorted_tag_names(DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), {for_input: true, category: category}))).to eq(sorted_tag_names([tag1, tag2])) - expect(sorted_tag_names(DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), {for_input: true}))).to eq(sorted_tag_names([tag3, tag4])) + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: category}))).to eq(sorted_tag_names([tag1, tag2])) + expect(sorted_tag_names(filter_allowed_tags({for_input: true}))).to eq(sorted_tag_names([tag3, tag4])) tag_group1.tags = [tag2, tag3, tag4] - expect(sorted_tag_names(DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), {for_input: true, category: category}))).to eq(sorted_tag_names([tag2, tag3, tag4])) - expect(sorted_tag_names(DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), {for_input: true}))).to eq(sorted_tag_names([tag1])) - expect(sorted_tag_names(DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), {for_input: true, category: other_category}))).to eq(sorted_tag_names([tag1])) + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: category}))).to eq(sorted_tag_names([tag2, tag3, tag4])) + expect(sorted_tag_names(filter_allowed_tags({for_input: true}))).to eq(sorted_tag_names([tag1])) + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: other_category}))).to eq(sorted_tag_names([tag1])) end it "groups and individual tags can be mixed" do @@ -90,9 +94,92 @@ describe "category tag restrictions" do category.allowed_tags = [tag4.name] category.reload - expect(sorted_tag_names(DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), {for_input: true, category: category}))).to eq(sorted_tag_names([tag1, tag2, tag4])) - expect(sorted_tag_names(DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), {for_input: true}))).to eq(sorted_tag_names([tag3])) - expect(sorted_tag_names(DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), {for_input: true, category: other_category}))).to eq(sorted_tag_names([tag3])) + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: category}))).to eq(sorted_tag_names([tag1, tag2, tag4])) + expect(sorted_tag_names(filter_allowed_tags({for_input: true}))).to eq(sorted_tag_names([tag3])) + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: other_category}))).to eq(sorted_tag_names([tag3])) + end + end + + context "tag groups with parent tag" do + it "filter_allowed_tags returns results based on whether parent tag is present or not" do + tag_group = Fabricate(:tag_group, parent_tag_id: tag1.id) + tag_group.tags = [tag3, tag4] + expect(sorted_tag_names(filter_allowed_tags({for_input: true}))).to eq(sorted_tag_names([tag1, tag2])) + expect(sorted_tag_names(filter_allowed_tags({for_input: true, selected_tags: [tag1.name]}))).to eq(sorted_tag_names([tag1, tag2, tag3, tag4])) + expect(sorted_tag_names(filter_allowed_tags({for_input: true, selected_tags: [tag1.name, tag3.name]}))).to eq(sorted_tag_names([tag1, tag2, tag3, tag4])) + end + + context "and category restrictions" do + let(:car_category) { Fabricate(:category) } + let(:other_category) { Fabricate(:category) } + let(:makes) { Fabricate(:tag_group, name: "Makes") } + let(:honda_group) { Fabricate(:tag_group, name: "Honda Models") } + let(:ford_group) { Fabricate(:tag_group, name: "Ford Models") } + + before do + @tags = {} + ['honda', 'ford', 'civic', 'accord', 'mustang', 'taurus'].each do |name| + @tags[name] = Fabricate(:tag, name: name) + end + + makes.tags = [@tags['honda'], @tags['ford']] + + honda_group.parent_tag_id = @tags['honda'].id + honda_group.save + honda_group.tags = [@tags['civic'], @tags['accord']] + + ford_group.parent_tag_id = @tags['ford'].id + ford_group.save + ford_group.tags = [@tags['mustang'], @tags['taurus']] + + car_category.allowed_tag_groups = [makes.name, honda_group.name, ford_group.name] + end + + it "handles all those rules" do + # car tags can't be used outside of car category: + expect(sorted_tag_names(filter_allowed_tags({for_input: true}))).to eq(sorted_tag_names([tag1, tag2, tag3, tag4])) + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: other_category}))).to eq(sorted_tag_names([tag1, tag2, tag3, tag4])) + + # in car category, a make tag must be given first: + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: car_category}))).to eq(['ford', 'honda']) + + # model tags depend on which make is chosen: + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: car_category, selected_tags: ['honda']}))).to eq(['accord', 'civic', 'ford', 'honda']) + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: car_category, selected_tags: ['ford']}))).to eq(['ford', 'honda', 'mustang', 'taurus']) + end + + it "can apply the tags to a topic" do + post = create_post(category: car_category, tags: ['ford', 'mustang']) + expect(post.topic.tags.map(&:name).sort).to eq(['ford', 'mustang']) + end + + context "limit one tag from each group" do + before do + makes.update_attributes(one_per_topic: true) + honda_group.update_attributes(one_per_topic: true) + ford_group.update_attributes(one_per_topic: true) + end + + it "can restrict one tag from each group" do + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: car_category}))).to eq(['ford', 'honda']) + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: car_category, selected_tags: ['honda']}))).to eq(['accord', 'civic', 'honda']) + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: car_category, selected_tags: ['ford']}))).to eq(['ford', 'mustang', 'taurus']) + expect(sorted_tag_names(filter_allowed_tags({for_input: true, category: car_category, selected_tags: ['ford', 'mustang']}))).to eq(['ford', 'mustang']) + end + + it "can apply the tags to a topic" do + post = create_post(category: car_category, tags: ['ford', 'mustang']) + expect(post.topic.tags.map(&:name).sort).to eq(['ford', 'mustang']) + end + + it "can remove extra tags from the same group" do + # A weird case that input field wouldn't allow. + # Only one tag from car makers is allowed, but we're saying that two have been selected. + names = filter_allowed_tags({for_input: true, category: car_category, selected_tags: ['honda', 'ford']}).map(&:name) + expect(names.include?('honda') && names.include?('ford')).to eq(false) + expect(names.include?('honda') || names.include?('ford')).to eq(true) + end + end end end end