Add code review remarks

Move spec to request spec

app/controllers/concerns/membership_actions.rb

@@ -24,16 +24,18 @@ module MembershipActions
 
     member = result[:member]
 
+    member_data = if member.expires?
+                    {
+                      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
+                    {}
+                  end
+
     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
+      render json: member_data
     else
       render json: { message: result[:message] }, status: :unprocessable_entity
     end

app/services/members/update_service.rb

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

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

@@ -30,12 +30,14 @@ module EE
 
         result = ::Members::UpdateService.new(current_user, override_params).execute(member, permission: :override)
 
-        if result[:status] == :success
-          respond_to do |format|
-            format.js { head :ok }
+        respond_to do |format|
+          format.js do
+            if result[:status] == :success
+              head :ok
+            else
+              render json: result[:message], status: :unprocessable_entity
+            end
           end
-        else
-          render json: result[:message], status: :unprocessable_entity
         end
       end
       # rubocop: enable CodeReuse/ActiveRecord

ee/app/models/ee/group_member.rb

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

ee/spec/controllers/groups/group_members_controller_spec.rb

@@ -57,53 +57,6 @@ RSpec.describe Groups::GroupMembersController do
       enable_external_authorization_service_check
     end
 
-    describe 'PUT #update' do
-      context 'when group has email domain feature enabled' do
-        let(:email) { 'test@gitlab.com' }
-        let(:member_user) { create(:user, email: email) }
-        let(:member) { group.add_guest(member_user) }
-
-        before do
-          stub_licensed_features(group_allowed_email_domains: true)
-          create(:allowed_email_domain, group: group)
-        end
-
-        subject do
-          put :update, xhr: true, params: {
-                                            group_member: {
-                                              access_level: 50
-                                            },
-                                            group_id: group,
-                                            id: member
-                                          }
-        end
-
-        context 'for a user with an email belonging to the allowed domain' do
-          it 'returns error status' do
-            subject
-
-            expect(response).to have_gitlab_http_status(:ok)
-          end
-        end
-
-        context 'for a user with an un-verified email belonging to a domain different from the allowed domain' do
-          let(:email) { 'test@gmail.com' }
-
-          it 'returns error status' do
-            subject
-
-            expect(response).to have_gitlab_http_status(:unprocessable_entity)
-          end
-
-          it 'returns error message' do
-            subject
-
-            expect(json_response).to eq({ 'message' => "User email 'test@gmail.com' does not match the allowed domain of gitlab.com" })
-          end
-        end
-      end
-    end
-
     describe 'POST #override' do
       let(:group) { create(:group_with_ldap_group_link) }
 

ee/spec/models/group_member_spec.rb

@@ -34,7 +34,7 @@ RSpec.describe GroupMember do
           group_member = build(:group_member, group: group, user: gmail_user)
 
           expect(group_member).to be_invalid
-          expect(group_member.errors[:user]).to include("email 'test@gmail.com' does not match the allowed domains of gitlab.com or acme.com")
+          expect(group_member.errors[:user]).to include("email 'test@gmail.com' does not match the allowed domains: gitlab.com, acme.com")
         end
 
         it 'shows proper error message for single domain limitation' do

ee/spec/requests/groups/group_members_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Groups::GroupMembersController do
+  include ExternalAuthorizationServiceHelpers
+
+  let(:user)  { create(:user) }
+  let(:group) { create(:group, :public) }
+  let(:membership) { create(:group_member, group: group) }
+
+  before do
+    group.add_owner(user)
+    sign_in(user)
+  end
+
+  describe 'PUT /groups/*group_id/-/group_members/:id' do
+    context 'when group has email domain feature enabled' do
+      let(:email) { 'test@gitlab.com' }
+      let(:member_user) { create(:user, email: email) }
+      let(:member) { group.add_guest(member_user) }
+
+      before do
+        stub_licensed_features(group_allowed_email_domains: true)
+        create(:allowed_email_domain, group: group)
+      end
+
+      subject do
+        put group_group_member_path(group_id: group, id: member), xhr: true, params: {
+                                          group_member: {
+                                            access_level: 50
+                                          }
+                                        }
+      end
+
+      context 'for a user with an email belonging to the allowed domain' do
+        it 'returns error status' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
+
+      context 'for a user with an un-verified email belonging to a domain different from the allowed domain' do
+        let(:email) { 'test@gmail.com' }
+
+        it 'returns error status' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        end
+
+        it 'returns error message' do
+          subject
+
+          expect(json_response).to eq({ 'message' => "User email 'test@gmail.com' does not match the allowed domain of gitlab.com" })
+        end
+      end
+    end
+  end
+end