Merge branch 'fix-mirror-settings-validations' into 'master'

Fix validations related to mirroring settings form.

Some of our users are changing to GitLab as their main repository, it
means that they're disabling the local mirror and setting up a remote
mirror instead. This fix helps to avoid the extra steps required in order
to achieve the desired configuration.

Closes: gitlab-com/support-forum#1044 gitlab-org/gitlab-ee#1062

Related: gitlab-org/gitlab-ce#17940

Also the UI has been modify to have a single update button:

#### Before
![1](/uploads/83c1d614a35911740154d4167f77f6a0/1.png)

#### After

![2](/uploads/87f73a32c35d7fe83558932cdc1a21d7/2.png)

See merge request !773

CHANGELOG-EE

@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.13.0 (unreleased)
   - Fix 500 error updating mirror URLs for projects
+  - Fix validations related to mirroring settings form. !773
 
 v 8.12.5 (unreleased)
 

app/models/project.rb

@@ -139,7 +139,7 @@ class Project < ActiveRecord::Base
   has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
   has_many :variables, dependent: :destroy, class_name: 'Ci::Variable', foreign_key: :gl_project_id
   has_many :triggers, dependent: :destroy, class_name: 'Ci::Trigger', foreign_key: :gl_project_id
-  has_many :remote_mirrors, dependent: :destroy
+  has_many :remote_mirrors, inverse_of: :project, dependent: :destroy
   has_many :environments, dependent: :destroy
   has_many :deployments, dependent: :destroy
   has_many :path_locks, dependent: :destroy

app/models/remote_mirror.rb

@@ -9,10 +9,10 @@ class RemoteMirror < ActiveRecord::Base
                  insecure_mode: true,
                  algorithm: 'aes-256-cbc'
 
-  belongs_to :project
+  belongs_to :project, inverse_of: :remote_mirrors
 
   validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true }
-  validate  :url_availability, if: :url_changed?
+  validate  :url_availability, if: -> (mirror) { mirror.url_changed? || mirror.enabled? }
 
   after_save :refresh_remote, if: :mirror_url_changed?
   after_update :reset_fields, if: :mirror_url_changed?
@@ -109,7 +109,7 @@ class RemoteMirror < ActiveRecord::Base
   private
 
   def url_availability
-    if project.import_url == url
+    if project.import_url == url && project.mirror?
       errors.add(:url, 'is already in use')
     end
   end

app/views/projects/mirrors/show.html.haml

 - page_title "Mirror Repository"
 
+.row
+  = form_errors(@project)
 .row.prepend-top-default.append-bottom-default
-  .col-lg-3
-    %h4.prepend-top-0
-      Pull from a remote repository
-    %p.light
-      Set up your project to automatically have its branches, tags, and commits updated from an upstream repository every hour.
-  .col-lg-9
-    %h5.prepend-top-0
-      Set up mirror repository
-    = form_for @project, url: namespace_project_mirror_path(@project.namespace, @project) do |f|
-      = form_errors(@project)
+  = form_for @project, url: namespace_project_mirror_path(@project.namespace, @project) do |f|
+    .col-lg-3
+      %h4.prepend-top-0
+        Pull from a remote repository
+      %p.light
+        Set up your project to automatically have its branches, tags, and commits updated from an upstream repository every hour.
+    .col-lg-9
+      %h5.prepend-top-0
+        Set up mirror repository
       = render "shared/mirror_update_button"
       - if @project.mirror_last_update_failed?
         .panel.panel-danger
@@ -42,17 +43,14 @@
           They need to have at least master access to this project.
       - if @project.builds_enabled?
         = render "shared/mirror_trigger_builds_setting", f: f
-      = f.submit "Update", class: "btn btn-save"
-  .col-sm-12
-    %hr
-  .col-lg-3
-    %h4.prepend-top-0
-      Push to a remote repository
-    %p.light
-      Set up the remote repository that you want to update with the content of the current repository every hour.
-  .col-lg-9
-    = form_for @project, url: namespace_project_mirror_path(@project.namespace, @project) do |f|
-      = form_errors(@project)
+    .col-sm-12
+      %hr
+    .col-lg-3
+      %h4.prepend-top-0
+        Push to a remote repository
+      %p.light
+        Set up the remote repository that you want to update with the content of the current repository every hour.
+    .col-lg-9
       = render "shared/remote_mirror_update_button", remote_mirror: @remote_mirror
       - if @remote_mirror.last_error.present?
         .panel.panel-danger
@@ -75,4 +73,6 @@
           = rm_form.label :url, "Git repository URL", class: "label-light"
           = rm_form.text_field :url, class: "form-control", placeholder: 'https://username:password@gitlab.company.com/group/project.git'
           = render "instructions"
-      = f.submit "Update", class: "btn btn-create"
+    .col-sm-12.text-center
+      %hr
+      = f.submit 'Save changes', class: 'btn btn-create', name: 'update_remote_mirror'

spec/controllers/projects/mirrors_controller_spec.rb

+require 'spec_helper'
+
+describe Projects::MirrorsController do
+  describe 'setting up a remote mirror' do
+    context 'when the current project is a mirror' do
+      before do
+        @project = create(:project, :mirror)
+        sign_in(@project.owner)
+      end
+
+      it 'allows to create a remote mirror' do
+        expect do
+          do_put(@project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => 'http://foo.com' } })
+        end.to change { RemoteMirror.count }.to(1)
+      end
+
+      context 'when remote mirror has the same URL' do
+        it 'does not allow to create the remote mirror' do
+          expect do
+            do_put(@project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => @project.import_url } })
+          end.not_to change { RemoteMirror.count }
+        end
+
+        context 'with disabled local mirror' do
+          it 'allows to create a remote mirror' do
+            expect do
+              do_put(@project, mirror: 0, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => @project.import_url } })
+            end.to change { RemoteMirror.count }.to(1)
+          end
+        end
+      end
+    end
+
+    context 'when the current project is not a mirror' do
+      it 'allows to create a remote mirror' do
+        project = create(:project)
+        sign_in(project.owner)
+
+        expect do
+          do_put(project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => 'http://foo.com' } })
+        end.to change { RemoteMirror.count }.to(1)
+      end
+    end
+
+    context 'when the current project has a remote mirror' do
+      before do
+        @project = create(:project)
+        @remote_mirror = @project.remote_mirrors.create!(enabled: 1, url: 'http://local.dev')
+        sign_in(@project.owner)
+      end
+
+      context 'when trying to create a mirror with the same URL' do
+        it 'should not setup the mirror' do
+          do_put(@project, mirror: true, import_url: @remote_mirror.url)
+
+          expect(@project.reload.mirror).to be_falsey
+          expect(@project.reload.import_url).to be_blank
+        end
+      end
+
+      context 'when trying to create a mirror with a different URL' do
+        it 'should setup the mirror' do
+          do_put(@project, mirror: true, mirror_user_id: @project.owner.id, import_url: 'http://test.com')
+
+          expect(@project.reload.mirror).to eq(true)
+          expect(@project.reload.import_url).to eq('http://test.com')
+        end
+      end
+    end
+  end
+
+  def do_put(project, options)
+    attrs = { namespace_id: project.namespace.to_param, project_id: project.to_param }
+    attrs.merge!(project: options)
+
+    put :update, attrs
+  end
+end