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.
This commit is contained in:
Robin Ward 2019-05-08 10:20:51 -04:00
parent 8dfb15a2e5
commit b380ed5282
30 changed files with 448 additions and 47 deletions

View File

@ -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);
}
}
});

View File

@ -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")

View File

@ -311,6 +311,8 @@ export default Ember.Object.extend({
if (hydrated) {
obj[subType] = hydrated;
delete obj[k];
} else {
obj[subType] = null;
}
}
}

View File

@ -0,0 +1,18 @@
{{#if enabled}}
<div class='reviewable-claimed-topic'>
{{#if claimedBy}}
<div class='claimed-by'>
{{avatar claimedBy imageSize="small"}}
<span class='claimed-username'>{{claimedBy.username}}</span>
</div>
{{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}}
</div>
{{/if}}

View File

@ -41,6 +41,14 @@
{{/if}}
</div>
<div class='reviewable-actions'>
{{#if claimEnabled}}
<div class='claimed-actions'>
<span class='help'>{{{claimHelp}}}</span>
{{reviewable-claimed-topic topicId=reviewable.topic.id claimedBy=reviewable.claimed_by}}
</div>
{{/if}}
{{#if canPerform}}
{{#if editing}}
{{d-button
class="btn-primary reviewable-action save-edit"
@ -71,6 +79,7 @@
label="review.edit"}}
{{/if}}
{{/if}}
{{/if}}
</div>
</div>

View File

@ -22,6 +22,7 @@
{{i18n "review.topics.unique_users" count=rt.stats.unique_users}}
</td>
<td class="reviewable-details">
{{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"}}
<span>{{i18n "review.topics.details"}}</span>

View File

@ -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;
}
}
}

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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)

View File

@ -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

View File

@ -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 <b>{{username}}</b>."
claim:
title: "claim this topic"
unclaim:
help: "remove this claim"
awaiting_approval: "Awaiting Approval"
delete: "Delete"
settings:

View File

@ -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:

View File

@ -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"

View File

@ -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

View File

@ -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

View File

@ -0,0 +1,5 @@
class DropClaimedById < ActiveRecord::Migration[5.2]
def up
remove_column :reviewables, :claimed_by_id
end
end

View File

@ -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?

View File

@ -0,0 +1,6 @@
# frozen_string_literal: true
Fabricator(:reviewable_claimed_topic) do
topic
user
end

View File

@ -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

View File

@ -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)

View File

@ -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

View File

@ -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)

View File

@ -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 });
});

View File

@ -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) {