Merge branch 'refactor-services' into 'master'

Refactor services to match EE signature

See merge request gitlab-org/gitlab-ce!14385

25 files changed, 85 insertions, 57 deletions

diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb
index 16590e66d61..fb6d8c0bb81 100644
--- a/app/controllers/admin/applications_controller.rb
+++ b/app/controllers/admin/applications_controller.rb
@@ -22,8 +22,7 @@ class Admin::ApplicationsController < Admin::ApplicationController
     @application = Doorkeeper::Application.new(application_params)
 
     if @application.save
-      flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
-      redirect_to admin_application_url(@application)
+      redirect_to_admin_page
     else
       render :new
     end
@@ -42,6 +41,13 @@ class Admin::ApplicationsController < Admin::ApplicationController
     redirect_to admin_applications_url, status: 302, notice: 'Application was successfully destroyed.'
   end
 
+  protected
+
+  def redirect_to_admin_page
+    flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
+    redirect_to admin_application_url(@application)
+  end
+
   private
 
   def set_application
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index cbcef70e957..676a7203c7d 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -128,7 +128,7 @@ class Admin::UsersController < Admin::ApplicationController
     end
 
     respond_to do |format|
-      result = Users::UpdateService.new(user, user_params_with_pass).execute do |user|
+      result = Users::UpdateService.new(current_user, user_params_with_pass.merge(user: user)).execute do |user|
         user.skip_reconfirmation!
       end
 
@@ -155,7 +155,7 @@ class Admin::UsersController < Admin::ApplicationController
 
   def remove_email
     email = user.emails.find(params[:email_id])
-    success = Emails::DestroyService.new(user, email: email.email).execute
+    success = Emails::DestroyService.new(current_user, user: user, email: email.email).execute
 
     respond_to do |format|
       if success
@@ -219,7 +219,7 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def update_user(&block)
-    result = Users::UpdateService.new(user).execute(&block)
+    result = Users::UpdateService.new(current_user, user: user).execute(&block)
 
     result[:status] == :success
   end
diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb
index 306afb65f10..10d2665c06a 100644
--- a/app/controllers/confirmations_controller.rb
+++ b/app/controllers/confirmations_controller.rb
@@ -12,10 +12,14 @@ class ConfirmationsController < Devise::ConfirmationsController
 
   def after_confirmation_path_for(resource_name, resource)
     if signed_in?(resource_name)
-      after_sign_in_path_for(resource)
+      after_sign_in(resource)
     else
       flash[:notice] += " Please sign in."
       new_session_path(resource_name)
     end
   end
+
+  def after_sign_in(resource)
+    after_sign_in_path_for(resource)
+  end
 end
diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb
index 2ae4785b12c..b02e64a132b 100644
--- a/app/controllers/oauth/applications_controller.rb
+++ b/app/controllers/oauth/applications_controller.rb
@@ -21,14 +21,20 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
     @application.owner = current_user
 
     if @application.save
-      flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
-      redirect_to oauth_application_url(@application)
+      redirect_to_oauth_application_page
     else
       set_index_vars
       render :index
     end
   end
 
+  protected
+
+  def redirect_to_oauth_application_page
+    flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
+    redirect_to oauth_application_url(@application)
+  end
+
   private
 
   def verify_user_oauth_applications_enabled
diff --git a/app/controllers/profiles/avatars_controller.rb b/app/controllers/profiles/avatars_controller.rb
index 408650aac54..39b9f8a84d1 100644
--- a/app/controllers/profiles/avatars_controller.rb
+++ b/app/controllers/profiles/avatars_controller.rb
@@ -2,7 +2,7 @@ class Profiles::AvatarsController < Profiles::ApplicationController
   def destroy
     @user = current_user
 
-    Users::UpdateService.new(@user).execute { |user| user.remove_avatar! }
+    Users::UpdateService.new(current_user, user: @user).execute { |user| user.remove_avatar! }
 
     redirect_to profile_path, status: 302
   end
diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index ddb67d1c4d1..97db84b92d4 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/app/controllers/profiles/emails_controller.rb
@@ -5,7 +5,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
   end
 
   def create
