FIX: Exclude claimed reviewables from user menu (#19179)

Users who can access the review queue can claim a pending reviewable(s) which means that the claimed reviewable(s) can only be handled by the user who claimed it. Currently, we show claimed reviewables in the user menu, but this can be annoying for other reviewers because they can't do anything about a reviewable claimed by someone. So this PR makes sure that we only show in the user menu reviewables that are claimed by nobody or claimed by the current user.

Internal topic: t/77235.

app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/review-section-link.js

@@ -57,7 +57,9 @@ export default class ReviewSectionLink extends BaseSectionLink {
   }
 
   get badgeText() {
-    if (this.currentUser.reviewable_count > 0) {
+    // force a tracker for reviewable_count by using .get to ensure badgeText
+    // rerenders when reviewable_count changes
+    if (this.currentUser.get("reviewable_count") > 0) {
       return I18n.t("sidebar.sections.community.links.review.pending_count", {
         count: this.currentUser.reviewable_count,
       });

app/controllers/reviewable_claimed_topics_controller.rb

@@ -51,5 +51,8 @@ class ReviewableClaimedTopicsController < ApplicationController
     end
 
     MessageBus.publish("/reviewable_claimed", data, group_ids: group_ids.to_a)
+    if SiteSetting.enable_experimental_sidebar_hamburger
+      Jobs.enqueue(:refresh_users_reviewable_counts, group_ids: group_ids.to_a)
+    end
   end
 end

app/controllers/reviewables_controller.rb

@@ -59,7 +59,7 @@ class ReviewablesController < ApplicationController
       meta: filters.merge(
         total_rows_reviewables: total_rows, types: meta_types, reviewable_types: Reviewable.types,
         reviewable_count: current_user.reviewable_count,
-        unseen_reviewable_count: current_user.unseen_reviewable_count
+        unseen_reviewable_count: Reviewable.unseen_reviewable_count(current_user)
       )
     }
     if (offset + PER_PAGE) < total_rows

app/jobs/regular/notify_reviewable.rb

@@ -99,12 +99,6 @@ class Jobs::NotifyReviewable < ::Jobs::Base
   end
 
   def notify_user(user, updates)
-    data = {
-      reviewable_count: user.reviewable_count,
-      unseen_reviewable_count: user.unseen_reviewable_count
-    }
-    data[:updates] = updates if updates.present?
-
-    user.publish_reviewable_counts(data)
+    user.publish_reviewable_counts(updates.present? ? { updates: updates } : nil)
   end
 end

app/jobs/regular/refresh_users_reviewable_counts.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class Jobs::RefreshUsersReviewableCounts < ::Jobs::Base
+  def execute(args)
+    group_ids = args[:group_ids]
+    return if group_ids.blank?
+    User.where(
+      id: GroupUser.where(group_id: group_ids).distinct.pluck(:user_id)
+    ).each(&:publish_reviewable_counts)
+  end
+end

app/models/reviewable.rb

@@ -431,6 +431,10 @@ class Reviewable < ActiveRecord::Base
     list_for(user).count
   end
 
+  def self.unseen_reviewable_count(user)
+    self.unseen_list_for(user).count
+  end
+
   def self.list_for(
     user,
     ids: nil,
@@ -447,7 +451,8 @@ class Reviewable < ActiveRecord::Base
     from_date: nil,
     to_date: nil,
     additional_filters: {},
-    preload: true
+    preload: true,
+    include_claimed_by_others: true
   )
     order = case sort_order
             when 'score_asc'
@@ -518,13 +523,18 @@ class Reviewable < ActiveRecord::Base
       )
     end
 
+    if !include_claimed_by_others
+      result = result
+        .joins("LEFT JOIN reviewable_claimed_topics rct ON reviewables.topic_id = rct.topic_id")
+        .where("rct.user_id IS NULL OR rct.user_id = ?", user.id)
+    end
     result = result.limit(limit) if limit
     result = result.offset(offset) if offset
     result
   end
 
   def self.unseen_list_for(user, preload: true, limit: nil)
-    results = list_for(user, preload: preload, limit: limit)
+    results = list_for(user, preload: preload, limit: limit, include_claimed_by_others: false)
     if user.last_seen_reviewable_id
       results = results.where(
         "reviewables.id > ?",
@@ -535,7 +545,7 @@ class Reviewable < ActiveRecord::Base
   end
 
   def self.user_menu_list_for(user, limit: 30)
-    list_for(user, limit: limit, status: :pending).to_a
+    list_for(user, limit: limit, status: :pending, include_claimed_by_others: false).to_a
   end
 
   def self.basic_serializers_for_list(reviewables, user)

app/models/user.rb

@@ -680,11 +680,10 @@ class User < ActiveRecord::Base
   end
 
   def reviewable_count
-    Reviewable.list_for(self).count
-  end
-
-  def unseen_reviewable_count
-    Reviewable.unseen_list_for(self).count
+    Reviewable.list_for(
+      self,
+      include_claimed_by_others: !redesigned_user_menu_enabled?
+    ).count
   end
 
   def saw_notification_id(notification_id)
@@ -716,17 +715,22 @@ class User < ActiveRecord::Base
     query = Reviewable.unseen_list_for(self, preload: false)
 
     if last_seen_reviewable_id
-      query = query.where("id > ?", last_seen_reviewable_id)
+      query = query.where("reviewables.id > ?", last_seen_reviewable_id)
     end
     max_reviewable_id = query.maximum(:id)
 
     if max_reviewable_id
       update!(last_seen_reviewable_id: max_reviewable_id)
-      publish_reviewable_counts(unseen_reviewable_count: self.unseen_reviewable_count)
+      publish_reviewable_counts
     end
   end
 
-  def publish_reviewable_counts(data)
+  def publish_reviewable_counts(extra_data = nil)
+    data = {
+      reviewable_count: self.reviewable_count,
+      unseen_reviewable_count: Reviewable.unseen_reviewable_count(self)
+    }
+    data.merge!(extra_data) if extra_data.present?
     MessageBus.publish("/reviewable_counts/#{self.id}", data, user_ids: [self.id])
   end
 

app/serializers/current_user_serializer.rb

@@ -355,6 +355,10 @@ class CurrentUserSerializer < BasicUserSerializer
     UserStatusSerializer.new(object.user_status, root: false)
   end
 
+  def unseen_reviewable_count
+    Reviewable.unseen_reviewable_count(object)
+  end
+
   def redesigned_user_menu_enabled
     object.redesigned_user_menu_enabled?
   end

app/serializers/reviewable_perform_result_serializer.rb

@@ -47,7 +47,7 @@ class ReviewablePerformResultSerializer < ApplicationSerializer
   end
 
   def unseen_reviewable_count
-    scope.user.unseen_reviewable_count
+    Reviewable.unseen_reviewable_count(scope.user)
   end
 
   def include_unseen_reviewable_count?

spec/jobs/refresh_users_reviewable_counts_spec.rb

@@ -0,0 +1,73 @@
+# frozen_string_literal: true
+
+RSpec.describe Jobs::RefreshUsersReviewableCounts do
+  fab!(:admin) { Fabricate(:admin) }
+  fab!(:moderator) { Fabricate(:moderator) }
+  fab!(:user) { Fabricate(:user) }
+
+  fab!(:group) { Fabricate(:group) }
+  fab!(:topic) { Fabricate(:topic) }
+  fab!(:reviewable1) { Fabricate(:reviewable, reviewable_by_group: group, reviewable_by_moderator: true, topic: topic) }
+
+  fab!(:reviewable2) { Fabricate(:reviewable, reviewable_by_moderator: false) }
+  fab!(:reviewable3) { Fabricate(:reviewable, reviewable_by_moderator: true) }
+
+  before do
+    SiteSetting.enable_category_group_moderation = true
+    group.add(user)
+    topic.category.update!(reviewable_by_group: group)
+    Group.refresh_automatic_groups!
+  end
+
+  describe '#execute' do
+    it "publishes reviewable counts for the members of the specified groups" do
+      messages = MessageBus.track_publish do
+        described_class.new.execute(group_ids: [Group::AUTO_GROUPS[:staff]])
+      end
+      expect(messages.size).to eq(2)
+
+      moderator_message = messages.find { |m| m.user_ids == [moderator.id] }
+      expect(moderator_message.channel).to eq("/reviewable_counts/#{moderator.id}")
+
+      admin_message = messages.find { |m| m.user_ids == [admin.id] }
+      expect(moderator_message.channel).to eq("/reviewable_counts/#{moderator.id}")
+
+      messages = MessageBus.track_publish do
+        described_class.new.execute(group_ids: [group.id])
+      end
+      expect(messages.size).to eq(1)
+
+      user_message = messages.find { |m| m.user_ids == [user.id] }
+      expect(user_message.channel).to eq("/reviewable_counts/#{user.id}")
+    end
+
+    it "published counts respect reviewables visibility" do
+      messages = MessageBus.track_publish do
+        described_class.new.execute(group_ids: [Group::AUTO_GROUPS[:staff], group.id])
+      end
+      expect(messages.size).to eq(3)
+
+      admin_message = messages.find { |m| m.user_ids == [admin.id] }
+      moderator_message = messages.find { |m| m.user_ids == [moderator.id] }
+      user_message = messages.find { |m| m.user_ids == [user.id] }
+
+      expect(admin_message.channel).to eq("/reviewable_counts/#{admin.id}")
+      expect(admin_message.data).to eq(
+        reviewable_count: 3,
+        unseen_reviewable_count: 3
+      )
+
+      expect(moderator_message.channel).to eq("/reviewable_counts/#{moderator.id}")
+      expect(moderator_message.data).to eq(
+        reviewable_count: 2,
+        unseen_reviewable_count: 2
+      )
+
+      expect(user_message.channel).to eq("/reviewable_counts/#{user.id}")
+      expect(user_message.data).to eq(
+        reviewable_count: 1,
+        unseen_reviewable_count: 1
+      )
+    end
+  end
+end

spec/models/reviewable_spec.rb

@@ -623,4 +623,48 @@ RSpec.describe Reviewable, type: :model do
       end
     end
   end
+
+  describe ".unseen_reviewable_count" do
+    fab!(:group) { Fabricate(:group) }
+    fab!(:user) { Fabricate(:user) }
+    fab!(:admin_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: false) }
+    fab!(:mod_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: true) }
+    fab!(:group_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: false, reviewable_by_group: group) }
+
+    it "doesn't include reviewables that can't be seen by the user" do
+      SiteSetting.enable_category_group_moderation = true
+      expect(Reviewable.unseen_reviewable_count(user)).to eq(0)
+      user.groups << group
+      user.save!
+      expect(Reviewable.unseen_reviewable_count(user)).to eq(1)
+      user.update!(moderator: true)
+      expect(Reviewable.unseen_reviewable_count(user)).to eq(2)
+      user.update!(admin: true)
+      expect(Reviewable.unseen_reviewable_count(user)).to eq(3)
+    end
+
+    it "returns count of unseen reviewables" do
+      user.update!(admin: true)
+      expect(Reviewable.unseen_reviewable_count(user)).to eq(3)
+      user.update!(last_seen_reviewable_id: mod_reviewable.id)
+      expect(Reviewable.unseen_reviewable_count(user)).to eq(1)
+      user.update!(last_seen_reviewable_id: group_reviewable.id)
+      expect(Reviewable.unseen_reviewable_count(user)).to eq(0)
+    end
+
+    it "doesn't include reviewables that are claimed by other users" do
+      user.update!(admin: true)
+
+      claimed_by_user = Fabricate(:reviewable, topic: Fabricate(:topic))
+      Fabricate(:reviewable_claimed_topic, topic: claimed_by_user.topic, user: user)
+
+      user2 = Fabricate(:user)
+      claimed_by_user2 = Fabricate(:reviewable, topic: Fabricate(:topic))
+      Fabricate(:reviewable_claimed_topic, topic: claimed_by_user2.topic, user: user2)
+
+      unclaimed = Fabricate(:reviewable, topic: Fabricate(:topic))
+
+      expect(Reviewable.unseen_reviewable_count(user)).to eq(5)
+    end
+  end
 end

