From 8e35f9ed571d21628ebc3e384663f520b68c4e3c Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Tue, 13 Jul 2021 16:16:23 -0500
Subject: [PATCH 01/13] Reset feature branch

Reset hard master and create banned user table

Add ban service spec

Add specs for banned user and ban service

Add unban service file

BannedUser object gets created and destroyed

Add unban service specs

Add specs for unban user

Run rake translate

Changelog: added
---
 app/controllers/admin/users_controller.rb     |  4 +-
 app/models/user.rb                            |  1 +
 app/models/users/banned_user.rb               | 12 ++++
 app/services/users/ban_service.rb             | 17 +++++
 app/services/users/unban_service.rb           | 38 +++++++++++
 .../20210713211008_create_banned_users.rb     | 10 +++
 db/schema_migrations/20210713211008           |  1 +
 db/structure.sql                              | 12 ++++
 locale/gitlab.pot                             |  6 ++
 spec/models/user_spec.rb                      |  1 +
 spec/models/users/banned_user_spec.rb         | 21 ++++++
 spec/services/users/ban_service_spec.rb       | 40 +++++++----
 spec/services/users/unban_service_spec.rb     | 66 +++++++++++++++++++
 13 files changed, 214 insertions(+), 15 deletions(-)
 create mode 100644 app/models/users/banned_user.rb
 create mode 100644 app/services/users/unban_service.rb
 create mode 100644 db/migrate/20210713211008_create_banned_users.rb
 create mode 100644 db/schema_migrations/20210713211008
 create mode 100644 spec/models/users/banned_user_spec.rb
 create mode 100644 spec/services/users/unban_service_spec.rb

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 700acc46d8d8a8..cc6b537870bef3 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -136,7 +136,9 @@ def ban
   end
 
   def unban
-    if update_user { |user| user.activate }
+    result = Users::UnbanService.new(current_user).execute(user)
+
+    if result[:status] == :success
       redirect_back_or_admin_user(notice: _("Successfully unbanned"))
     else
       redirect_back_or_admin_user(alert: _("Error occurred. User was not unbanned"))
diff --git a/app/models/user.rb b/app/models/user.rb
index 80b8c9173d1f86..1532c6d65d39d7 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -205,6 +205,7 @@ def update_tracked_fields!(request)
   has_one :user_canonical_email
   has_one :credit_card_validation, class_name: '::Users::CreditCardValidation'
   has_one :atlassian_identity, class_name: 'Atlassian::Identity'
+  has_one :banned_user, class_name: '::Users::BannedUser'
 
   has_many :reviews, foreign_key: :author_id, inverse_of: :author
 
diff --git a/app/models/users/banned_user.rb b/app/models/users/banned_user.rb
new file mode 100644
index 00000000000000..ed987802d23b9b
--- /dev/null
+++ b/app/models/users/banned_user.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+module Users
+  class BannedUser < ApplicationRecord
+    self.primary_key = :user_id
+
+    belongs_to :user
+
+    validates :user, presence: true
+    validates :user_id, uniqueness: { message: "banned user already exists" }
+  end
+end
diff --git a/app/services/users/ban_service.rb b/app/services/users/ban_service.rb
index 247ed14966bedb..6b09edccb679da 100644
--- a/app/services/users/ban_service.rb
+++ b/app/services/users/ban_service.rb
@@ -7,7 +7,10 @@ def initialize(current_user)
     end
 
     def execute(user)
+      return error(_('You are not allowed to ban a user'), :forbidden) unless allowed?
+
       if user.ban
+        create_banned_user(user)
         log_event(user)
         success
       else
@@ -18,6 +21,20 @@ def execute(user)
 
     private
 
