update code based on feedback

7 files changed, 14 insertions, 15 deletions

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 3b90cd77be0..7b65836eef7 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -210,10 +210,8 @@ class Admin::UsersController < Admin::ApplicationController
     ]
   end
 
-  def update_user
-    result = Users::UpdateService.new(user).execute do |user|
-      yield(user)
-    end
+  def update_user(&block)
+    result = Users::UpdateService.new(user).execute(&block)
 
     result[:status] == :success
   end
diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb
index 1a4f77639e7..a864f86f3dd 100644
--- a/app/controllers/profiles/two_factor_auths_controller.rb
+++ b/app/controllers/profiles/two_factor_auths_controller.rb
@@ -41,8 +41,10 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
 
   def create
     if current_user.validate_and_consume_otp!(params[:pin_code])
+      codes = nil
+
       Users::UpdateService.new(current_user, otp_required_for_login: true).execute! do |user|
-        @codes = user.generate_otp_backup_codes!
+        codes = user.generate_otp_backup_codes!
       end
 
       render 'create'
diff --git a/app/services/users/build_service.rb b/app/services/users/build_service.rb
index 363135ef09b..ff234a3440f 100644
--- a/app/services/users/build_service.rb
+++ b/app/services/users/build_service.rb
@@ -1,5 +1,4 @@
 module Users
-  # Service for building a new user.
   class BuildService < BaseService
     def initialize(current_user, params = {})
       @current_user = current_user
diff --git a/app/services/users/create_service.rb b/app/services/users/create_service.rb
index e22f7225ae2..74abc017cea 100644
--- a/app/services/users/create_service.rb
+++ b/app/services/users/create_service.rb
@@ -1,5 +1,4 @@
 module Users
-  # Service for creating a new user.
   class CreateService < BaseService
     def initialize(current_user, params = {})
       @current_user = current_user
diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index 2037664f56a..dfbd6016c3f 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -1,5 +1,4 @@
 module Users
-  # Service for updating a user.
   class UpdateService < BaseService
     def initialize(user, params = {})
       @user = user
@@ -7,6 +6,8 @@ module Users
     end
 
     def execute(validate: true, &block)
+      yield(@user) if block_given?
+
       assign_attributes(&block)
 
       if @user.save(validate: validate)
@@ -27,8 +28,6 @@ module Users
     private
 
     def assign_attributes(&block)
-      yield(@user) if block_given?
-
       @user.assign_attributes(params) if params.any?
     end
   end
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 9b035808693..f1c79970ba4 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -132,11 +132,13 @@ module API
           return { success: false, message: 'Two-factor authentication is not enabled for this user' }
         end
 
+        codes = nil
+
         ::Users::UpdateService.new(user).execute! do |user|
-          @codes = user.generate_otp_backup_codes!
+          codes = user.generate_otp_backup_codes!
         end
 
-        { success: true, recovery_codes: @codes }
+        { success: true, recovery_codes: codes }
       end
 
       post "/notify_post_receive" do
diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb
index 6c33a232cb0..0b2f840c462 100644
--- a/spec/services/users/update_service_spec.rb
+++ b/spec/services/users/update_service_spec.rb
@@ -7,7 +7,7 @@ describe Users::UpdateService, services: true do
     it 'updates the name' do
       result = update_user(user, name: 'New Name')
 
-      expect(result).to eq({ status: :success })
+      expect(result).to eq(status: :success)
       expect(user.name).to eq('New Name')
     end
 
@@ -30,9 +30,9 @@ describe Users::UpdateService, services: true do
       expect(user.name).to eq('New Name')
     end
 
-    it 'returns an error result when record cannot be updated' do
+    it 'raises an error when record cannot be updated' do
       expect do
-        update_user(user, { email: 'invalid' })
+        update_user(user, email: 'invalid')
       end.to raise_error(ActiveRecord::RecordInvalid)
     end