spec/models/user_spec.rb

@@ -2974,33 +2974,6 @@ RSpec.describe User do
     end
   end
 
-  describe "#unseen_reviewable_count" do
-    fab!(:admin_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: false) }
-    fab!(:mod_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: true) }
-    fab!(:group_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: false, reviewable_by_group: group) }
-
-    it "doesn't include reviewables that can't be seen by the user" do
-      SiteSetting.enable_category_group_moderation = true
-      expect(user.unseen_reviewable_count).to eq(0)
-      user.groups << group
-      user.save!
-      expect(user.unseen_reviewable_count).to eq(1)
-      user.update!(moderator: true)
-      expect(user.unseen_reviewable_count).to eq(2)
-      user.update!(admin: true)
-      expect(user.unseen_reviewable_count).to eq(3)
-    end
-
-    it "returns count of unseen reviewables" do
-      user.update!(admin: true)
-      expect(user.unseen_reviewable_count).to eq(3)
-      user.update!(last_seen_reviewable_id: mod_reviewable.id)
-      expect(user.unseen_reviewable_count).to eq(1)
-      user.update!(last_seen_reviewable_id: group_reviewable.id)
-      expect(user.unseen_reviewable_count).to eq(0)
-    end
-  end
-
   describe "#bump_last_seen_reviewable!" do
     it "doesn't error if there are no reviewables" do
       Reviewable.destroy_all