+    def allowed?
+      can?(current_user, :admin_all_resources)
+    end
+
+    def create_banned_user(user)
+      Users::BannedUser.create(user: user)
+    end
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def hide_contributions(user)
+      Users::HideContributionsWorker.perform_async(user.id)
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
     def log_event(user)
       Gitlab::AppLogger.info(message: "User banned", user: "#{user.username}", email: "#{user.email}", banned_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
     end
diff --git a/app/services/users/unban_service.rb b/app/services/users/unban_service.rb
new file mode 100644
index 00000000000000..00af867d8a0bf7
--- /dev/null
+++ b/app/services/users/unban_service.rb
@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+module Users
+  class UnbanService < BaseService
+    def initialize(current_user)
+      @current_user = current_user
+    end
+
+    def execute(user)
+      return error(_('You are not allowed to unban a user'), :forbidden) unless allowed?
+
+      if user.activate
+        destroy_banned_user(user)
+        log_event(user)
+        success
+      else
+        messages = user.errors.full_messages
+        error(messages.uniq.join('. '))
+      end
+    end
+
+    private
+
+    def allowed?
+      can?(current_user, :admin_all_resources)
+    end
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def destroy_banned_user(user)
+      Users::BannedUser.find_by(user: user).destroy
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
+    def log_event(user)
+      Gitlab::AppLogger.info(message: "User unbanned", user: "#{user.username}", email: "#{user.email}", banned_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
+    end
+  end
+end
diff --git a/db/migrate/20210713211008_create_banned_users.rb b/db/migrate/20210713211008_create_banned_users.rb
new file mode 100644
index 00000000000000..dd16dfc31307fa
--- /dev/null
+++ b/db/migrate/20210713211008_create_banned_users.rb
@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+class CreateBannedUsers < ActiveRecord::Migration[6.1]
+  def change
+    create_table :banned_users, id: false do |t|
+      t.timestamps_with_timezone null: false
+      t.references :user, primary_key: true, default: nil, foreign_key: { on_delete: :cascade }, type: :bigint, index: false, null: false
+    end
+  end
+end
diff --git a/db/schema_migrations/20210713211008 b/db/schema_migrations/20210713211008
new file mode 100644
index 00000000000000..75ccad3e3483be
--- /dev/null
+++ b/db/schema_migrations/20210713211008
@@ -0,0 +1 @@
+f66d8f3bc32996fe7743cc98cfb96fedd86784d38c8debb5143b7adabdfebd18
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index da2d4c50068944..7a7e5208e28991 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -9901,6 +9901,12 @@ CREATE SEQUENCE badges_id_seq
 
 ALTER SEQUENCE badges_id_seq OWNED BY badges.id;
 
+CREATE TABLE banned_users (
+    created_at timestamp with time zone NOT NULL,
+    updated_at timestamp with time zone NOT NULL,
+    user_id bigint NOT NULL
+);
+
 CREATE TABLE batched_background_migration_jobs (
     id bigint NOT NULL,
     created_at timestamp with time zone NOT NULL,
@@ -20979,6 +20985,9 @@ ALTER TABLE ONLY background_migration_jobs
 ALTER TABLE ONLY badges
     ADD CONSTRAINT badges_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY banned_users
+    ADD CONSTRAINT banned_users_pkey PRIMARY KEY (user_id);
+
 ALTER TABLE ONLY batched_background_migration_jobs
     ADD CONSTRAINT batched_background_migration_jobs_pkey PRIMARY KEY (id);
 
@@ -28082,6 +28091,9 @@ ALTER TABLE ONLY merge_trains
 ALTER TABLE ONLY ci_runner_namespaces
     ADD CONSTRAINT fk_rails_f9d9ed3308 FOREIGN KEY (namespace_id) REFERENCES namespaces(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY banned_users
+    ADD CONSTRAINT fk_rails_fa5bb598e5 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY requirements_management_test_reports
     ADD CONSTRAINT fk_rails_fb3308ad55 FOREIGN KEY (requirement_id) REFERENCES requirements(id) ON DELETE CASCADE;
 
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 5a8c95a0ebff86..dfcfe2357b07e3 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -37308,6 +37308,9 @@ msgstr ""
 msgid "You are not allowed to approve a user"
 msgstr ""
 
+msgid "You are not allowed to ban a user"
+msgstr ""
+
 msgid "You are not allowed to log in using password"
 msgstr ""
 
@@ -37317,6 +37320,9 @@ msgstr ""
 msgid "You are not allowed to reject a user"
 msgstr ""
 
+msgid "You are not allowed to unban a user"
+msgstr ""
+
 msgid "You are not allowed to unlink your primary login account"
 msgstr ""
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 0eb769c65cd9f5..c9b0c0f0d8ad76 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -89,6 +89,7 @@
     it { is_expected.to have_one(:atlassian_identity) }
     it { is_expected.to have_one(:user_highest_role) }
     it { is_expected.to have_one(:credit_card_validation) }
+    it { is_expected.to have_one(:banned_user) }
     it { is_expected.to have_many(:snippets).dependent(:destroy) }
     it { is_expected.to have_many(:members) }
     it { is_expected.to have_many(:project_members) }
diff --git a/spec/models/users/banned_user_spec.rb b/spec/models/users/banned_user_spec.rb
new file mode 100644
index 00000000000000..dda8d82c7982a2
--- /dev/null
+++ b/spec/models/users/banned_user_spec.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::BannedUser do
+  it { is_expected.to belong_to(:user) }
+
+  it 'validates uniqueness of banned user id' do
+    subject.user = create(:user)
+    is_expected.to validate_uniqueness_of(:user_id).with_message("banned user already exists")
+  end
+
+  context 'creates a new banned user' do
+    let(:user) { create(:user) }
+
+    it 'creates a new banned user object with is_banned ban state', :aggregate_failures do
+      expect { described_class.new(user: user).save! }.to change { described_class.count }.by(1)
+      expect(described_class.last.user_id).to eq(user.id)
+    end
+  end
+end
diff --git a/spec/services/users/ban_service_spec.rb b/spec/services/users/ban_service_spec.rb
index 0e6ac615da5f40..2ead6fb378ea64 100644
--- a/spec/services/users/ban_service_spec.rb
+++ b/spec/services/users/ban_service_spec.rb
@@ -4,23 +4,20 @@
 
 RSpec.describe Users::BanService do
   let(:current_user) { create(:admin) }
+  let(:user) { create(:user) }
 
   subject(:service) { described_class.new(current_user) }
 
   describe '#execute' do
     subject(:operation) { service.execute(user) }
 
-    context 'when successful' do
-      let(:user) { create(:user) }
-
+    context 'when successful', :enable_admin_mode do
       it { is_expected.to eq(status: :success) }
 
-      it "bans the user" do
-        expect { operation }.to change { user.state }.to('banned')
-      end
-
-      it "blocks the user" do
-        expect { operation }.to change { user.blocked? }.from(false).to(true)
+      it 'bans the user and creates a BannedUser', :aggregate_failures do
+        expect { operation }.to change { Users::BannedUser.count }.by(1)
+        expect(user.state).to eq('banned')
+        expect(user.blocked?).to be_truthy
       end
 
       it 'logs ban in application logs' do
@@ -33,17 +30,32 @@
     end
 
     context 'when failed' do
-      let(:user) { create(:user, :blocked) }
+      context 'when user is already blocked', :enable_admin_mode do
+        before do
+          user.block!
+        end
 
-      it 'returns error result' do
-        aggregate_failures 'error result' do
+        it 'error result' do
           expect(operation[:status]).to eq(:error)
           expect(operation[:message]).to match(/State cannot transition/)
         end
+
+        it 'does not ban the user', :aggregate_failures do
+          expect { operation }.not_to change { Users::BannedUser.count }
+          expect(user.state).to eq('blocked')
+        end
       end
 
-      it "does not change the user's state" do
-        expect { operation }.not_to change { user.state }
+      context 'when user is not an admin' do
+        it 'error result' do
+          expect(operation[:status]).to eq(:error)
+          expect(operation[:message]).to match(/You are not allowed to ban a user/)
+        end
+
+        it 'does not ban the user', :aggregate_failures do
+          expect { operation }.not_to change { Users::BannedUser.count }
+          expect(user.state).to eq('active')
+        end
       end
     end
   end
diff --git a/spec/services/users/unban_service_spec.rb b/spec/services/users/unban_service_spec.rb
new file mode 100644
index 00000000000000..eb93763ad9221a
--- /dev/null
+++ b/spec/services/users/unban_service_spec.rb
@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::UnbanService do
+  let(:current_user) { create(:admin) }
+  let(:user) { create(:user) }
+
+  subject(:service) { described_class.new(current_user) }
+
+  describe '#execute' do
+    subject(:operation) { service.execute(user) }
+
+    context 'when successful', :enable_admin_mode do
+      before do
+        Users::BanService.new(current_user).execute(user)
+      end
+
+      it { is_expected.to eq(status: :success) }
+
+      it 'unbans the user', :aggregate_failures do
+        expect { operation }.to change {user.state}.from('banned').to('active')
+      end
+
+      it 'removes the BannedUser object' do
+        expect { operation }.to change { Users::BannedUser.count }.by(-1)
+      end
+
+      it 'logs ban in application logs' do
+        allow(Gitlab::AppLogger).to receive(:info)
+
+        operation
+
+        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User unbanned", user: "#{user.username}", email: "#{user.email}", banned_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
+      end
+    end
+
+    context 'when failed' do
+      context 'when user is not already banned', :enable_admin_mode do
+        it 'error result' do
+          expect(operation[:status]).to eq(:error)
+          expect(operation[:message]).to match(/State cannot transition/)
+        end
+
+        it 'does not change the user state', :aggregate_failures do
+          expect { operation }.not_to change { user.state }
+        end
+      end
+
+      context 'when user is not an admin' do
+        before do
+          Users::BanService.new(current_user).execute(user)
+        end
+
+        it 'error result' do
+          expect(operation[:status]).to eq(:error)
+          expect(operation[:message]).to match(/You are not allowed to unban a user/)
+        end
+
+        it 'does not change the user state', :aggregate_failures do
+          expect { operation }.not_to change { user.state }
+        end
+      end
+    end
+  end
+end
-- 
GitLab


From ed9cd0535d97a94ecbb0388b831d3108fb2a1d70 Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Tue, 13 Jul 2021 19:40:48 -0500
Subject: [PATCH 02/13] Remove hide contributions method

---
 app/services/users/ban_service.rb | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/app/services/users/ban_service.rb b/app/services/users/ban_service.rb
index 6b09edccb679da..7724e95dc71ec5 100644
--- a/app/services/users/ban_service.rb
+++ b/app/services/users/ban_service.rb
@@ -29,12 +29,6 @@ def create_banned_user(user)
       Users::BannedUser.create(user: user)
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def hide_contributions(user)
-      Users::HideContributionsWorker.perform_async(user.id)
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-
     def log_event(user)
       Gitlab::AppLogger.info(message: "User banned", user: "#{user.username}", email: "#{user.email}", banned_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
     end
-- 
GitLab


From 53f56e7f7aef26e5792ad6c901e6947ca05c565d Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Tue, 13 Jul 2021 20:18:50 -0500
Subject: [PATCH 03/13] Add aggregate failures

---
 spec/controllers/admin/users_controller_spec.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb
index 6dc5c38cb7638f..8118db89fff6a8 100644
--- a/spec/controllers/admin/users_controller_spec.rb
+++ b/spec/controllers/admin/users_controller_spec.rb
@@ -372,7 +372,7 @@
       context 'when unsuccessful' do
         let(:user) { create(:user, :blocked) }
 
-        it 'does not ban user' do
+        it 'does not ban user', :aggregate_failures do
           put :ban, params: { id: user.username }
 
           user.reload
@@ -387,7 +387,7 @@
         stub_feature_flags(ban_user_feature_flag: false)
       end
 
-      it 'does not ban user, renders 404' do
+      it 'does not ban user, renders 404', :aggregate_failures do
         put :ban, params: { id: user.username }
 
         user.reload
@@ -400,7 +400,7 @@
   describe 'PUT unban/:id' do
     let(:banned_user) { create(:user, :banned) }
 
-    it 'unbans user' do
+    it 'unbans user', :aggregate_failures do
       put :unban, params: { id: banned_user.username }
 
       banned_user.reload
-- 
GitLab


From 65c956fa43f0acd98132da92d9dce9860d4293e5 Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Tue, 13 Jul 2021 20:49:05 -0500
Subject: [PATCH 04/13] Enable admin mode

---
 spec/controllers/admin/users_controller_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb
index 8118db89fff6a8..fc1fd214b2fbf8 100644
--- a/spec/controllers/admin/users_controller_spec.rb
+++ b/spec/controllers/admin/users_controller_spec.rb
@@ -397,7 +397,7 @@
     end
   end
 
-  describe 'PUT unban/:id' do
+  describe 'PUT unban/:id', :enable_admin_mode do
     let(:banned_user) { create(:user, :banned) }
 
     it 'unbans user', :aggregate_failures do
-- 
GitLab


From 3b3da5a4d5921137a0de291f9bb1990b83524a46 Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Thu, 15 Jul 2021 16:34:29 -0500
Subject: [PATCH 05/13] Update state machine transition

Update ban and unban specs

Remove enable admin mode

Remove unneeded method

Remove unneeded method

Add specs for ban and unban

DRY ban and unban service

Consolidate ban and unban user service
into one file

DRY spec shared example

Create Users BaseService

Need to fix specs

Fix Base Service

Update specs

Use error instead of service error

Change service error to permission error
---
 app/controllers/admin/users_controller.rb     |  4 +-
 app/models/user.rb                            |  9 ++-
 app/services/users/ban_service.rb             | 36 ---------
 app/services/users/ban_user_service.rb        | 19 +++++
 app/services/users/base_service.rb            | 25 +++++++
 app/services/users/unban_service.rb           | 38 ----------
 app/services/users/unban_user_service.rb      | 19 +++++
 .../20210713211008_create_banned_users.rb     | 18 ++++-
 locale/gitlab.pot                             |  7 +-
 .../admin/users_controller_spec.rb            | 19 ++---
 spec/models/user_spec.rb                      | 36 +++++++++
 spec/models/users/banned_user_spec.rb         |  9 ---
 spec/services/users/ban_service_spec.rb       | 62 ----------------
 spec/services/users/ban_user_service_spec.rb  | 70 ++++++++++++++++++
 spec/services/users/base_service_spec.rb      | 14 ++++
 spec/services/users/unban_service_spec.rb     | 66 -----------------
 .../services/users/unban_user_service_spec.rb | 74 +++++++++++++++++++
 17 files changed, 291 insertions(+), 234 deletions(-)
 delete mode 100644 app/services/users/ban_service.rb
 create mode 100644 app/services/users/ban_user_service.rb
 create mode 100644 app/services/users/base_service.rb
 delete mode 100644 app/services/users/unban_service.rb
 create mode 100644 app/services/users/unban_user_service.rb
 delete mode 100644 spec/services/users/ban_service_spec.rb
 create mode 100644 spec/services/users/ban_user_service_spec.rb
 create mode 100644 spec/services/users/base_service_spec.rb
 delete mode 100644 spec/services/users/unban_service_spec.rb
 create mode 100644 spec/services/users/unban_user_service_spec.rb

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index cc6b537870bef3..cd0b9093ee9177 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -126,7 +126,7 @@ def unblock
   end
 
   def ban
-    result = Users::BanService.new(current_user).execute(user)
+    result = Users::BanUserService.new(current_user).execute(user)
 
     if result[:status] == :success
       redirect_back_or_admin_user(notice: _("Successfully banned"))
@@ -136,7 +136,7 @@ def ban
   end
 
   def unban
-    result = Users::UnbanService.new(current_user).execute(user)
+    result = Users::UnbanUserService.new(current_user).execute(user)
 
     if result[:status] == :success
       redirect_back_or_admin_user(notice: _("Successfully unbanned"))
diff --git a/app/models/user.rb b/app/models/user.rb
index 1532c6d65d39d7..928c63bf25e3b8 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -327,7 +327,6 @@ def update_tracked_fields!(request)
       transition deactivated: :blocked
       transition ldap_blocked: :blocked
       transition blocked_pending_approval: :blocked
-      transition banned: :blocked
     end
 
     event :ldap_block do
@@ -381,6 +380,14 @@ def blocked?
       NotificationService.new.user_deactivated(user.name, user.notification_email)
     end
     # rubocop: enable CodeReuse/ServiceClass
+
+    after_transition active: :banned do |user|
+      user.create_banned_user
+    end
+
+    after_transition banned: :active do |user|
+      user.banned_user&.destroy
+    end
   end
 
   # Scopes
diff --git a/app/services/users/ban_service.rb b/app/services/users/ban_service.rb
deleted file mode 100644
index 7724e95dc71ec5..00000000000000
--- a/app/services/users/ban_service.rb
+++ /dev/null
@@ -1,36 +0,0 @@
-# frozen_string_literal: true
-
-module Users
-  class BanService < BaseService
-    def initialize(current_user)
-      @current_user = current_user
-    end
-
-    def execute(user)
-      return error(_('You are not allowed to ban a user'), :forbidden) unless allowed?
-
-      if user.ban
-        create_banned_user(user)
-        log_event(user)
-        success
-      else
-        messages = user.errors.full_messages
-        error(messages.uniq.join('. '))
-      end
-    end
-
-    private
-
-    def allowed?
-      can?(current_user, :admin_all_resources)
-    end
-
-    def create_banned_user(user)
-      Users::BannedUser.create(user: user)
-    end
-
-    def log_event(user)
-      Gitlab::AppLogger.info(message: "User banned", user: "#{user.username}", email: "#{user.email}", banned_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
-    end
-  end
-end
diff --git a/app/services/users/ban_user_service.rb b/app/services/users/ban_user_service.rb
new file mode 100644
index 00000000000000..43c8a1300a7edb
--- /dev/null
+++ b/app/services/users/ban_user_service.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module Users
+  class BanUserService < Users::BaseService
+    ACTION = :ban
+
+    def execute(user)
+      return permission_error(ACTION) unless allowed?
+
+      if user.ban
+        log_event(user, ACTION)
+        success(user: user)
+      else
+        messages = user.errors.full_messages
+        error(messages.uniq.join('. '), :forbidden)
+      end
+    end
+  end
+end
diff --git a/app/services/users/base_service.rb b/app/services/users/base_service.rb
new file mode 100644
index 00000000000000..6b628b8f1f3cfd
--- /dev/null
+++ b/app/services/users/base_service.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+module Users
+  class BaseService
+    include Services::ReturnServiceResponses
+
+    def initialize(current_user)
+      @current_user = current_user
+    end
+
+    private
+
+    def permission_error(action)
+      error(_("You are not allowed to %{action} a user" % { action: action.to_s }), :forbidden)
+    end
+
+    def allowed?
+      @current_user.can?(:admin_all_resources)
+    end
+
+    def log_event(user, action)
+      Gitlab::AppLogger.info(message: "User #{action}", user: "#{user.username}", email: "#{user.email}", "#{action}_by": "#{@current_user.username}", ip_address: "#{@current_user.current_sign_in_ip}")
+    end
+  end
+end
diff --git a/app/services/users/unban_service.rb b/app/services/users/unban_service.rb
deleted file mode 100644
index 00af867d8a0bf7..00000000000000
--- a/app/services/users/unban_service.rb
+++ /dev/null
@@ -1,38 +0,0 @@
-# frozen_string_literal: true
-
-module Users
-  class UnbanService < BaseService
-    def initialize(current_user)
-      @current_user = current_user
-    end
-
-    def execute(user)
-      return error(_('You are not allowed to unban a user'), :forbidden) unless allowed?
-
-      if user.activate
-        destroy_banned_user(user)
-        log_event(user)
-        success
-      else
-        messages = user.errors.full_messages
-        error(messages.uniq.join('. '))
-      end
-    end
-
-    private
-
-    def allowed?
-      can?(current_user, :admin_all_resources)
-    end
-
-    # rubocop: disable CodeReuse/ActiveRecord
-    def destroy_banned_user(user)
-      Users::BannedUser.find_by(user: user).destroy
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-
-    def log_event(user)
-      Gitlab::AppLogger.info(message: "User unbanned", user: "#{user.username}", email: "#{user.email}", banned_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
-    end
-  end
-end
diff --git a/app/services/users/unban_user_service.rb b/app/services/users/unban_user_service.rb
new file mode 100644
index 00000000000000..61538b07f20725
--- /dev/null
+++ b/app/services/users/unban_user_service.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module Users
+  class UnbanUserService < Users::BaseService
+    ACTION = :unban
+
+    def execute(user)
+      return permission_error(ACTION) unless allowed?
+
+      if user.activate
+        log_event(user, ACTION)
+        success(user: user)
+      else
+        messages = user.errors.full_messages
+        error(messages.uniq.join('. '), :forbidden)
+      end
+    end
+  end
+end
diff --git a/db/migrate/20210713211008_create_banned_users.rb b/db/migrate/20210713211008_create_banned_users.rb
index dd16dfc31307fa..7e5eb7f95b84f5 100644
--- a/db/migrate/20210713211008_create_banned_users.rb
+++ b/db/migrate/20210713211008_create_banned_users.rb
@@ -1,10 +1,20 @@
 # frozen_string_literal: true
 
 class CreateBannedUsers < ActiveRecord::Migration[6.1]
-  def change
-    create_table :banned_users, id: false do |t|
-      t.timestamps_with_timezone null: false
-      t.references :user, primary_key: true, default: nil, foreign_key: { on_delete: :cascade }, type: :bigint, index: false, null: false
+  include Gitlab::Database::MigrationHelpers
+
+  def up
+    with_lock_retries do
+      create_table :banned_users, id: false do |t|
+        t.timestamps_with_timezone null: false
+        t.references :user, primary_key: true, default: nil, foreign_key: { on_delete: :cascade }, type: :bigint, index: false, null: false
+      end
+    end
+  end
+
+  def down
+    with_lock_retries do
+      drop_table :banned_users
     end
   end
 end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index dfcfe2357b07e3..78cfc9bc07c4a7 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -37305,10 +37305,10 @@ msgstr ""
 msgid "You are going to turn on confidentiality. Only team members with %{strongStart}at least Reporter access%{strongEnd} will be able to see and leave comments on the %{issuableType}."
 msgstr ""
 
-msgid "You are not allowed to approve a user"
+msgid "You are not allowed to %{action} a user"
 msgstr ""
 
-msgid "You are not allowed to ban a user"
+msgid "You are not allowed to approve a user"
 msgstr ""
 
 msgid "You are not allowed to log in using password"
@@ -37320,9 +37320,6 @@ msgstr ""
 msgid "You are not allowed to reject a user"
 msgstr ""
 
-msgid "You are not allowed to unban a user"
-msgstr ""
-
 msgid "You are not allowed to unlink your primary login account"
 msgstr ""
 
diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb
index fc1fd214b2fbf8..be21fffb2966b4 100644
--- a/spec/controllers/admin/users_controller_spec.rb
+++ b/spec/controllers/admin/users_controller_spec.rb
@@ -359,20 +359,19 @@
     end
   end
 
-  describe 'PUT ban/:id' do
+  describe 'PUT ban/:id', :aggregate_failures do
     context 'when ban_user_feature_flag is enabled' do
       it 'bans user' do
         put :ban, params: { id: user.username }
 
-        user.reload
-        expect(user.banned?).to be_truthy
+        expect(user.reload.banned?).to be_truthy
         expect(flash[:notice]).to eq _('Successfully banned')
       end
 
       context 'when unsuccessful' do
         let(:user) { create(:user, :blocked) }
 
-        it 'does not ban user', :aggregate_failures do
+        it 'does not ban user' do
           put :ban, params: { id: user.username }
 
           user.reload
@@ -387,24 +386,22 @@
         stub_feature_flags(ban_user_feature_flag: false)
       end
 
-      it 'does not ban user, renders 404', :aggregate_failures do
+      it 'does not ban user, renders 404' do
         put :ban, params: { id: user.username }
 
-        user.reload
-        expect(user.banned?).to be_falsey
+        expect(user.reload.banned?).to be_falsey
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
   end
 
-  describe 'PUT unban/:id', :enable_admin_mode do
+  describe 'PUT unban/:id', :aggregate_failures do
     let(:banned_user) { create(:user, :banned) }
 
-    it 'unbans user', :aggregate_failures do
+    it 'unbans user' do
       put :unban, params: { id: banned_user.username }
 
-      banned_user.reload
-      expect(banned_user.banned?).to be_falsey
+      expect(banned_user.reload.banned?).to be_falsey
       expect(flash[:notice]).to eq _('Successfully unbanned')
     end
   end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index c9b0c0f0d8ad76..9b89fe67bf5f08 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1951,6 +1951,42 @@
     end
   end
 
+  describe 'banning and unbanning a user', :aggregate_failures do
+    let(:user) { create(:user) }
+
+    context 'an active user' do
+      it 'bans and blocks the user' do
+        user.ban
+
+        expect(user.banned?).to eq(true)
+        expect(user.blocked?).to eq(true)
+      end
+
+      it 'creates a BannedUser record' do
+        expect { user.ban }.to change { Users::BannedUser.count }.by(1)
+        expect(Users::BannedUser.last.user_id).to eq(user.id)
+      end
+    end
+
+    context 'unbanning a user' do
+      before do
+        user.ban!
+      end
+
+      it 'activates the user' do
+        user.activate
+
+        expect(user.banned?).to eq(false)
+        expect(user.blocked?).to eq(false)
+        expect(user.active?).to eq(true)
+      end
+
+      it 'deletes the BannedUser record' do
+        expect { user.activate }.to change { Users::BannedUser.count }.by(-1)
+      end
+    end
+  end
+
   describe '.filter_items' do
     let(:user) { double }
 
diff --git a/spec/models/users/banned_user_spec.rb b/spec/models/users/banned_user_spec.rb
index dda8d82c7982a2..135f0a4d0490d4 100644
--- a/spec/models/users/banned_user_spec.rb
+++ b/spec/models/users/banned_user_spec.rb
@@ -9,13 +9,4 @@
     subject.user = create(:user)
     is_expected.to validate_uniqueness_of(:user_id).with_message("banned user already exists")
   end
-
-  context 'creates a new banned user' do
-    let(:user) { create(:user) }
-
-    it 'creates a new banned user object with is_banned ban state', :aggregate_failures do
-      expect { described_class.new(user: user).save! }.to change { described_class.count }.by(1)
-      expect(described_class.last.user_id).to eq(user.id)
-    end
-  end
 end
diff --git a/spec/services/users/ban_service_spec.rb b/spec/services/users/ban_service_spec.rb
deleted file mode 100644
index 2ead6fb378ea64..00000000000000
--- a/spec/services/users/ban_service_spec.rb
+++ /dev/null
@@ -1,62 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Users::BanService do
-  let(:current_user) { create(:admin) }
-  let(:user) { create(:user) }
-
-  subject(:service) { described_class.new(current_user) }
-
-  describe '#execute' do
-    subject(:operation) { service.execute(user) }
-
-    context 'when successful', :enable_admin_mode do
-      it { is_expected.to eq(status: :success) }
-
-      it 'bans the user and creates a BannedUser', :aggregate_failures do
-        expect { operation }.to change { Users::BannedUser.count }.by(1)
-        expect(user.state).to eq('banned')
-        expect(user.blocked?).to be_truthy
-      end
-
-      it 'logs ban in application logs' do
-        allow(Gitlab::AppLogger).to receive(:info)
-
-        operation
-
-        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User banned", user: "#{user.username}", email: "#{user.email}", banned_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
-      end
-    end
-
-    context 'when failed' do
-      context 'when user is already blocked', :enable_admin_mode do
-        before do
-          user.block!
-        end
-
-        it 'error result' do
-          expect(operation[:status]).to eq(:error)
-          expect(operation[:message]).to match(/State cannot transition/)
-        end
-
-        it 'does not ban the user', :aggregate_failures do
-          expect { operation }.not_to change { Users::BannedUser.count }
-          expect(user.state).to eq('blocked')
-        end
-      end
-
-      context 'when user is not an admin' do
-        it 'error result' do
-          expect(operation[:status]).to eq(:error)
-          expect(operation[:message]).to match(/You are not allowed to ban a user/)
-        end
-
-        it 'does not ban the user', :aggregate_failures do
-          expect { operation }.not_to change { Users::BannedUser.count }
-          expect(user.state).to eq('active')
-        end
-      end
-    end
-  end
-end
diff --git a/spec/services/users/ban_user_service_spec.rb b/spec/services/users/ban_user_service_spec.rb
new file mode 100644
index 00000000000000..006490431d6f37
--- /dev/null
+++ b/spec/services/users/ban_user_service_spec.rb
@@ -0,0 +1,70 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::BanUserService do
+  let(:current_user) { create(:admin) }
+  let(:user) { create(:user) }
+
+  subject(:service) { described_class.new(current_user) }
+
+  shared_examples 'does not modify the BannedUser record or user state' do
+    it 'does not modify the BannedUser record or user state' do
+      expect { operation }.not_to change { Users::BannedUser.count }
+      expect { operation }.not_to change { user.state }
+    end
+  end
+
+  context 'ban', :aggregate_failures do
+    subject(:operation) { service.execute(user) }
+
+    context 'when successful', :enable_admin_mode do
+      it 'returns success status' do
+        response = operation
+
+        expect(response[:status]).to eq(:success)
+      end
+
+      it 'bans the user' do
+        expect { operation }.to change { user.state }.from('active').to('banned')
+      end
+
+      it 'creates a BannedUser' do
+        expect { operation }.to change { Users::BannedUser.count }.by(1)
+        expect(Users::BannedUser.last.user_id).to eq(user.id)
+      end
+
+      it 'logs ban in application logs' do
+        allow(Gitlab::AppLogger).to receive(:info)
+
+        operation
+
+        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User ban", user: "#{user.username}", email: "#{user.email}", ban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
+      end
+    end
+
+    context 'when failed' do
+      context 'when user is blocked', :enable_admin_mode do
+        before do
+          user.block!
+        end
+
+        it 'returns state error message' do
+          expect(operation[:status]).to eq(:error)
+          expect(operation[:message]).to match(/State cannot transition/)
+        end
+
+        it_behaves_like 'does not modify the BannedUser record or user state'
+      end
+
+      context 'when user is not an admin' do
+        it 'returns permissions error message' do
+          expect(operation[:status]).to eq(:error)
+          expect(operation[:message]).to match(/You are not allowed to ban a user/)
+        end
+
+        it_behaves_like 'does not modify the BannedUser record or user state'
+      end
+    end
+  end
+end
diff --git a/spec/services/users/base_service_spec.rb b/spec/services/users/base_service_spec.rb
new file mode 100644
index 00000000000000..12d352b94ce85c
--- /dev/null
+++ b/spec/services/users/base_service_spec.rb
@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::BaseService do
+  let(:admin) { create(:admin) }
+  let(:base_service) { described_class.new(admin) }
+
+  describe '#initialize' do
+    it 'sets the current_user instance value' do
+      expect(base_service.instance_values["current_user"]).to eq(admin)
+    end
+  end
+end
diff --git a/spec/services/users/unban_service_spec.rb b/spec/services/users/unban_service_spec.rb
deleted file mode 100644
index eb93763ad9221a..00000000000000
--- a/spec/services/users/unban_service_spec.rb
+++ /dev/null
@@ -1,66 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Users::UnbanService do
-  let(:current_user) { create(:admin) }
-  let(:user) { create(:user) }
-
-  subject(:service) { described_class.new(current_user) }
-
-  describe '#execute' do
-    subject(:operation) { service.execute(user) }
-
-    context 'when successful', :enable_admin_mode do
-      before do
-        Users::BanService.new(current_user).execute(user)
-      end
-
-      it { is_expected.to eq(status: :success) }
-
-      it 'unbans the user', :aggregate_failures do
-        expect { operation }.to change {user.state}.from('banned').to('active')
-      end
-
-      it 'removes the BannedUser object' do
-        expect { operation }.to change { Users::BannedUser.count }.by(-1)
-      end
-
-      it 'logs ban in application logs' do
-        allow(Gitlab::AppLogger).to receive(:info)
-
-        operation
-
-        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User unbanned", user: "#{user.username}", email: "#{user.email}", banned_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
-      end
-    end
-
-    context 'when failed' do
-      context 'when user is not already banned', :enable_admin_mode do
-        it 'error result' do
-          expect(operation[:status]).to eq(:error)
-          expect(operation[:message]).to match(/State cannot transition/)
-        end
-
-        it 'does not change the user state', :aggregate_failures do
-          expect { operation }.not_to change { user.state }
-        end
-      end
-
-      context 'when user is not an admin' do
-        before do
-          Users::BanService.new(current_user).execute(user)
-        end
-
-        it 'error result' do
-          expect(operation[:status]).to eq(:error)
-          expect(operation[:message]).to match(/You are not allowed to unban a user/)
-        end
-
-        it 'does not change the user state', :aggregate_failures do
-          expect { operation }.not_to change { user.state }
-        end
-      end
-    end
-  end
-end
diff --git a/spec/services/users/unban_user_service_spec.rb b/spec/services/users/unban_user_service_spec.rb
new file mode 100644
index 00000000000000..4ae45642ae421a
--- /dev/null
+++ b/spec/services/users/unban_user_service_spec.rb
@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::UnbanUserService do
+  let(:current_user) { create(:admin) }
+  let(:user) { create(:user) }
+
+  subject(:service) { described_class.new(current_user) }
+
+  shared_examples 'does not modify the BannedUser record or user state' do
+    it 'does not modify the BannedUser record or user state' do
+      expect { operation }.not_to change { Users::BannedUser.count }
+      expect { operation }.not_to change { user.state }
+    end
+  end
+
+  context 'unban', :aggregate_failures do
+    subject(:operation) { service.execute(user) }
+
+    context 'when successful', :enable_admin_mode do
+      before do
+        user.ban!
+      end
+
+      it 'returns success status' do
+        response = operation
+
+        expect(response[:status]).to eq(:success)
+      end
+
+      it 'unbans the user' do
+        expect { operation }.to change { user.state }.from('banned').to('active')
+      end
+
+      it 'removes the BannedUser' do
+        expect { operation }.to change { Users::BannedUser.count }.by(-1)
+        expect(user.reload.banned_user).to be_nil
+      end
+
+      it 'logs unban in application logs' do
+        allow(Gitlab::AppLogger).to receive(:info)
+
+        operation
+
+        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User unban", user: "#{user.username}", email: "#{user.email}", unban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
+      end
+    end
+
+    context 'when failed' do
+      context 'when user is already active', :enable_admin_mode do
+        it 'returns state error message' do
+          expect(operation[:status]).to eq(:error)
+          expect(operation[:message]).to match(/State cannot transition/)
+        end
+
+        it_behaves_like 'does not modify the BannedUser record or user state'
+      end
+
+      context 'when user is not an admin' do
+        before do
+          user.ban!
+        end
+
+        it 'returns permissions error message' do
+          expect(operation[:status]).to eq(:error)
+          expect(operation[:message]).to match(/You are not allowed to unban a user/)
+        end
+
+        it_behaves_like 'does not modify the BannedUser record or user state'
+      end
+    end
+  end
+end
-- 
GitLab


From 61c34c4e8bbc10fbefcb067dc4fe407d318e411f Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Thu, 22 Jul 2021 14:38:40 -0500
Subject: [PATCH 06/13] Rename Users BaseService

DRY ban and unban service
---
 app/controllers/admin/users_controller.rb     |   4 +-
 app/services/users/ban_user_service.rb        |  19 ---
 .../users/banned_user_base_service.rb         |  34 +++++
 app/services/users/banned_user_service.rb     |  29 ++++
 app/services/users/base_service.rb            |  25 ----
 app/services/users/unban_user_service.rb      |  19 ---
 spec/services/users/ban_user_service_spec.rb  |  70 ---------
 ...ec.rb => banned_user_base_service_spec.rb} |   2 +-
 .../users/banned_user_service_spec.rb         | 140 ++++++++++++++++++
 .../services/users/unban_user_service_spec.rb |  74 ---------
 10 files changed, 206 insertions(+), 210 deletions(-)
 delete mode 100644 app/services/users/ban_user_service.rb
 create mode 100644 app/services/users/banned_user_base_service.rb
 create mode 100644 app/services/users/banned_user_service.rb
 delete mode 100644 app/services/users/base_service.rb
 delete mode 100644 app/services/users/unban_user_service.rb
 delete mode 100644 spec/services/users/ban_user_service_spec.rb
 rename spec/services/users/{base_service_spec.rb => banned_user_base_service_spec.rb} (86%)
 create mode 100644 spec/services/users/banned_user_service_spec.rb
 delete mode 100644 spec/services/users/unban_user_service_spec.rb

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index cd0b9093ee9177..bd37c362a4f2b7 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -126,7 +126,7 @@ def unblock
   end
 
   def ban
-    result = Users::BanUserService.new(current_user).execute(user)
+    result = Users::BannedUserService.new(current_user).execute(user, :ban)
 
     if result[:status] == :success
       redirect_back_or_admin_user(notice: _("Successfully banned"))
@@ -136,7 +136,7 @@ def ban
   end
 
   def unban
-    result = Users::UnbanUserService.new(current_user).execute(user)
+    result = Users::BannedUserService.new(current_user).execute(user, :unban)
 
     if result[:status] == :success
       redirect_back_or_admin_user(notice: _("Successfully unbanned"))
diff --git a/app/services/users/ban_user_service.rb b/app/services/users/ban_user_service.rb
deleted file mode 100644
index 43c8a1300a7edb..00000000000000
--- a/app/services/users/ban_user_service.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-# frozen_string_literal: true
-
-module Users
-  class BanUserService < Users::BaseService
-    ACTION = :ban
-
-    def execute(user)
-      return permission_error(ACTION) unless allowed?
-
-      if user.ban
-        log_event(user, ACTION)
-        success(user: user)
-      else
-        messages = user.errors.full_messages
-        error(messages.uniq.join('. '), :forbidden)
-      end
-    end
-  end
-end
diff --git a/app/services/users/banned_user_base_service.rb b/app/services/users/banned_user_base_service.rb
new file mode 100644
index 00000000000000..9025924ea87961
--- /dev/null
+++ b/app/services/users/banned_user_base_service.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+module Users
+  class BannedUserBaseService < BaseService
+
+    def initialize(current_user)
+      @current_user = current_user
+    end
+
+    private
+
+    attr_reader :current_user
+
+    def permission_error(action)
+      error(_("You are not allowed to %{action} a user" % { action: action.to_s }), :forbidden)
+    end
+
+    def invalid_action_error
+      error(_('Invalid action'), :forbidden)
+    end
+
+    def allowed?
+      can?(current_user, :admin_all_resources)
+    end
+
+    def valid_action?(action)
+      action == :ban || action == :unban
+    end
+
+    def log_event(user, action)
+      Gitlab::AppLogger.info(message: "User #{action}", user: "#{user.username}", email: "#{user.email}", "#{action}_by": "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
+    end
+  end
+end
diff --git a/app/services/users/banned_user_service.rb b/app/services/users/banned_user_service.rb
new file mode 100644
index 00000000000000..9bf00b7688092c
--- /dev/null
+++ b/app/services/users/banned_user_service.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+module Users
+  class BannedUserService < BannedUserBaseService
+   # ACTION = :ban
+
+    def execute(user, action)
+      return permission_error(action) unless allowed?
+      return invalid_action_error unless valid_action?(action)
+
+      if update_user(user, action)
+        log_event(user, action)
+        success
+      else
+        messages = user.errors.full_messages
+        error(messages.uniq.join('. '))
+      end
+    end
+
+    def update_user(user, action)
+      case action
+      when :ban
+        user.ban
+      when :unban
+        user.activate
+      end
+    end
+  end
+end
diff --git a/app/services/users/base_service.rb b/app/services/users/base_service.rb
deleted file mode 100644
index 6b628b8f1f3cfd..00000000000000
--- a/app/services/users/base_service.rb
+++ /dev/null
@@ -1,25 +0,0 @@
-# frozen_string_literal: true
-
-module Users
-  class BaseService
-    include Services::ReturnServiceResponses
-
-    def initialize(current_user)
-      @current_user = current_user
-    end
-
-    private
-
-    def permission_error(action)
-      error(_("You are not allowed to %{action} a user" % { action: action.to_s }), :forbidden)
-    end
-
-    def allowed?
-      @current_user.can?(:admin_all_resources)
-    end
-
-    def log_event(user, action)
-      Gitlab::AppLogger.info(message: "User #{action}", user: "#{user.username}", email: "#{user.email}", "#{action}_by": "#{@current_user.username}", ip_address: "#{@current_user.current_sign_in_ip}")
-    end
-  end
-end
diff --git a/app/services/users/unban_user_service.rb b/app/services/users/unban_user_service.rb
deleted file mode 100644
index 61538b07f20725..00000000000000
--- a/app/services/users/unban_user_service.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-# frozen_string_literal: true
-
-module Users
-  class UnbanUserService < Users::BaseService
-    ACTION = :unban
-
-    def execute(user)
-      return permission_error(ACTION) unless allowed?
-
-      if user.activate
-        log_event(user, ACTION)
-        success(user: user)
-      else
-        messages = user.errors.full_messages
-        error(messages.uniq.join('. '), :forbidden)
-      end
-    end
-  end
-end
diff --git a/spec/services/users/ban_user_service_spec.rb b/spec/services/users/ban_user_service_spec.rb
deleted file mode 100644
index 006490431d6f37..00000000000000
--- a/spec/services/users/ban_user_service_spec.rb
+++ /dev/null
@@ -1,70 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Users::BanUserService do
-  let(:current_user) { create(:admin) }
-  let(:user) { create(:user) }
-
-  subject(:service) { described_class.new(current_user) }
-
-  shared_examples 'does not modify the BannedUser record or user state' do
-    it 'does not modify the BannedUser record or user state' do
-      expect { operation }.not_to change { Users::BannedUser.count }
-      expect { operation }.not_to change { user.state }
-    end
-  end
-
-  context 'ban', :aggregate_failures do
-    subject(:operation) { service.execute(user) }
-
-    context 'when successful', :enable_admin_mode do
-      it 'returns success status' do
-        response = operation
-
-        expect(response[:status]).to eq(:success)
-      end
-
-      it 'bans the user' do
-        expect { operation }.to change { user.state }.from('active').to('banned')
-      end
-
-      it 'creates a BannedUser' do
-        expect { operation }.to change { Users::BannedUser.count }.by(1)
-        expect(Users::BannedUser.last.user_id).to eq(user.id)
-      end
-
-      it 'logs ban in application logs' do
-        allow(Gitlab::AppLogger).to receive(:info)
-
-        operation
-
-        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User ban", user: "#{user.username}", email: "#{user.email}", ban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
-      end
-    end
-
-    context 'when failed' do
-      context 'when user is blocked', :enable_admin_mode do
-        before do
-          user.block!
-        end
-
-        it 'returns state error message' do
-          expect(operation[:status]).to eq(:error)
-          expect(operation[:message]).to match(/State cannot transition/)
-        end
-
-        it_behaves_like 'does not modify the BannedUser record or user state'
-      end
-
-      context 'when user is not an admin' do
-        it 'returns permissions error message' do
-          expect(operation[:status]).to eq(:error)
-          expect(operation[:message]).to match(/You are not allowed to ban a user/)
-        end
-
-        it_behaves_like 'does not modify the BannedUser record or user state'
-      end
-    end
-  end
-end
diff --git a/spec/services/users/base_service_spec.rb b/spec/services/users/banned_user_base_service_spec.rb
similarity index 86%
rename from spec/services/users/base_service_spec.rb
rename to spec/services/users/banned_user_base_service_spec.rb
index 12d352b94ce85c..29a549f0f49923 100644
--- a/spec/services/users/base_service_spec.rb
+++ b/spec/services/users/banned_user_base_service_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Users::BaseService do
+RSpec.describe Users::BannedUserBaseService do
   let(:admin) { create(:admin) }
   let(:base_service) { described_class.new(admin) }
 
diff --git a/spec/services/users/banned_user_service_spec.rb b/spec/services/users/banned_user_service_spec.rb
new file mode 100644
index 00000000000000..f71c1d20338b8b
--- /dev/null
+++ b/spec/services/users/banned_user_service_spec.rb
@@ -0,0 +1,140 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::BannedUserService do
+  let(:current_user) { create(:admin) }
+  let(:user) { create(:user) }
+
+  subject(:service) { described_class.new(current_user) }
+
+  shared_examples 'user is in the wrong state' do
+    it 'returns state error message' do
+      expect(operation[:status]).to eq(:error)
+      expect(operation[:message]).to match(/State cannot transition/)
+    end
+  end
+
+  shared_examples 'does not modify the BannedUser record or user state' do
+    it 'does not modify the BannedUser record or user state' do
+      expect { operation }.not_to change { Users::BannedUser.count }
+      expect { operation }.not_to change { user.state }
+    end
+  end
+
+  context 'ban', :aggregate_failures do
+    subject(:operation) { service.execute(user, :ban) }
+
+    context 'when successful', :enable_admin_mode do
+      it 'returns success status' do
+        response = operation
+
+        expect(response[:status]).to eq(:success)
+      end
+
+      it 'bans the user' do
+        expect { operation }.to change { user.state }.from('active').to('banned')
+      end
+
+      it 'creates a BannedUser' do
+        expect { operation }.to change { Users::BannedUser.count }.by(1)
+        expect(Users::BannedUser.last.user_id).to eq(user.id)
+      end
+
+      it 'logs ban in application logs' do
+        allow(Gitlab::AppLogger).to receive(:info)
+
+        operation
+
+        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User ban", user: "#{user.username}", email: "#{user.email}", ban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
+      end
+    end
+
+    context 'when failed' do
+      context 'when user is blocked', :enable_admin_mode do
+        before do
+          user.block!
+        end
+
+        it_behaves_like 'user is in the wrong state'
+
+        it_behaves_like 'does not modify the BannedUser record or user state'
+      end
+
+      context 'when user is not an admin' do
+        it 'returns permissions error message' do
+          expect(operation[:status]).to eq(:error)
+          expect(operation[:message]).to match(/You are not allowed to ban a user/)
+        end
+
+        it_behaves_like 'does not modify the BannedUser record or user state'
+      end
+    end
+  end
+
+  context 'unban', :aggregate_failures do
+    subject(:operation) { service.execute(user, :unban) }
+
+    context 'when successful', :enable_admin_mode do
+      before do
+        user.ban!
+      end
+
+      it 'returns success status' do
+        response = operation
+
+        expect(response[:status]).to eq(:success)
+      end
+
+      it 'unbans the user' do
+        expect { operation }.to change { user.state }.from('banned').to('active')
+      end
+
+      it 'removes the BannedUser' do
+        expect { operation }.to change { Users::BannedUser.count }.by(-1)
+        expect(user.reload.banned_user).to be_nil
+      end
+
+      it 'logs unban in application logs' do
+        allow(Gitlab::AppLogger).to receive(:info)
+
+        operation
+
+        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User unban", user: "#{user.username}", email: "#{user.email}", unban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
+      end
+    end
+
+    context 'when failed' do
+      context 'when user is already active', :enable_admin_mode do
+        it_behaves_like 'user is in the wrong state'
+
+
+        it_behaves_like 'does not modify the BannedUser record or user state'
+      end
+
+      context 'when user is not an admin' do
+        before do
+          user.ban!
+        end
+
+        it 'returns permissions error message' do
+          expect(operation[:status]).to eq(:error)
+          expect(operation[:message]).to match(/You are not allowed to unban a user/)
+        end
+
+        it_behaves_like 'does not modify the BannedUser record or user state'
+      end
+    end
+  end
+
+  context 'invalid action', :enable_admin_mode do
+    subject(:operation) { service.execute(user, :invalid) }
+
+    it 'returns invalid action error message' do
+      expect(operation[:status]).to eq(:error)
+      expect(operation[:message]).to match(/Invalid action/)
+    end
+
+    it_behaves_like 'does not modify the BannedUser record or user state'
+  end
+end
diff --git a/spec/services/users/unban_user_service_spec.rb b/spec/services/users/unban_user_service_spec.rb
deleted file mode 100644
index 4ae45642ae421a..00000000000000
--- a/spec/services/users/unban_user_service_spec.rb
+++ /dev/null
@@ -1,74 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Users::UnbanUserService do
-  let(:current_user) { create(:admin) }
-  let(:user) { create(:user) }
-
-  subject(:service) { described_class.new(current_user) }
-
-  shared_examples 'does not modify the BannedUser record or user state' do
-    it 'does not modify the BannedUser record or user state' do
-      expect { operation }.not_to change { Users::BannedUser.count }
-      expect { operation }.not_to change { user.state }
-    end
-  end
-
-  context 'unban', :aggregate_failures do
-    subject(:operation) { service.execute(user) }
-
-    context 'when successful', :enable_admin_mode do
-      before do
-        user.ban!
-      end
-
-      it 'returns success status' do
-        response = operation
-
-        expect(response[:status]).to eq(:success)
-      end
-
-      it 'unbans the user' do
-        expect { operation }.to change { user.state }.from('banned').to('active')
-      end
-
-      it 'removes the BannedUser' do
-        expect { operation }.to change { Users::BannedUser.count }.by(-1)
-        expect(user.reload.banned_user).to be_nil
-      end
-
-      it 'logs unban in application logs' do
-        allow(Gitlab::AppLogger).to receive(:info)
-
-        operation
-
-        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User unban", user: "#{user.username}", email: "#{user.email}", unban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
-      end
-    end
-
-    context 'when failed' do
-      context 'when user is already active', :enable_admin_mode do
-        it 'returns state error message' do
-          expect(operation[:status]).to eq(:error)
-          expect(operation[:message]).to match(/State cannot transition/)
-        end
-
-        it_behaves_like 'does not modify the BannedUser record or user state'
-      end
-
-      context 'when user is not an admin' do
-        before do
-          user.ban!
-        end
-
-        it 'returns permissions error message' do
-          expect(operation[:status]).to eq(:error)
-          expect(operation[:message]).to match(/You are not allowed to unban a user/)
-        end
-
-        it_behaves_like 'does not modify the BannedUser record or user state'
-      end
-    end
-  end
-end
-- 
GitLab


From 367478b95303fbc1819d711350daa27218407900 Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Thu, 22 Jul 2021 14:47:06 -0500
Subject: [PATCH 07/13] Run rubocop linter

---
 app/services/users/banned_user_base_service.rb  | 1 -
 app/services/users/banned_user_service.rb       | 2 --
 spec/services/users/banned_user_service_spec.rb | 1 -
 3 files changed, 4 deletions(-)

diff --git a/app/services/users/banned_user_base_service.rb b/app/services/users/banned_user_base_service.rb
index 9025924ea87961..ab931ab470072d 100644
--- a/app/services/users/banned_user_base_service.rb
+++ b/app/services/users/banned_user_base_service.rb
@@ -2,7 +2,6 @@
 
 module Users
   class BannedUserBaseService < BaseService
-
     def initialize(current_user)
       @current_user = current_user
     end
diff --git a/app/services/users/banned_user_service.rb b/app/services/users/banned_user_service.rb
index 9bf00b7688092c..4c6710e7ef4cdb 100644
--- a/app/services/users/banned_user_service.rb
+++ b/app/services/users/banned_user_service.rb
@@ -2,8 +2,6 @@
 
 module Users
   class BannedUserService < BannedUserBaseService
-   # ACTION = :ban
-
     def execute(user, action)
       return permission_error(action) unless allowed?
       return invalid_action_error unless valid_action?(action)
diff --git a/spec/services/users/banned_user_service_spec.rb b/spec/services/users/banned_user_service_spec.rb
index f71c1d20338b8b..20fbf62e270254 100644
--- a/spec/services/users/banned_user_service_spec.rb
+++ b/spec/services/users/banned_user_service_spec.rb
@@ -108,7 +108,6 @@
       context 'when user is already active', :enable_admin_mode do
         it_behaves_like 'user is in the wrong state'
 
-
         it_behaves_like 'does not modify the BannedUser record or user state'
       end
 
-- 
GitLab


From 61693764ad61071bc3a16d051d8573424348978e Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Thu, 22 Jul 2021 14:54:47 -0500
Subject: [PATCH 08/13] Run bin rake translate

---
 app/services/users/banned_user_base_service.rb | 16 ++++++++--------
 locale/gitlab.pot                              |  3 +++
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/app/services/users/banned_user_base_service.rb b/app/services/users/banned_user_base_service.rb
index ab931ab470072d..26f9f02b5cc917 100644
--- a/app/services/users/banned_user_base_service.rb
+++ b/app/services/users/banned_user_base_service.rb
@@ -10,14 +10,6 @@ def initialize(current_user)
 
     attr_reader :current_user
 
-    def permission_error(action)
-      error(_("You are not allowed to %{action} a user" % { action: action.to_s }), :forbidden)
-    end
-
-    def invalid_action_error
-      error(_('Invalid action'), :forbidden)
-    end
-
     def allowed?
       can?(current_user, :admin_all_resources)
     end
@@ -26,6 +18,14 @@ def valid_action?(action)
       action == :ban || action == :unban
     end
 
+    def permission_error(action)
+      error(_("You are not allowed to %{action} a user" % { action: action.to_s }), :forbidden)
+    end
+
+    def invalid_action_error
+      error(_('Invalid action'), :forbidden)
+    end
+
     def log_event(user, action)
       Gitlab::AppLogger.info(message: "User #{action}", user: "#{user.username}", email: "#{user.email}", "#{action}_by": "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
     end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 78cfc9bc07c4a7..2c096af6349039 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -17773,6 +17773,9 @@ msgstr ""
 msgid "Invalid URL"
 msgstr ""
 
+msgid "Invalid action"
+msgstr ""
+
 msgid "Invalid container_name"
 msgstr ""
 
-- 
GitLab


From 346cfb94945d55ad60730b684d7e181a3ba6d989 Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Fri, 23 Jul 2021 16:42:37 -0500
Subject: [PATCH 09/13] Ban and unban user service

Move execute to base service

Run rubocop linter

Rename user ban spec

Run translation script
---
 app/controllers/admin/users_controller.rb     |   4 +-
 app/services/users/ban_user_service.rb        |  16 ++
 .../users/banned_user_base_service.rb         |  26 ++--
 app/services/users/banned_user_service.rb     |  27 ----
 app/services/users/unban_user_service.rb      |  16 ++
 locale/gitlab.pot                             |   3 -
 spec/models/user_spec.rb                      |   4 +-
 spec/services/users/ban_user_service_spec.rb  |  74 ++++++++++
 .../users/banned_user_service_spec.rb         | 139 ------------------
 .../services/users/unban_user_service_spec.rb |  78 ++++++++++
 10 files changed, 203 insertions(+), 184 deletions(-)
 create mode 100644 app/services/users/ban_user_service.rb
 delete mode 100644 app/services/users/banned_user_service.rb
 create mode 100644 app/services/users/unban_user_service.rb
 create mode 100644 spec/services/users/ban_user_service_spec.rb
 delete mode 100644 spec/services/users/banned_user_service_spec.rb
 create mode 100644 spec/services/users/unban_user_service_spec.rb

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index bd37c362a4f2b7..cd0b9093ee9177 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -126,7 +126,7 @@ def unblock
   end
 
   def ban
-    result = Users::BannedUserService.new(current_user).execute(user, :ban)
+    result = Users::BanUserService.new(current_user).execute(user)
 
     if result[:status] == :success
       redirect_back_or_admin_user(notice: _("Successfully banned"))
@@ -136,7 +136,7 @@ def ban
   end
 
   def unban
-    result = Users::BannedUserService.new(current_user).execute(user, :unban)
+    result = Users::UnbanUserService.new(current_user).execute(user)
 
     if result[:status] == :success
       redirect_back_or_admin_user(notice: _("Successfully unbanned"))
diff --git a/app/services/users/ban_user_service.rb b/app/services/users/ban_user_service.rb
new file mode 100644
index 00000000000000..e463108d5f3051
--- /dev/null
+++ b/app/services/users/ban_user_service.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module Users
+  class BanUserService < BannedUserBaseService
+    attr_reader :action
+    def initialize(current_user)
+      super
+
+      @action = :ban
+    end
+
+    def update_user(user)
+      user.ban
+    end
+  end
+end
diff --git a/app/services/users/banned_user_base_service.rb b/app/services/users/banned_user_base_service.rb
index 26f9f02b5cc917..2372468151c809 100644
--- a/app/services/users/banned_user_base_service.rb
+++ b/app/services/users/banned_user_base_service.rb
@@ -6,27 +6,31 @@ def initialize(current_user)
       @current_user = current_user
     end
 
+    def execute(user)
+      return permission_error unless allowed?
+
+      if update_user(user)
+        log_event(user)
+        success
+      else
+        messages = user.errors.full_messages
+        error(messages.uniq.join('. '))
+      end
+    end
+
     private
 
-    attr_reader :current_user
+    attr_reader :current_user, :action
 
     def allowed?
       can?(current_user, :admin_all_resources)
     end
 
-    def valid_action?(action)
-      action == :ban || action == :unban
-    end
-
-    def permission_error(action)
+    def permission_error
       error(_("You are not allowed to %{action} a user" % { action: action.to_s }), :forbidden)
     end
 
-    def invalid_action_error
-      error(_('Invalid action'), :forbidden)
-    end
-
-    def log_event(user, action)
+    def log_event(user)
       Gitlab::AppLogger.info(message: "User #{action}", user: "#{user.username}", email: "#{user.email}", "#{action}_by": "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
     end
   end
diff --git a/app/services/users/banned_user_service.rb b/app/services/users/banned_user_service.rb
deleted file mode 100644
index 4c6710e7ef4cdb..00000000000000
--- a/app/services/users/banned_user_service.rb
+++ /dev/null
@@ -1,27 +0,0 @@
-# frozen_string_literal: true
-
-module Users
-  class BannedUserService < BannedUserBaseService
-    def execute(user, action)
-      return permission_error(action) unless allowed?
-      return invalid_action_error unless valid_action?(action)
-
-      if update_user(user, action)
-        log_event(user, action)
-        success
-      else
-        messages = user.errors.full_messages
-        error(messages.uniq.join('. '))
-      end
-    end
-
-    def update_user(user, action)
-      case action
-      when :ban
-        user.ban
-      when :unban
-        user.activate
-      end
-    end
-  end
-end
diff --git a/app/services/users/unban_user_service.rb b/app/services/users/unban_user_service.rb
new file mode 100644
index 00000000000000..2765aea4056129
--- /dev/null
+++ b/app/services/users/unban_user_service.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module Users
+  class UnbanUserService < BannedUserBaseService
+    attr_reader :action
+    def initialize(current_user)
+      super
+
+      @action = :unban
+    end
+
+    def update_user(user)
+      user.activate
+    end
+  end
+end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 2c096af6349039..78cfc9bc07c4a7 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -17773,9 +17773,6 @@ msgstr ""
 msgid "Invalid URL"
 msgstr ""
 
-msgid "Invalid action"
-msgstr ""
-
 msgid "Invalid container_name"
 msgstr ""
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 9b89fe67bf5f08..cd68d5e96bb62b 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1954,7 +1954,7 @@
   describe 'banning and unbanning a user', :aggregate_failures do
     let(:user) { create(:user) }
 
-    context 'an active user' do
+    context 'banning a user' do
       it 'bans and blocks the user' do
         user.ban
 
@@ -1977,12 +1977,12 @@
         user.activate
 
         expect(user.banned?).to eq(false)
-        expect(user.blocked?).to eq(false)
         expect(user.active?).to eq(true)
       end
 
       it 'deletes the BannedUser record' do
         expect { user.activate }.to change { Users::BannedUser.count }.by(-1)
+        expect(Users::BannedUser.where(user_id: user.id)).not_to exist
       end
     end
   end
diff --git a/spec/services/users/ban_user_service_spec.rb b/spec/services/users/ban_user_service_spec.rb
new file mode 100644
index 00000000000000..e8488a9019f878
--- /dev/null
+++ b/spec/services/users/ban_user_service_spec.rb
@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::BanUserService do
+  let(:current_user) { create(:admin) }
+  let(:user) { create(:user) }
+
+  subject(:service) { described_class.new(current_user) }
+
+  shared_examples 'does not modify the BannedUser record or user state' do
+    it 'does not modify the BannedUser record or user state' do
+      expect { ban_user }.not_to change { Users::BannedUser.count }
+      expect { ban_user }.not_to change { user.state }
+    end
+  end
+
+  context 'ban', :aggregate_failures do
+    subject(:ban_user) { service.execute(user) }
+
+    context 'when successful', :enable_admin_mode do
+      it 'returns success status' do
+        response = ban_user
+
+        expect(response[:status]).to eq(:success)
+      end
+
+      it 'bans the user' do
+        expect { ban_user }.to change { user.state }.from('active').to('banned')
+      end
+
+      it 'creates a BannedUser' do
+        expect { ban_user }.to change { Users::BannedUser.count }.by(1)
+        expect(Users::BannedUser.last.user_id).to eq(user.id)
+      end
+
+      it 'logs ban in application logs' do
+        allow(Gitlab::AppLogger).to receive(:info)
+
+        ban_user
+
+        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User ban", user: "#{user.username}", email: "#{user.email}", ban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
+      end
+    end
+
+    context 'when failed' do
+      context 'when user is blocked', :enable_admin_mode do
+        before do
+          user.block!
+        end
+
+        it 'returns state error message' do
+          response = ban_user
+
+          expect(response[:status]).to eq(:error)
+          expect(response[:message]).to match(/State cannot transition/)
+        end
+
+        it_behaves_like 'does not modify the BannedUser record or user state'
+      end
+
+      context 'when user is not an admin' do
+        it 'returns permissions error message' do
+          response = ban_user
+
+          expect(response[:status]).to eq(:error)
+          expect(response[:message]).to match(/You are not allowed to ban a user/)
+        end
+
+        it_behaves_like 'does not modify the BannedUser record or user state'
+      end
+    end
+  end
+end
diff --git a/spec/services/users/banned_user_service_spec.rb b/spec/services/users/banned_user_service_spec.rb
deleted file mode 100644
index 20fbf62e270254..00000000000000
--- a/spec/services/users/banned_user_service_spec.rb
+++ /dev/null
@@ -1,139 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Users::BannedUserService do
-  let(:current_user) { create(:admin) }
-  let(:user) { create(:user) }
-
-  subject(:service) { described_class.new(current_user) }
-
-  shared_examples 'user is in the wrong state' do
-    it 'returns state error message' do
-      expect(operation[:status]).to eq(:error)
-      expect(operation[:message]).to match(/State cannot transition/)
-    end
-  end
-
-  shared_examples 'does not modify the BannedUser record or user state' do
-    it 'does not modify the BannedUser record or user state' do
-      expect { operation }.not_to change { Users::BannedUser.count }
-      expect { operation }.not_to change { user.state }
-    end
-  end
-
-  context 'ban', :aggregate_failures do
-    subject(:operation) { service.execute(user, :ban) }
-
-    context 'when successful', :enable_admin_mode do
-      it 'returns success status' do
-        response = operation
-
-        expect(response[:status]).to eq(:success)
-      end
-
-      it 'bans the user' do
-        expect { operation }.to change { user.state }.from('active').to('banned')
-      end
-
-      it 'creates a BannedUser' do
-        expect { operation }.to change { Users::BannedUser.count }.by(1)
-        expect(Users::BannedUser.last.user_id).to eq(user.id)
-      end
-
-      it 'logs ban in application logs' do
-        allow(Gitlab::AppLogger).to receive(:info)
-
-        operation
-
-        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User ban", user: "#{user.username}", email: "#{user.email}", ban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
-      end
-    end
-
-    context 'when failed' do
-      context 'when user is blocked', :enable_admin_mode do
-        before do
-          user.block!
-        end
-
-        it_behaves_like 'user is in the wrong state'
-
-        it_behaves_like 'does not modify the BannedUser record or user state'
-      end
-
-      context 'when user is not an admin' do
-        it 'returns permissions error message' do
-          expect(operation[:status]).to eq(:error)
-          expect(operation[:message]).to match(/You are not allowed to ban a user/)
-        end
-
-        it_behaves_like 'does not modify the BannedUser record or user state'
-      end
-    end
-  end
-
-  context 'unban', :aggregate_failures do
-    subject(:operation) { service.execute(user, :unban) }
-
-    context 'when successful', :enable_admin_mode do
-      before do
-        user.ban!
-      end
-
-      it 'returns success status' do
-        response = operation
-
-        expect(response[:status]).to eq(:success)
-      end
-
-      it 'unbans the user' do
-        expect { operation }.to change { user.state }.from('banned').to('active')
-      end
-
-      it 'removes the BannedUser' do
-        expect { operation }.to change { Users::BannedUser.count }.by(-1)
-        expect(user.reload.banned_user).to be_nil
-      end
-
-      it 'logs unban in application logs' do
-        allow(Gitlab::AppLogger).to receive(:info)
-
-        operation
-
-        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User unban", user: "#{user.username}", email: "#{user.email}", unban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
-      end
-    end
-
-    context 'when failed' do
-      context 'when user is already active', :enable_admin_mode do
-        it_behaves_like 'user is in the wrong state'
-
-        it_behaves_like 'does not modify the BannedUser record or user state'
-      end
-
-      context 'when user is not an admin' do
-        before do
-          user.ban!
-        end
-
-        it 'returns permissions error message' do
-          expect(operation[:status]).to eq(:error)
-          expect(operation[:message]).to match(/You are not allowed to unban a user/)
-        end
-
-        it_behaves_like 'does not modify the BannedUser record or user state'
-      end
-    end
-  end
-
-  context 'invalid action', :enable_admin_mode do
-    subject(:operation) { service.execute(user, :invalid) }
-
-    it 'returns invalid action error message' do
-      expect(operation[:status]).to eq(:error)
-      expect(operation[:message]).to match(/Invalid action/)
-    end
-
-    it_behaves_like 'does not modify the BannedUser record or user state'
-  end
-end
diff --git a/spec/services/users/unban_user_service_spec.rb b/spec/services/users/unban_user_service_spec.rb
new file mode 100644
index 00000000000000..e8741c2f5936fe
--- /dev/null
+++ b/spec/services/users/unban_user_service_spec.rb
@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::UnbanUserService do
+  let(:current_user) { create(:admin) }
+  let(:user) { create(:user) }
+
+  subject(:service) { described_class.new(current_user) }
+
+  shared_examples 'does not modify the BannedUser record or user state' do
+    it 'does not modify the BannedUser record or user state' do
+      expect { unban_user }.not_to change { Users::BannedUser.count }
+      expect { unban_user }.not_to change { user.state }
+    end
+  end
+
+  context 'unban', :aggregate_failures do
+    subject(:unban_user) { service.execute(user) }
+
+    context 'when successful', :enable_admin_mode do
+      before do
+        user.ban!
+      end
+
+      it 'returns success status' do
+        response = unban_user
+
+        expect(response[:status]).to eq(:success)
+      end
+
+      it 'unbans the user' do
+        expect { unban_user }.to change { user.state }.from('banned').to('active')
+      end
+
+      it 'removes the BannedUser' do
+        expect { unban_user }.to change { Users::BannedUser.count }.by(-1)
+        expect(user.reload.banned_user).to be_nil
+      end
+
+      it 'logs unban in application logs' do
+        allow(Gitlab::AppLogger).to receive(:info)
+
+        unban_user
+
+        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User unban", user: "#{user.username}", email: "#{user.email}", unban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
+      end
+    end
+
+    context 'when failed' do
+      context 'when user is already active', :enable_admin_mode do
+        it 'returns state error message' do
+          response = unban_user
+
+          expect(response[:status]).to eq(:error)
+          expect(response[:message]).to match(/State cannot transition/)
+        end
+
+        it_behaves_like 'does not modify the BannedUser record or user state'
+      end
+
+      context 'when user is not an admin' do
+        before do
+          user.ban!
+        end
+
+        it 'returns permissions error message' do
+          response = unban_user
+
+          expect(response[:status]).to eq(:error)
+          expect(response[:message]).to match(/You are not allowed to unban a user/)
+        end
+
+        it_behaves_like 'does not modify the BannedUser record or user state'
+      end
+    end
+  end
+end
-- 
GitLab


From 34e075199c46d9bffe06208323af867c211813d7 Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Mon, 26 Jul 2021 15:03:33 -0500
Subject: [PATCH 10/13] Use action method instead of override initialize

---
 app/services/users/ban_user_service.rb         | 7 ++-----
 app/services/users/banned_user_base_service.rb | 2 +-
 app/services/users/unban_user_service.rb       | 7 ++-----
 3 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/app/services/users/ban_user_service.rb b/app/services/users/ban_user_service.rb
index e463108d5f3051..854c7f12bcd3f0 100644
--- a/app/services/users/ban_user_service.rb
+++ b/app/services/users/ban_user_service.rb
@@ -2,11 +2,8 @@
 
 module Users
   class BanUserService < BannedUserBaseService
-    attr_reader :action
-    def initialize(current_user)
-      super
-
-      @action = :ban
+    def action
+      :ban
     end
 
     def update_user(user)
diff --git a/app/services/users/banned_user_base_service.rb b/app/services/users/banned_user_base_service.rb
index 2372468151c809..16041075941428 100644
--- a/app/services/users/banned_user_base_service.rb
+++ b/app/services/users/banned_user_base_service.rb
@@ -20,7 +20,7 @@ def execute(user)
 
     private
 
-    attr_reader :current_user, :action
+    attr_reader :current_user
 
     def allowed?
       can?(current_user, :admin_all_resources)
diff --git a/app/services/users/unban_user_service.rb b/app/services/users/unban_user_service.rb
index 2765aea4056129..bf27461e7de58c 100644
--- a/app/services/users/unban_user_service.rb
+++ b/app/services/users/unban_user_service.rb
@@ -2,11 +2,8 @@
 
 module Users
   class UnbanUserService < BannedUserBaseService
-    attr_reader :action
-    def initialize(current_user)
-      super
-
-      @action = :unban
+    def action
+      :unban
     end
 
     def update_user(user)
-- 
GitLab


From 428018ecf832fbffc718a39f0dbd588f046d8988 Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Tue, 27 Jul 2021 17:32:00 -0500
Subject: [PATCH 11/13] Apply MR review suggestions

Rename service, modify specs

Run rubocop linter
---
 app/controllers/admin/users_controller.rb            |  4 ++--
 app/models/users/banned_user.rb                      |  2 +-
 .../users/{ban_user_service.rb => ban_service.rb}    | 12 +++++++-----
 .../{unban_user_service.rb => unban_service.rb}      | 12 +++++++-----
 locale/gitlab.pot                                    |  3 +++
 spec/models/users/banned_user_spec.rb                |  4 +++-
 ...{ban_user_service_spec.rb => ban_service_spec.rb} | 10 +++-------
 ...an_user_service_spec.rb => unban_service_spec.rb} | 10 +++-------
 8 files changed, 29 insertions(+), 28 deletions(-)
 rename app/services/users/{ban_user_service.rb => ban_service.rb} (71%)
 rename app/services/users/{unban_user_service.rb => unban_service.rb} (71%)
 rename spec/services/users/{ban_user_service_spec.rb => ban_service_spec.rb} (81%)
 rename spec/services/users/{unban_user_service_spec.rb => unban_service_spec.rb} (81%)

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index cd0b9093ee9177..cc6b537870bef3 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -126,7 +126,7 @@ def unblock
   end
 
   def ban
-    result = Users::BanUserService.new(current_user).execute(user)
+    result = Users::BanService.new(current_user).execute(user)
 
     if result[:status] == :success
       redirect_back_or_admin_user(notice: _("Successfully banned"))
@@ -136,7 +136,7 @@ def ban
   end
 
   def unban
-    result = Users::UnbanUserService.new(current_user).execute(user)
+    result = Users::UnbanService.new(current_user).execute(user)
 
     if result[:status] == :success
       redirect_back_or_admin_user(notice: _("Successfully unbanned"))
diff --git a/app/models/users/banned_user.rb b/app/models/users/banned_user.rb
index ed987802d23b9b..c52b6d4b7284f0 100644
--- a/app/models/users/banned_user.rb
+++ b/app/models/users/banned_user.rb
@@ -7,6 +7,6 @@ class BannedUser < ApplicationRecord
     belongs_to :user
 
     validates :user, presence: true
-    validates :user_id, uniqueness: { message: "banned user already exists" }
+    validates :user_id, uniqueness: { message: _("banned user already exists") }
   end
 end
diff --git a/app/services/users/ban_user_service.rb b/app/services/users/ban_service.rb
similarity index 71%
rename from app/services/users/ban_user_service.rb
rename to app/services/users/ban_service.rb
index 854c7f12bcd3f0..9b956b4c52f021 100644
--- a/app/services/users/ban_user_service.rb
+++ b/app/services/users/ban_service.rb
@@ -1,13 +1,15 @@
 # frozen_string_literal: true
 
 module Users
-  class BanUserService < BannedUserBaseService
-    def action
-      :ban
-    end
-
+  class BanService < BannedUserBaseService
     def update_user(user)
       user.ban
     end
+
+    private
+
+    def action
+      :ban
+    end
   end
 end
diff --git a/app/services/users/unban_user_service.rb b/app/services/users/unban_service.rb
similarity index 71%
rename from app/services/users/unban_user_service.rb
rename to app/services/users/unban_service.rb
index bf27461e7de58c..de1f5f1d99d96d 100644
--- a/app/services/users/unban_user_service.rb
+++ b/app/services/users/unban_service.rb
@@ -1,13 +1,15 @@
 # frozen_string_literal: true
 
 module Users
-  class UnbanUserService < BannedUserBaseService
-    def action
-      :unban
-    end
-
+  class UnbanService < BannedUserBaseService
     def update_user(user)
       user.activate
     end
+
+    private
+
+    def action
+      :unban
+    end
   end
 end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 78cfc9bc07c4a7..9c2c90aaa96b70 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -38210,6 +38210,9 @@ msgstr ""
 msgid "authored"
 msgstr ""
 
+msgid "banned user already exists"
+msgstr ""
+
 msgid "blocks"
 msgstr ""
 
diff --git a/spec/models/users/banned_user_spec.rb b/spec/models/users/banned_user_spec.rb
index 135f0a4d0490d4..cbe42fb6d79681 100644
--- a/spec/models/users/banned_user_spec.rb
+++ b/spec/models/users/banned_user_spec.rb
@@ -3,10 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe Users::BannedUser do
+  subject(:banned_user) { described_class.new }
+
   it { is_expected.to belong_to(:user) }
 
   it 'validates uniqueness of banned user id' do
-    subject.user = create(:user)
+    banned_user.user = create(:user)
     is_expected.to validate_uniqueness_of(:user_id).with_message("banned user already exists")
   end
 end
diff --git a/spec/services/users/ban_user_service_spec.rb b/spec/services/users/ban_service_spec.rb
similarity index 81%
rename from spec/services/users/ban_user_service_spec.rb
rename to spec/services/users/ban_service_spec.rb
index e8488a9019f878..fedb8efff9cb73 100644
--- a/spec/services/users/ban_user_service_spec.rb
+++ b/spec/services/users/ban_service_spec.rb
@@ -2,12 +2,10 @@
 
 require 'spec_helper'
 
-RSpec.describe Users::BanUserService do
+RSpec.describe Users::BanService do
   let(:current_user) { create(:admin) }
   let(:user) { create(:user) }
 
-  subject(:service) { described_class.new(current_user) }
-
   shared_examples 'does not modify the BannedUser record or user state' do
     it 'does not modify the BannedUser record or user state' do
       expect { ban_user }.not_to change { Users::BannedUser.count }
@@ -16,7 +14,7 @@
   end
 
   context 'ban', :aggregate_failures do
-    subject(:ban_user) { service.execute(user) }
+    subject(:ban_user) { described_class.new(current_user).execute(user) }
 
     context 'when successful', :enable_admin_mode do
       it 'returns success status' do
@@ -35,11 +33,9 @@
       end
 
       it 'logs ban in application logs' do
-        allow(Gitlab::AppLogger).to receive(:info)
+        expect(Gitlab::AppLogger).to receive(:info).with(message: "User ban", user: "#{user.username}", email: "#{user.email}", ban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
 
         ban_user
-
-        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User ban", user: "#{user.username}", email: "#{user.email}", ban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
       end
     end
 
diff --git a/spec/services/users/unban_user_service_spec.rb b/spec/services/users/unban_service_spec.rb
similarity index 81%
rename from spec/services/users/unban_user_service_spec.rb
rename to spec/services/users/unban_service_spec.rb
index e8741c2f5936fe..0a60cba67eb742 100644
--- a/spec/services/users/unban_user_service_spec.rb
+++ b/spec/services/users/unban_service_spec.rb
@@ -2,12 +2,10 @@
 
 require 'spec_helper'
 
-RSpec.describe Users::UnbanUserService do
+RSpec.describe Users::UnbanService do
   let(:current_user) { create(:admin) }
   let(:user) { create(:user) }
 
-  subject(:service) { described_class.new(current_user) }
-
   shared_examples 'does not modify the BannedUser record or user state' do
     it 'does not modify the BannedUser record or user state' do
       expect { unban_user }.not_to change { Users::BannedUser.count }
@@ -16,7 +14,7 @@
   end
 
   context 'unban', :aggregate_failures do
-    subject(:unban_user) { service.execute(user) }
+    subject(:unban_user) { described_class.new(current_user).execute(user) }
 
     context 'when successful', :enable_admin_mode do
       before do
@@ -39,11 +37,9 @@
       end
 
       it 'logs unban in application logs' do
-        allow(Gitlab::AppLogger).to receive(:info)
+        expect(Gitlab::AppLogger).to receive(:info).with(message: "User unban", user: "#{user.username}", email: "#{user.email}", unban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
 
         unban_user
-
-        expect(Gitlab::AppLogger).to have_received(:info).with(message: "User unban", user: "#{user.username}", email: "#{user.email}", unban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}")
       end
     end
 
-- 
GitLab


From d11f2b69809d0f555e125dc33e3c811c0dffa253 Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Thu, 29 Jul 2021 18:52:26 -0500
Subject: [PATCH 12/13] Apply maintainer suggestions

Apply maintainer review suggestions
---
 app/services/users/ban_service.rb         |  4 ++--
 app/services/users/unban_service.rb       |  4 ++--
 spec/models/users/banned_user_spec.rb     | 15 ++++++++++-----
 spec/services/users/ban_service_spec.rb   |  3 ++-
 spec/services/users/unban_service_spec.rb |  3 ++-
 5 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/app/services/users/ban_service.rb b/app/services/users/ban_service.rb
index 9b956b4c52f021..88e92ebff9b3a6 100644
--- a/app/services/users/ban_service.rb
+++ b/app/services/users/ban_service.rb
@@ -2,12 +2,12 @@
 
 module Users
   class BanService < BannedUserBaseService
+    private
+
     def update_user(user)
       user.ban
     end
 
-    private
-
     def action
       :ban
     end
diff --git a/app/services/users/unban_service.rb b/app/services/users/unban_service.rb
index de1f5f1d99d96d..363783cf240a6a 100644
--- a/app/services/users/unban_service.rb
+++ b/app/services/users/unban_service.rb
@@ -2,12 +2,12 @@
 
 module Users
   class UnbanService < BannedUserBaseService
+    private
+
     def update_user(user)
       user.activate
     end
 
-    private
-
     def action
       :unban
     end
diff --git a/spec/models/users/banned_user_spec.rb b/spec/models/users/banned_user_spec.rb
index cbe42fb6d79681..f7d6d637377ede 100644
--- a/spec/models/users/banned_user_spec.rb
+++ b/spec/models/users/banned_user_spec.rb
@@ -3,12 +3,17 @@
 require 'spec_helper'
 
 RSpec.describe Users::BannedUser do
-  subject(:banned_user) { described_class.new }
+  before do
+    create(:user, :banned)
+  end
 
-  it { is_expected.to belong_to(:user) }
+  describe 'relationships' do
+    it { is_expected.to belong_to(:user) }
+  end
 
-  it 'validates uniqueness of banned user id' do
-    banned_user.user = create(:user)
-    is_expected.to validate_uniqueness_of(:user_id).with_message("banned user already exists")
+  describe 'validations' do
+    it 'validates uniqueness of banned user id' do
+      is_expected.to validate_uniqueness_of(:user_id).with_message("banned user already exists")
+    end
   end
 end
diff --git a/spec/services/users/ban_service_spec.rb b/spec/services/users/ban_service_spec.rb
index fedb8efff9cb73..6f49ee08782614 100644
--- a/spec/services/users/ban_service_spec.rb
+++ b/spec/services/users/ban_service_spec.rb
@@ -3,9 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe Users::BanService do
-  let(:current_user) { create(:admin) }
   let(:user) { create(:user) }
 
+  let_it_be(:current_user) { create(:admin) }
+
   shared_examples 'does not modify the BannedUser record or user state' do
     it 'does not modify the BannedUser record or user state' do
       expect { ban_user }.not_to change { Users::BannedUser.count }
diff --git a/spec/services/users/unban_service_spec.rb b/spec/services/users/unban_service_spec.rb
index 0a60cba67eb742..b2b3140ccb30ef 100644
--- a/spec/services/users/unban_service_spec.rb
+++ b/spec/services/users/unban_service_spec.rb
@@ -3,9 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe Users::UnbanService do
-  let(:current_user) { create(:admin) }
   let(:user) { create(:user) }
 
+  let_it_be(:current_user) { create(:admin) }
+
   shared_examples 'does not modify the BannedUser record or user state' do
     it 'does not modify the BannedUser record or user state' do
       expect { unban_user }.not_to change { Users::BannedUser.count }
-- 
GitLab


From 78d6640793312f4c2d3a09dcc7efed880fddcb08 Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Thu, 29 Jul 2021 18:59:59 -0500
Subject: [PATCH 13/13] Add another test

---
 spec/models/users/banned_user_spec.rb | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/spec/models/users/banned_user_spec.rb b/spec/models/users/banned_user_spec.rb
index f7d6d637377ede..b55c4821d05ce6 100644
--- a/spec/models/users/banned_user_spec.rb
+++ b/spec/models/users/banned_user_spec.rb
@@ -3,15 +3,17 @@
 require 'spec_helper'
 
 RSpec.describe Users::BannedUser do
-  before do
-    create(:user, :banned)
-  end
-
   describe 'relationships' do
     it { is_expected.to belong_to(:user) }
   end
 
   describe 'validations' do
+    before do
+      create(:user, :banned)
+    end
+
+    it { is_expected.to validate_presence_of(:user) }
+
     it 'validates uniqueness of banned user id' do
       is_expected.to validate_uniqueness_of(:user_id).with_message("banned user already exists")
     end
-- 
GitLab