From b380ed5282ef76f3f9a0996ab8a5786be3da362b Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Wed, 8 May 2019 10:20:51 -0400 Subject: [PATCH] FEATURE: Claim Reviewables by Topic This is a feature that used to be present in discourse-assign but is much easier to implement in core. It also allows a topic to be assigned without it claiming for review and vice versa and allows it to work with category group reviewers. --- .../reviewable-claimed-topic.js.es6 | 33 ++++++++++ .../components/reviewable-item.js.es6 | 37 +++++++++++ .../javascripts/discourse/models/store.js.es6 | 2 + .../components/reviewable-claimed-topic.hbs | 18 ++++++ .../templates/components/reviewable-item.hbs | 59 +++++++++-------- .../discourse/templates/review-topics.hbs | 1 + .../stylesheets/common/base/reviewables.scss | 25 ++++++++ .../reviewable_claimed_topics_controller.rb | 20 ++++++ app/controllers/reviewables_controller.rb | 52 +++++++++++++-- app/models/reviewable.rb | 15 +++-- app/models/reviewable_claimed_topic.rb | 14 +++++ app/models/reviewable_flagged_post.rb | 1 - app/models/reviewable_queued_post.rb | 1 - app/models/reviewable_user.rb | 1 - app/serializers/reviewable_serializer.rb | 22 +++++-- .../reviewable_topic_serializer.rb | 11 ++++ config/locales/client.en.yml | 9 +++ config/locales/server.en.yml | 3 + config/routes.rb | 2 + config/site_settings.yml | 10 +++ ...141327_create_reviewable_claimed_topics.rb | 10 +++ .../20190508141824_drop_claimed_by_id.rb | 5 ++ lib/guardian/user_guardian.rb | 4 ++ .../reviewable_claimed_topic_fabricator.rb | 6 ++ spec/models/reviewable_claimed_topic_spec.rb | 12 ++++ spec/models/reviewable_flagged_post_spec.rb | 1 + ...viewable_claimed_topics_controller_spec.rb | 63 +++++++++++++++++++ spec/requests/reviewables_controller_spec.rb | 41 +++++++++++- .../helpers/store-pretender.js.es6 | 7 ++- test/javascripts/models/store-test.js.es6 | 10 ++- 30 files changed, 448 insertions(+), 47 deletions(-) create mode 100644 app/assets/javascripts/discourse/components/reviewable-claimed-topic.js.es6 create mode 100644 app/assets/javascripts/discourse/templates/components/reviewable-claimed-topic.hbs create mode 100644 app/controllers/reviewable_claimed_topics_controller.rb create mode 100644 app/models/reviewable_claimed_topic.rb create mode 100644 db/migrate/20190508141327_create_reviewable_claimed_topics.rb create mode 100644 db/post_migrate/20190508141824_drop_claimed_by_id.rb create mode 100644 spec/fabricators/reviewable_claimed_topic_fabricator.rb create mode 100644 spec/models/reviewable_claimed_topic_spec.rb create mode 100644 spec/requests/reviewable_claimed_topics_controller_spec.rb diff --git a/app/assets/javascripts/discourse/components/reviewable-claimed-topic.js.es6 b/app/assets/javascripts/discourse/components/reviewable-claimed-topic.js.es6 new file mode 100644 index 00000000000..8d9bd6c2701 --- /dev/null +++ b/app/assets/javascripts/discourse/components/reviewable-claimed-topic.js.es6 @@ -0,0 +1,33 @@ +import computed from "ember-addons/ember-computed-decorators"; +import { popupAjaxError } from "discourse/lib/ajax-error"; +import { ajax } from "discourse/lib/ajax"; + +export default Ember.Component.extend({ + tagName: "", + + @computed + enabled() { + return this.siteSettings.reviewable_claiming !== "disabled"; + }, + + actions: { + unclaim() { + ajax(`/reviewable_claimed_topics/${this.get("topicId")}`, { + method: "DELETE" + }).then(() => { + this.set("claimedBy", null); + }); + }, + + claim() { + let claim = this.store.createRecord("reviewable-claimed-topic"); + + claim + .save({ topic_id: this.get("topicId") }) + .then(() => { + this.set("claimedBy", this.currentUser); + }) + .catch(popupAjaxError); + } + } +}); diff --git a/app/assets/javascripts/discourse/components/reviewable-item.js.es6 b/app/assets/javascripts/discourse/components/reviewable-item.js.es6 index ffce8c88a99..27e8aeef61f 100644 --- a/app/assets/javascripts/discourse/components/reviewable-item.js.es6 +++ b/app/assets/javascripts/discourse/components/reviewable-item.js.es6 @@ -18,6 +18,43 @@ export default Ember.Component.extend({ return type.dasherize(); }, + @computed("siteSettings.reviewable_claiming", "reviewable.topic") + claimEnabled(claimMode, topic) { + return claimMode !== "disabled" && !!topic; + }, + + @computed( + "claimEnabled", + "siteSettings.reviewable_claiming", + "reviewable.claimed_by" + ) + canPerform(claimEnabled, claimMode, claimedBy) { + if (!claimEnabled) { + return true; + } + + if (claimedBy) { + return claimedBy.id === this.currentUser.id; + } + + return claimMode !== "required"; + }, + + @computed("siteSettings.reviewable_claiming", "reviewable.claimed_by") + claimHelp(claimMode, claimedBy) { + if (claimedBy) { + return claimedBy.id === this.currentUser.id + ? I18n.t("review.claim_help.claimed_by_you") + : I18n.t("review.claim_help.claimed_by_other", { + username: claimedBy.username + }); + } + + return claimMode === "optional" + ? I18n.t("review.claim_help.optional") + : I18n.t("review.claim_help.required"); + }, + // Find a component to render, if one exists. For example: // `ReviewableUser` will return `reviewable-user` @computed("reviewable.type") diff --git a/app/assets/javascripts/discourse/models/store.js.es6 b/app/assets/javascripts/discourse/models/store.js.es6 index 486da7ca2c4..4e6e68dab6e 100644 --- a/app/assets/javascripts/discourse/models/store.js.es6 +++ b/app/assets/javascripts/discourse/models/store.js.es6 @@ -311,6 +311,8 @@ export default Ember.Object.extend({ if (hydrated) { obj[subType] = hydrated; delete obj[k]; + } else { + obj[subType] = null; } } } diff --git a/app/assets/javascripts/discourse/templates/components/reviewable-claimed-topic.hbs b/app/assets/javascripts/discourse/templates/components/reviewable-claimed-topic.hbs new file mode 100644 index 00000000000..92f3fccfb4d --- /dev/null +++ b/app/assets/javascripts/discourse/templates/components/reviewable-claimed-topic.hbs @@ -0,0 +1,18 @@ +{{#if enabled}} +
+ {{#if claimedBy}} +
+ {{avatar claimedBy imageSize="small"}} + {{claimedBy.username}} +
+ {{d-button + icon="times" + class="btn-small unclaim" + action=(action "unclaim") + disabled=unassigning + title="review.unclaim.help"}} + {{else}} + {{d-button icon="user-plus" class="btn-small claim" title="review.claim.title" action=(action "claim")}} + {{/if}} +
+{{/if}} diff --git a/app/assets/javascripts/discourse/templates/components/reviewable-item.hbs b/app/assets/javascripts/discourse/templates/components/reviewable-item.hbs index dc1c908e55a..7df7ee102eb 100644 --- a/app/assets/javascripts/discourse/templates/components/reviewable-item.hbs +++ b/app/assets/javascripts/discourse/templates/components/reviewable-item.hbs @@ -41,34 +41,43 @@ {{/if}}
- {{#if editing}} - {{d-button - class="btn-primary reviewable-action save-edit" - disabled=updating - icon="check" - action=(action "saveEdit") - label="review.save"}} - {{d-button - class="btn-danger reviewable-action cancel-edit" - disabled=updating - icon="times" - action=(action "cancelEdit") - label="review.cancel"}} - {{else}} - {{#each reviewable.bundled_actions as |bundle|}} - {{reviewable-bundled-action - bundle=bundle - performAction=(action "perform") - reviewableUpdating=updating}} - {{/each}} + {{#if claimEnabled}} +
+ {{{claimHelp}}} + {{reviewable-claimed-topic topicId=reviewable.topic.id claimedBy=reviewable.claimed_by}} +
+ {{/if}} - {{#if reviewable.can_edit}} + {{#if canPerform}} + {{#if editing}} {{d-button - class="reviewable-action edit" + class="btn-primary reviewable-action save-edit" disabled=updating - icon="pencil-alt" - action=(action "edit") - label="review.edit"}} + icon="check" + action=(action "saveEdit") + label="review.save"}} + {{d-button + class="btn-danger reviewable-action cancel-edit" + disabled=updating + icon="times" + action=(action "cancelEdit") + label="review.cancel"}} + {{else}} + {{#each reviewable.bundled_actions as |bundle|}} + {{reviewable-bundled-action + bundle=bundle + performAction=(action "perform") + reviewableUpdating=updating}} + {{/each}} + + {{#if reviewable.can_edit}} + {{d-button + class="reviewable-action edit" + disabled=updating + icon="pencil-alt" + action=(action "edit") + label="review.edit"}} + {{/if}} {{/if}} {{/if}}
diff --git a/app/assets/javascripts/discourse/templates/review-topics.hbs b/app/assets/javascripts/discourse/templates/review-topics.hbs index 39a76f3fd8f..029c5c310a0 100644 --- a/app/assets/javascripts/discourse/templates/review-topics.hbs +++ b/app/assets/javascripts/discourse/templates/review-topics.hbs @@ -22,6 +22,7 @@ {{i18n "review.topics.unique_users" count=rt.stats.unique_users}} + {{reviewable-claimed-topic topicId=rt.id claimedBy=rt.claimed_by}} {{#link-to "review.index" (query-params topic_id=rt.id) class="btn btn-primary btn-small"}} {{d-icon "list"}} {{i18n "review.topics.details"}} diff --git a/app/assets/stylesheets/common/base/reviewables.scss b/app/assets/stylesheets/common/base/reviewables.scss index ec2c2df8e4f..3f4a183b8df 100644 --- a/app/assets/stylesheets/common/base/reviewables.scss +++ b/app/assets/stylesheets/common/base/reviewables.scss @@ -116,6 +116,30 @@ } } +.reviewable-claimed-topic { + display: flex; + align-items: center; + .btn-small { + margin-left: 0.5em; + } +} + +.reviewable-actions .claimed-actions { + display: flex; + width: 100%; + justify-content: space-between; + align-items: center; + margin-bottom: 0.5em; +} + +.claimed-by { + display: flex; + align-items: center; + .claimed-username { + margin-left: 0.5em; + } +} + .reviewable-topics { width: 100%; @@ -132,6 +156,7 @@ .btn { display: flex; align-items: center; + margin-left: 1em; } } } diff --git a/app/controllers/reviewable_claimed_topics_controller.rb b/app/controllers/reviewable_claimed_topics_controller.rb new file mode 100644 index 00000000000..948558c8323 --- /dev/null +++ b/app/controllers/reviewable_claimed_topics_controller.rb @@ -0,0 +1,20 @@ +class ReviewableClaimedTopicsController < ApplicationController + requires_login + + def create + topic = Topic.find_by(id: params[:reviewable_claimed_topic][:topic_id]) + guardian.ensure_can_claim_reviewable_topic!(topic) + ReviewableClaimedTopic.create!(user_id: current_user.id, topic_id: topic.id) + render json: success_json + end + + def destroy + topic = Topic.find_by(id: params[:id]) + raise Discourse::NotFound if topic.blank? + + guardian.ensure_can_claim_reviewable_topic!(topic) + ReviewableClaimedTopic.where(topic_id: topic.id).delete_all + + render json: success_json + end +end diff --git a/app/controllers/reviewables_controller.rb b/app/controllers/reviewables_controller.rb index 3e1a0400826..63c4bb56332 100644 --- a/app/controllers/reviewables_controller.rb +++ b/app/controllers/reviewables_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class ReviewablesController < ApplicationController requires_login @@ -30,12 +32,20 @@ class ReviewablesController < ApplicationController total_rows = Reviewable.list_for(current_user, filters).count reviewables = Reviewable.list_for(current_user, filters.merge(limit: PER_PAGE, offset: offset)).to_a + claimed_topics = ReviewableClaimedTopic.claimed_hash(reviewables.map { |r| r.topic_id }.uniq) + # This is a bit awkward, but ActiveModel serializers doesn't seem to serialize STI. Note `hash` # is mutated by the serializer and contains the side loaded records which must be merged in the end. hash = {} json = { reviewables: reviewables.map! do |r| - result = r.serializer.new(r, root: nil, hash: hash, scope: guardian).as_json + result = r.serializer.new( + r, + root: nil, + hash: hash, + scope: guardian, + claimed_topics: claimed_topics + ).as_json hash[:bundled_actions].uniq! (hash['actions'] || []).uniq! result @@ -78,7 +88,17 @@ class ReviewablesController < ApplicationController end topics = Topic.where(id: topic_ids).order('reviewable_score DESC') - render_serialized(topics, ReviewableTopicSerializer, root: 'reviewable_topics', stats: stats) + render_serialized( + topics, + ReviewableTopicSerializer, + root: 'reviewable_topics', + stats: stats, + claimed_topics: ReviewableClaimedTopic.claimed_hash(topic_ids), + rest_serializer: true, + meta: { + types: meta_types + } + ) end def show @@ -88,6 +108,7 @@ class ReviewablesController < ApplicationController reviewable, reviewable.serializer, rest_serializer: true, + claimed_topics: ReviewableClaimedTopic.claimed_hash([reviewable.topic_id]), root: 'reviewable', meta: { types: meta_types @@ -106,6 +127,10 @@ class ReviewablesController < ApplicationController def update reviewable = find_reviewable + if error = claim_error?(reviewable) + return render_json_error(error) + end + editable = reviewable.editable_for(guardian) raise Discourse::InvalidAccess.new unless editable.present? @@ -136,8 +161,15 @@ class ReviewablesController < ApplicationController def perform args = { version: params[:version].to_i } + result = nil begin - result = find_reviewable.perform(current_user, params[:action_id].to_sym, args) + reviewable = find_reviewable + + if error = claim_error?(reviewable) + return render_json_error(error) + end + + result = reviewable.perform(current_user, params[:action_id].to_sym, args) rescue Reviewable::InvalidAction => e # Consider InvalidAction an InvalidAccess raise Discourse::InvalidAccess.new(e.message) @@ -169,6 +201,17 @@ class ReviewablesController < ApplicationController protected + def claim_error?(reviewable) + return if SiteSetting.reviewable_claiming == "disabled" || reviewable.topic_id.blank? + + claimed_by_id = ReviewableClaimedTopic.where(topic_id: reviewable.topic_id).pluck(:user_id)[0] + if SiteSetting.reviewable_claiming == "required" && claimed_by_id.blank? + return I18n.t('reviewables.must_claim') + end + + claimed_by_id.present? && claimed_by_id != current_user.id + end + def find_reviewable reviewable = Reviewable.viewable_by(current_user).where(id: params[:reviewable_id]).first raise Discourse::NotFound.new if reviewable.blank? @@ -189,7 +232,8 @@ protected { created_by: 'user', target_created_by: 'user', - reviewed_by: 'user' + reviewed_by: 'user', + claimed_by: 'user' } end diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb index 01c52f92a5c..a823657710d 100644 --- a/app/models/reviewable.rb +++ b/app/models/reviewable.rb @@ -180,18 +180,26 @@ class Reviewable < ActiveRecord::Base end def apply_review_group - return unless SiteSetting.enable_category_group_review? && category.present? && category.reviewable_by_group_id + return unless SiteSetting.enable_category_group_review? && + category.present? && + category.reviewable_by_group_id + self.reviewable_by_group_id = category.reviewable_by_group_id end def actions_for(guardian, args = nil) args ||= {} - Actions.new(self, guardian).tap { |a| build_actions(a, guardian, args) } + + Actions.new(self, guardian).tap do |actions| + build_actions(actions, guardian, args) + end end def editable_for(guardian, args = nil) args ||= {} - EditableFields.new(self, guardian, args).tap { |a| build_editable_fields(a, guardian, args) } + EditableFields.new(self, guardian, args).tap do |fields| + build_editable_fields(fields, guardian, args) + end end # subclasses must implement "build_actions" to list the actions they're capable of @@ -492,7 +500,6 @@ end # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null # reviewable_by_group_id :integer -# claimed_by_id :integer # category_id :integer # topic_id :integer # score :float default(0.0), not null diff --git a/app/models/reviewable_claimed_topic.rb b/app/models/reviewable_claimed_topic.rb new file mode 100644 index 00000000000..ab73670cf76 --- /dev/null +++ b/app/models/reviewable_claimed_topic.rb @@ -0,0 +1,14 @@ +class ReviewableClaimedTopic < ActiveRecord::Base + belongs_to :topic + belongs_to :user + + def self.claimed_hash(topic_ids) + result = {} + if SiteSetting.reviewable_claiming != 'disabled' + ReviewableClaimedTopic.where(topic_id: topic_ids).includes(:user).each do |rct| + result[rct.topic_id] = rct.user + end + end + result + end +end diff --git a/app/models/reviewable_flagged_post.rb b/app/models/reviewable_flagged_post.rb index 9cc622d231d..ae23fe2a71d 100644 --- a/app/models/reviewable_flagged_post.rb +++ b/app/models/reviewable_flagged_post.rb @@ -294,7 +294,6 @@ end # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null # reviewable_by_group_id :integer -# claimed_by_id :integer # category_id :integer # topic_id :integer # score :float default(0.0), not null diff --git a/app/models/reviewable_queued_post.rb b/app/models/reviewable_queued_post.rb index 63804864d50..11084d58bd6 100644 --- a/app/models/reviewable_queued_post.rb +++ b/app/models/reviewable_queued_post.rb @@ -145,7 +145,6 @@ end # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null # reviewable_by_group_id :integer -# claimed_by_id :integer # category_id :integer # topic_id :integer # score :float default(0.0), not null diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb index 23659e31b1f..7cc4b0d2656 100644 --- a/app/models/reviewable_user.rb +++ b/app/models/reviewable_user.rb @@ -101,7 +101,6 @@ end # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null # reviewable_by_group_id :integer -# claimed_by_id :integer # category_id :integer # topic_id :integer # score :float default(0.0), not null diff --git a/app/serializers/reviewable_serializer.rb b/app/serializers/reviewable_serializer.rb index 51775af412e..1836e8da062 100644 --- a/app/serializers/reviewable_serializer.rb +++ b/app/serializers/reviewable_serializer.rb @@ -25,26 +25,36 @@ class ReviewableSerializer < ApplicationSerializer has_many :editable_fields, serializer: ReviewableEditableFieldSerializer, embed: :objects has_many :reviewable_scores, serializer: ReviewableScoreSerializer has_many :bundled_actions, serializer: ReviewableBundledActionSerializer + has_one :claimed_by, serializer: BasicUserSerializer, root: 'users' # Used to keep track of our payload attributes class_attribute :_payload_for_serialization def bundled_actions - object.actions_for(scope).bundles - end - - def reviewable_actions - object.actions_for(scope).to_a + args = {} + args[:claimed_by] = claimed_by if @options[:claimed_topics] + object.actions_for(scope, args).bundles end def editable_fields - object.editable_for(scope).to_a + args = {} + args[:claimed_by] = claimed_by if @options[:claimed_topics] + object.editable_for(scope, args).to_a end def can_edit editable_fields.present? end + def claimed_by + return nil unless @options[:claimed_topics].present? + @options[:claimed_topics][object.topic_id] + end + + def include_claimed_by? + @options[:claimed_topics] + end + def self.create_attribute(name, field) attribute(name) diff --git a/app/serializers/reviewable_topic_serializer.rb b/app/serializers/reviewable_topic_serializer.rb index e61111defbb..5925da73a16 100644 --- a/app/serializers/reviewable_topic_serializer.rb +++ b/app/serializers/reviewable_topic_serializer.rb @@ -13,7 +13,18 @@ class ReviewableTopicSerializer < ApplicationSerializer :reviewable_score ) + has_one :claimed_by, serializer: BasicUserSerializer, root: 'users' + def stats @options[:stats][object.id] end + + def claimed_by + @options[:claimed_topics][object.id] + end + + def include_claimed_by? + @options[:claimed_topics] + end + end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 1f38f5a51dd..d35fb8209fd 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -359,6 +359,15 @@ en: placeholder: "type the message title here" review: + claim_help: + optional: "You can claim this item to prevent others from reviewing it." + required: "You must claim items before you can review them." + claimed_by_you: "You've claimed this item and can review it." + claimed_by_other: "This item can only be reviewed by {{username}}." + claim: + title: "claim this topic" + unclaim: + help: "remove this claim" awaiting_approval: "Awaiting Approval" delete: "Delete" settings: diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index a5d4a6e93a1..4d10b092aab 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1750,6 +1750,7 @@ en: auto_silence_fast_typers_on_first_post: "Automatically silence users that do not meet min_first_post_typing_time" auto_silence_fast_typers_max_trust_level: "Maximum trust level to auto silence fast typers" auto_silence_first_post_regex: "Case insensitive regex that if passed will cause first post by user to be silenced and sent to approval queue. Example: raging|a[bc]a , will cause all posts containing raging or aba or aca to be silenced on first. Only applies to first post." + reviewable_claiming: "Does reviewable content need to be claimed before it can be acted upon?" reviewable_default_topics: "Show reviewable content grouped by topic by default" reviewable_default_visibility: "Don't show reviewable items unless they meet this priority" @@ -4417,6 +4418,8 @@ en: webhook_deactivation_reason: "Your webhook has been automatically deactivated. We received multiple '%{status}' HTTP status failure responses." reviewables: + must_claim: "You must claim items before acting on them." + user_claimed: "This item has been claimed by another user." missing_version: "You must supply a version parameter" conflict: "There was an update conflict preventing you from doing that." reasons: diff --git a/config/routes.rb b/config/routes.rb index 32cc4efe378..d3868db2d0e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -328,6 +328,8 @@ Discourse::Application.routes.draw do put "review/:reviewable_id" => "reviewables#update", constraints: { reviewable_id: /\d+/ } delete "review/:reviewable_id" => "reviewables#destroy", constraints: { reviewable_id: /\d+/ } + resources :reviewable_claimed_topics + get "session/sso" => "session#sso" get "session/sso_login" => "session#sso_login" get "session/sso_provider" => "session#sso_provider" diff --git a/config/site_settings.yml b/config/site_settings.yml index c4f7ad6f152..d3d61f901e7 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1386,6 +1386,16 @@ spam: auto_silence_fast_typers_on_first_post: true auto_silence_fast_typers_max_trust_level: 0 auto_silence_first_post_regex: "" + + reviewable_claiming: + client: true + type: enum + default: disabled + choices: + - disabled + - optional + - required + reviewable_default_topics: default: false client: true diff --git a/db/migrate/20190508141327_create_reviewable_claimed_topics.rb b/db/migrate/20190508141327_create_reviewable_claimed_topics.rb new file mode 100644 index 00000000000..72f049ba2be --- /dev/null +++ b/db/migrate/20190508141327_create_reviewable_claimed_topics.rb @@ -0,0 +1,10 @@ +class CreateReviewableClaimedTopics < ActiveRecord::Migration[5.2] + def change + create_table :reviewable_claimed_topics do |t| + t.integer :user_id, null: false + t.integer :topic_id, null: false + t.timestamps + end + add_index :reviewable_claimed_topics, :topic_id, unique: true + end +end diff --git a/db/post_migrate/20190508141824_drop_claimed_by_id.rb b/db/post_migrate/20190508141824_drop_claimed_by_id.rb new file mode 100644 index 00000000000..7e0630348ec --- /dev/null +++ b/db/post_migrate/20190508141824_drop_claimed_by_id.rb @@ -0,0 +1,5 @@ +class DropClaimedById < ActiveRecord::Migration[5.2] + def up + remove_column :reviewables, :claimed_by_id + end +end diff --git a/lib/guardian/user_guardian.rb b/lib/guardian/user_guardian.rb index 75f130eb310..4b7fda4135c 100644 --- a/lib/guardian/user_guardian.rb +++ b/lib/guardian/user_guardian.rb @@ -1,6 +1,10 @@ # mixin for all Guardian methods dealing with user permissions module UserGuardian + def can_claim_reviewable_topic?(topic) + SiteSetting.reviewable_claiming != 'disabled' && can_review_topic?(topic) + end + def can_pick_avatar?(user_avatar, upload) return false unless self.user return true if is_admin? diff --git a/spec/fabricators/reviewable_claimed_topic_fabricator.rb b/spec/fabricators/reviewable_claimed_topic_fabricator.rb new file mode 100644 index 00000000000..b8f7b52e11f --- /dev/null +++ b/spec/fabricators/reviewable_claimed_topic_fabricator.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +Fabricator(:reviewable_claimed_topic) do + topic + user +end diff --git a/spec/models/reviewable_claimed_topic_spec.rb b/spec/models/reviewable_claimed_topic_spec.rb new file mode 100644 index 00000000000..09cef31b918 --- /dev/null +++ b/spec/models/reviewable_claimed_topic_spec.rb @@ -0,0 +1,12 @@ +require 'rails_helper' + +RSpec.describe ReviewableClaimedTopic, type: :model do + + it "ensures uniqueness" do + claimed = Fabricate(:reviewable_claimed_topic) + expect(-> { + ReviewableClaimedTopic.create!(topic_id: claimed.topic_id, user_id: Fabricate(:user).id) + }).to raise_error(ActiveRecord::RecordNotUnique) + end + +end diff --git a/spec/models/reviewable_flagged_post_spec.rb b/spec/models/reviewable_flagged_post_spec.rb index 8e838274d84..59f447929b0 100644 --- a/spec/models/reviewable_flagged_post_spec.rb +++ b/spec/models/reviewable_flagged_post_spec.rb @@ -26,6 +26,7 @@ RSpec.describe ReviewableFlaggedPost, type: :model do let(:guardian) { Guardian.new(moderator) } describe "actions_for" do + it "returns appropriate defaults" do actions = reviewable.actions_for(guardian) expect(actions.has?(:agree_and_hide)).to eq(true) diff --git a/spec/requests/reviewable_claimed_topics_controller_spec.rb b/spec/requests/reviewable_claimed_topics_controller_spec.rb new file mode 100644 index 00000000000..01df86f3f2d --- /dev/null +++ b/spec/requests/reviewable_claimed_topics_controller_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe ReviewableClaimedTopicsController do + fab!(:moderator) { Fabricate(:moderator) } + + describe '#create' do + let(:topic) { Fabricate(:topic) } + let(:params) do + { reviewable_claimed_topic: { topic_id: topic.id } } + end + + it "requires you to be logged in" do + post "/reviewable_claimed_topics.json", params: params + expect(response.code).to eq("403") + end + + context "when logged in" do + + before do + sign_in(moderator) + end + + it "will raise an error if you can't claim the topic" do + post "/reviewable_claimed_topics.json", params: params + expect(response.code).to eq("403") + end + + it "will return 200 if the user can claim the topic" do + SiteSetting.reviewable_claiming = 'optional' + post "/reviewable_claimed_topics.json", params: params + expect(response.code).to eq("200") + expect(ReviewableClaimedTopic.where(user_id: moderator.id, topic_id: topic.id).exists?).to eq(true) + end + end + end + + describe '#destroy' do + let(:claimed) { Fabricate(:reviewable_claimed_topic) } + + before do + sign_in(moderator) + end + + it "404s for a missing topic" do + delete "/reviewable_claimed_topics/111111111.json" + expect(response.code).to eq("404") + end + + it "403s when you can't claim the topic" do + delete "/reviewable_claimed_topics/#{claimed.topic_id}.json" + expect(response.code).to eq("403") + end + + it "works when the feature is enabled" do + SiteSetting.reviewable_claiming = 'optional' + delete "/reviewable_claimed_topics/#{claimed.topic_id}.json" + expect(response.code).to eq("200") + expect(ReviewableClaimedTopic.where(topic_id: claimed.topic_id).exists?).to eq(false) + end + end +end diff --git a/spec/requests/reviewables_controller_spec.rb b/spec/requests/reviewables_controller_spec.rb index c310b1b1089..56a9dc2ba14 100644 --- a/spec/requests/reviewables_controller_spec.rb +++ b/spec/requests/reviewables_controller_spec.rb @@ -290,6 +290,29 @@ describe ReviewablesController do expect(other_reviewable.reload.version).to eq(0) end + context "claims" do + let(:qp) { Fabricate(:reviewable_queued_post) } + + it "fails when reviewables must be claimed" do + SiteSetting.reviewable_claiming = 'required' + put "/review/#{qp.id}/perform/approve_post.json?version=#{qp.version}" + expect(response.code).to eq("422") + end + + it "fails when optional claims are claimed by others" do + SiteSetting.reviewable_claiming = 'optional' + ReviewableClaimedTopic.create!(topic_id: qp.topic_id, user: Fabricate(:admin)) + put "/review/#{qp.id}/perform/approve_post.json?version=#{qp.version}" + expect(response.code).to eq("422") + end + + it "works when claims are optional" do + SiteSetting.reviewable_claiming = 'optional' + put "/review/#{qp.id}/perform/approve_post.json?version=#{qp.version}" + expect(response.code).to eq("200") + end + end + describe "simultaneous perform" do it "fails when the version is wrong" do put "/review/#{reviewable.id}/perform/approve_user.json?version=#{reviewable.version + 1}" @@ -314,7 +337,23 @@ describe ReviewablesController do expect(json['reviewable_topics']).to be_blank end - it "returns json listing the topics " do + it "includes claimed information" do + SiteSetting.reviewable_claiming = 'optional' + PostActionCreator.spam(user0, post0) + moderator = Fabricate(:moderator) + ReviewableClaimedTopic.create!(user: moderator, topic: post0.topic) + + get "/review/topics.json" + expect(response.code).to eq("200") + json = ::JSON.parse(response.body) + json_topic = json['reviewable_topics'].find { |rt| rt['id'] == post0.topic_id } + expect(json_topic['claimed_by_id']).to eq(moderator.id) + + json_user = json['users'].find { |u| u['id'] == json_topic['claimed_by_id'] } + expect(json_user).to be_present + end + + it "returns json listing the topics" do PostActionCreator.spam(user0, post0) PostActionCreator.off_topic(user0, post1) PostActionCreator.spam(user0, post2) diff --git a/test/javascripts/helpers/store-pretender.js.es6 b/test/javascripts/helpers/store-pretender.js.es6 index 0c81d027a6e..e30cd10572b 100644 --- a/test/javascripts/helpers/store-pretender.js.es6 +++ b/test/javascripts/helpers/store-pretender.js.es6 @@ -11,7 +11,8 @@ const _moreWidgets = [ const fruits = [ { id: 1, name: "apple", farmer_id: 1, color_ids: [1, 2], category_id: 4 }, { id: 2, name: "banana", farmer_id: 1, color_ids: [3], category_id: 3 }, - { id: 3, name: "grape", farmer_id: 2, color_ids: [2], category_id: 5 } + { id: 3, name: "grape", farmer_id: 2, color_ids: [2], category_id: 5 }, + { id: 4, name: "orange", farmer_id: null, color_ids: [2], category_id: 5 } ]; const farmers = [ @@ -28,8 +29,8 @@ const colors = [ export default function(helpers) { const { response, success, parsePostData } = helpers; - this.get("/fruits/:id", function() { - const fruit = fruits[0]; + this.get("/fruits/:id", function(request) { + const fruit = fruits.find(f => f.id === parseInt(request.params.id)); return response({ __rest_serializer: "1", fruit, farmers, colors }); }); diff --git a/test/javascripts/models/store-test.js.es6 b/test/javascripts/models/store-test.js.es6 index a9ce8e2c59c..f8d9ba1c033 100644 --- a/test/javascripts/models/store-test.js.es6 +++ b/test/javascripts/models/store-test.js.es6 @@ -142,7 +142,7 @@ QUnit.test("destroyRecord when new", function(assert) { QUnit.test("find embedded", function(assert) { const store = createStore(); - return store.find("fruit", 2).then(function(f) { + return store.find("fruit", 1).then(function(f) { assert.ok(f.get("farmer"), "it has the embedded object"); const fruitCols = f.get("colors"); @@ -154,6 +154,14 @@ QUnit.test("find embedded", function(assert) { }); }); +QUnit.test("embedded records can be cleared", async assert => { + const store = createStore(); + let f = await store.find("fruit", 4); + f.set("farmer", { dummy: "object" }); + f = await store.find("fruit", 4); + assert.ok(!f.get("farmer")); +}); + QUnit.test("meta types", function(assert) { const store = createStore(); return store.find("barn", 1).then(function(f) {