@@ -3063,7 +3036,7 @@ RSpec.describe User do
       expect(messages.first).to have_attributes(
         channel: "/reviewable_counts/#{user.id}",
         user_ids: [user.id],
-        data: { unseen_reviewable_count: 0 }
+        data: { unseen_reviewable_count: 0, reviewable_count: 1 }
       )
     end
   end

spec/requests/notifications_controller_spec.rb

@@ -245,6 +245,28 @@ RSpec.describe NotificationsController do
             ])
           end
 
+          it "doesn't include reviewables that are claimed by someone that's not the current user" do
+            SiteSetting.enable_experimental_sidebar_hamburger = true
+            user.update!(admin: true)
+
+            claimed_by_user = Fabricate(:reviewable, topic: Fabricate(:topic), created_at: 5.minutes.ago)
+            Fabricate(:reviewable_claimed_topic, topic: claimed_by_user.topic, user: user)
+
+            user2 = Fabricate(:user)
+            claimed_by_user2 = Fabricate(:reviewable, topic: Fabricate(:topic))
+            Fabricate(:reviewable_claimed_topic, topic: claimed_by_user2.topic, user: user2)
+
+            unclaimed = Fabricate(:reviewable, topic: Fabricate(:topic), created_at: 10.minutes.ago)
+
+            get "/notifications.json", params: { recent: true }
+            expect(response.status).to eq(200)
+            expect(response.parsed_body["pending_reviewables"].map { |r| r["id"] }).to eq([
+              pending_reviewable.id,
+              claimed_by_user.id,
+              unclaimed.id,
+            ])
+          end
+
           it "doesn't include reviewables when the setting is disabled" do
             SiteSetting.enable_experimental_sidebar_hamburger = false
             user.update!(admin: true)

