FIX: Don't show silence button on staff users and display similar users (#28423)

This commit fixes a bug where the silence button is incorrectly displayed on the admin page of a staff user. It's not actually possible to silence a staff user because the backend correctly prevents it, but the frontend isn't checking if the button should be displayed.

Another small bug that this commit fixes is the similar users list not showing up inside the silence/suspend modals due to also a bug in the frontend.

I've also changed the way similar users are loaded so that they're not returned by the `admin/users#show` endpoint anymore and moved them into a new endpoint that the penalize modals (suspend and silence) can call directly to retrieve the list of users. This is done because the similar users list is never shown on the admin user page (`/admin/users/:user_id/:username`); they're only needed when the suspend or silence modals are opened.

Internal topic: t/130014.

app/assets/javascripts/admin/addon/components/admin-penalty-similar-users.hbs

@@ -3,42 +3,44 @@
     {{html-safe
       (i18n
         "admin.user.other_matches"
-        (hash count=this.user.similar_users_count username=this.user.username)
+        (hash count=@user.similar_users_count username=@user.username)
       )
     }}
   </p>
 
-  <table class="table">
-    <thead>
-      <tr>
-        <th></th>
-        <th>{{i18n "username"}}</th>
-        <th>{{i18n "last_seen"}}</th>
-        <th>{{i18n "admin.user.topics_entered"}}</th>
-        <th>{{i18n "admin.user.posts_read_count"}}</th>
-        <th>{{i18n "admin.user.time_read"}}</th>
-        <th>{{i18n "created"}}</th>
-      </tr>
-    </thead>
-
-    <tbody>
-      {{#each this.user.similar_users as |user|}}
+  <ConditionalLoadingSpinner @condition={{this.isLoading}}>
+    <table class="table">
+      <thead>
         <tr>
-          <td>
-            <Input
-              @type="checkbox"
-              disabled={{not (get user this.penaltyField)}}
-              {{on "click" (fn this.selectUserId user.id)}}
-            />
-          </td>
-          <td>{{avatar user imageSize="small"}} {{user.username}}</td>
-          <td>{{format-duration user.last_seen_age}}</td>
-          <td>{{number user.topics_entered}}</td>
-          <td>{{number user.posts_read_count}}</td>
-          <td>{{format-duration user.time_read}}</td>
-          <td>{{format-duration user.created_at_age}}</td>
+          <th></th>
+          <th>{{i18n "username"}}</th>
+          <th>{{i18n "last_seen"}}</th>
+          <th>{{i18n "admin.user.topics_entered"}}</th>
+          <th>{{i18n "admin.user.posts_read_count"}}</th>
+          <th>{{i18n "admin.user.time_read"}}</th>
+          <th>{{i18n "created"}}</th>
         </tr>
-      {{/each}}
-    </tbody>
-  </table>
+      </thead>
+
+      <tbody>
+        {{#each this.similarUsers as |user|}}
+          <tr>
+            <td>
+              <Input
+                @type="checkbox"
+                disabled={{not (get user this.penaltyField)}}
+                {{on "click" (fn this.selectUserId user.id)}}
+              />
+            </td>
+            <td>{{avatar user imageSize="small"}} {{user.username}}</td>
+            <td>{{format-duration user.last_seen_age}}</td>
+            <td>{{number user.topics_entered}}</td>
+            <td>{{number user.posts_read_count}}</td>
+            <td>{{format-duration user.time_read}}</td>
+            <td>{{format-duration user.created_at_age}}</td>
+          </tr>
+        {{/each}}
+      </tbody>
+    </table>
+  </ConditionalLoadingSpinner>
 </div>

app/assets/javascripts/admin/addon/components/admin-penalty-similar-users.js

@@ -1,12 +1,21 @@
-import Component from "@ember/component";
+import Component from "@glimmer/component";
+import { tracked } from "@glimmer/tracking";
 import { action } from "@ember/object";
-import { tagName } from "@ember-decorators/component";
-import discourseComputed from "discourse-common/utils/decorators";
+import { ajax } from "discourse/lib/ajax";
 
-@tagName("")
 export default class AdminPenaltySimilarUsers extends Component {
-  @discourseComputed("penaltyType")
-  penaltyField(penaltyType) {
+  @tracked isLoading;
+  @tracked similarUsers = [];
+  selectedUserIds = [];
+
+  constructor() {
+    super(...arguments);
+
+    this.loadSimilarUsers();
+  }
+
+  get penaltyField() {
+    const penaltyType = this.args.penaltyType;
     if (penaltyType === "suspend") {
       return "can_be_suspended";
     } else if (penaltyType === "silence") {
@@ -14,16 +23,26 @@ export default class AdminPenaltySimilarUsers extends Component {
     }
   }
 
-  @action
-  selectUserId(userId, event) {
-    if (!this.selectedUserIds) {
-      return;
-    }
-
-    if (event.target.checked) {
-      this.selectedUserIds.pushObject(userId);
-    } else {
-      this.selectedUserIds.removeObject(userId);
+  async loadSimilarUsers() {
+    this.isLoading = true;
+    try {
+      const data = await ajax(
+        `/admin/users/${this.args.user.id}/similar-users.json`
+      );
+      this.similarUsers = data.users;
+    } finally {
+      this.isLoading = false;
     }
   }
+
+  @action
+  selectUserId(userId, event) {
+    if (event.target.checked) {
+      this.selectedUserIds.push(userId);
+    } else {
+      this.selectedUserIds = this.selectedUserIds.filter((id) => id !== userId);
+    }
+
+    this.args.onUsersChanged(this.selectedUserIds);
+  }
 }

app/assets/javascripts/admin/addon/components/modal/penalize-user.hbs

@@ -46,11 +46,12 @@
           @postEdit={{this.postEdit}}
         />
       {{/if}}
-      {{#if this.user.similar_users}}
+      {{#if @model.user.similar_users_count}}
         <AdminPenaltySimilarUsers
           @penaltyType={{@model.penaltyType}}
-          @user={{this.user}}
+          @user={{@model.user}}
           @selectedUserIds={{this.otherUserIds}}
+          @onUsersChanged={{this.similarUsersChanged}}
         />
       {{/if}}
     {{else}}
@@ -60,9 +61,9 @@
         <div class="cant-silence">{{i18n "admin.user.cant_silence"}}</div>
       {{/if}}
     {{/if}}
-    <div class="penalty-history">{{html-safe this.penaltyHistory}}</div>
   </:body>
   <:footer>
+    <div class="penalty-history">{{html-safe this.penaltyHistory}}</div>
     <DButton
       class="btn-danger perform-penalize"
       @action={{this.penalizeUser}}

app/assets/javascripts/admin/addon/components/modal/penalize-user.js

@@ -122,4 +122,9 @@ export default class PenalizeUser extends Component {
 
     this.args.closeModal();
   }
+
+  @action
+  similarUsersChanged(userIds) {
+    this.otherUserIds = userIds;
+  }
 }

app/assets/javascripts/admin/addon/templates/user-index.hbs

@@ -543,13 +543,15 @@
           />
           {{i18n "admin.user.silence_explanation"}}
         {{else}}
-          <DButton
-            @action={{this.showSilenceModal}}
-            @icon="microphone-slash"
-            @label="admin.user.silence"
-            class="btn-danger silence-user"
-          />
-          {{i18n "admin.user.silence_explanation"}}
+          {{#if this.model.canSilence}}
+            <DButton
+              @action={{this.showSilenceModal}}
+              @icon="microphone-slash"
+              @label="admin.user.silence"
+              class="btn-danger silence-user"
+            />
+            {{i18n "admin.user.silence_explanation"}}
+          {{/if}}
         {{/if}}
       </ConditionalLoadingSpinner>
     </div>

app/assets/stylesheets/common/admin/penalty.scss

@@ -91,7 +91,7 @@
     position: sticky;
     bottom: 0;
     background-color: var(--secondary);
-    padding: 0.5em 0 1em 0;
+    padding: 0 0 1em 0;
   }
   .penalty-history::before {
     position: absolute;

app/controllers/admin/users_controller.rb

@@ -46,18 +46,28 @@ class Admin::UsersController < Admin::StaffController
     @user = User.find_by(id: params[:id])
     raise Discourse::NotFound unless @user
 
-    similar_users =
-      User
-        .real
-        .where.not(id: @user.id)
-        .where(ip_address: @user.ip_address, admin: false, moderator: false)
-
     render_serialized(
       @user,
       AdminDetailedUserSerializer,
       root: false,
-      similar_users: similar_users.limit(MAX_SIMILAR_USERS),
-      similar_users_count: similar_users.count,
+      similar_users_count: @user.similar_users.count,
+    )
+  end
+
+  def similar_users
+    @user = User.find_by(id: params[:user_id])
+    raise Discourse::NotFound if !@user
+
+    render_json_dump(
+      {
+        users:
+          ActiveModel::ArraySerializer.new(
+            @user.similar_users.limit(MAX_SIMILAR_USERS),
+            each_serializer: SimilarAdminUserSerializer,
+            scope: guardian,
+            root: false,
+          ),
+      },
     )
   end
 

app/models/user.rb

@@ -1884,6 +1884,13 @@ class User < ActiveRecord::Base
     update(required_fields_version: UserRequiredFieldsVersion.current)
   end
 
+  def similar_users
+    User
+      .real
+      .where.not(id: self.id)
+      .where(ip_address: self.ip_address, admin: false, moderator: false)
+  end
+
   protected
 
   def badge_grant

app/serializers/admin_detailed_user_serializer.rb

@@ -36,7 +36,6 @@ class AdminDetailedUserSerializer < AdminUserSerializer
              :can_delete_sso_record,
              :api_key_count,
              :external_ids,
-             :similar_users,
              :similar_users_count
 
   has_one :approved_by, serializer: BasicUserSerializer, embed: :objects
@@ -157,25 +156,12 @@ class AdminDetailedUserSerializer < AdminUserSerializer
     external_ids
   end
 
-  def similar_users
-    ActiveModel::ArraySerializer.new(
-      @options[:similar_users],
-      each_serializer: SimilarAdminUserSerializer,
-      scope: scope,
-      root: false,
-    ).as_json
-  end
-
-  def include_similar_users?
-    @options[:similar_users].present?
-  end
-
   def similar_users_count
     @options[:similar_users_count]
   end
 
   def include_similar_users_count?
-    @options[:similar_users].present?
+    @options[:similar_users_count].present?
   end
 
   def can_delete_sso_record

config/routes.rb

@@ -161,6 +161,7 @@ Discourse::Application.routes.draw do
         post "reset-bounce-score"
         put "disable_second_factor"
         delete "sso_record"
+        get "similar-users.json" => "users#similar_users"
       end
       get "users/:id.json" => "users#show", :defaults => { format: "json" }
       get "users/:id/:username" => "users#show",

spec/requests/admin/users_controller_spec.rb

@@ -109,7 +109,7 @@ RSpec.describe Admin::UsersController do
         expect(response.parsed_body["id"]).to eq(user.id)
       end
 
-      it "includes similar users who aren't admin or mods" do
+      it "includes count of similiar users" do
         Fabricate(:user, ip_address: "88.88.88.88")
         Fabricate(:admin, ip_address: user.ip_address)
         Fabricate(:moderator, ip_address: user.ip_address)
@@ -118,11 +118,7 @@ RSpec.describe Admin::UsersController do
         get "/admin/users/#{user.id}.json"
 
         expect(response.status).to eq(200)
-        expect(response.parsed_body["id"]).to eq(user.id)
         expect(response.parsed_body["similar_users_count"]).to eq(1)
-        expect(response.parsed_body["similar_users"].map { |u| u["id"] }).to contain_exactly(
-          similar_user.id,
-        )
       end
     end
 
@@ -138,6 +134,22 @@ RSpec.describe Admin::UsersController do
     end
   end
 
+  describe "#similar_users" do
+    before { sign_in(admin) }
+
+    it "includes similar users who aren't admin or mods" do
+      Fabricate(:user, ip_address: "88.88.88.88")
+      Fabricate(:admin, ip_address: user.ip_address)
+      Fabricate(:moderator, ip_address: user.ip_address)
+      similar_user = Fabricate(:user, ip_address: user.ip_address)
+
+      get "/admin/users/#{user.id}/similar-users.json"
+
+      expect(response.status).to eq(200)
+      expect(response.parsed_body["users"].map { |u| u["id"] }).to contain_exactly(similar_user.id)
+    end
+  end
+
   describe "#approve" do
     let(:evil_trout) { Fabricate(:evil_trout) }
 

spec/requests/api/schemas/json/admin_user_response.json

@@ -170,6 +170,9 @@
     "api_key_count": {
       "type": "integer"
     },
+    "similar_users_count": {
+      "type": "integer"
+    },
     "single_sign_on_record": {
       "type": ["string", "null"]
     },

spec/system/admin_user_spec.rb

@@ -0,0 +1,69 @@
+# frozen_string_literal: true
+
+describe "Admin User Page", type: :system do
+  fab!(:current_user) { Fabricate(:admin) }
+
+  let(:admin_user_page) { PageObjects::Pages::AdminUser.new }
+  let(:suspend_user_modal) { PageObjects::Modals::PenalizeUser.new("suspend") }
+  let(:silence_user_modal) { PageObjects::Modals::PenalizeUser.new("silence") }
+
+  before { sign_in(current_user) }
+
+  context "when visiting an admin's page" do
+    fab!(:admin)
+
+    before { admin_user_page.visit(admin) }
+
+    it "doesn't display the suspend or silence buttons" do
+      expect(admin_user_page).to have_no_suspend_button
+      expect(admin_user_page).to have_no_silence_button
+    end
+  end
+
+  context "when visiting a moderator's page" do
+    fab!(:moderator)
+
+    before { admin_user_page.visit(moderator) }
+
+    it "doesn't display the suspend or silence buttons" do
+      expect(admin_user_page).to have_no_suspend_button
+      expect(admin_user_page).to have_no_silence_button
+    end
+  end
+
+  context "when visting a regular user's page" do
+    fab!(:user) { Fabricate(:user, ip_address: "93.123.44.90") }
+    fab!(:similar_user) { Fabricate(:user, ip_address: user.ip_address) }
+    fab!(:another_mod) { Fabricate(:moderator, ip_address: user.ip_address) }
+    fab!(:another_admin) { Fabricate(:admin, ip_address: user.ip_address) }
+
+    before { admin_user_page.visit(user) }
+
+    it "displays the suspend and silence buttons" do
+      expect(admin_user_page).to have_suspend_button
+      expect(admin_user_page).to have_silence_button
+    end
+
+    describe "the suspend user modal" do
+      it "displays the list of users who share the same IP but are not mods or admins" do
+        admin_user_page.click_suspend_button
+
+        expect(suspend_user_modal.similar_users).to contain_exactly(similar_user.username)
+        expect(admin_user_page.similar_users_warning).to include(
+          I18n.t("admin_js.admin.user.other_matches", count: 1, username: user.username),
+        )
+      end
+    end
+
+    describe "the silence user modal" do
+      it "displays the list of users who share the same IP but are not mods or admins" do
+        admin_user_page.click_silence_button
+
+        expect(silence_user_modal.similar_users).to contain_exactly(similar_user.username)
+        expect(admin_user_page.similar_users_warning).to include(
+          I18n.t("admin_js.admin.user.other_matches", count: 1, username: user.username),
+        )
+      end
+    end
+  end
+end

spec/system/page_objects/modals/penalize_user.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Modals
+    class PenalizeUser < PageObjects::Modals::Base
+      def initialize(penalty_type)
+        @penalty_type = penalty_type
+      end
+
+      def similar_users
+        modal.all("table tbody tr td:nth-child(2)").map(&:text)
+      end
+
+      def modal
+        find(".d-modal.#{@penalty_type}-user-modal")
+      end
+    end
+  end
+end

spec/system/page_objects/pages/admin_user.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Pages
+    class AdminUser < PageObjects::Pages::Base
+      def visit(user)
+        page.visit("/admin/users/#{user.id}/#{user.username}")
+      end
+
+      def has_suspend_button?
+        has_css?(".btn-danger.suspend-user")
+      end
+
+      def has_no_suspend_button?
+        has_no_css?(".btn-danger.suspend-user")
+      end
+
+      def has_silence_button?
+        has_css?(".btn-danger.silence-user")
+      end
+
+      def has_no_silence_button?
+        has_no_css?(".btn-danger.silence-user")
+      end
+
+      def click_suspend_button
+        find(".btn-danger.suspend-user").click
+      end
+
+      def click_silence_button
+        find(".btn-danger.silence-user").click
+      end
+
+      def similar_users_warning
+        find(".penalty-similar-users .alert-warning")["innerHTML"]
+      end
+    end
+  end
+end