more refactoring based on feedback
This commit is contained in:
parent
785cbb79e2
commit
c9fd3dc42c
|
@ -152,10 +152,10 @@ class Admin::UsersController < Admin::ApplicationController
|
|||
|
||||
def remove_email
|
||||
email = user.emails.find(params[:email_id])
|
||||
Emails::DestroyService.new(current_user, user, email: email.email).execute
|
||||
success = Emails::DestroyService.new(current_user, user, email: email.email).execute
|
||||
|
||||
respond_to do |format|
|
||||
if result[:status] == :success
|
||||
if success
|
||||
format.html { redirect_back_or_admin_user(notice: "Successfully removed email.") }
|
||||
format.json { head :ok }
|
||||
else
|
||||
|
|
|
@ -1,9 +1,10 @@
|
|||
class Profiles::AvatarsController < Profiles::ApplicationController
|
||||
def destroy
|
||||
@user = current_user
|
||||
@user.remove_avatar!
|
||||
|
||||
Users::UpdateService.new(@user, @user).execute
|
||||
Users::UpdateService.new(@user, @user).execute do |user|
|
||||
user.remove_avatar!
|
||||
end
|
||||
|
||||
redirect_to profile_path, status: 302
|
||||
end
|
||||
|
|
|
@ -41,9 +41,10 @@ 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!
|
||||
Users::UpdateService.new(current_user, current_user).execute!
|
||||
Users::UpdateService.new(current_user, current_user).execute! do |user|
|
||||
user.otp_required_for_login = true
|
||||
@codes = user.generate_otp_backup_codes!
|
||||
end
|
||||
|
||||
render 'create'
|
||||
else
|
||||
|
@ -70,8 +71,9 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
|
|||
end
|
||||
|
||||
def codes
|
||||
@codes = current_user.generate_otp_backup_codes!
|
||||
Users::UpdateService.new(current_user, current_user).execute!
|
||||
Users::UpdateService.new(current_user, current_user).execute! do |user|
|
||||
@codes = user.generate_otp_backup_codes!
|
||||
end
|
||||
end
|
||||
|
||||
def destroy
|
||||
|
|
|
@ -60,10 +60,11 @@ class SessionsController < Devise::SessionsController
|
|||
|
||||
return unless user && user.require_password?
|
||||
|
||||
token = user.generate_reset_token
|
||||
Users::UpdateService.new(user, user).execute
|
||||
Users::UpdateService.new(user, 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
|
||||
|
||||
|
|
|
@ -5,11 +5,5 @@ module Emails
|
|||
@user = user
|
||||
@email = opts[:email]
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def can_manage_emails?
|
||||
@current_user == @user || @current_user.admin?
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -1,8 +1,6 @@
|
|||
module Emails
|
||||
class CreateService < ::Emails::BaseService
|
||||
def execute(skip_authorization: false)
|
||||
raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_manage_emails?
|
||||
|
||||
def execute
|
||||
@user.emails.create(email: @email)
|
||||
end
|
||||
end
|
||||
|
|
|
@ -1,8 +1,6 @@
|
|||
module Emails
|
||||
class DestroyService < ::Emails::BaseService
|
||||
def execute(skip_authorization: false)
|
||||
raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_manage_emails?
|
||||
|
||||
def execute
|
||||
Email.find_by_email(@email).destroy && update_secondary_emails!
|
||||
end
|
||||
|
||||
|
|
|
@ -10,7 +10,7 @@ module Users
|
|||
def execute(skip_authorization: false, validate: true, &block)
|
||||
assign_attributes(skip_authorization, &block)
|
||||
|
||||
if @user.save(validate: validate) || !@user.changed? && @user.errors.empty?
|
||||
if @user.save(validate: validate) || @user.errors.empty?
|
||||
success
|
||||
else
|
||||
error(@user.errors.full_messages.uniq.join('. '))
|
||||
|
@ -18,9 +18,9 @@ module Users
|
|||
end
|
||||
|
||||
def execute!(skip_authorization: false, &block)
|
||||
assign_attributes(skip_authorization, &block)
|
||||
result = execute(*args, &block)
|
||||
|
||||
@user.save! if @user.changed?
|
||||
raise SomeCustomException(result[:message]) unless result[:status] == :success
|
||||
end
|
||||
|
||||
private
|
||||
|
|
|
@ -132,10 +132,11 @@ module API
|
|||
return { success: false, message: 'Two-factor authentication is not enabled for this user' }
|
||||
end
|
||||
|
||||
codes = user.generate_otp_backup_codes!
|
||||
::Users::UpdateService.new(user, user).execute!
|
||||
::Users::UpdateService.new(user, user).execute! do |user|
|
||||
@codes = user.generate_otp_backup_codes!
|
||||
end
|
||||
|
||||
{ success: true, recovery_codes: codes }
|
||||
{ success: true, recovery_codes: @codes }
|
||||
end
|
||||
|
||||
post "/notify_post_receive" do
|
||||
|
|
|
@ -16,8 +16,7 @@ module Gitlab
|
|||
def self.allowed?(user)
|
||||
self.open(user) do |access|
|
||||
if access.allowed?
|
||||
user.last_credential_check_at = Time.now
|
||||
Users::UpdateService.new(user, user).execute
|
||||
Users::UpdateService.new(user, user, last_credential_check_a: Time.now).execute
|
||||
|
||||
true
|
||||
else
|
||||
|
|
|
@ -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,6 +397,7 @@ 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)
|
||||
|
@ -403,12 +405,14 @@ describe API::Users do
|
|||
|
||||
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)
|
||||
|
@ -416,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')
|
||||
|
@ -429,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
|
||||
|
@ -442,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)
|
||||
|
@ -449,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
|
||||
|
@ -465,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
|
||||
|
@ -515,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
|
||||
|
@ -522,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
|
||||
|
|
Loading…
Reference in New Issue