Merge branch 'fix/persistent-import-data' into 'master'

Fix import_data being saved as a result of an invalid import_url

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/19479

Seem that in all cases the `import_url` was an empty string. The `import_url` is now validated correctly and no longer creates the `import_data`even if it fails validation. <= Should cover cases 1, 3, and 4 of https://gitlab.com/gitlab-org/gitlab-ce/issues/19479#note_13005276

Also, it now rescues from all exceptions when importing from a URL, so we make sure the `import_status` is failed after that. Covers case 2 of https://gitlab.com/gitlab-org/gitlab-ce/issues/19479#note_13005276

See merge request !5206

6 files changed, 40 insertions, 8 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 6a061a86abe..6b24b2c5b32 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -100,7 +100,10 @@ v 8.9.6
   - Overwrite Host and X-Forwarded-Host headers in NGINX !5213
   - Keeps issue number when importing from Gitlab.com
 
-v 8.9.6 (unreleased)
+v 8.9.7 (unreleased)
+  - Fix import_data wrongly saved as a result of an invalid import_url
+
+v 8.9.6
   - Fix importing of events under notes for GitLab projects
 
 v 8.9.5
diff --git a/app/models/project.rb b/app/models/project.rb
index a66b750cd48..e7b9835692d 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -162,7 +162,7 @@ class Project < ActiveRecord::Base
   validates :namespace, presence: true
   validates_uniqueness_of :name, scope: :namespace_id
   validates_uniqueness_of :path, scope: :namespace_id
-  validates :import_url, addressable_url: true, if: :external_import?
+  validates :import_url, addressable_url: true, if: :import_url
   validates :star_count, numericality: { greater_than_or_equal_to: 0 }
   validate :check_limit, on: :create
   validate :avatar_type,
@@ -464,8 +464,8 @@ class Project < ActiveRecord::Base
     return super(value) unless Gitlab::UrlSanitizer.valid?(value)
 
     import_url = Gitlab::UrlSanitizer.new(value)
-    create_or_update_import_data(credentials: import_url.credentials)
     super(import_url.sanitized_url)
+    create_or_update_import_data(credentials: import_url.credentials)
   end
 
   def import_url
@@ -477,7 +477,13 @@ class Project < ActiveRecord::Base
     end
   end
 
+  def valid_import_url?
+    valid? || errors.messages[:import_url].nil?
+  end
+
   def create_or_update_import_data(data: nil, credentials: nil)
+    return unless valid_import_url?
+
     project_import_data = import_data || build_import_data
     if data
       project_import_data.data ||= {}
diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb
index 163ebf26c84..cdad0426b02 100644
--- a/app/services/projects/import_service.rb
+++ b/app/services/projects/import_service.rb
@@ -43,7 +43,7 @@ module Projects
     def import_repository
       begin
         gitlab_shell.import_repository(project.repository_storage_path, project.path_with_namespace, project.import_url)
-      rescue Gitlab::Shell::Error => e
+      rescue => e
         raise Error,  "Error importing repository #{project.import_url} into #{project.path_with_namespace} - #{e.message}"
       end
     end
diff --git a/app/views/projects/new.html.haml b/app/views/projects/new.html.haml
index 9b00bdedc27..c72d0140bb9 100644
--- a/app/views/projects/new.html.haml
+++ b/app/views/projects/new.html.haml
@@ -154,4 +154,9 @@
       $('.import_gitlab_project').attr('disabled',true);
       $('.import_gitlab_project').attr('title', 'Project path required.');
     }
-  })
+  });
+
+  $('.import_git').click(function( event ) {
+    $projectImportUrl = $('#project_import_url')
+    $projectImportUrl.attr('disabled', !$projectImportUrl.attr('disabled'))
+  });
\ No newline at end of file
diff --git a/app/views/shared/_import_form.html.haml b/app/views/shared/_import_form.html.haml
index 627814bcfae..65a3a6bddab 100644
--- a/app/views/shared/_import_form.html.haml
+++ b/app/views/shared/_import_form.html.haml
@@ -2,7 +2,7 @@
   = f.label :import_url, class: 'control-label' do
     %span Git repository URL
   .col-sm-10
-    = f.text_field :import_url, class: 'form-control', placeholder: 'https://username:password@gitlab.company.com/group/project.git'
+    = f.text_field :import_url, class: 'form-control', placeholder: 'https://username:password@gitlab.company.com/group/project.git', disabled: true
 
     .well.prepend-top-20
       %ul
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index d2269854354..e842c58dd82 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -130,17 +130,35 @@ describe Project, models: true do
       end
     end
 
-    it 'should not allow an invalid URI as import_url' do
+    it 'does not allow an invalid URI as import_url' do
       project2 = build(:project, import_url: 'invalid://')
 
       expect(project2).not_to be_valid
     end
 
-    it 'should allow a valid URI as import_url' do
+    it 'does allow a valid URI as import_url' do
       project2 = build(:project, import_url: 'ssh://test@gitlab.com/project.git')
 
       expect(project2).to be_valid
     end
+
+    it 'does not allow to introduce an empty URI' do
+      project2 = build(:project, import_url: '')
+
+      expect(project2).not_to be_valid
+    end
+
+    it 'does not produce import data on an empty URI' do
+      project2 = build(:project, import_url: '')
+
+      expect(project2.import_data).to be_nil
+    end
+
+    it 'does not produce import data on an invalid URI' do
+      project2 = build(:project, import_url: 'test://')
+
+      expect(project2.import_data).to be_nil
+    end
   end
 
   describe 'default_scope' do