From 9621dd0c9d31508bdac2e2e226537302b560ef10 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 20 Sep 2017 11:00:06 +0200 Subject: refactor services to match EE signature --- app/controllers/admin/users_controller.rb | 6 +++--- app/controllers/profiles/avatars_controller.rb | 2 +- app/controllers/profiles/emails_controller.rb | 4 ++-- app/controllers/profiles/notifications_controller.rb | 2 +- app/controllers/profiles/passwords_controller.rb | 6 +++--- app/controllers/profiles/preferences_controller.rb | 2 +- app/controllers/profiles/two_factor_auths_controller.rb | 6 +++--- app/controllers/profiles_controller.rb | 10 +++++----- app/controllers/sessions_controller.rb | 2 +- app/models/user.rb | 10 +++++----- app/services/emails/destroy_service.rb | 4 ++-- app/services/users/update_service.rb | 12 +++++++----- lib/api/internal.rb | 2 +- lib/api/notification_settings.rb | 2 +- lib/api/users.rb | 10 +++++----- lib/gitlab/ldap/access.rb | 2 +- lib/gitlab/o_auth/user.rb | 2 +- 17 files changed, 43 insertions(+), 41 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index cbcef70e957..8b6119548ee 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, user_params_with_pass).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, 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).execute(&block) result[:status] == :success end diff --git a/app/controllers/profiles/avatars_controller.rb b/app/controllers/profiles/avatars_controller.rb index 408650aac54..5a94dc88455 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).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..8328d230276 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, current_user, email_params).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, 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/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb index 960b7512602..45d7bca27bb 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, current_user, user_params).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..08b438de9e2 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, @user, password_attributes).execute if result[:status] == :success - Users::UpdateService.new(@user, password_expires_at: nil).execute + Users::UpdateService.new(current_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, @user, password_attributes).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..a13b9a616cb 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, user, preferences_params).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..b590846257b 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, 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, 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, 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..9e85997cf6d 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, user_params).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).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).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).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, 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..a0bd10d9726 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).execute do |user| @token = user.generate_reset_token end diff --git a/app/models/user.rb b/app/models/user.rb index 09c9b3250eb..e9a3aea44c4 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, 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, self, email: email).execute + Emails::CreateService.new(self, 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, 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).execute(validate: false) user ensure Gitlab::ExclusiveLease.cancel(lease_key, uuid) diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb index d586b9dfe0c..02dd803fca6 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).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..fddeeb9d52a 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -12,12 +12,8 @@ module Users assign_attributes(&block) - user_exists = @user.persisted? - if @user.save(validate: validate) - notify_new_user(@user, nil) unless user_exists - - success + notify_success else error(@user.errors.full_messages.uniq.join('. ')) end @@ -33,6 +29,12 @@ module Users private + def notify_success + notify_new_user(@user, nil) unless @user.persisted? + + 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..3d331ffdce2 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).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..1c5592d952d 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, 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..b157940eaa6 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, user_params.except(:extern_uid, :provider)).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, user, declared_params(include_missing: false)).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, 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, current_user, declared_params).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, 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..4e75729ca73 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, 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..eb32e41c995 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, gl_user).execute! gl_user.block if block_after_save -- cgit v1.2.1 From 7188975cb54a2b6902544340a91f94aa1fa82d3c Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 20 Sep 2017 12:00:33 +0200 Subject: update initializers --- app/services/emails/base_service.rb | 3 ++- app/services/users/update_service.rb | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb index ace49889097..8810f6d8803 100644 --- a/app/services/emails/base_service.rb +++ b/app/services/emails/base_service.rb @@ -1,6 +1,7 @@ module Emails class BaseService - def initialize(user, opts) + def initialize(current_user, user, opts) + @current_user = current_user @user = user @email = opts[:email] end diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index fddeeb9d52a..05cb1655325 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -2,7 +2,8 @@ module Users class UpdateService < BaseService include NewUserNotifier - def initialize(user, params = {}) + def initialize(current_user, user, params = {}) + @current_user = current_user @user = user @params = params.dup end -- cgit v1.2.1 From faa95ba4fbeb93e6af19770f7aade54aa9d30304 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 20 Sep 2017 12:31:35 +0200 Subject: fix users update service --- app/services/users/update_service.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index 05cb1655325..9f013af38dc 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -11,10 +11,12 @@ module Users def execute(validate: true, &block) yield(@user) if block_given? + user_exists = @user.persisted? + assign_attributes(&block) if @user.save(validate: validate) - notify_success + notify_success(user_exists) else error(@user.errors.full_messages.uniq.join('. ')) end @@ -30,8 +32,8 @@ module Users private - def notify_success - notify_new_user(@user, nil) unless @user.persisted? + def notify_success(user_exists) + notify_new_user(@user, nil) unless user_exists success end -- cgit v1.2.1 From f2e9ef102713344938e425d68d1a017403a710e0 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 20 Sep 2017 13:36:00 +0200 Subject: fix specs --- spec/services/emails/create_service_spec.rb | 2 +- spec/services/emails/destroy_service_spec.rb | 2 +- spec/services/users/update_service_spec.rb | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb index 641d5538de8..1c8b81c222c 100644 --- a/spec/services/emails/create_service_spec.rb +++ b/spec/services/emails/create_service_spec.rb @@ -4,7 +4,7 @@ describe Emails::CreateService do let(:user) { create(:user) } let(:opts) { { email: 'new@email.com' } } - subject(:service) { described_class.new(user, opts) } + subject(:service) { described_class.new(user, user, opts) } describe '#execute' do it 'creates an email with valid attributes' do diff --git a/spec/services/emails/destroy_service_spec.rb b/spec/services/emails/destroy_service_spec.rb index 1f4294dd905..138c3ff33b0 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, 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..707f83b3359 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, user, opts).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, 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, 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, user, opts).execute! end end end -- cgit v1.2.1 From 4a6ec7c947b8ae5360b220ec8c6c91b2221d2091 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 21 Sep 2017 10:29:09 +0200 Subject: refactor some controllers to make them EE friendly --- app/controllers/admin/applications_controller.rb | 10 ++++++++-- app/controllers/confirmations_controller.rb | 6 +++++- app/controllers/oauth/applications_controller.rb | 10 ++++++++-- 3 files changed, 21 insertions(+), 5 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/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 -- cgit v1.2.1 From cbb90d8f84d1f79560066d8ea3c6346906e7da94 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 21 Sep 2017 10:55:24 +0200 Subject: refactor keys controller --- app/controllers/profiles/keys_controller.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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 -- cgit v1.2.1 From 67d06dee30379fc93be196e2cf509660d1661aea Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 27 Sep 2017 11:48:33 +0200 Subject: refactor users update service --- app/controllers/admin/users_controller.rb | 4 ++-- app/controllers/profiles/avatars_controller.rb | 2 +- app/controllers/profiles/notifications_controller.rb | 2 +- app/controllers/profiles/passwords_controller.rb | 6 +++--- app/controllers/profiles/preferences_controller.rb | 2 +- app/controllers/profiles/two_factor_auths_controller.rb | 6 +++--- app/controllers/profiles_controller.rb | 10 +++++----- app/controllers/sessions_controller.rb | 2 +- app/models/user.rb | 6 +++--- app/services/emails/destroy_service.rb | 2 +- app/services/users/update_service.rb | 4 ++-- lib/api/internal.rb | 2 +- lib/api/notification_settings.rb | 2 +- lib/api/users.rb | 2 +- lib/gitlab/ldap/access.rb | 2 +- lib/gitlab/o_auth/user.rb | 2 +- spec/services/users/update_service_spec.rb | 8 ++++---- 17 files changed, 32 insertions(+), 32 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 8b6119548ee..b25040382d8 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(current_user, 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 @@ -219,7 +219,7 @@ class Admin::UsersController < Admin::ApplicationController end def update_user(&block) - result = Users::UpdateService.new(current_user, user).execute(&block) + result = Users::UpdateService.new(current_user, user: user).execute(&block) result[:status] == :success end diff --git a/app/controllers/profiles/avatars_controller.rb b/app/controllers/profiles/avatars_controller.rb index 5a94dc88455..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(current_user, @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/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb index 45d7bca27bb..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, 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 08b438de9e2..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(current_user, @user, password_attributes).execute + result = Users::UpdateService.new(current_user, password_attributes.merge(user: @user)).execute if result[:status] == :success - Users::UpdateService.new(current_user, @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(current_user, @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 a13b9a616cb..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(current_user, 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 b590846257b..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, 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, 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, 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 9e85997cf6d..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(current_user, @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(current_user, @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(current_user, @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(current_user, @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(current_user, @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 a0bd10d9726..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(current_user, 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 e9a3aea44c4..94674528012 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, self).execute(validate: false) + Users::UpdateService.new(self, user: self).execute(validate: false) end attr_accessor :force_random_password @@ -1000,7 +1000,7 @@ class User < ActiveRecord::Base if attempts_exceeded? lock_access! unless access_locked? else - Users::UpdateService.new(self, 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, 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/destroy_service.rb b/app/services/emails/destroy_service.rb index 02dd803fca6..44011cc36c8 100644 --- a/app/services/emails/destroy_service.rb +++ b/app/services/emails/destroy_service.rb @@ -7,7 +7,7 @@ module Emails private def update_secondary_emails! - result = ::Users::UpdateService.new(@current_user, @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 9f013af38dc..e2e720a5703 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -2,9 +2,9 @@ module Users class UpdateService < BaseService include NewUserNotifier - def initialize(current_user, user, params = {}) + def initialize(current_user, params = {}) @current_user = current_user - @user = user + @user = params[:user] @params = params.dup end diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 3d331ffdce2..a0557a609ca 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -136,7 +136,7 @@ module API codes = nil - ::Users::UpdateService.new(current_user, 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 1c5592d952d..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, 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 b157940eaa6..ce5d75965d5 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(current_user, 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 diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 4e75729ca73..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, 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 eb32e41c995..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, gl_user).execute! + Users::UpdateService.new(gl_user, user: gl_user).execute! gl_user.block if block_after_save diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb index 707f83b3359..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, 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, 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, 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, user, opts).execute! + described_class.new(user, opts.merge(user: user)).execute! end end end -- cgit v1.2.1 From e07819cbf70511ad38c107dc593ec87606567cdc Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 27 Sep 2017 12:08:10 +0200 Subject: fix update service --- app/services/users/update_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index e2e720a5703..15ca1a55a5b 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -4,7 +4,7 @@ module Users def initialize(current_user, params = {}) @current_user = current_user - @user = params[:user] + @user = params.delete(:user) @params = params.dup end -- cgit v1.2.1 From 1dcb7111107ed5a6b6258613d804b8da56af8b35 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 27 Sep 2017 16:39:10 +0200 Subject: refactor emails service --- app/controllers/admin/users_controller.rb | 2 +- app/controllers/profiles/emails_controller.rb | 4 ++-- app/models/user.rb | 4 ++-- app/services/emails/base_service.rb | 4 ++-- lib/api/users.rb | 8 ++++---- spec/services/emails/create_service_spec.rb | 4 ++-- spec/services/emails/destroy_service_spec.rb | 2 +- 7 files changed, 14 insertions(+), 14 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index b25040382d8..676a7203c7d 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -155,7 +155,7 @@ class Admin::UsersController < Admin::ApplicationController def remove_email email = user.emails.find(params[:email_id]) - success = Emails::DestroyService.new(current_user, user, email: email.email).execute + success = Emails::DestroyService.new(current_user, user: user, email: email.email).execute respond_to do |format| if success diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb index 8328d230276..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, 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, 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/models/user.rb b/app/models/user.rb index 94674528012..103ac78783f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -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, self, email: email).execute - Emails::CreateService.new(self, 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 diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb index 8810f6d8803..7f591c89411 100644 --- a/app/services/emails/base_service.rb +++ b/app/services/emails/base_service.rb @@ -1,8 +1,8 @@ module Emails class BaseService - def initialize(current_user, user, opts) + def initialize(current_user, opts) @current_user = current_user - @user = user + @user = opts.delete(:user) @email = opts[:email] end end diff --git a/lib/api/users.rb b/lib/api/users.rb index ce5d75965d5..99024d1f0ad 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -326,7 +326,7 @@ module API user = User.find_by(id: params.delete(:id)) not_found!('User') unless user - email = Emails::CreateService.new(current_user, 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, 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, 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, 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/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb index 1c8b81c222c..75812c17309 100644 --- a/spec/services/emails/create_service_spec.rb +++ b/spec/services/emails/create_service_spec.rb @@ -2,9 +2,9 @@ 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, user, opts) } + subject(:service) { described_class.new(user, opts) } describe '#execute' do it 'creates an email with valid attributes' do diff --git a/spec/services/emails/destroy_service_spec.rb b/spec/services/emails/destroy_service_spec.rb index 138c3ff33b0..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, user, email: email.email) } + subject(:service) { described_class.new(user, user: user, email: email.email) } describe '#execute' do it 'removes an email' do -- cgit v1.2.1