Mirrors with invalid import URLs will now be considered valid unless a significant change is made directly to the import URL.39731-cleanup-mirrors-with-invalid-url

2 files changed, 34 insertions, 4 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 6678733e43e..a2a2ced7fc6 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -249,7 +249,7 @@ class Project < ActiveRecord::Base
 
   validates :namespace, presence: true
   validates :name, uniqueness: { scope: :namespace_id }
-  validates :import_url, addressable_url: true, if: :external_import?
+  validates :import_url, addressable_url: true, if: [:external_import?, :import_url_changed?]
   validates :import_url, importable_url: true, if: [:external_import?, :import_url_changed?]
   validates :star_count, numericality: { greater_than_or_equal_to: 0 }
   validate :check_limit, on: :create
@@ -560,6 +560,8 @@ class Project < ActiveRecord::Base
   end
 
   def add_import_job
+    return mark_import_as_failed('Import URL is invalid.') unless valid_import_url?
+
     job_id =
       if forked?
         RepositoryForkWorker.perform_async(id,
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 7338e341359..85b54a9fe39 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -188,10 +188,18 @@ describe Project do
       end
     end
 
-    it 'does not allow an invalid URI as import_url' do
-      project2 = build(:project, import_url: 'invalid://')
+    context 'with invalid import URI' do
+      let(:project2) { build(:project, import_url: 'invalid://') }
 
-      expect(project2).not_to be_valid
+      it 'does not allow an invalid URI as import_url' do
+        expect(project2).not_to be_valid
+      end
+
+      it 'is valid if invalid URI was already set and did not change during save' do
+        project2.save!(validate: false)
+
+        expect(project2).to be_valid
+      end
     end
 
     it 'does allow a valid URI as import_url' do
@@ -1734,6 +1742,26 @@ describe Project do
   describe '#add_import_job' do
     let(:import_jid) { '123' }
 
+    context 'with invalid import URI' do
+      let(:project2) { build(:project, import_url: 'invalid://') }
+
+      before do
+        project2.save!(validate: false)
+      end
+
+      it 'does not schedule a RepositoryImportWorker job' do
+        expect(RepositoryImportWorker).not_to receive(:perform_async)
+
+        project2.add_import_job
+      end
+
+      it 'sets import error' do
+        project2.add_import_job
+
+        expect(project2.reload.import_error).to eq('Import URL is invalid.')
+      end
+    end
+
     context 'forked' do
       let(:forked_project_link) { create(:forked_project_link, :forked_to_empty_project) }
       let(:forked_from_project) { forked_project_link.forked_from_project }