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

6 files changed, 26 insertions, 35 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 1616175709f..b3703d71e72 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -586,7 +586,11 @@ class Project < ActiveRecord::Base
   end
 
   def to_param
-    path
+    if persisted? && errors.include?(:path)
+      path_was
+    else
+      path
+    end
   end
 
   def to_reference(_from_project = nil)
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index 032c3b182fb..921ca6748d3 100644
--- a/app/services/projects/update_service.rb
+++ b/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
diff --git a/app/views/projects/update.js.haml b/app/views/projects/update.js.haml
index 938d44efe29..dcf1f767bf7 100644
--- a/app/views/projects/update.js.haml
+++ b/app/views/projects/update.js.haml
@@ -1,4 +1,4 @@
-- if @project.errors.blank?
+- if @project.valid?
   :plain
     location.href = "#{edit_namespace_project_path(@project.namespace, @project)}";
 - else
diff --git a/spec/features/projects/project_settings_spec.rb b/spec/features/projects/project_settings_spec.rb
index 5562680d6cc..3de25d7af7d 100644
--- a/spec/features/projects/project_settings_spec.rb
+++ b/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
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index c884bb31199..be5c53c44be 100644
--- a/spec/models/project_spec.rb
+++ b/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
diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb
index 6337daca9c7..e8b9e6b9238 100644
--- a/spec/services/projects/update_service_spec.rb
+++ b/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