-    @email = Emails::CreateService.new(current_user, email_params).execute
+    @email = Emails::CreateService.new(current_user, email_params.merge(user: current_user)).execute
 
     if @email.errors.blank?
       NotificationService.new.new_email(@email)
@@ -19,7 +19,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
   def destroy
     @email = current_user.emails.find(params[:id])
 
-    Emails::DestroyService.new(current_user, email: @email.email).execute
+    Emails::DestroyService.new(current_user, user: current_user, email: @email.email).execute
 
     respond_to do |format|
       format.html { redirect_to profile_emails_url, status: 302 }
diff --git a/app/controllers/profiles/keys_controller.rb b/app/controllers/profiles/keys_controller.rb
index 89d6d7f1b52..069e6a810f2 100644
--- a/app/controllers/profiles/keys_controller.rb
+++ b/app/controllers/profiles/keys_controller.rb
@@ -14,7 +14,7 @@ class Profiles::KeysController < Profiles::ApplicationController
     @key = Keys::CreateService.new(current_user, key_params).execute
 
     if @key.persisted?
-      redirect_to profile_key_path(@key)
+      redirect_to_profile_key_path
     else
       @keys = current_user.keys.select(&:persisted?)
       render :index
@@ -50,6 +50,12 @@ class Profiles::KeysController < Profiles::ApplicationController
     end
   end
 
+  protected
+
+  def redirect_to_profile_key_path
+    redirect_to profile_key_path(@key)
+  end
+
   private
 
   def key_params
diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb
index 960b7512602..8a38ba65d4c 100644
--- a/app/controllers/profiles/notifications_controller.rb
+++ b/app/controllers/profiles/notifications_controller.rb
@@ -7,7 +7,7 @@ class Profiles::NotificationsController < Profiles::ApplicationController
   end
 
   def update
-    result = Users::UpdateService.new(current_user, user_params).execute
+    result = Users::UpdateService.new(current_user, user_params.merge(user: current_user)).execute
 
     if result[:status] == :success
       flash[:notice] = "Notification settings saved"
diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb
index 7beb52dd8e8..dcfcb855ab5 100644
--- a/app/controllers/profiles/passwords_controller.rb
+++ b/app/controllers/profiles/passwords_controller.rb
@@ -21,10 +21,10 @@ class Profiles::PasswordsController < Profiles::ApplicationController
       password_automatically_set: false
     }
 
-    result = Users::UpdateService.new(@user, password_attributes).execute
+    result = Users::UpdateService.new(current_user, password_attributes.merge(user: @user)).execute
 
     if result[:status] == :success
-      Users::UpdateService.new(@user, password_expires_at: nil).execute
+      Users::UpdateService.new(current_user, user: @user, password_expires_at: nil).execute
 
       redirect_to root_path, notice: 'Password successfully changed'
     else
@@ -46,7 +46,7 @@ class Profiles::PasswordsController < Profiles::ApplicationController
       return
     end
 
-    result = Users::UpdateService.new(@user, password_attributes).execute
+    result = Users::UpdateService.new(current_user, password_attributes.merge(user: @user)).execute
 
     if result[:status] == :success
       flash[:notice] = "Password was successfully updated. Please login with it"
diff --git a/app/controllers/profiles/preferences_controller.rb b/app/controllers/profiles/preferences_controller.rb
index cce2a847b53..ed0f98179eb 100644
--- a/app/controllers/profiles/preferences_controller.rb
+++ b/app/controllers/profiles/preferences_controller.rb
@@ -6,7 +6,7 @@ class Profiles::PreferencesController < Profiles::ApplicationController
 
   def update
     begin
-      result = Users::UpdateService.new(user, preferences_params).execute
+      result = Users::UpdateService.new(current_user, preferences_params.merge(user: user)).execute
 
       if result[:status] == :success
         flash[:notice] = 'Preferences saved.'
diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb
index 1a4f77639e7..aa9789f8a0f 100644
--- a/app/controllers/profiles/two_factor_auths_controller.rb
+++ b/app/controllers/profiles/two_factor_auths_controller.rb
@@ -10,7 +10,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
       current_user.otp_grace_period_started_at = Time.current
     end
 
