From e9ea0102a50e90cfd7c1e16915a423ddf7c4122e Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Sat, 15 Dec 2018 08:01:35 +0800
Subject: [PATCH] FIX: Consistency about our response for invalid user id in
 `Admin::UsersController`.

---
 app/controllers/admin/users_controller.rb    |  7 +++--
 spec/requests/admin/users_controller_spec.rb | 32 +++++++++++---------
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index ae0ed9de28b..14627a10873 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -26,7 +26,8 @@ class Admin::UsersController < Admin::AdminController
                                     :revoke_api_key,
                                     :anonymize,
                                     :reset_bounce_score,
-                                    :disable_second_factor]
+                                    :disable_second_factor,
+                                    :delete_posts_batch]
 
   def index
     users = ::AdminUserIndexQuery.new(params).find_users
@@ -46,8 +47,7 @@ class Admin::UsersController < Admin::AdminController
   end
 
   def delete_posts_batch
-    user = User.find_by(id: params[:user_id])
-    deleted_posts = user.delete_posts_in_batches(guardian)
+    deleted_posts = @user.delete_posts_in_batches(guardian)
     # staff action logs will have an entry for each post
 
     render json: { posts_deleted: deleted_posts.length }
@@ -563,6 +563,7 @@ class Admin::UsersController < Admin::AdminController
 
   def fetch_user
     @user = User.find_by(id: params[:user_id])
+    raise Discourse::NotFound unless @user
   end
 
   def refresh_browser(user)
diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb
index 6671cdd0393..33d8e7b19e7 100644
--- a/spec/requests/admin/users_controller_spec.rb
+++ b/spec/requests/admin/users_controller_spec.rb
@@ -241,9 +241,9 @@ RSpec.describe Admin::UsersController do
       expect(AdminConfirmation.exists_for?(another_user.id)).to eq(false)
     end
 
-    it "returns a 403 if the username doesn't exist" do
+    it "returns a 404 if the username doesn't exist" do
       put "/admin/users/123123/grant_admin.json"
-      expect(response.status).to eq(403)
+      expect(response.status).to eq(404)
     end
 
     it 'updates the admin flag' do
@@ -300,9 +300,9 @@ RSpec.describe Admin::UsersController do
       expect(response.status).to eq(404)
     end
 
-    it "returns a 422 if the username doesn't exist" do
+    it "returns a 404 if the username doesn't exist" do
       put "/admin/users/123123/trust_level.json"
-      expect(response.status).to eq(422)
+      expect(response.status).to eq(404)
     end
 
     it "upgrades the user's trust level" do
@@ -347,9 +347,9 @@ RSpec.describe Admin::UsersController do
       expect(response.status).to eq(404)
     end
 
-    it "returns a 403 if the username doesn't exist" do
+    it "returns a 404 if the username doesn't exist" do
       put "/admin/users/123123/grant_moderation.json"
-      expect(response.status).to eq(403)
+      expect(response.status).to eq(404)
     end
 
     it 'updates the moderator flag' do
@@ -394,7 +394,7 @@ RSpec.describe Admin::UsersController do
 
     it "returns a 404 if the user doesn't exist" do
       put "/admin/users/123123/primary_group.json"
-      expect(response.status).to eq(403)
+      expect(response.status).to eq(404)
     end
 
     it "changes the user's primary group" do
@@ -554,9 +554,9 @@ RSpec.describe Admin::UsersController do
       expect(reg_user).not_to be_silenced
     end
 
-    it "returns a 403 if the user doesn't exist" do
+    it "returns a 404 if the user doesn't exist" do
       put "/admin/users/123123/silence.json"
-      expect(response.status).to eq(403)
+      expect(response.status).to eq(404)
     end
 
     it "punishes the user for spamming" do
@@ -626,7 +626,7 @@ RSpec.describe Admin::UsersController do
 
     it "returns a 403 if the user doesn't exist" do
       put "/admin/users/123123/unsilence.json"
-      expect(response.status).to eq(403)
+      expect(response.status).to eq(404)
     end
 
     it "unsilences the user" do
@@ -943,6 +943,14 @@ RSpec.describe Admin::UsersController do
   end
 
   describe "#delete_posts_batch" do
+    describe 'when user is is invalid' do
+      it 'should return the right response' do
+        put "/admin/users/nothing/delete_posts_batch.json"
+
+        expect(response.status).to eq(404)
+      end
+    end
+
     context "when there are user posts" do
       before do
         post = Fabricate(:post, user: user)
@@ -951,8 +959,6 @@ RSpec.describe Admin::UsersController do
       end
 
       it 'returns how many posts were deleted' do
-        sign_in(admin)
-
         put "/admin/users/#{user.id}/delete_posts_batch.json"
         expect(response.status).to eq(200)
         expect(JSON.parse(response.body)["posts_deleted"]).to eq(3)
@@ -961,8 +967,6 @@ RSpec.describe Admin::UsersController do
 
     context "when there are no posts left to be deleted" do
       it "returns correct json" do
-        sign_in(admin)
-
         put "/admin/users/#{user.id}/delete_posts_batch.json"
         expect(response.status).to eq(200)
         expect(JSON.parse(response.body)["posts_deleted"]).to eq(0)