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.
2024-11-22 13:09:18 +08:00 · 2021-05-26 02:47:35 +03:00 · 2021-05-26 02:47:35 +03:00 · 197e3f24ce
commit 197e3f24ce
parent 8c83803109
11 changed files with 151 additions and 51 deletions
--- 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`;
    }
--- 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() {
--- 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}}
  </div>
  <div class="reviewable-actions">
-    {{#if claimEnabled}}
-      <div class="claimed-actions">
-        <span class="help">{{html-safe claimHelp}}</span>
-        {{reviewable-claimed-topic topicId=topicId claimedBy=reviewable.claimed_by}}
-      </div>
-    {{/if}}
+    {{#if reviewable.stale}}
+      <div class="stale-help">{{i18n "review.stale_help"}}</div>
+    {{else}}
+      {{#if claimEnabled}}
+        <div class="claimed-actions">
+          <span class="help">{{html-safe claimHelp}}</span>
+          {{reviewable-claimed-topic topicId=topicId claimedBy=reviewable.claimed_by}}
+        </div>
+      {{/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}}
--- 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);
+  });
 });
--- 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);
--- 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
--- 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
--- 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
--- 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."
--- 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

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