Refactor code based on feedback

ee/lib/api/scim.rb

@@ -61,7 +61,7 @@ module API
             scim_error!(message: 'Email has already been taken') if email_taken?(parsed_hash[:email], identity)
 
             result = ::Users::UpdateService.new(identity.user,
-                                                parsed_hash.except(:extern_uid, :provider)
+                                                parsed_hash.except(:extern_uid)
                                                   .merge(user: identity.user)).execute
 
             result[:status] == :success

ee/lib/ee/gitlab/scim/error.rb

@@ -3,7 +3,19 @@
 module EE
   module Gitlab
     module Scim
-      class Error < EE::Gitlab::Scim::NotFound
+      class Error < Grape::Entity
+        expose :schemas
+        expose :detail, safe: true
+        expose :status
+
+        private
+
+        DEFAULT_SCHEMA = 'urn:ietf:params:scim:api:messages:2.0:Error'
+
+        def schemas
+          [DEFAULT_SCHEMA]
+        end
+
         def status
           409
         end

ee/lib/ee/gitlab/scim/not_found.rb

@@ -3,19 +3,7 @@
 module EE
   module Gitlab
     module Scim
-      class NotFound < Grape::Entity
-        expose :schemas
-        expose :detail, safe: true
-        expose :status
-
-        private
-
-        DEFAULT_SCHEMA = 'urn:ietf:params:scim:api:messages:2.0:Error'
-
-        def schemas
-          [DEFAULT_SCHEMA]
-        end
-
+      class NotFound < EE::Gitlab::Scim::Error
         def status
           404
         end

ee/lib/ee/gitlab/scim/params_parser.rb

@@ -25,11 +25,11 @@ module EE
         end
 
         def deprovision_user?
-          hash[:active] == false
+          data[:active] == false
         end
 
         def to_hash
-          @hash ||=
+          @data ||=
             begin
               hash = {}
 
@@ -40,8 +40,8 @@ module EE
             end
         end
 
-        alias_method :hash, :to_hash
-        private :hash
+        alias_method :data, :to_hash
+        private :data
 
         private
 

ee/lib/ee/gitlab/scim/users.rb

@@ -35,7 +35,7 @@ module EE
 
         # We only support a single resource at the moment
         def resources
-          object.present? ? [object] : []
+          [object].reject(&:empty?)
         end
       end
     end

ee/spec/requests/api/scim_spec.rb

@@ -105,6 +105,46 @@ describe API::Scim do
         it 'updates the name' do
           expect(user.reload.name).to eq('new_name')
         end
+
+        it 'responds with an empty response' do
+          expect(json_response).to eq({})
+        end
+      end
+
+      context 'email' do
+        context 'non existent email' do
+          before do
+            params = { Operations: [{ 'op': 'Replace', 'path': 'emails[type eq "work"].value', 'value': 'new@mail.com' }] }.to_query
+
+            patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}")
+          end
+
+          it 'updates the email' do
+            expect(user.reload.unconfirmed_email).to eq('new@mail.com')
+          end
+
+          it 'responds with 204' do
+            expect(response).to have_gitlab_http_status(204)
+          end
+        end
+
+        context 'existent email' do
+          before do
+            create(:user, email: 'new@mail.com')
+
+            params = { Operations: [{ 'op': 'Replace', 'path': 'emails[type eq "work"].value', 'value': 'new@mail.com' }] }.to_query
+
+            patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}")
+          end
+
+          it 'does not update a duplicated email' do
+            expect(user.reload.unconfirmed_email).not_to eq('new@mail.com')
+          end
+
+          it 'responds with 209' do
+            expect(response).to have_gitlab_http_status(409)
+          end
+        end
       end
 
       context 'Remove user' do
@@ -138,6 +178,10 @@ describe API::Scim do
       it 'removes the identity link' do
         expect { identity.reload }.to raise_error(ActiveRecord::RecordNotFound)
       end
+
+      it 'responds with an empty response' do
+        expect(json_response).to eq({})
+      end
     end
 
     it 'responds with 404 if there is no user' do