Merge branch 'feature/add-new-services' into 'master'

app/controllers/admin/users_controller.rb

@@ -54,7 +54,7 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def block
-    if user.block
+    if update_user { |user| user.block }
       redirect_back_or_admin_user(notice: "Successfully blocked")
     else
       redirect_back_or_admin_user(alert: "Error occurred. User was not blocked")
@@ -64,7 +64,7 @@ class Admin::UsersController < Admin::ApplicationController
   def unblock
     if user.ldap_blocked?
       redirect_back_or_admin_user(alert: "This user cannot be unlocked manually from GitLab")
-    elsif user.activate
+    elsif update_user { |user| user.activate }
       redirect_back_or_admin_user(notice: "Successfully unblocked")
     else
       redirect_back_or_admin_user(alert: "Error occurred. User was not unblocked")
@@ -72,7 +72,7 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def unlock
-    if user.unlock_access!
+    if update_user { |user| user.unlock_access! }
       redirect_back_or_admin_user(alert: "Successfully unlocked")
     else
       redirect_back_or_admin_user(alert: "Error occurred. User was not unlocked")
@@ -80,7 +80,7 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def confirm
-    if user.confirm
+    if update_user { |user| user.confirm }
       redirect_back_or_admin_user(notice: "Successfully confirmed")
     else
       redirect_back_or_admin_user(alert: "Error occurred. User was not confirmed")
@@ -88,7 +88,8 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def disable_two_factor
-    user.disable_two_factor!
+    update_user { |user| user.disable_two_factor! }
+
     redirect_to admin_user_path(user),
       notice: 'Two-factor Authentication has been disabled for this user'
   end
@@ -124,15 +125,18 @@ class Admin::UsersController < Admin::ApplicationController
     end
 
     respond_to do |format|
-      user.skip_reconfirmation!
-      if user.update_attributes(user_params_with_pass)
+      result = Users::UpdateService.new(user, user_params_with_pass).execute do |user|
+        user.skip_reconfirmation!
+      end
+
+      if result[:status] == :success
         format.html { redirect_to [:admin, user], notice: 'User was successfully updated.' }
         format.json { head :ok }
       else
         # restore username to keep form action url.
         user.username = params[:id]
         format.html { render "edit" }
-        format.json { render json: user.errors, status: :unprocessable_entity }
+        format.json { render json: [result[:message]], status: result[:status] }
       end
     end
   end
@@ -148,13 +152,16 @@ class Admin::UsersController < Admin::ApplicationController
 
   def remove_email
     email = user.emails.find(params[:email_id])
-    email.destroy
-
-    user.update_secondary_emails!
+    success = Emails::DestroyService.new(user, email: email.email).execute
 
     respond_to do |format|
-      format.html { redirect_back_or_admin_user(notice: "Successfully removed email.") }
-      format.js { head :ok }
+      if success
+        format.html { redirect_back_or_admin_user(notice: 'Successfully removed email.') }
+        format.json { head :ok }
+      else
+        format.html { redirect_back_or_admin_user(alert: 'There was an error removing the e-mail.') }
+        format.json { render json: 'There was an error removing the e-mail.', status: 400 }
+      end
     end
   end
 
@@ -202,4 +209,10 @@ class Admin::UsersController < Admin::ApplicationController
       :website_url
     ]
   end
+
+  def update_user(&block)
+    result = Users::UpdateService.new(user).execute(&block)
+
+    result[:status] == :success
+  end
 end

app/controllers/profiles/avatars_controller.rb

 class Profiles::AvatarsController < Profiles::ApplicationController
   def destroy
     @user = current_user
-    @user.remove_avatar!
 
-    @user.save
+    Users::UpdateService.new(@user).execute { |user| user.remove_avatar! }
 
     redirect_to profile_path, status: 302
   end

app/controllers/profiles/emails_controller.rb

@@ -5,9 +5,9 @@ class Profiles::EmailsController < Profiles::ApplicationController
   end
 
   def create
-    @email = current_user.emails.new(email_params)
+    @email = Emails::CreateService.new(current_user, email_params).execute
 
