Merge branch 'mk-fix-user-namespace-rename' into 'master'

Make username update fail if namespace update fails

Closes gitlab-com/support-forum#2316

See merge request !13642

app/models/user.rb

@@ -837,7 +837,12 @@ class User < ActiveRecord::Base
     create_namespace!(path: username, name: username) unless namespace
 
     if username_changed?
-      namespace.update_attributes(path: username, name: username)
+      unless namespace.update_attributes(path: username, name: username)
+        namespace.errors.each do |attribute, message|
+          self.errors.add(:"namespace_#{attribute}", message)
+        end
+        raise ActiveRecord::RecordInvalid.new(namespace)
+      end
     end
   end
 

changelogs/unreleased/mk-fix-user-namespace-rename.yml

+---
+title: Make username update fail if the namespace update fails
+merge_request: 13642
+author:
+type: fixed

spec/migrations/remove_dot_git_from_usernames_spec.rb

@@ -51,7 +51,6 @@ describe RemoveDotGitFromUsernames do
     namespace.path = path
     namespace.save!(validate: false)
 
-    user.username = path
-    user.save!(validate: false)
+    user.update_column(:username, path)
   end
 end

spec/models/user_spec.rb

@@ -2024,4 +2024,65 @@ describe User do
       expect(user.projects_limit_left).to eq(5)
     end
   end
+
+  describe '#ensure_namespace_correct' do
+    context 'for a new user' do
+      let(:user) { build(:user) }
+
+      it 'creates the namespace' do
+        expect(user.namespace).to be_nil
+        user.save!
+        expect(user.namespace).not_to be_nil
+      end
+    end
+
+    context 'for an existing user' do
+      let(:username) { 'foo' }
+      let(:user) { create(:user, username: username) }
+
+      context 'when the user is updated' do
+        context 'when the username is changed' do
+          let(:new_username) { 'bar' }
+
+          it 'changes the namespace (just to compare to when username is not changed)' do
+            expect do
+              user.update_attributes!(username: new_username)
+            end.to change { user.namespace.updated_at }
+          end
+
+          it 'updates the namespace name' do
+            user.update_attributes!(username: new_username)
+            expect(user.namespace.name).to eq(new_username)
+          end
+
+          it 'updates the namespace path' do
+            user.update_attributes!(username: new_username)
+            expect(user.namespace.path).to eq(new_username)
+          end
+
+          context 'when there is a validation error (namespace name taken) while updating namespace' do
+            let!(:conflicting_namespace) { create(:group, name: new_username, path: 'quz') }
+
+            it 'causes the user save to fail' do
+              expect(user.update_attributes(username: new_username)).to be_falsey
+              expect(user.namespace.errors.messages[:name].first).to eq('has already been taken')
+            end
+
+            it 'adds the namespace errors to the user' do
+              user.update_attributes(username: new_username)
+              expect(user.errors.full_messages.first).to eq('Namespace name has already been taken')
+            end
+          end
+        end
+
+        context 'when the username is not changed' do
+          it 'does not change the namespace' do
+            expect do
+              user.update_attributes!(email: 'asdf@asdf.com')
+            end.not_to change { user.namespace.updated_at }
+          end
+        end
+      end
+    end
+  end
 end

spec/services/users/update_service_spec.rb

@@ -12,9 +12,22 @@ describe Users::UpdateService do
     end
 
     it 'returns an error result when record cannot be updated' do
+      result = {}
       expect do
-        update_user(user, { email: 'invalid' })
+        result = update_user(user, { email: 'invalid' })
       end.not_to change { user.reload.email }
+      expect(result[:status]).to eq(:error)
+      expect(result[:message]).to eq('Email is invalid')
+    end
+
+    it 'includes namespace error messages' do
+      create(:group, name: 'taken', path: 'something_else')
+      result = {}
+      expect do
+        result = update_user(user, { username: 'taken' })
+      end.not_to change { user.reload.username }
+      expect(result[:status]).to eq(:error)
+      expect(result[:message]).to eq('Namespace name has already been taken')
     end
 
     def update_user(user, opts)