-    Users::UpdateService.new(current_user).execute!
+    Users::UpdateService.new(current_user, user: current_user).execute!
 
     if two_factor_authentication_required? && !current_user.two_factor_enabled?
       two_factor_authentication_reason(
@@ -41,7 +41,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
 
   def create
     if current_user.validate_and_consume_otp!(params[:pin_code])
-      Users::UpdateService.new(current_user, otp_required_for_login: true).execute! do |user|
+      Users::UpdateService.new(current_user, user: current_user, otp_required_for_login: true).execute! do |user|
         @codes = user.generate_otp_backup_codes!
       end
 
@@ -70,7 +70,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
   end
 
   def codes
-    Users::UpdateService.new(current_user).execute! do |user|
+    Users::UpdateService.new(current_user, user: current_user).execute! do |user|
       @codes = user.generate_otp_backup_codes!
     end
   end
diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb
index d83824fef06..5d87037f012 100644
--- a/app/controllers/profiles_controller.rb
+++ b/app/controllers/profiles_controller.rb
@@ -10,7 +10,7 @@ class ProfilesController < Profiles::ApplicationController
 
   def update
     respond_to do |format|
-      result = Users::UpdateService.new(@user, user_params).execute
+      result = Users::UpdateService.new(current_user, user_params.merge(user: @user)).execute
 
       if result[:status] == :success
         message = "Profile was successfully updated"
@@ -25,7 +25,7 @@ class ProfilesController < Profiles::ApplicationController
   end
 
   def reset_private_token
-    Users::UpdateService.new(@user).execute! do |user|
+    Users::UpdateService.new(current_user, user: @user).execute! do |user|
       user.reset_authentication_token!
     end
 
@@ -35,7 +35,7 @@ class ProfilesController < Profiles::ApplicationController
   end
 
   def reset_incoming_email_token
-    Users::UpdateService.new(@user).execute! do |user|
+    Users::UpdateService.new(current_user, user: @user).execute! do |user|
       user.reset_incoming_email_token!
     end
 
@@ -45,7 +45,7 @@ class ProfilesController < Profiles::ApplicationController
   end
 
   def reset_rss_token
-    Users::UpdateService.new(@user).execute! do |user|
+    Users::UpdateService.new(current_user, user: @user).execute! do |user|
       user.reset_rss_token!
     end
 
@@ -61,7 +61,7 @@ class ProfilesController < Profiles::ApplicationController
   end
 
   def update_username
-    result = Users::UpdateService.new(@user, username: user_params[:username]).execute
+    result = Users::UpdateService.new(current_user, user: @user, username: user_params[:username]).execute
 
     options = if result[:status] == :success
                 { notice: "Username successfully changed" }
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
index be6491d042c..fe3bb117410 100644
--- a/app/controllers/sessions_controller.rb
+++ b/app/controllers/sessions_controller.rb
@@ -55,7 +55,7 @@ class SessionsController < Devise::SessionsController
 
     return unless user && user.require_password_creation?
 
-    Users::UpdateService.new(user).execute do |user|
+    Users::UpdateService.new(current_user, user: user).execute do |user|
       @token = user.generate_reset_token
     end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 09c9b3250eb..103ac78783f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -60,7 +60,7 @@ class User < ActiveRecord::Base
     lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i)
     return unless lease.try_obtain
 
-    Users::UpdateService.new(self).execute(validate: false)
+    Users::UpdateService.new(self, user: self).execute(validate: false)
   end
 
   attr_accessor :force_random_password
@@ -526,8 +526,8 @@ class User < ActiveRecord::Base
   def update_emails_with_primary_email
     primary_email_record = emails.find_by(email: email)
     if primary_email_record
-      Emails::DestroyService.new(self, email: email).execute
-      Emails::CreateService.new(self, email: email_was).execute
+      Emails::DestroyService.new(self, user: self, email: email).execute
+      Emails::CreateService.new(self, user: self, email: email_was).execute
     end
   end
 
@@ -1000,7 +1000,7 @@ class User < ActiveRecord::Base
     if attempts_exceeded?
       lock_access! unless access_locked?
     else
-      Users::UpdateService.new(self).execute(validate: false)
+      Users::UpdateService.new(self, user: self).execute(validate: false)
     end
   end
 
@@ -1186,7 +1186,7 @@ class User < ActiveRecord::Base
       &creation_block
     )
 