-    if @email.save
+    if @email.errors.blank?
       NotificationService.new.new_email(@email)
     else
       flash[:alert] = @email.errors.full_messages.first
@@ -18,9 +18,8 @@ class Profiles::EmailsController < Profiles::ApplicationController
 
   def destroy
     @email = current_user.emails.find(params[:id])
-    @email.destroy
 
-    current_user.update_secondary_emails!
+    Emails::DestroyService.new(current_user, email: @email.email).execute
 
     respond_to do |format|
       format.html { redirect_to profile_emails_url, status: 302 }

app/controllers/profiles/notifications_controller.rb

@@ -7,7 +7,9 @@ class Profiles::NotificationsController < Profiles::ApplicationController
   end
 
   def update
-    if current_user.update_attributes(user_params)
+    result = Users::UpdateService.new(current_user, user_params).execute
+
+    if result[:status] == :success
       flash[:notice] = "Notification settings saved"
     else
       flash[:alert] = "Failed to save new settings"

app/controllers/profiles/passwords_controller.rb

@@ -15,17 +15,17 @@ class Profiles::PasswordsController < Profiles::ApplicationController
       return
     end
 
-    new_password = user_params[:password]
-    new_password_confirmation = user_params[:password_confirmation]
-
-    result = @user.update_attributes(
-      password: new_password,
-      password_confirmation: new_password_confirmation,
+    password_attributes = {
+      password: user_params[:password],
+      password_confirmation: user_params[:password_confirmation],
       password_automatically_set: false
-    )
+    }
+
+    result = Users::UpdateService.new(@user, password_attributes).execute
+
+    if result[:status] == :success
+      Users::UpdateService.new(@user, password_expires_at: nil).execute
 
-    if result
-      @user.update_attributes(password_expires_at: nil)
       redirect_to root_path, notice: 'Password successfully changed'
     else
       render :new
@@ -46,7 +46,9 @@ class Profiles::PasswordsController < Profiles::ApplicationController
       return
     end
 
-    if @user.update_attributes(password_attributes)
+    result = Users::UpdateService.new(@user, password_attributes).execute
+
+    if result[:status] == :success
       flash[:notice] = "Password was successfully updated. Please login with it"
       redirect_to new_user_session_path
     else

app/controllers/profiles/preferences_controller.rb

@@ -6,7 +6,9 @@ class Profiles::PreferencesController < Profiles::ApplicationController
 
   def update
     begin
-      if @user.update_attributes(preferences_params)
+      result = Users::UpdateService.new(user, preferences_params).execute
+
+      if result[:status] == :success
         flash[:notice] = 'Preferences saved.'
       else
         flash[:alert] = 'Failed to save preferences.'

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
 
