Rephrase error message and add code review remarks

Add cr remarks

Add cr remarks

Add cr remarks

Add cr remarks

Add cr remarks

app/controllers/concerns/membership_actions.rb

@@ -22,7 +22,7 @@ module MembershipActions
       .new(current_user, update_params)
       .execute(member)
 
-    member = result.fetch(:member, nil)
+    member = result[:member]
 
     if result[:status] == :success
       if member.expires?

app/services/members/update_service.rb

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

ee/app/models/ee/group_member.rb

@@ -91,7 +91,7 @@ module EE
     end
 
     def email_does_not_match_any_allowed_domains(email)
-      _("email '%{email}' does not match the allowed domains of %{email_domains}" %
+      _("email '%{email}' does not match the allowed domain of %{email_domains}" %
         { email: email, email_domains: ::Gitlab::Utils.to_exclusive_sentence(group_allowed_email_domains.map(&:domain)) })
     end
 

ee/lib/ee/api/members.rb

@@ -23,12 +23,12 @@ module EE
               .new(current_user, { override: true })
               .execute(member, permission: :override)
 
-            updated_member = result.fetch(:member, nil)
+            updated_member = result[:member]
 
             if result[:status] == :success
               present_member(updated_member)
             else
-              render_validation_error!(member)
+              render_validation_error!(updated_member)
             end
           end
 
@@ -45,12 +45,12 @@ module EE
               .new(current_user, { override: false })
               .execute(member, permission: :override)
 
-            updated_member = result.fetch(:member, nil)
+            updated_member = result[:member]
 
             if result[:status] == :success
               present_member(updated_member)
             else
-              render_validation_error!(member)
+              render_validation_error!(updated_member)
             end
           end
 

ee/spec/controllers/groups/group_members_controller_spec.rb

@@ -89,7 +89,6 @@ RSpec.describe Groups::GroupMembersController do
         context 'for a user with an un-verified email belonging to a domain different from the allowed domain' do
           let(:email) { 'test@gmail.com' }
 
-          context 'for a user with an email belonging to the allowed domain' do
           it 'returns error status' do
             subject
 
@@ -99,8 +98,7 @@ RSpec.describe Groups::GroupMembersController do
           it 'returns error message' do
             subject
 
-              expect(json_response).to eq({ 'message' => "User email 'test@gmail.com' does not match the allowed domains of gitlab.com" })
-            end
+            expect(json_response).to eq({ 'message' => "User email 'test@gmail.com' does not match the allowed domain of gitlab.com" })
           end
         end
       end

lib/api/members.rb

@@ -142,12 +142,12 @@ module API
             .new(current_user, declared_params(include_missing: false))
             .execute(member)
 
-          updated_member = result.fetch(:member, nil)
+          updated_member = result[:member]
 
           if result[:status] == :success
             present_members updated_member
           else
-            render_validation_error!(member)
+            render_validation_error!(updated_member)
           end
         end
         # rubocop: enable CodeReuse/ActiveRecord

locale/gitlab.pot

@@ -33508,7 +33508,7 @@ msgstr ""
 msgid "element is not a hierarchy"
 msgstr ""
 
-msgid "email '%{email}' does not match the allowed domains of %{email_domains}"
+msgid "email '%{email}' does not match the allowed domain of %{email_domains}"
 msgstr ""
 
 msgid "email '%{email}' is not a verified email."

spec/services/members/update_service_spec.rb

@@ -13,9 +13,11 @@ RSpec.describe Members::UpdateService do
     { access_level: Gitlab::Access::MAINTAINER }
   end
 
+  subject { described_class.new(current_user, params).execute(member, permission: permission) }
+
   shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
     it 'raises Gitlab::Access::AccessDeniedError' do
-      expect { described_class.new(current_user, params).execute(member, permission: permission) }
+      expect { subject }
         .to raise_error(Gitlab::Access::AccessDeniedError)
     end
   end
@@ -24,14 +26,14 @@ 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).fetch(:member)
+      updated_member = subject.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)
+      result = subject.fetch(:status)
 
       expect(result).to eq(:success)
     end
@@ -41,7 +43,7 @@ RSpec.describe Members::UpdateService 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).fetch(:member)
+          updated_member = subject.fetch(:member)
 
           expect(updated_member).to be_valid
           expect(updated_member.access_level).to eq(Gitlab::Access::GUEST)
@@ -73,7 +75,7 @@ RSpec.describe Members::UpdateService do
       let(:params) { { expires_at: 2.days.ago } }
 
       it 'returns error status' do
-        result = described_class.new(current_user, params).execute(member, permission: permission)
+        result = subject
 
         expect(result[:status]).to eq(:error)
       end