Fix Project#to_param to keep invalid project suitable for use in URLs

app/models/project.rb

@@ -586,8 +586,12 @@ class Project < ActiveRecord::Base
   end
 
   def to_param
+    if persisted? && errors.include?(:path)
+      path_was
+    else
       path
     end
+  end
 
   def to_reference(_from_project = nil)
     path_with_namespace

app/services/projects/update_service.rb

@@ -23,17 +23,7 @@ module Projects
         if project.previous_changes.include?('path')
           project.rename_repo
         end
-      else
-        restore_attributes
-        false
       end
     end
-
-    private
-
-    def restore_attributes
-      project.path = project.path_was if project.errors.include?(:path)
-      project.name = project.name_was if project.errors.include?(:name)
-    end
   end
 end

app/views/projects/update.js.haml

-- if @project.errors.blank?
+- if @project.valid?
   :plain
     location.href = "#{edit_namespace_project_path(@project.namespace, @project)}";
 - else

spec/features/projects/project_settings_spec.rb

@@ -32,8 +32,8 @@ describe 'Edit Project Settings', feature: true do
 
       click_button 'Rename project'
 
-      expect(page).to have_field 'Project name', with: 'sample'
-      expect(page).to have_field 'Path', with: 'gitlab'
+      expect(page).to have_field 'Project name', with: 'foo&bar'
+      expect(page).to have_field 'Path', with: 'foo&bar'
       expect(page).to have_content "Name can contain only letters, digits, '_', '.', dash and space. It must start with letter, digit or '_'."
       expect(page).to have_content "Path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'"
     end

spec/models/project_spec.rb

@@ -372,6 +372,24 @@ describe Project, models: true do
 
       it { expect(@project.to_param).to eq('gitlabhq') }
     end
+
+    context 'with invalid path' do
+      it 'returns previous path to keep project suitable for use in URLs when persisted' do
+        project = create(:empty_project, path: 'gitlab')
+        project.path = 'foo&bar'
+
+        expect(project).not_to be_valid
+        expect(project.to_param).to eq 'gitlab'
+      end
+
+      it 'returns current path when new record' do
+        project = build(:empty_project, path: 'gitlab')
+        project.path = 'foo&bar'
+
+        expect(project).not_to be_valid
+        expect(project.to_param).to eq 'foo&bar'
+      end
+    end
   end
 
   describe '#repository' do

spec/services/projects/update_service_spec.rb

@@ -139,27 +139,6 @@ describe Projects::UpdateService, services: true do
     end
   end
 
-  context 'for invalid project path/name' do
-    let(:user) { create(:user, admin: true) }
-    let(:project) { create(:empty_project, path: 'gitlab', name: 'sample') }
-    let(:params) { { path: 'foo&bar', name: 'foo&bar' } }
-
-    it 'resets to previous values to keep project in a valid state' do
-      update_project(project, user, params)
-
-      expect(project.path).to eq 'gitlab'
-      expect(project.name).to eq 'sample'
-    end
-
-    it 'keeps error messages' do
-      update_project(project, user, params)
-
-      expect(project.errors).not_to be_blank
-      expect(project.errors[:name]).to include("can contain only letters, digits, '_', '.', dash and space. It must start with letter, digit or '_'.")
-      expect(project.errors[:path]).to include("can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'")
-    end
-  end
-
   def update_project(project, user, opts)
     Projects::UpdateService.new(project, user, opts).execute
   end