-    current_user.save! if current_user.changed?
+    Users::UpdateService.new(current_user).execute!
 
     if two_factor_authentication_required? && !current_user.two_factor_enabled?
       two_factor_authentication_reason(
@@ -41,9 +41,9 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
 
   def create
     if current_user.validate_and_consume_otp!(params[:pin_code])
-      current_user.otp_required_for_login = true
-      @codes = current_user.generate_otp_backup_codes!
-      current_user.save!
+      Users::UpdateService.new(current_user, otp_required_for_login: true).execute! do |user|
+        @codes = user.generate_otp_backup_codes!
+      end
 
       render 'create'
     else
@@ -70,8 +70,9 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
   end
 
   def codes
-    @codes = current_user.generate_otp_backup_codes!
-    current_user.save!
+    Users::UpdateService.new(current_user).execute! do |user|
+      @codes = user.generate_otp_backup_codes!
+    end
   end
 
   def destroy

app/controllers/profiles_controller.rb

@@ -12,39 +12,47 @@ class ProfilesController < Profiles::ApplicationController
     user_params.except!(:email) if @user.external_email?
 
     respond_to do |format|
-      if @user.update_attributes(user_params)
+      result = Users::UpdateService.new(@user, user_params).execute
+
+      if result[:status] == :success
         message = "Profile was successfully updated"
+
         format.html { redirect_back_or_default(default: { action: 'show' }, options: { notice: message }) }
         format.json { render json: { message: message } }
       else
-        message = @user.errors.full_messages.uniq.join('. ')
-        format.html { redirect_back_or_default(default: { action: 'show' }, options: { alert: "Failed to update profile. #{message}" }) }
-        format.json { render json: { message: message }, status: :unprocessable_entity }
+        format.html { redirect_back_or_default(default: { action: 'show' }, options: { alert: result[:message] }) }
+        format.json { render json: result }
       end
     end
   end
 
   def reset_private_token
-    if current_user.reset_authentication_token!
-      flash[:notice] = "Private token was successfully reset"
+    Users::UpdateService.new(@user).execute! do |user|
+      user.reset_authentication_token!
     end
 
+    flash[:notice] = "Private token was successfully reset"
+
     redirect_to profile_account_path
   end
 
   def reset_incoming_email_token
-    if current_user.reset_incoming_email_token!
-      flash[:notice] = "Incoming email token was successfully reset"
+    Users::UpdateService.new(@user).execute! do |user|
+      user.reset_incoming_email_token!
     end
 
+    flash[:notice] = "Incoming email token was successfully reset"
+
     redirect_to profile_account_path
   end
 
   def reset_rss_token
-    if current_user.reset_rss_token!
-      flash[:notice] = "RSS token was successfully reset"
+    Users::UpdateService.new(@user).execute! do |user|
+      user.reset_rss_token!
     end
 
+    flash[:notice] = "RSS token was successfully reset"
+
     redirect_to profile_account_path
   end
 
@@ -55,12 +63,13 @@ class ProfilesController < Profiles::ApplicationController
   end
 
   def update_username
-    if @user.update_attributes(username: user_params[:username])
-      options = { notice: "Username successfully changed" }
-    else
-      message = @user.errors.full_messages.uniq.join('. ')
-      options = { alert: "Username change failed - #{message}" }
-    end
+    result = Users::UpdateService.new(@user, username: user_params[:username]).execute
+
+    options = if result[:status] == :success
+                { notice: "Username successfully changed" }
+              else
+                { alert: "Username change failed - #{result[:message]}" }
+              end
 
     redirect_back_or_default(default: { action: 'show' }, options: options)
   end

app/controllers/sessions_controller.rb

@@ -60,10 +60,11 @@ class SessionsController < Devise::SessionsController
 
     return unless user && user.require_password?
 
-    token = user.generate_reset_token
-    user.save
+    Users::UpdateService.new(user).execute do |user|
+      @token = user.generate_reset_token
+    end
 
-    redirect_to edit_user_password_path(reset_password_token: token),
+    redirect_to edit_user_password_path(reset_password_token: @token),
       notice: "Please create a password for your new account."
   end
 

app/models/user.rb

@@ -53,7 +53,7 @@ class User < ActiveRecord::Base
     lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i)
     return unless lease.try_obtain
 
-    save(validate: false)
+    Users::UpdateService.new(self).execute(validate: false)
   end
 
   attr_accessor :force_random_password
@@ -494,10 +494,8 @@ class User < ActiveRecord::Base
   def update_emails_with_primary_email
     primary_email_record = emails.find_by(email: email)
     if primary_email_record
-      primary_email_record.destroy
-      emails.create(email: email_was)
-
-      update_secondary_emails!
+      Emails::DestroyService.new(self, email: email).execute
+      Emails::CreateService.new(self, email: email_was).execute
     end
   end
 
@@ -965,7 +963,7 @@ class User < ActiveRecord::Base
     if attempts_exceeded?
       lock_access! unless access_locked?
     else
-      save(validate: false)
+      Users::UpdateService.new(self).execute(validate: false)
     end
   end
 
@@ -1129,7 +1127,8 @@ class User < ActiveRecord::Base
       email: email,
       &creation_block
     )
-    user.save(validate: false)
+
+    Users::UpdateService.new(user).execute(validate: false)
     user
   ensure
     Gitlab::ExclusiveLease.cancel(lease_key, uuid)

app/services/emails/base_service.rb

+module Emails
+  class BaseService
+    def initialize(user, opts)
+      @user = user
+      @email = opts[:email]
+    end
+  end
+end

app/services/emails/create_service.rb

