From 9ca6ebe8fe0e1a3bea8af23082705dc97ef6315e Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Thu, 29 Mar 2018 00:10:26 +0530 Subject: [PATCH 1/2] FEATURE: enforce tagging on categories --- .../discourse/models/category.js.es6 | 3 +- .../components/edit-category-settings.hbs | 9 +++ app/assets/stylesheets/common/base/modal.scss | 19 +++--- app/controllers/categories_controller.rb | 1 + app/models/category.rb | 3 +- app/models/post_custom_field.rb | 1 + app/models/shared_draft.rb | 16 +++++ app/models/tag_group_permission.rb | 17 ++++++ app/models/web_crawler_request.rb | 14 +++++ app/serializers/basic_category_serializer.rb | 3 +- config/locales/client.en.yml | 1 + config/locales/server.en.yml | 1 + ...add_minimum_required_tags_to_categories.rb | 5 ++ lib/discourse_tagging.rb | 12 +++- lib/topic_creator.rb | 19 ++++-- spec/components/discourse_tagging_spec.rb | 45 ++++++++++++++ spec/components/topic_creator_spec.rb | 59 +++++++++++++++++++ 17 files changed, 209 insertions(+), 19 deletions(-) create mode 100644 db/migrate/20180331125522_add_minimum_required_tags_to_categories.rb diff --git a/app/assets/javascripts/discourse/models/category.js.es6 b/app/assets/javascripts/discourse/models/category.js.es6 index 1eca95047e9..e30a2ba7dd2 100644 --- a/app/assets/javascripts/discourse/models/category.js.es6 +++ b/app/assets/javascripts/discourse/models/category.js.es6 @@ -108,7 +108,8 @@ const Category = RestModel.extend({ num_featured_topics: this.get('num_featured_topics'), default_view: this.get('default_view'), subcategory_list_style: this.get('subcategory_list_style'), - default_top_period: this.get('default_top_period') + default_top_period: this.get('default_top_period'), + minimum_required_tags: this.get('minimum_required_tags') }, type: id ? 'PUT' : 'POST' }); diff --git a/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs b/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs index 2c9735d40cc..2e657aa1562 100644 --- a/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs +++ b/app/assets/javascripts/discourse/templates/components/edit-category-settings.hbs @@ -152,4 +152,13 @@ {{/unless}} +{{#if siteSettings.tagging_enabled}} +
+ +
+{{/if}} + {{plugin-outlet name="category-custom-settings" args=(hash category=category)}} diff --git a/app/assets/stylesheets/common/base/modal.scss b/app/assets/stylesheets/common/base/modal.scss index f4c70b4b193..cd5fe5fabb3 100644 --- a/app/assets/stylesheets/common/base/modal.scss +++ b/app/assets/stylesheets/common/base/modal.scss @@ -29,13 +29,13 @@ .modal-header { display: flex; align-items: center; - padding: 10px 15px; + padding: 10px 15px; border-bottom: 1px solid $primary-low; h3 { margin-bottom: 0; } .modal-close { - order: 2; + order: 2; margin-left: auto; } } @@ -372,6 +372,12 @@ } } } + + section.minimum-required-tags { + input[type=number] { + width: 50px; + } + } } .incoming-email-modal { @@ -421,22 +427,22 @@ } .change-timestamp { - + .date-picker { width: 10em; } - + #date-container { .pika-single { position: relative !important; // overriding another important display: inline-block; } } - + input[type=time] { width: 6em; } - + form { margin: 0; } @@ -491,4 +497,3 @@ width: 95%; } - \ No newline at end of file diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index e2fa98fda1a..6fb2bd07ccc 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -283,6 +283,7 @@ class CategoriesController < ApplicationController :default_view, :subcategory_list_style, :default_top_period, + :minimum_required_tags, custom_fields: [params[:custom_fields].try(:keys)], permissions: [*p.try(:keys)], allowed_tags: [], diff --git a/app/models/category.rb b/app/models/category.rb index 426da630adb..432509ded68 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -573,10 +573,11 @@ end # default_top_period :string(20) default("all") # mailinglist_mirror :boolean default(FALSE), not null # suppress_from_latest :boolean default(FALSE) +# minimum_required_tags :integer default(0) # # Indexes # # index_categories_on_email_in (email_in) UNIQUE # index_categories_on_topic_count (topic_count) -# unique_index_categories_on_name ((COALESCE(parent_category_id, '-1'::integer)), name) UNIQUE +# unique_index_categories_on_name (COALESCE(parent_category_id, '-1'::integer), name) UNIQUE # diff --git a/app/models/post_custom_field.rb b/app/models/post_custom_field.rb index 999500de527..1eedcc88486 100644 --- a/app/models/post_custom_field.rb +++ b/app/models/post_custom_field.rb @@ -15,6 +15,7 @@ end # # Indexes # +# idx_post_custom_fields_akismet (post_id) # index_post_custom_fields_on_name_and_value (name, "left"(value, 200)) # index_post_custom_fields_on_post_id_and_name (post_id,name) # diff --git a/app/models/shared_draft.rb b/app/models/shared_draft.rb index 3f4dccee95a..a8a1b93b89e 100644 --- a/app/models/shared_draft.rb +++ b/app/models/shared_draft.rb @@ -2,3 +2,19 @@ class SharedDraft < ActiveRecord::Base belongs_to :topic belongs_to :category end + +# == Schema Information +# +# Table name: shared_drafts +# +# topic_id :integer not null +# category_id :integer not null +# created_at :datetime not null +# updated_at :datetime not null +# id :integer not null, primary key +# +# Indexes +# +# index_shared_drafts_on_category_id (category_id) +# index_shared_drafts_on_topic_id (topic_id) UNIQUE +# diff --git a/app/models/tag_group_permission.rb b/app/models/tag_group_permission.rb index 2d77af228ff..bec35735653 100644 --- a/app/models/tag_group_permission.rb +++ b/app/models/tag_group_permission.rb @@ -7,3 +7,20 @@ class TagGroupPermission < ActiveRecord::Base @permission_types ||= Enum.new(full: 1) #, see: 2 end end + +# == Schema Information +# +# Table name: tag_group_permissions +# +# id :integer not null, primary key +# tag_group_id :integer not null +# group_id :integer not null +# permission_type :integer default(1), not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_tag_group_permissions_on_group_id (group_id) +# index_tag_group_permissions_on_tag_group_id (tag_group_id) +# diff --git a/app/models/web_crawler_request.rb b/app/models/web_crawler_request.rb index edcad4c9ad3..8f082222d82 100644 --- a/app/models/web_crawler_request.rb +++ b/app/models/web_crawler_request.rb @@ -74,3 +74,17 @@ class WebCrawlerRequest < ActiveRecord::Base request_id(date: date, user_agent: user_agent) end end + +# == Schema Information +# +# Table name: web_crawler_requests +# +# id :integer not null, primary key +# date :date not null +# user_agent :string not null +# count :integer default(0), not null +# +# Indexes +# +# index_web_crawler_requests_on_date_and_user_agent (date,user_agent) UNIQUE +# diff --git a/app/serializers/basic_category_serializer.rb b/app/serializers/basic_category_serializer.rb index 01a088976ee..4149a32cf26 100644 --- a/app/serializers/basic_category_serializer.rb +++ b/app/serializers/basic_category_serializer.rb @@ -24,7 +24,8 @@ class BasicCategorySerializer < ApplicationSerializer :num_featured_topics, :default_view, :subcategory_list_style, - :default_top_period + :default_top_period, + :minimum_required_tags has_one :uploaded_logo, embed: :object, serializer: CategoryUploadSerializer has_one :uploaded_background, embed: :object, serializer: CategoryUploadSerializer diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index a3c56dd2594..d17851eaa25 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2267,6 +2267,7 @@ en: default_position: "Default Position" position_disabled: "Categories will be displayed in order of activity. To control the order of categories in lists, " position_disabled_click: 'enable the "fixed category positions" setting.' + minimum_required_tags: 'Minimum number of tags required in a topic:' parent: "Parent Category" notifications: watching: diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 89a9d05f484..dbd732ff388 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -3632,6 +3632,7 @@ en: title: "Tags" staff_tag_disallowed: "The tag \"%{tag}\" may only be applied by staff." staff_tag_remove_disallowed: "The tag \"%{tag}\" may only be removed by staff." + minimum_required_tags: "You must select at least %{count} tags." rss_by_tag: "Topics tagged %{tag}" finish_installation: diff --git a/db/migrate/20180331125522_add_minimum_required_tags_to_categories.rb b/db/migrate/20180331125522_add_minimum_required_tags_to_categories.rb new file mode 100644 index 00000000000..646e370d012 --- /dev/null +++ b/db/migrate/20180331125522_add_minimum_required_tags_to_categories.rb @@ -0,0 +1,5 @@ +class AddMinimumRequiredTagsToCategories < ActiveRecord::Migration[5.1] + def change + add_column :categories, :minimum_required_tags, :integer, default: 0 + end +end diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 2ae28445d22..c9b0e14e6ec 100644 --- a/lib/discourse_tagging.rb +++ b/lib/discourse_tagging.rb @@ -26,10 +26,16 @@ module DiscourseTagging end end - if tag_names.present? - category = topic.category - tag_names = tag_names + old_tag_names if append + category = topic.category + tag_names = tag_names + old_tag_names if append + # validate minimum required tags for a category + if !guardian.is_staff? && category && category.minimum_required_tags > 0 && tag_names.length < category.minimum_required_tags + topic.errors[:base] << I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags) + return false + end + + if tag_names.present? # guardian is explicitly nil cause we don't want to strip all # staff tags that already passed validation tags = filter_allowed_tags( diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index 2c1103ace69..6e63e6b803f 100644 --- a/lib/topic_creator.rb +++ b/lib/topic_creator.rb @@ -24,11 +24,6 @@ class TopicCreator # this allows us to add errors valid = topic.valid? - # not sure where this should go - if !@guardian.is_staff? && staff_only = DiscourseTagging.staff_only_tags(@opts[:tags]) - topic.errors[:base] << I18n.t("tags.staff_tag_disallowed", tag: staff_only.join(" ")) - end - DiscourseEvent.trigger(:after_validate_topic, topic, self) valid &&= topic.errors.empty? @@ -159,7 +154,19 @@ class TopicCreator end def setup_tags(topic) - DiscourseTagging.tag_topic_by_names(topic, @guardian, @opts[:tags]) + if @opts[:tags].blank? + unless @guardian.is_staff? + # Validate minimum required tags for a category + category = find_category + if category.present? && category.minimum_required_tags > 0 + topic.errors[:base] << I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags) + rollback_from_errors!(topic) + end + end + else + valid_tags = DiscourseTagging.tag_topic_by_names(topic, @guardian, @opts[:tags]) + rollback_from_errors!(topic) unless valid_tags + end end def setup_auto_close_time(topic) diff --git a/spec/components/discourse_tagging_spec.rb b/spec/components/discourse_tagging_spec.rb index adc6c05a879..d9660ef2d32 100644 --- a/spec/components/discourse_tagging_spec.rb +++ b/spec/components/discourse_tagging_spec.rb @@ -75,4 +75,49 @@ describe DiscourseTagging do expect(tags).to contain_exactly(tag1, tag2, tag3) end end + + describe 'tag_topic_by_names' do + context 'staff-only tags' do + let(:topic) { Fabricate(:topic) } + + before do + SiteSetting.staff_tags = "alpha" + end + + it "regular users can't add staff-only tags" do + valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), ['alpha']) + expect(valid).to eq(false) + expect(topic.errors[:base]&.first).to eq(I18n.t("tags.staff_tag_disallowed", tag: 'alpha')) + end + + it 'staff can add staff-only tags' do + valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(admin), ['alpha']) + expect(valid).to eq(true) + expect(topic.errors[:base]).to be_empty + end + end + + context 'respects category minimum_required_tags setting' do + let(:category) { Fabricate(:category, minimum_required_tags: 2) } + let(:topic) { Fabricate(:topic, category: category) } + + it 'when tags are less than minimum_required_tags' do + valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), [tag1.name]) + expect(valid).to eq(false) + expect(topic.errors[:base]&.first).to eq(I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags)) + end + + it 'when tags are equal to minimum_required_tags' do + valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), [tag1.name, tag2.name]) + expect(valid).to eq(true) + expect(topic.errors[:base]).to be_empty + end + + it 'lets admin tag a topic regardless of minimum_required_tags' do + valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(admin), [tag1.name]) + expect(valid).to eq(true) + expect(topic.errors[:base]).to be_empty + end + end + end end diff --git a/spec/components/topic_creator_spec.rb b/spec/components/topic_creator_spec.rb index ceb81d4e392..d90e8e15f69 100644 --- a/spec/components/topic_creator_spec.rb +++ b/spec/components/topic_creator_spec.rb @@ -60,6 +60,65 @@ describe TopicCreator do end end + context 'tags' do + let!(:tag1) { Fabricate(:tag, name: "fun") } + let!(:tag2) { Fabricate(:tag, name: "fun2") } + + before do + SiteSetting.tagging_enabled = true + SiteSetting.min_trust_to_create_tag = 0 + SiteSetting.min_trust_level_to_tag_topics = 0 + end + + context 'regular tags' do + it "user can add tags to topic" do + topic = TopicCreator.create(user, Guardian.new(user), valid_attrs.merge(tags: [tag1.name])) + expect(topic).to be_valid + expect(topic.tags.length).to eq(1) + end + end + + context 'staff-only tags' do + before do + SiteSetting.staff_tags = "alpha" + end + + it "regular users can't add staff-only tags" do + expect do + TopicCreator.create(user, Guardian.new(user), valid_attrs.merge(tags: ['alpha'])) + end.to raise_error(ActiveRecord::Rollback) + end + + it 'staff can add staff-only tags' do + topic = TopicCreator.create(admin, Guardian.new(admin), valid_attrs.merge(tags: ['alpha'])) + expect(topic).to be_valid + expect(topic.tags.length).to eq(1) + end + end + + context 'minimum_required_tags is present' do + let!(:category) { Fabricate(:category, name: "beta", minimum_required_tags: 2) } + + it "fails for regular user if minimum_required_tags is not satisfied" do + expect do + TopicCreator.create(user, Guardian.new(user), valid_attrs.merge(category: "beta")) + end.to raise_error(ActiveRecord::Rollback) + end + + it "lets admin create a topic regardless of minimum_required_tags" do + topic = TopicCreator.create(admin, Guardian.new(admin), valid_attrs.merge(tags: [tag1.name], category: "beta")) + expect(topic).to be_valid + expect(topic.tags.length).to eq(1) + end + + it "works for regular user if minimum_required_tags is satisfied" do + topic = TopicCreator.create(user, Guardian.new(user), valid_attrs.merge(tags: [tag1.name, tag2.name], category: "beta")) + expect(topic).to be_valid + expect(topic.tags.length).to eq(2) + end + end + end + context 'private message' do context 'success cases' do From 48d43b33cc63c4f51a77243a9bdacd63d6cad9cf Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Fri, 6 Apr 2018 23:15:33 +0530 Subject: [PATCH 2/2] add client side validation for category minimum_required_tags --- .../discourse/controllers/composer.js.es6 | 10 +++++- .../discourse/models/composer.js.es6 | 32 +++++++++++++++---- .../discourse/templates/composer.hbs | 3 +- config/locales/client.en.yml | 1 + lib/topic_creator.rb | 2 +- spec/components/topic_creator_spec.rb | 7 ++++ 6 files changed, 45 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/composer.js.es6 b/app/assets/javascripts/discourse/controllers/composer.js.es6 index 863836c1baf..414e117897e 100644 --- a/app/assets/javascripts/discourse/controllers/composer.js.es6 +++ b/app/assets/javascripts/discourse/controllers/composer.js.es6 @@ -779,11 +779,19 @@ export default Ember.Controller.extend({ @computed('model.categoryId', 'lastValidatedAt') categoryValidation(categoryId, lastValidatedAt) { - if( !this.siteSettings.allow_uncategorized_topics && !categoryId) { + if(!this.siteSettings.allow_uncategorized_topics && !categoryId) { return InputValidation.create({ failed: true, reason: I18n.t('composer.error.category_missing'), lastShownAt: lastValidatedAt }); } }, + @computed('model.category', 'model.tags', 'lastValidatedAt') + tagValidation(category, tags, lastValidatedAt) { + const tagsArray = tags || []; + if (this.site.get('can_tag_topics') && category && category.get('minimum_required_tags') > tagsArray.length) { + return InputValidation.create({ failed: true, reason: I18n.t('composer.error.tags_missing', {count: category.get('minimum_required_tags')}), lastShownAt: lastValidatedAt }); + } + }, + collapse() { this._saveDraft(); this.set('model.composeState', Composer.DRAFT); diff --git a/app/assets/javascripts/discourse/models/composer.js.es6 b/app/assets/javascripts/discourse/models/composer.js.es6 index 94a3e328001..8e4c6b60aed 100644 --- a/app/assets/javascripts/discourse/models/composer.js.es6 +++ b/app/assets/javascripts/discourse/models/composer.js.es6 @@ -105,6 +105,11 @@ const Composer = RestModel.extend({ return categoryId ? this.site.categories.findBy('id', categoryId) : null; }, + @computed('category') + minimumRequiredTags(category) { + return (category && category.get('minimum_required_tags') > 0) ? category.get('minimum_required_tags') : null; + }, + creatingTopic: Em.computed.equal('action', CREATE_TOPIC), creatingSharedDraft: Em.computed.equal('action', CREATE_SHARED_DRAFT), creatingPrivateMessage: Em.computed.equal('action', PRIVATE_MESSAGE), @@ -246,28 +251,41 @@ const Composer = RestModel.extend({ return options; }, - // whether to disable the post button - cantSubmitPost: function() { + @computed + isStaffUser() { + const currentUser = Discourse.User.current(); + return currentUser && currentUser.get('staff'); + }, + + @computed('loading', 'canEditTitle', 'titleLength', 'targetUsernames', 'replyLength', 'categoryId', 'missingReplyCharacters', 'tags', 'topicFirstPost', 'minimumRequiredTags', 'isStaffUser') + cantSubmitPost(loading, canEditTitle, titleLength, targetUsernames, replyLength, categoryId, missingReplyCharacters, tags, topicFirstPost, minimumRequiredTags, isStaffUser) { // can't submit while loading - if (this.get('loading')) return true; + if (loading) return true; // title is required when // - creating a new topic/private message // - editing the 1st post - if (this.get('canEditTitle') && !this.get('titleLengthValid')) return true; + if (canEditTitle && !this.get('titleLengthValid')) return true; // reply is always required - if (this.get('missingReplyCharacters') > 0) return true; + if (missingReplyCharacters > 0) return true; + + if (this.site.get('can_tag_topics') && !isStaffUser && topicFirstPost && minimumRequiredTags) { + const tagsArray = tags || []; + if (tagsArray.length < minimumRequiredTags) { + return true; + } + } if (this.get("privateMessage")) { // need at least one user when sending a PM - return this.get('targetUsernames') && (this.get('targetUsernames').trim() + ',').indexOf(',') === 0; + return targetUsernames && (targetUsernames.trim() + ',').indexOf(',') === 0; } else { // has a category? (when needed) return this.get('requiredCategoryMissing'); } - }.property('loading', 'canEditTitle', 'titleLength', 'targetUsernames', 'replyLength', 'categoryId', 'missingReplyCharacters'), + }, @computed('canCategorize', 'categoryId') requiredCategoryMissing(canCategorize, categoryId) { diff --git a/app/assets/javascripts/discourse/templates/composer.hbs b/app/assets/javascripts/discourse/templates/composer.hbs index cdda936d575..204c01ac729 100644 --- a/app/assets/javascripts/discourse/templates/composer.hbs +++ b/app/assets/javascripts/discourse/templates/composer.hbs @@ -64,7 +64,8 @@ {{/if}} {{#if canEditTags}} - {{mini-tag-chooser tags=model.tags tabindex="4" categoryId=model.categoryId}} + {{mini-tag-chooser tags=model.tags tabindex="4" categoryId=model.categoryId minimum=model.minimumRequiredTags}} + {{popup-input-tip validation=tagValidation}} {{/if}} diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index d17851eaa25..97c5e7a8e5a 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1303,6 +1303,7 @@ en: post_length: "Post must be at least {{min}} characters" try_like: 'Have you tried the button?' category_missing: "You must choose a category" + tags_missing: "You must choose at least {{count}} tags" save_edit: "Save Edit" reply_original: "Reply on Original Topic" diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index 6e63e6b803f..6394156fcb9 100644 --- a/lib/topic_creator.rb +++ b/lib/topic_creator.rb @@ -155,7 +155,7 @@ class TopicCreator def setup_tags(topic) if @opts[:tags].blank? - unless @guardian.is_staff? + unless @guardian.is_staff? || !guardian.can_tag?(topic) # Validate minimum required tags for a category category = find_category if category.present? && category.minimum_required_tags > 0 diff --git a/spec/components/topic_creator_spec.rb b/spec/components/topic_creator_spec.rb index d90e8e15f69..bb9586bfdb4 100644 --- a/spec/components/topic_creator_spec.rb +++ b/spec/components/topic_creator_spec.rb @@ -116,6 +116,13 @@ describe TopicCreator do expect(topic).to be_valid expect(topic.tags.length).to eq(2) end + + it "lets new user create a topic if they don't have sufficient trust level to tag topics" do + SiteSetting.min_trust_level_to_tag_topics = 1 + new_user = Fabricate(:newuser) + topic = TopicCreator.create(new_user, Guardian.new(new_user), valid_attrs.merge(category: "beta")) + expect(topic).to be_valid + end end end