spec/requests/reviewable_claimed_topics_controller_spec.rb

@@ -17,12 +17,11 @@ RSpec.describe ReviewableClaimedTopicsController do
 
     context "when logged in" do
       before do
+        SiteSetting.reviewable_claiming = 'optional'
         sign_in(moderator)
       end
 
       it "works" do
-        SiteSetting.reviewable_claiming = 'optional'
-
         messages = MessageBus.track_publish("/reviewable_claimed") do
           post "/reviewable_claimed_topics.json", params: params
           expect(response.status).to eq(200)
@@ -61,7 +60,6 @@ RSpec.describe ReviewableClaimedTopicsController do
       end
 
       it "works with deleted topics" do
-        SiteSetting.reviewable_claiming = 'optional'
         first_post = topic.first_post || Fabricate(:post, topic: topic)
         PostDestroyer.new(Discourse.system_user, first_post).destroy
 
@@ -72,20 +70,41 @@ RSpec.describe ReviewableClaimedTopicsController do
       end
 
       it "raises an error if user cannot claim the topic" do
+        SiteSetting.reviewable_claiming = 'disabled'
         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.status).to eq(409)
       end
+
+      it "queues a sidekiq job to refresh reviewable counts for users who can see the reviewable" do
+        SiteSetting.enable_experimental_sidebar_hamburger = true
+        SiteSetting.enable_category_group_moderation = true
+
+        not_notified = Fabricate(:user)
+
+        group = Fabricate(:group)
+        topic.category.update!(reviewable_by_group: group)
+        reviewable.update!(reviewable_by_group: group)
+
+        notified = Fabricate(:user)
+        group.add(notified)
+
+        expect_enqueued_with(
+          job: :refresh_users_reviewable_counts,
+          args: { group_ids: [Group::AUTO_GROUPS[:staff], group.id] }
+        ) do
+          post "/reviewable_claimed_topics.json", params: params
+          expect(response.status).to eq(200)
+        end
+      end
     end
   end
 
@@ -137,5 +156,28 @@ RSpec.describe ReviewableClaimedTopicsController do
 
       expect(response.status).to eq(403)
     end
+
+    it "queues a sidekiq job to refresh reviewable counts for users who can see the reviewable" do
+      SiteSetting.reviewable_claiming = 'optional'
+      SiteSetting.enable_experimental_sidebar_hamburger = true
+      SiteSetting.enable_category_group_moderation = true
+
+      not_notified = Fabricate(:user)
+
+      group = Fabricate(:group)
+      topic.category.update!(reviewable_by_group: group)
+      reviewable.update!(reviewable_by_group: group)
+
+      notified = Fabricate(:user)
+      group.add(notified)
+
+      expect_enqueued_with(
+        job: :refresh_users_reviewable_counts,
+        args: { group_ids: [Group::AUTO_GROUPS[:staff], group.id] }
+      ) do
+        delete "/reviewable_claimed_topics/#{claimed.topic_id}.json"
+        expect(response.status).to eq(200)
+      end
+    end
   end
 end