-    Users::UpdateService.new(user).execute(validate: false)
+    Users::UpdateService.new(user, user: user).execute(validate: false)
     user
   ensure
     Gitlab::ExclusiveLease.cancel(lease_key, uuid)
diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb
index ace49889097..7f591c89411 100644
--- a/app/services/emails/base_service.rb
+++ b/app/services/emails/base_service.rb
@@ -1,7 +1,8 @@
 module Emails
   class BaseService
-    def initialize(user, opts)
-      @user = user
+    def initialize(current_user, opts)
+      @current_user = current_user
+      @user = opts.delete(:user)
       @email = opts[:email]
     end
   end
diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb
index d586b9dfe0c..44011cc36c8 100644
--- a/app/services/emails/destroy_service.rb
+++ b/app/services/emails/destroy_service.rb
@@ -1,13 +1,13 @@
 module Emails
   class DestroyService < ::Emails::BaseService
     def execute
-      Email.find_by_email!(@email).destroy && update_secondary_emails!
+      update_secondary_emails! if Email.find_by_email!(@email).destroy
     end
 
     private
 
     def update_secondary_emails!
-      result = ::Users::UpdateService.new(@user).execute do |user|
+      result = ::Users::UpdateService.new(@current_user, user: @user).execute do |user|
         user.update_secondary_emails!
       end
 
diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index 6188b8a4349..15ca1a55a5b 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -2,22 +2,21 @@ module Users
   class UpdateService < BaseService
     include NewUserNotifier
 
-    def initialize(user, params = {})
-      @user = user
+    def initialize(current_user, params = {})
+      @current_user = current_user
+      @user = params.delete(:user)
       @params = params.dup
     end
 
     def execute(validate: true, &block)
       yield(@user) if block_given?
 
-      assign_attributes(&block)
-
       user_exists = @user.persisted?
 
-      if @user.save(validate: validate)
-        notify_new_user(@user, nil) unless user_exists
+      assign_attributes(&block)
 
-        success
+      if @user.save(validate: validate)
+        notify_success(user_exists)
       else
         error(@user.errors.full_messages.uniq.join('. '))
       end
@@ -33,6 +32,12 @@ module Users
 
     private
 
+    def notify_success(user_exists)
+      notify_new_user(@user, nil) unless user_exists
+
+      success
+    end
+
     def assign_attributes(&block)
       if @user.user_synced_attributes_metadata
         params.except!(*@user.user_synced_attributes_metadata.read_only_attributes)
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index c0fef56378f..a0557a609ca 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -136,7 +136,7 @@ module API
 
         codes = nil
 
-        ::Users::UpdateService.new(user).execute! do |user|
+        ::Users::UpdateService.new(current_user, user: user).execute! do |user|
           codes = user.generate_otp_backup_codes!
         end
 
diff --git a/lib/api/notification_settings.rb b/lib/api/notification_settings.rb
index bcc0833aa5c..0266bf2f717 100644
--- a/lib/api/notification_settings.rb
+++ b/lib/api/notification_settings.rb
@@ -35,7 +35,7 @@ module API
             new_notification_email = params.delete(:notification_email)
 
             if new_notification_email
-              ::Users::UpdateService.new(current_user, notification_email: new_notification_email).execute
+              ::Users::UpdateService.new(current_user, user: current_user, notification_email: new_notification_email).execute
             end
 
             notification_setting.update(declared_params(include_missing: false))
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 77ac24ec68d..99024d1f0ad 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -166,7 +166,7 @@ module API
 
         user_params[:password_expires_at] = Time.now if user_params[:password].present?
 
-        result = ::Users::UpdateService.new(user, user_params.except(:extern_uid, :provider)).execute
+        result = ::Users::UpdateService.new(current_user, user_params.except(:extern_uid, :provider).merge(user: user)).execute
 
         if result[:status] == :success
           present user, with: Entities::UserPublic
@@ -326,7 +326,7 @@ module API
         user = User.find_by(id: params.delete(:id))
         not_found!('User') unless user
 
-        email = Emails::CreateService.new(user, declared_params(include_missing: false)).execute
+        email = Emails::CreateService.new(current_user, declared_params(include_missing: false).merge(user: user)).execute
 
         if email.errors.blank?
           NotificationService.new.new_email(email)
