Merge branch 'fix-19538' into 'master'

Fix renaming repository when name contains invalid chars under settings

Fixes #19538

See merge request !5490

7 files changed, 69 insertions, 3 deletions

diff --git a/CHANGELOG b/CHANGELOG
index df54682dd10..a4dbce95268 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -10,6 +10,7 @@ v 8.11.0 (unreleased)
   - Clean up unused routes (Josef Strzibny)
   - Add green outline to New Branch button. !5447 (winniehell)
   - Retrieve rendered HTML from cache in one request
+  - Fix renaming repository when name contains invalid chararacters under project settings
   - Nokogiri's various parsing methods are now instrumented
   - Add build event color in HipChat messages (David Eisner)
   - Make fork counter always clickable. !5463 (winniehell)
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 f06311511cc..921ca6748d3 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -3,7 +3,7 @@ module Projects
     def execute
       # check that user is allowed to set specified visibility_level
       new_visibility = params[:visibility_level]
-      
+
       if new_visibility && new_visibility.to_i != project.visibility_level
         unless can?(current_user, :change_visibility_level, project) &&
           Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml
index 921155e970b..b282aa52b25 100644
--- a/app/views/projects/edit.html.haml
+++ b/app/views/projects/edit.html.haml
@@ -4,6 +4,7 @@
       %h4.prepend-top-0
         Project settings
     .col-lg-9
+      .project-edit-errors
       = form_for [@project.namespace.becomes(Namespace), @project], remote: true, html: { multipart: true, class: "edit-project" }, authenticity_token: true do |f|
         %fieldset.append-bottom-0
           .form-group
@@ -190,6 +191,7 @@
       %h4.prepend-top-0.warning-title
         Rename repository
     .col-lg-9
+      = render 'projects/errors'
       = form_for([@project.namespace.becomes(Namespace), @project]) do |f|
         .form-group.project_name_holder
           = f.label :name, class: 'label-light' do
diff --git a/app/views/projects/update.js.haml b/app/views/projects/update.js.haml
index 7d9bd08385a..dcf1f767bf7 100644
--- a/app/views/projects/update.js.haml
+++ b/app/views/projects/update.js.haml
@@ -6,4 +6,4 @@
     $(".project-edit-errors").html("#{escape_javascript(render('errors'))}");
     $('.save-project-loader').hide();
     $('.project-edit-container').show();
-    $('.project-edit-content .btn-save').enable();
+    $('.edit-project .btn-save').enable();
diff --git a/spec/features/projects/project_settings_spec.rb b/spec/features/projects/project_settings_spec.rb
new file mode 100644
index 00000000000..3de25d7af7d
--- /dev/null
+++ b/spec/features/projects/project_settings_spec.rb
@@ -0,0 +1,41 @@
+require 'spec_helper'
+
+describe 'Edit Project Settings', feature: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:empty_project, path: 'gitlab', name: 'sample') }
+
+  before do
+    login_as(user)
+    project.team << [user, :master]
+  end
+
+  describe 'Project settings', js: true do
+    it 'shows errors for invalid project name' do
+      visit edit_namespace_project_path(project.namespace, project)
+
+      fill_in 'project_name_edit', with: 'foo&bar'
+
+      click_button 'Save changes'
+
+      expect(page).to have_field 'project_name_edit', 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_button 'Save changes'
+    end
+  end
+
+  describe 'Rename repository' do
+    it 'shows errors for invalid project path/name' do
+      visit edit_namespace_project_path(project.namespace, project)
+
+      fill_in 'Project name', with: 'foo&bar'
+      fill_in 'Path', with: 'foo&bar'
+
+      click_button 'Rename project'
+
+      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
+  end
+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