Add error handling to update member service

And change all places accordingly

app/controllers/concerns/membership_actions.rb

@@ -18,18 +18,24 @@ module MembershipActions
   def update
     update_params = params.require(root_params_key).permit(:access_level, :expires_at)
     member = membershipable.members_and_requesters.find(params[:id])
-    member = Members::UpdateService
+    result = Members::UpdateService
       .new(current_user, update_params)
       .execute(member)
 
-    if member.expires?
-      render json: {
-        expires_in: helpers.distance_of_time_in_words_to_now(member.expires_at),
-        expires_soon: member.expires_soon?,
-        expires_at_formatted: member.expires_at.to_time.in_time_zone.to_s(:medium)
-      }
+    member = result.fetch(:member)
+
+    if result[:status] == :success
+      if member.expires?
+        render json: {
+          expires_in: helpers.distance_of_time_in_words_to_now(member.expires_at),
+          expires_soon: member.expires_soon?,
+          expires_at_formatted: member.expires_at.to_time.in_time_zone.to_s(:medium)
+        }
+      else
+        render json: {}
+      end
     else
-      render json: {}
+      render json: result[:message], status: :unprocessable_entity
     end
   end
 

app/services/members/update_service.rb

@@ -16,7 +16,9 @@ module Members
         enqueue_delete_todos(member) if downgrading_to_guest?
       end
 
-      member
+      return success(member: member) unless member.errors.any?
+
+      error(member.errors.full_messages.to_sentence)
     end
 
     private

ee/app/controllers/ee/groups/group_members_controller.rb

@@ -27,13 +27,15 @@ module EE
       # rubocop: disable CodeReuse/ActiveRecord
       def override
         member = membershipable_members.find_by!(id: params[:id])
-        updated_member = ::Members::UpdateService.new(current_user, override_params)
-          .execute(member, permission: :override)
 
-        if updated_member.valid?
+        result = ::Members::UpdateService.new(current_user, override_params).execute(member, permission: :override)
+
+        if result[:status] == :success
           respond_to do |format|
             format.js { head :ok }
           end
+        else
+          render json: result[:message], status: :unprocessable_entity
         end
       end
       # rubocop: enable CodeReuse/ActiveRecord

ee/lib/ee/api/members.rb

@@ -19,11 +19,17 @@ module EE
           post ":id/members/:user_id/override" do
             member = find_member(params)
 
-            updated_member = ::Members::UpdateService
+            result = ::Members::UpdateService
               .new(current_user, { override: true })
               .execute(member, permission: :override)
 
-            present_member(updated_member)
+            updated_member = result.fetch(:member, nil)
+
+            if result[:status] == :success
+              present_member(updated_member)
+            else
+              render_validation_error!(member)
+            end
           end
 
           desc 'Remove an LDAP group member access level override.' do
@@ -35,11 +41,17 @@ module EE
           delete ":id/members/:user_id/override" do
             member = find_member(params)
 
-            updated_member = ::Members::UpdateService
+            result = ::Members::UpdateService
               .new(current_user, { override: false })
               .execute(member, permission: :override)
 
-            present_member(updated_member)
+            updated_member = result.fetch(:member, nil)
+
+            if result[:status] == :success
+              present_member(updated_member)
+            else
+              render_validation_error!(member)
+            end
           end
 
           desc 'Gets a list of billable users of root group.' do

lib/api/members.rb

@@ -137,15 +137,17 @@ module API
           authorize_admin_source!(source_type, source)
 
           member = source_members(source).find_by!(user_id: params[:user_id])
-          updated_member =
-            ::Members::UpdateService
-              .new(current_user, declared_params(include_missing: false))
-              .execute(member)
 
-          if updated_member.valid?
+          result = ::Members::UpdateService
+            .new(current_user, declared_params(include_missing: false))
+            .execute(member)
+
+          updated_member = result.fetch(:member, nil)
+
+          if result[:status] == :success
             present_members updated_member
           else
-            render_validation_error!(updated_member)
+            render_validation_error!(member)
           end
         end
         # rubocop: enable CodeReuse/ActiveRecord

spec/services/members/update_service_spec.rb

@@ -24,18 +24,25 @@ RSpec.describe Members::UpdateService do
     it 'updates the member' do
       expect(TodosDestroyer::EntityLeaveWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name)
 
-      updated_member = described_class.new(current_user, params).execute(member, permission: permission)
+      updated_member = described_class.new(current_user, params).execute(member, permission: permission).fetch(:member)
 
       expect(updated_member).to be_valid
       expect(updated_member.access_level).to eq(Gitlab::Access::MAINTAINER)
     end
 
+    it 'returns success status' do
+      result = described_class.new(current_user, params).execute(member, permission: permission).fetch(:status)
+
+      expect(result).to eq(:success)
+    end
+
+
     context 'when member is downgraded to guest' do
       shared_examples 'schedules to delete confidential todos' do
         it do
           expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once
 
-          updated_member = described_class.new(current_user, params).execute(member, permission: permission)
+          updated_member = described_class.new(current_user, params).execute(member, permission: permission).fetch(:member)
 
           expect(updated_member).to be_valid
           expect(updated_member.access_level).to eq(Gitlab::Access::GUEST)
@@ -62,6 +69,16 @@ RSpec.describe Members::UpdateService do
         expect { described_class.new(current_user, params) }.to raise_error(ArgumentError, 'invalid value for Integer(): "invalid"')
       end
     end
+
+    context 'when member is not valid' do
+      let(:params) { { expires_at: 2.days.ago } }
+
+      it 'returns error status' do
+        result = described_class.new(current_user, params).execute(member, permission: permission)
+
+        expect(result[:status]).to eq(:error)
+      end
+    end
   end
 
   before do