+module Emails
+  class CreateService < ::Emails::BaseService
+    def execute
+      @user.emails.create(email: @email)
+    end
+  end
+end

app/services/emails/destroy_service.rb

+module Emails
+  class DestroyService < ::Emails::BaseService
+    def execute
+      Email.find_by_email!(@email).destroy && update_secondary_emails!
+    end
+
+    private
+
+    def update_secondary_emails!
+      result = ::Users::UpdateService.new(@user).execute do |user|
+        user.update_secondary_emails!
+      end
+
+      result[:status] == 'success'
+    end
+  end
+end

app/services/users/build_service.rb

 module Users
-  # Service for building a new user.
   class BuildService < BaseService
     def initialize(current_user, params = {})
       @current_user = current_user

app/services/users/create_service.rb

 module Users
-  # Service for creating a new user.
   class CreateService < BaseService
     def initialize(current_user, params = {})
       @current_user = current_user

app/services/users/update_service.rb

+module Users
+  class UpdateService < BaseService
+    def initialize(user, params = {})
+      @user = user
+      @params = params.dup
+    end
+
+    def execute(validate: true, &block)
+      yield(@user) if block_given?
+
+      assign_attributes(&block)
+
+      if @user.save(validate: validate)
+        success
+      else
+        error(@user.errors.full_messages.uniq.join('. '))
+      end
+    end
+
+    def execute!(*args, &block)
+      result = execute(*args, &block)
+
+      raise ActiveRecord::RecordInvalid.new(@user) unless result[:status] == :success
+
+      true
+    end
+
+    private
+
+    def assign_attributes(&block)
+      @user.assign_attributes(params) if params.any?
+    end
+  end
+end

lib/api/internal.rb

@@ -132,8 +132,11 @@ module API
           return { success: false, message: 'Two-factor authentication is not enabled for this user' }
         end
 
-        codes = user.generate_otp_backup_codes!
-        user.save!
+        codes = nil
+
+        ::Users::UpdateService.new(user).execute! do |user|
+          codes = user.generate_otp_backup_codes!
+        end
 
         { success: true, recovery_codes: codes }
       end

lib/api/notification_settings.rb

@@ -34,7 +34,10 @@ module API
           notification_setting.transaction do
             new_notification_email = params.delete(:notification_email)
 
-            current_user.update(notification_email: new_notification_email) if new_notification_email
+            if new_notification_email
+              ::Users::UpdateService.new(current_user, notification_email: new_notification_email).execute
+            end
+
             notification_setting.update(declared_params(include_missing: false))
           end
         rescue ArgumentError => e # catch level enum error

lib/api/users.rb

@@ -98,7 +98,7 @@ module API
         authenticated_as_admin!
 
         params = declared_params(include_missing: false)
-        user = ::Users::CreateService.new(current_user, params).execute
+        user = ::Users::CreateService.new(current_user, params).execute(skip_authorization: true)
 
         if user.persisted?
           present user, with: Entities::UserPublic
@@ -156,7 +156,9 @@ module API
 
         user_params[:password_expires_at] = Time.now if user_params[:password].present?
 
-        if user.update_attributes(user_params.except(:extern_uid, :provider))
+        result = ::Users::UpdateService.new(user, user_params.except(:extern_uid, :provider)).execute
+
+        if result[:status] == :success
           present user, with: Entities::UserPublic
         else
           render_validation_error!(user)
@@ -234,9 +236,9 @@ module API
         user = User.find_by(id: params.delete(:id))
         not_found!('User') unless user
 
-        email = user.emails.new(declared_params(include_missing: false))
+        email = Emails::CreateService.new(user, declared_params(include_missing: false)).execute
 
-        if email.save
+        if email.errors.blank?
           NotificationService.new.new_email(email)
           present email, with: Entities::Email
         else
@@ -274,8 +276,7 @@ module API
         email = user.emails.find_by(id: params[:email_id])
         not_found!('Email') unless email
 
-        email.destroy
-        user.update_secondary_emails!
+        Emails::DestroyService.new(user, email: email.email).execute
       end
 
       desc 'Delete a user. Available only for admins.' do
@@ -487,9 +488,9 @@ module API
         requires :email, type: String, desc: 'The new email'
       end
       post "emails" do