@@ -367,7 +367,7 @@ module API
         not_found!('Email') unless email
 
         destroy_conditionally!(email) do |email|
-          Emails::DestroyService.new(current_user, email: email.email).execute
+          Emails::DestroyService.new(current_user, user: user, email: email.email).execute
         end
 
         user.update_secondary_emails!
@@ -672,7 +672,7 @@ module API
         requires :email, type: String, desc: 'The new email'
       end
       post "emails" do
-        email = Emails::CreateService.new(current_user, declared_params).execute
+        email = Emails::CreateService.new(current_user, declared_params.merge(user: current_user)).execute
 
         if email.errors.blank?
           NotificationService.new.new_email(email)
@@ -691,7 +691,7 @@ module API
         not_found!('Email') unless email
 
         destroy_conditionally!(email) do |email|
-          Emails::DestroyService.new(current_user, email: email.email).execute
+          Emails::DestroyService.new(current_user, user: current_user, email: email.email).execute
         end
 
         current_user.update_secondary_emails!
diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb
index fb68627dedf..e60ceba27c8 100644
--- a/lib/gitlab/ldap/access.rb
+++ b/lib/gitlab/ldap/access.rb
@@ -16,7 +16,7 @@ module Gitlab
       def self.allowed?(user)
         self.open(user) do |access|
           if access.allowed?
-            Users::UpdateService.new(user, last_credential_check_at: Time.now).execute
+            Users::UpdateService.new(user, user: user, last_credential_check_at: Time.now).execute
 
             true
           else
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index 7704bf715e4..e06d4dc45f7 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -32,7 +32,7 @@ module Gitlab
 
         block_after_save = needs_blocking?
 
-        Users::UpdateService.new(gl_user).execute!
+        Users::UpdateService.new(gl_user, user: gl_user).execute!
 
         gl_user.block if block_after_save
 
diff --git a/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb
index 641d5538de8..75812c17309 100644
--- a/spec/services/emails/create_service_spec.rb
+++ b/spec/services/emails/create_service_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Emails::CreateService do
   let(:user) { create(:user) }
-  let(:opts) { { email: 'new@email.com' } }
+  let(:opts) { { email: 'new@email.com', user: user } }
 
   subject(:service) { described_class.new(user, opts) }
 
diff --git a/spec/services/emails/destroy_service_spec.rb b/spec/services/emails/destroy_service_spec.rb
index 1f4294dd905..7726fc0ef81 100644
--- a/spec/services/emails/destroy_service_spec.rb
+++ b/spec/services/emails/destroy_service_spec.rb
@@ -4,7 +4,7 @@ describe Emails::DestroyService do
   let!(:user) { create(:user) }
   let!(:email) { create(:email, user: user) }
 
-  subject(:service) { described_class.new(user, email: email.email) }
+  subject(:service) { described_class.new(user, user: user, email: email.email) }
 
   describe '#execute' do
     it 'removes an email' do
diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb
index 6ee35a33b2d..f8d4a47b212 100644
--- a/spec/services/users/update_service_spec.rb
+++ b/spec/services/users/update_service_spec.rb
@@ -31,13 +31,13 @@ describe Users::UpdateService do
     end
 
     def update_user(user, opts)
-      described_class.new(user, opts).execute
+      described_class.new(user, opts.merge(user: user)).execute
     end
   end
 
   describe '#execute!' do
     it 'updates the name' do
-      service = described_class.new(user, name: 'New Name')
+      service = described_class.new(user, user: user, name: 'New Name')
       expect(service).not_to receive(:notify_new_user)
 
       result = service.execute!
@@ -55,7 +55,7 @@ describe Users::UpdateService do
     it 'fires system hooks when a new user is saved' do
       system_hook_service = spy(:system_hook_service)
       user = build(:user)
-      service = described_class.new(user, name: 'John Doe')
+      service = described_class.new(user, user: user, name: 'John Doe')
       expect(service).to receive(:notify_new_user).and_call_original
       expect(service).to receive(:system_hook_service).and_return(system_hook_service)
 
@@ -65,7 +65,7 @@ describe Users::UpdateService do
     end
 
     def update_user(user, opts)
-      described_class.new(user, opts).execute!
+      described_class.new(user, opts.merge(user: user)).execute!
     end
   end
 end