From ad6ff2238045d6b7873144eb302aa953c9f1fc66 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Tue, 12 Jul 2016 16:21:28 +0200 Subject: [PATCH 1/8] fixes a few issues to do with import_url not being saved correctly for imports. This should prevent the import_data to be created when it should not and output an error properly validating before creating it. --- app/models/project.rb | 4 ++-- app/services/projects/import_service.rb | 2 +- spec/models/project_spec.rb | 22 ++++++++++++++++++++-- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index a66b750cd48..57ea948390b 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) if valid? end def import_url 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/spec/models/project_spec.rb b/spec/models/project_spec.rb index 5a27ccbab0a..d6bb8d0d54f 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 From 1d849129f73f72e3f8693f9f2a8267e228f2b9e2 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Tue, 12 Jul 2016 16:48:56 +0200 Subject: [PATCH 2/8] added changelog --- CHANGELOG | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index aed0d041ea6..2b68226a054 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -74,6 +74,9 @@ v 8.10.0 (unreleased) - Add reminder to not paste private SSH keys !4399 (Ingo Blechschmidt) - Remove duplicate `description` field in `MergeRequest` entities (Ben Boeckel) +v 8.9.7 (unreleased) + - Fix import_data wrongly saved as a result of an invalid import_url + v 8.9.6 (unreleased) - Fix importing of events under notes for GitLab projects From b2007676e7f88bf7bfa1bda14f6d9143f548183a Mon Sep 17 00:00:00 2001 From: James Lopez Date: Tue, 12 Jul 2016 14:52:52 +0000 Subject: [PATCH 3/8] Update CHANGELOG --- CHANGELOG | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 2b68226a054..523fd2d6ba3 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -77,7 +77,7 @@ v 8.10.0 (unreleased) v 8.9.7 (unreleased) - Fix import_data wrongly saved as a result of an invalid import_url -v 8.9.6 (unreleased) +v 8.9.6 - Fix importing of events under notes for GitLab projects v 8.9.5 From 9b234825e45884d24d78de81f5934ed765a27302 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 13 Jul 2016 09:26:40 +0200 Subject: [PATCH 4/8] fix specs --- app/models/project.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/project.rb b/app/models/project.rb index 57ea948390b..f85c6a65858 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -465,7 +465,7 @@ class Project < ActiveRecord::Base import_url = Gitlab::UrlSanitizer.new(value) super(import_url.sanitized_url) - create_or_update_import_data(credentials: import_url.credentials) if valid? + create_or_update_import_data(credentials: import_url.credentials) unless errors.messages[:import_url] end def import_url From c607b1c941fbfaca22dd78dae2fd67d873b56828 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 13 Jul 2016 10:40:03 +0200 Subject: [PATCH 5/8] fix more specs --- app/models/project.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/project.rb b/app/models/project.rb index f85c6a65858..3306fb86282 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -465,7 +465,7 @@ class Project < ActiveRecord::Base import_url = Gitlab::UrlSanitizer.new(value) super(import_url.sanitized_url) - create_or_update_import_data(credentials: import_url.credentials) unless errors.messages[:import_url] + create_or_update_import_data(credentials: import_url.credentials) if valid_import_url? end def import_url @@ -477,6 +477,10 @@ 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) project_import_data = import_data || build_import_data if data From dd63955032aeebfa76b774b008820879e7d37353 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 13 Jul 2016 13:57:43 +0200 Subject: [PATCH 6/8] updated create_or_update_import_data to use guard clause --- app/models/project.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/project.rb b/app/models/project.rb index 72c4f591420..7dc9f396e9e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -465,7 +465,7 @@ class Project < ActiveRecord::Base import_url = Gitlab::UrlSanitizer.new(value) super(import_url.sanitized_url) - create_or_update_import_data(credentials: import_url.credentials) if valid_import_url? + create_or_update_import_data(credentials: import_url.credentials) end def import_url @@ -482,6 +482,8 @@ class Project < ActiveRecord::Base 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 ||= {} From e2d6521d97289a6e3fbccf6948fe44dcb587a8d4 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 13 Jul 2016 17:01:24 +0200 Subject: [PATCH 7/8] some JS magic to fix empty URL bug --- app/views/projects/new.html.haml | 10 +++++++++- app/views/shared/_import_form.html.haml | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/app/views/projects/new.html.haml b/app/views/projects/new.html.haml index 9b00bdedc27..37f559c1bed 100644 --- a/app/views/projects/new.html.haml +++ b/app/views/projects/new.html.haml @@ -154,4 +154,12 @@ $('.import_gitlab_project').attr('disabled',true); $('.import_gitlab_project').attr('title', 'Project path required.'); } - }) + }); + + $('.import_git').click(function( event ) { + if($('#project_import_url').attr('disabled')) { + $('#project_import_url').attr('disabled', false); + } else { + $('#project_import_url').attr('disabled', true); + } + }); \ 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 From 001c9aa3137e6648fe3994eca4237f9283d0ee6e Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 14 Jul 2016 16:14:30 +0200 Subject: [PATCH 8/8] udpated JS based on feedback --- app/views/projects/new.html.haml | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/app/views/projects/new.html.haml b/app/views/projects/new.html.haml index 37f559c1bed..c72d0140bb9 100644 --- a/app/views/projects/new.html.haml +++ b/app/views/projects/new.html.haml @@ -157,9 +157,6 @@ }); $('.import_git').click(function( event ) { - if($('#project_import_url').attr('disabled')) { - $('#project_import_url').attr('disabled', false); - } else { - $('#project_import_url').attr('disabled', true); - } + $projectImportUrl = $('#project_import_url') + $projectImportUrl.attr('disabled', !$projectImportUrl.attr('disabled')) }); \ No newline at end of file