From 197e3f24ce34975d26e9246c54216ecf91e290fb Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Wed, 26 May 2021 02:47:35 +0300 Subject: [PATCH] FEATURE: Show stale reviewable to other clients (#13114) The previous commits removed reviewables leading to a bad user experience. This commit updates the status, replaces actions with a message and greys out the reviewable. --- .../app/components/reviewable-item.js | 7 +- .../discourse/app/routes/review-index.js | 12 ++++ .../templates/components/reviewable-item.hbs | 72 ++++++++++--------- .../discourse/tests/acceptance/review-test.js | 28 +++++++- .../stylesheets/common/base/reviewables.scss | 4 ++ app/controllers/reviewables_controller.rb | 4 +- app/jobs/regular/notify_reviewable.rb | 40 +++++++++-- app/models/reviewable.rb | 10 ++- config/locales/client.en.yml | 1 + spec/models/reviewable_spec.rb | 20 ++++-- spec/requests/reviewables_controller_spec.rb | 4 +- 11 files changed, 151 insertions(+), 51 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/reviewable-item.js b/app/assets/javascripts/discourse/app/components/reviewable-item.js index bf436f4a073..23071cac963 100644 --- a/app/assets/javascripts/discourse/app/components/reviewable-item.js +++ b/app/assets/javascripts/discourse/app/components/reviewable-item.js @@ -29,12 +29,17 @@ export default Component.extend({ @discourseComputed( "reviewable.type", + "reviewable.stale", "siteSettings.blur_tl0_flagged_posts_media", "reviewable.target_created_by_trust_level" ) - customClasses(type, blurEnabled, trustLevel) { + customClasses(type, stale, blurEnabled, trustLevel) { let classes = type.dasherize(); + if (stale) { + classes = `${classes} reviewable-stale`; + } + if (blurEnabled && trustLevel === 0) { classes = `${classes} blur-images`; } diff --git a/app/assets/javascripts/discourse/app/routes/review-index.js b/app/assets/javascripts/discourse/app/routes/review-index.js index b4d15e68e1e..187c91c9538 100644 --- a/app/assets/javascripts/discourse/app/routes/review-index.js +++ b/app/assets/javascripts/discourse/app/routes/review-index.js @@ -55,6 +55,18 @@ export default DiscourseRoute.extend({ }); } }); + + this.messageBus.subscribe("/reviewable_counts", (data) => { + if (data.updates) { + this.controller.reviewables.forEach((reviewable) => { + const updates = data.updates[reviewable.id]; + if (updates) { + reviewable.setProperties(updates); + reviewable.set("stale", true); + } + }); + } + }); }, deactivate() { diff --git a/app/assets/javascripts/discourse/app/templates/components/reviewable-item.hbs b/app/assets/javascripts/discourse/app/templates/components/reviewable-item.hbs index 79f25ca62c1..9288db16d2a 100644 --- a/app/assets/javascripts/discourse/app/templates/components/reviewable-item.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/reviewable-item.hbs @@ -44,42 +44,46 @@ {{/if}}
- {{#if claimEnabled}} -
- {{html-safe claimHelp}} - {{reviewable-claimed-topic topicId=topicId claimedBy=reviewable.claimed_by}} -
- {{/if}} + {{#if reviewable.stale}} +
{{i18n "review.stale_help"}}
+ {{else}} + {{#if claimEnabled}} +
+ {{html-safe claimHelp}} + {{reviewable-claimed-topic topicId=topicId claimedBy=reviewable.claimed_by}} +
+ {{/if}} - {{#if canPerform}} - {{#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 reviewable.can_edit}} + {{#if canPerform}} + {{#if editing}} {{d-button - class="reviewable-action edit" - disabled=updating - icon="pencil-alt" - action=(action "edit") - label="review.edit"}} + class="btn-primary reviewable-action save-edit" + disabled=disabled + icon="check" + action=(action "saveEdit") + label="review.save"}} + {{d-button + class="btn-danger reviewable-action cancel-edit" + disabled=disabled + icon="times" + action=(action "cancelEdit") + label="review.cancel"}} + {{else}} + {{#each reviewable.bundled_actions as |bundle|}} + {{reviewable-bundled-action + bundle=bundle + performAction=(action "perform") + reviewableUpdating=disabled}} + {{/each}} + + {{#if reviewable.can_edit}} + {{d-button + class="reviewable-action edit" + disabled=disabled + icon="pencil-alt" + action=(action "edit") + label="review.edit"}} + {{/if}} {{/if}} {{/if}} {{/if}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/review-test.js b/app/assets/javascripts/discourse/tests/acceptance/review-test.js index ad7041a207d..dbdf2f02898 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/review-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/review-test.js @@ -1,4 +1,8 @@ -import { acceptance, queryAll } from "discourse/tests/helpers/qunit-helpers"; +import { + acceptance, + publishToMessageBus, + queryAll, +} from "discourse/tests/helpers/qunit-helpers"; import { click, fillIn, visit } from "@ember/test-helpers"; import I18n from "I18n"; import selectKit from "discourse/tests/helpers/select-kit-helper"; @@ -193,4 +197,26 @@ acceptance("Review", function (needs) { ); assert.equal(queryAll(`${topic} .category-name`).text().trim(), "support"); }); + + test("Reviewables can become stale", async function (assert) { + await visit("/review"); + + const reviewable = find("[data-reviewable-id=1234]")[0]; + assert.notOk(reviewable.className.includes("reviewable-stale")); + assert.equal(find("[data-reviewable-id=1234] .status .pending").length, 1); + assert.equal(find(".stale-help").length, 0); + + publishToMessageBus("/reviewable_counts", { + review_count: 1, + updates: { + 1234: { status: 1 }, + }, + }); + + await visit("/review"); // wait for re-render + + assert.ok(reviewable.className.includes("reviewable-stale")); + assert.equal(find("[data-reviewable-id=1234] .status .approved").length, 1); + assert.equal(find(".stale-help").length, 1); + }); }); diff --git a/app/assets/stylesheets/common/base/reviewables.scss b/app/assets/stylesheets/common/base/reviewables.scss index 7fbbb1a5944..5a222223741 100644 --- a/app/assets/stylesheets/common/base/reviewables.scss +++ b/app/assets/stylesheets/common/base/reviewables.scss @@ -276,6 +276,10 @@ padding-bottom: 1em; } +.reviewable-stale { + opacity: 0.7; +} + .blur-images { img:not(.avatar):not(.emoji) { filter: blur(10px); diff --git a/app/controllers/reviewables_controller.rb b/app/controllers/reviewables_controller.rb index f9249991b09..5880bb7babf 100644 --- a/app/controllers/reviewables_controller.rb +++ b/app/controllers/reviewables_controller.rb @@ -12,11 +12,11 @@ class ReviewablesController < ApplicationController offset = params[:offset].to_i if params[:type].present? - raise Discourse::InvalidParameter.new(:type) unless Reviewable.valid_type?(params[:type]) + raise Discourse::InvalidParameters.new(:type) unless Reviewable.valid_type?(params[:type]) end status = (params[:status] || 'pending').to_sym - raise Discourse::InvalidParameter.new(:status) unless allowed_statuses.include?(status) + raise Discourse::InvalidParameters.new(:status) unless allowed_statuses.include?(status) topic_id = params[:topic_id] ? params[:topic_id].to_i : nil category_id = params[:category_id] ? params[:category_id].to_i : nil diff --git a/app/jobs/regular/notify_reviewable.rb b/app/jobs/regular/notify_reviewable.rb index 88632a66ae8..3205129333a 100644 --- a/app/jobs/regular/notify_reviewable.rb +++ b/app/jobs/regular/notify_reviewable.rb @@ -15,28 +15,58 @@ class Jobs::NotifyReviewable < ::Jobs::Base counts[r.reviewable_by_group_id] += 1 if r.reviewable_by_group_id end + all_updates = Hash.new { |h, k| h[k] = {} } + + if args[:updated_reviewable_ids].present? + Reviewable.where(id: args[:updated_reviewable_ids]).each do |r| + payload = { status: r.status } + + all_updates[:admins][r.id] = payload + all_updates[:moderators][r.id] = payload if r.reviewable_by_moderator? + all_updates[r.reviewable_by_group_id][r.id] = payload if r.reviewable_by_group_id + end + end + # admins - notify(counts[:admins], User.real.admins.pluck(:id)) + notify( + User.real.admins.pluck(:id), + count: counts[:admins], + updates: all_updates[:admins], + ) # moderators if reviewable.reviewable_by_moderator? - notify(counts[:moderators], User.real.moderators.where("id NOT IN (?)", @contacted).pluck(:id)) + notify( + User.real.moderators.where("id NOT IN (?)", @contacted).pluck(:id), + count: counts[:moderators], + updates: all_updates[:moderators], + ) end # category moderators if SiteSetting.enable_category_group_moderation? && (group = reviewable.reviewable_by_group) group.users.includes(:group_users).where("users.id NOT IN (?)", @contacted).find_each do |user| - count = user.group_users.map { |gu| counts[gu.group_id] }.sum - notify(count, [user.id]) + count = 0 + updates = {} + + user.group_users.each do |gu| + count += counts[gu.group_id] || 0 + updates.merge!(all_updates[gu.group_id] || {}) + end + + notify([user.id], count: count, updates: updates) end end end protected - def notify(count, user_ids) + def notify(user_ids, count:, updates:) return if user_ids.blank? + data = { reviewable_count: count } + data[:updates] = updates if updates.present? + MessageBus.publish("/reviewable_counts", data, user_ids: user_ids) @contacted += user_ids end diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb index 0e8af0bcb02..3fc8364413c 100644 --- a/app/models/reviewable.rb +++ b/app/models/reviewable.rb @@ -339,7 +339,6 @@ class Reviewable < ActiveRecord::Base # the result of the operation and whether the status of the reviewable changed. def perform(performed_by, action_id, args = nil) args ||= {} - # Support this action or any aliases aliases = self.class.action_aliases valid = [ action_id, aliases.to_a.select { |k, v| v == action_id }.map(&:first) ].flatten @@ -367,7 +366,14 @@ class Reviewable < ActiveRecord::Base if result && result.after_commit result.after_commit.call end - Jobs.enqueue(:notify_reviewable, reviewable_id: self.id) if update_count + + if update_count || result.remove_reviewable_ids.present? + Jobs.enqueue( + :notify_reviewable, + reviewable_id: self.id, + updated_reviewable_ids: result.remove_reviewable_ids, + ) + end result end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index a8f20973066..ad257fd1bd8 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -426,6 +426,7 @@ en: type_bonus: name: "type bonus" title: "Certain reviewable types can be assigned a bonus by staff to make them a higher priority." + stale_help: "This reviewable has been resolved by someone else." claim_help: optional: "You can claim this item to prevent others from reviewing it." required: "You must claim items before you can review them." diff --git a/spec/models/reviewable_spec.rb b/spec/models/reviewable_spec.rb index 2572da5e906..f616c460db9 100644 --- a/spec/models/reviewable_spec.rb +++ b/spec/models/reviewable_spec.rb @@ -300,6 +300,7 @@ RSpec.describe Reviewable, type: :model do job = Jobs::NotifyReviewable.jobs.last expect(job["args"].first["reviewable_id"]).to eq(reviewable.id) + expect(job["args"].first["updated_reviewable_ids"]).to contain_exactly(reviewable.id) end it "triggers a notification on pending -> reject" do @@ -312,22 +313,33 @@ RSpec.describe Reviewable, type: :model do job = Jobs::NotifyReviewable.jobs.last expect(job["args"].first["reviewable_id"]).to eq(reviewable.id) + expect(job["args"].first["updated_reviewable_ids"]).to contain_exactly(reviewable.id) end - it "doesn't trigger a notification on approve -> reject" do + it "triggers a notification on approve -> reject to update status" do reviewable = Fabricate(:reviewable_queued_post, status: Reviewable.statuses[:approved]) expect do reviewable.perform(moderator, :reject_post) - end.to_not change { Jobs::NotifyReviewable.jobs.size } + end.to change { Jobs::NotifyReviewable.jobs.size }.by(1) + + job = Jobs::NotifyReviewable.jobs.last + + expect(job["args"].first["reviewable_id"]).to eq(reviewable.id) + expect(job["args"].first["updated_reviewable_ids"]).to contain_exactly(reviewable.id) end - it "doesn't trigger a notification on reject -> approve" do + it "triggers a notification on reject -> approve to update status" do reviewable = Fabricate(:reviewable_queued_post, status: Reviewable.statuses[:rejected]) expect do reviewable.perform(moderator, :approve_post) - end.to_not change { Jobs::NotifyReviewable.jobs.size } + end.to change { Jobs::NotifyReviewable.jobs.size }.by(1) + + job = Jobs::NotifyReviewable.jobs.last + + expect(job["args"].first["reviewable_id"]).to eq(reviewable.id) + expect(job["args"].first["updated_reviewable_ids"]).to contain_exactly(reviewable.id) end end diff --git a/spec/requests/reviewables_controller_spec.rb b/spec/requests/reviewables_controller_spec.rb index c277a5eb2fe..5b751099b5b 100644 --- a/spec/requests/reviewables_controller_spec.rb +++ b/spec/requests/reviewables_controller_spec.rb @@ -105,7 +105,7 @@ describe ReviewablesController do it "raises an error with an invalid type" do get "/review.json?type=ReviewableMadeUp" - expect(response.code).to eq("500") + expect(response.code).to eq("400") end it "supports filtering by status" do @@ -135,7 +135,7 @@ describe ReviewablesController do it "raises an error with an invalid status" do get "/review.json?status=xyz" - expect(response.code).to eq("500") + expect(response.code).to eq("400") end it "supports filtering by category_id" do