From 533495169e447663e6fd0d654c3e609ddf103d11 Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Fri, 21 Feb 2020 19:11:50 +0200 Subject: [PATCH] FEATURE: Publish a message when reviewable claimer changes (#9019) This commit ensures that all users are kept in sync and no user can claim a topic that has been claimed already. --- .../discourse/routes/review-index.js.es6 | 20 +++++ .../reviewable_claimed_topics_controller.rb | 33 +++++++- ...viewable_claimed_topics_controller_spec.rb | 82 +++++++++++-------- 3 files changed, 97 insertions(+), 38 deletions(-) diff --git a/app/assets/javascripts/discourse/routes/review-index.js.es6 b/app/assets/javascripts/discourse/routes/review-index.js.es6 index 3da7513e6d0..3f2a8abc4a3 100644 --- a/app/assets/javascripts/discourse/routes/review-index.js.es6 +++ b/app/assets/javascripts/discourse/routes/review-index.js.es6 @@ -30,6 +30,26 @@ export default DiscourseRoute.extend({ }); }, + activate() { + this.messageBus.subscribe("/reviewable_claimed", data => { + const reviewables = this.controller.reviewables; + if (reviewables) { + const user = data.user + ? this.store.createRecord("user", data.user) + : null; + reviewables.forEach(reviewable => { + if (data.topic_id === reviewable.topic.id) { + reviewable.set("claimed_by", user); + } + }); + } + }); + }, + + deactivate() { + this.messageBus.unsubscribe("/reviewable_claimed"); + }, + actions: { refreshRoute() { this.refresh(); diff --git a/app/controllers/reviewable_claimed_topics_controller.rb b/app/controllers/reviewable_claimed_topics_controller.rb index 01f2b8e37b4..1111ee91821 100644 --- a/app/controllers/reviewable_claimed_topics_controller.rb +++ b/app/controllers/reviewable_claimed_topics_controller.rb @@ -6,13 +6,18 @@ class ReviewableClaimedTopicsController < ApplicationController 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) + + begin + ReviewableClaimedTopic.create!(user_id: current_user.id, topic_id: topic.id) + rescue ActiveRecord::RecordInvalid + return render_json_error(I18n.t('reviewables.conflict'), status: 409) + end + topic.reviewables.find_each do |reviewable| reviewable.log_history(:claimed, current_user) end - render json: success_json - rescue ActiveRecord::RecordNotUnique - # This is just in case the validation fails under concurrency + + notify_users(topic, current_user) render json: success_json end @@ -26,6 +31,26 @@ class ReviewableClaimedTopicsController < ApplicationController reviewable.log_history(:unclaimed, current_user) end + notify_users(topic, nil) render json: success_json end + + private + + def notify_users(topic, claimed_by) + user_ids = User.staff.pluck(:id) + + if SiteSetting.enable_category_group_review? && group_id = topic.category&.reviewable_by_group_id.presence + user_ids.concat(GroupUser.where(group_id: group_id).pluck(:user_id)) + user_ids.uniq! + end + + if claimed_by.present? + data = { topic_id: topic.id, user: BasicUserSerializer.new(claimed_by, root: false).as_json } + else + data = { topic_id: topic.id } + end + + MessageBus.publish("/reviewable_claimed", data, user_ids: user_ids) + end end diff --git a/spec/requests/reviewable_claimed_topics_controller_spec.rb b/spec/requests/reviewable_claimed_topics_controller_spec.rb index 3d3a29cbb8e..c4dcafa7403 100644 --- a/spec/requests/reviewable_claimed_topics_controller_spec.rb +++ b/spec/requests/reviewable_claimed_topics_controller_spec.rb @@ -5,72 +5,86 @@ require 'rails_helper' describe ReviewableClaimedTopicsController do fab!(:moderator) { Fabricate(:moderator) } - describe '#create' do - fab!(:topic) { Fabricate(:topic) } - fab!(:reviewable) { Fabricate(:reviewable_flagged_post, topic: topic) } - let(:params) do - { reviewable_claimed_topic: { topic_id: topic.id } } - end + fab!(:topic) { Fabricate(:topic) } + fab!(:reviewable) { Fabricate(:reviewable_flagged_post, topic: topic) } - it "requires you to be logged in" do + describe '#create' do + let(:params) { { reviewable_claimed_topic: { topic_id: topic.id } } } + + it "requires user to be logged in" do post "/reviewable_claimed_topics.json", params: params - expect(response.code).to eq("403") + + expect(response.status).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 + it "works" do SiteSetting.reviewable_claiming = 'optional' - post "/reviewable_claimed_topics.json", params: params - expect(response.code).to eq("200") + + messages = MessageBus.track_publish { post "/reviewable_claimed_topics.json", params: params } + + expect(response.status).to eq(200) expect(ReviewableClaimedTopic.where(user_id: moderator.id, topic_id: topic.id).exists?).to eq(true) expect(topic.reviewables.first.history.where(reviewable_history_type: ReviewableHistory.types[:claimed]).size).to eq(1) + expect(messages.size).to eq(1) + expect(messages[0].channel).to eq("/reviewable_claimed") + expect(messages[0].data[:topic_id]).to eq(topic.id) + expect(messages[0].data[:user][:id]).to eq(moderator.id) end - it "won't an error if you claim twice" do + it "raises an error if user cannot claim the topic" do + post "/reviewable_claimed_topics.json", params: params + + expect(response.status).to eq(403) + end + + it "raises an error if topic is already claimed" do SiteSetting.reviewable_claiming = 'optional' + post "/reviewable_claimed_topics.json", params: params expect(ReviewableClaimedTopic.where(user_id: moderator.id, topic_id: topic.id).exists?).to eq(true) + post "/reviewable_claimed_topics.json", params: params - expect(response.code).to eq("200") + expect(response.status).to eq(409) end end end describe '#destroy' do - fab!(:topic) { Fabricate(:topic) } - fab!(:reviewable) { Fabricate(:reviewable_flagged_post, topic: topic) } fab!(:claimed) { Fabricate(:reviewable_claimed_topic, topic: 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 + it "works" do SiteSetting.reviewable_claiming = 'optional' - delete "/reviewable_claimed_topics/#{claimed.topic_id}.json" - expect(response.code).to eq("200") + + messages = MessageBus.track_publish { delete "/reviewable_claimed_topics/#{claimed.topic_id}.json" } + + expect(response.status).to eq(200) expect(ReviewableClaimedTopic.where(topic_id: claimed.topic_id).exists?).to eq(false) expect(topic.reviewables.first.history.where(reviewable_history_type: ReviewableHistory.types[:unclaimed]).size).to eq(1) + expect(messages.size).to eq(1) + expect(messages[0].channel).to eq("/reviewable_claimed") + expect(messages[0].data[:topic_id]).to eq(topic.id) + expect(messages[0].data[:user]).to eq(nil) + end + + it "raises an error if topic is missing" do + delete "/reviewable_claimed_topics/111111111.json" + + expect(response.status).to eq(404) + end + + it "raises an error if user cannot claim the topic" do + delete "/reviewable_claimed_topics/#{claimed.topic_id}.json" + + expect(response.status).to eq(403) end end end