-        email = current_user.emails.new(declared_params)
+        email = Emails::CreateService.new(current_user, declared_params).execute
 
-        if email.save
+        if email.errors.blank?
           NotificationService.new.new_email(email)
           present email, with: Entities::Email
         else
@@ -505,8 +506,7 @@ module API
         email = current_user.emails.find_by(id: params[:email_id])
         not_found!('Email') unless email
 
-        email.destroy
-        current_user.update_secondary_emails!
+        Emails::DestroyService.new(current_user, email: email.email).execute
       end
 
       desc 'Get a list of user activities'

lib/gitlab/ldap/access.rb

@@ -16,8 +16,8 @@ module Gitlab
       def self.allowed?(user)
         self.open(user) do |access|
           if access.allowed?
-            user.last_credential_check_at = Time.now
-            user.save
+            Users::UpdateService.new(user, last_credential_check_a: Time.now).execute
+
             true
           else
             false

lib/gitlab/o_auth/user.rb

@@ -32,7 +32,7 @@ module Gitlab
 
         block_after_save = needs_blocking?
 
-        gl_user.save!
+        Users::UpdateService.new(gl_user).execute!
 
         gl_user.block if block_after_save
 

spec/controllers/profiles/preferences_controller_spec.rb

@@ -43,7 +43,8 @@ describe Profiles::PreferencesController do
           dashboard: 'stars'
         }.with_indifferent_access
 
-        expect(user).to receive(:update_attributes).with(prefs)
+        expect(user).to receive(:assign_attributes).with(prefs)
+        expect(user).to receive(:save)
 
         go params: prefs
       end
@@ -51,7 +52,7 @@ describe Profiles::PreferencesController do
 
     context 'on failed update' do
       it 'sets the flash' do
-        expect(user).to receive(:update_attributes).and_return(false)
+        expect(user).to receive(:save).and_return(false)
 
         go
 

spec/features/profiles/password_spec.rb

@@ -25,7 +25,7 @@ describe 'Profile > Password', feature: true do
         end
       end
 
-      it 'does not contains the current password field after an error' do
+      it 'does not contain the current password field after an error' do
         fill_passwords('mypassword', 'mypassword2')
 
         expect(page).to have_no_field('user[current_password]')

spec/requests/api/users_spec.rb

@@ -364,6 +364,7 @@ describe API::Users do
 
     it "updates user with new bio" do
       put api("/users/#{user.id}", admin), { bio: 'new test bio' }
+
       expect(response).to have_http_status(200)
       expect(json_response['bio']).to eq('new test bio')
       expect(user.reload.bio).to eq('new test bio')
@@ -396,13 +397,22 @@ describe API::Users do
 
     it 'updates user with his own email' do
       put api("/users/#{user.id}", admin), email: user.email
+
       expect(response).to have_http_status(200)
       expect(json_response['email']).to eq(user.email)
       expect(user.reload.email).to eq(user.email)
     end
 
+    it 'updates user with a new email' do
+      put api("/users/#{user.id}", admin), email: 'new@email.com'
+
+      expect(response).to have_http_status(200)
+      expect(user.reload.notification_email).to eq('new@email.com')
+    end
+
     it 'updates user with his own username' do
       put api("/users/#{user.id}", admin), username: user.username
+
       expect(response).to have_http_status(200)
       expect(json_response['username']).to eq(user.username)
       expect(user.reload.username).to eq(user.username)
@@ -410,12 +420,14 @@ describe API::Users do
 
     it "updates user's existing identity" do
       put api("/users/#{omniauth_user.id}", admin), provider: 'ldapmain', extern_uid: '654321'
+
       expect(response).to have_http_status(200)
       expect(omniauth_user.reload.identities.first.extern_uid).to eq('654321')
     end
 
     it 'updates user with new identity' do
       put api("/users/#{user.id}", admin), provider: 'github', extern_uid: 'john'
+
       expect(response).to have_http_status(200)
       expect(user.reload.identities.first.extern_uid).to eq('john')
       expect(user.reload.identities.first.provider).to eq('github')
@@ -423,12 +435,14 @@ describe API::Users do
 
     it "updates admin status" do
       put api("/users/#{user.id}", admin), { admin: true }
+
       expect(response).to have_http_status(200)
       expect(user.reload.admin).to eq(true)
     end
 
     it "updates external status" do
       put api("/users/#{user.id}", admin), { external: true }
+
       expect(response.status).to eq 200
       expect(json_response['external']).to eq(true)
       expect(user.reload.external?).to be_truthy
@@ -436,6 +450,7 @@ describe API::Users do
 
     it "does not update admin status" do
       put api("/users/#{admin_user.id}", admin), { can_create_group: false }
+
       expect(response).to have_http_status(200)
       expect(admin_user.reload.admin).to eq(true)
       expect(admin_user.can_create_group).to eq(false)
@@ -443,6 +458,7 @@ describe API::Users do
 
     it "does not allow invalid update" do
       put api("/users/#{user.id}", admin), { email: 'invalid email' }
+
       expect(response).to have_http_status(400)
       expect(user.reload.email).not_to eq('invalid email')
     end
@@ -459,6 +475,7 @@ describe API::Users do
 
     it "returns 404 for non-existing user" do
       put api("/users/999999", admin), { bio: 'update should fail' }
+
       expect(response).to have_http_status(404)
       expect(json_response['message']).to eq('404 User Not Found')
     end
@@ -509,6 +526,7 @@ describe API::Users do
 
       it 'returns 409 conflict error if email address exists' do
         put api("/users/#{@user.id}", admin), email: 'test@example.com'
+
         expect(response).to have_http_status(409)
         expect(@user.reload.email).to eq(@user.email)
       end
@@ -516,6 +534,7 @@ describe API::Users do
       it 'returns 409 conflict error if username taken' do
         @user_id = User.all.last.id
         put api("/users/#{@user.id}", admin), username: 'test'
+
         expect(response).to have_http_status(409)
         expect(@user.reload.username).to eq(@user.username)
       end

spec/services/emails/create_service_spec.rb

+require 'spec_helper'
+
+describe Emails::CreateService, services: true do
+  let(:user) { create(:user) }
+  let(:opts) { { email: 'new@email.com' } }
+
+  subject(:service) { described_class.new(user, opts) }
+
+  describe '#execute' do
+    it 'creates an email with valid attributes' do
+      expect { service.execute }.to change { Email.count }.by(1)
+      expect(Email.where(opts)).not_to be_empty
+    end
+
+    it 'has the right user association' do
+      service.execute
+
+      expect(user.emails).to eq(Email.where(opts))
+    end
+  end
+end

spec/services/emails/destroy_service_spec.rb

+require 'spec_helper'
+
+describe Emails::DestroyService, services: true do
+  let!(:user) { create(:user) }
+  let!(:email) { create(:email, user: user) }
+
+  subject(:service) { described_class.new(user, email: email.email) }
+
+  describe '#execute' do
+    it 'removes an email' do
+      expect { service.execute }.to change { user.emails.count }.by(-1)
+    end
+  end
+end

spec/services/users/update_service_spec.rb

+require 'spec_helper'
+
+describe Users::UpdateService, services: true do
+  let(:user) { create(:user) }
+
+  describe '#execute' do
+    it 'updates the name' do
+      result = update_user(user, name: 'New Name')
+
+      expect(result).to eq(status: :success)
+      expect(user.name).to eq('New Name')
+    end
+
+    it 'returns an error result when record cannot be updated' do
+      expect do
+        update_user(user, { email: 'invalid' })
+      end.not_to change { user.reload.email }
+    end
+
+    def update_user(user, opts)
+      described_class.new(user, opts).execute
+    end
+  end
+
+  describe '#execute!' do
+    it 'updates the name' do
+      result = update_user(user, name: 'New Name')
+
+      expect(result).to be true
+      expect(user.name).to eq('New Name')
+    end
+
+    it 'raises an error when record cannot be updated' do
+      expect do
+        update_user(user, email: 'invalid')
+      end.to raise_error(ActiveRecord::RecordInvalid)
+    end
+
+    def update_user(user, opts)
+      described_class.new(user, opts).execute!
+    end
+  end
+end