writes feature test suite

app/controllers/projects/mirrors_controller.rb

@@ -48,6 +48,6 @@ class Projects::MirrorsController < Projects::ApplicationController
   def mirror_params
     params.require(:project).permit(:mirror, :import_url, :mirror_user_id,
                                     :mirror_trigger_builds, :sync_time,
-                                    remote_mirrors_attributes: [:url, :id, :enabled, :remote_sync_time])
+                                    remote_mirrors_attributes: [:url, :id, :enabled, :sync_time])
   end
 end

app/models/project.rb

@@ -35,7 +35,6 @@ class Project < ActiveRecord::Base
   default_value_for :container_registry_enabled, gitlab_config_features.container_registry
   default_value_for(:repository_storage) { current_application_settings.pick_repository_storage }
   default_value_for(:shared_runners_enabled) { current_application_settings.shared_runners_enabled }
-  default_value_for :sync_time, gitlab_config_features.sync_time
   default_value_for :issues_enabled, gitlab_config_features.issues
   default_value_for :merge_requests_enabled, gitlab_config_features.merge_requests
   default_value_for :builds_enabled, gitlab_config_features.builds

app/models/remote_mirror.rb

@@ -15,10 +15,8 @@ class RemoteMirror < ActiveRecord::Base
 
   belongs_to :project, inverse_of: :remote_mirrors
 
-  default_value_for :remote_sync_time, gitlab_config_features.sync_time
-
   validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true }
-  validates :remote_sync_time,
+  validates :sync_time,
     presence: true,
     inclusion: { in: Gitlab::Mirror.sync_time_options.values }
 

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

@@ -46,7 +46,7 @@
       - if @project.builds_enabled?
         = render "shared/mirror_trigger_builds_setting", f: f
       .form-group
-        = f.label :sync_time, "Mirror synchronisation time", class: "label-light append-bottom-0"
+        = f.label :sync_time, "Synchronisation time", class: "label-light append-bottom-0"
         = f.select :sync_time, options_for_select(Gitlab::Mirror.sync_time_options, @project.sync_time), {}, class: 'form-control'
     .col-sm-12
       %hr
@@ -81,8 +81,8 @@
           = rm_form.text_field :url, class: "form-control", placeholder: 'https://username:password@gitlab.company.com/group/project.git'
           = render "instructions"
         .form-group
-          = rm_form.label :remote_sync_time, "Remote Mirror synchronisation time", class: "label-light append-bottom-0"
-          = rm_form.select :remote_sync_time, options_for_select(Gitlab::Mirror.sync_time_options, @remote_mirror.remote_sync_time), {}, class: 'form-control'
+          = rm_form.label :sync_time, "Synchronisation time", class: "label-light append-bottom-0"
+          = rm_form.select :sync_time, options_for_select(Gitlab::Mirror.sync_time_options, @remote_mirror.sync_time), {}, class: 'form-control'
     .col-sm-12.text-center
       %hr
       = f.submit 'Save changes', class: 'btn btn-create', name: 'update_remote_mirror'

app/workers/update_all_mirrors_worker.rb

@@ -10,7 +10,7 @@ class UpdateAllMirrorsWorker
     fail_stuck_mirrors!
 
     mirrors_to_sync.find_each(batch_size: 200) do |project|
-      RepositoryUpdateMirrorDispatchWorker.perform_in(rand(project.sync_time / 2), project.id)
+      RepositoryUpdateMirrorDispatchWorker.perform_in(rand((project.sync_time / 2).minutes), project.id)
     end
   end
 
@@ -27,7 +27,7 @@ class UpdateAllMirrorsWorker
   private
 
   def mirrors_to_sync
-    Project.where(mirror: true, sync_time: Gitlab::Mirror.sync_times)
+    Project.mirror.where(sync_time: Gitlab::Mirror.sync_times)
   end
 
   def try_obtain_lease

app/workers/update_all_remote_mirrors_worker.rb

@@ -17,6 +17,6 @@ class UpdateAllRemoteMirrorsWorker
   private
 
   def remote_mirrors_to_sync
-    RemoteMirror.where(remote_sync_time: Gitlab::Mirror.sync_times)
+    RemoteMirror.where(sync_time: Gitlab::Mirror.sync_times)
   end
 end

config/initializers/1_settings.rb

@@ -254,7 +254,6 @@ Settings.gitlab['default_projects_features'] ||= {}
 Settings.gitlab['webhook_timeout'] ||= 10
 Settings.gitlab['max_attachment_size'] ||= 10
 Settings.gitlab['session_expire_delay'] ||= 10080
-Settings.gitlab.default_projects_features['sync_time'] ||= 60
 Settings.gitlab.default_projects_features['issues']             = true if Settings.gitlab.default_projects_features['issues'].nil?
 Settings.gitlab.default_projects_features['merge_requests']     = true if Settings.gitlab.default_projects_features['merge_requests'].nil?
 Settings.gitlab.default_projects_features['wiki']               = true if Settings.gitlab.default_projects_features['wiki'].nil?

db/migrate/20170130113559_add_sync_schedule_to_projects_and_remote_projects.rb

@@ -6,12 +6,12 @@ class AddSyncScheduleToProjectsAndRemoteProjects < ActiveRecord::Migration
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:remote_mirrors, :remote_sync_time, :integer, default: 60)
+    add_column_with_default(:remote_mirrors, :sync_time, :integer, default: 60)
     add_column_with_default(:projects, :sync_time, :integer, default: 60)
   end
 
   def down
     remove_column :projects, :sync_time
-    remove_column :remote_mirrors, :remote_sync_time
+    remove_column :remote_mirrors, :sync_time
   end
 end

db/schema.rb

@@ -1206,7 +1206,7 @@ ActiveRecord::Schema.define(version: 20170204181513) do
     t.string "encrypted_credentials_salt"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
-    t.integer "remote_sync_time", default: 60, null: false
+    t.integer "sync_time", default: 60, null: false
   end
 
   add_index "remote_mirrors", ["project_id"], name: "index_remote_mirrors_on_project_id", using: :btree

lib/gitlab/mirror.rb

@@ -4,7 +4,7 @@ module Gitlab
     HOURLY  = 60
     DAYLY   = 1440
 
-    PRECRON = 14.minutes
+    INTERVAL_BEFORE_FIFTEEN = 14.minutes
 
     class << self
       def sync_time_options
@@ -20,26 +20,21 @@ module Gitlab
         sync_times << DAYLY  if at_beginning_of_day?
         sync_times << HOURLY if at_beginning_of_hour?
 
-        return sync_times
+        sync_times
       end
 
       def at_beginning_of_day?
         start_at = DateTime.now.at_beginning_of_day
-        end_at = start_at + PRECRON
+        end_at = start_at + INTERVAL_BEFORE_FIFTEEN
 
-        include_with_range?(start_at, end_at)
+        DateTime.now.between?(start_at, end_at)
       end
 
       def at_beginning_of_hour?
         start_at = DateTime.now.at_beginning_of_hour
-        end_at = start_at + PRECRON
+        end_at = start_at + INTERVAL_BEFORE_FIFTEEN
 
-        include_with_range?(start_at, end_at)
-      end
-
-      def include_with_range?(start_at, end_at)
-        window = start_at...end_at
-        window.include?(DateTime.now)
+        DateTime.now.between?(start_at, end_at)
       end
     end
   end

spec/controllers/projects/mirrors_controller_spec.rb

 require 'spec_helper'
 
 describe Projects::MirrorsController do
+  let(:sync_times) { [Gitlab::Mirror::FIFTEEN, Gitlab::Mirror::HOURLY, Gitlab::Mirror::DAYLY] }
+
+  describe 'setting up a mirror' do
+    context 'when the current project is a mirror' do
+      before do
+        @project = create(:project, :mirror)
+        sign_in(@project.owner)
+      end
+
+      context 'sync_time update' do
+        it 'allows sync_time update with valid time' do
+          sync_times.each do |sync_time|
+            expect do
+              do_put(@project, sync_time: sync_time)
+            end.to change { Project.mirror.where(sync_time: sync_time).count }.by(1)
+          end
+        end
+
+        it 'fails to update sync_time with invalid time' do
+          expect do
+            do_put(@project, sync_time: 1000)
+          end.not_to change { @project.sync_time }
+        end
+      end
+    end
+  end
+
   describe 'setting up a remote mirror' do
     context 'when the current project is a mirror' do
       before do
@@ -14,6 +41,24 @@ describe Projects::MirrorsController do
         end.to change { RemoteMirror.count }.to(1)
       end
 
+      context 'sync_time update' do
+        it 'allows sync_time update with valid time' do
+          sync_times.each do |sync_time|
+            expect do
+              do_put(@project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => 'http://foo.com', 'sync_time' => sync_time } })
+            end.to change { RemoteMirror.where(sync_time: sync_time).count }.by(1)
+          end
+        end
+
+        it 'fails to update sync_time with invalid time' do
+          expect(@project.remote_mirrors.count).to eq(0)
+
+          expect do
+            do_put(@project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => 'http://foo.com', 'sync_time' => 1000 } })
+          end.not_to change { @project.remote_mirrors.count }
+        end
+      end
+
       context 'when remote mirror has the same URL' do
         it 'does not allow to create the remote mirror' do
           expect do

spec/factories/projects.rb

@@ -59,6 +59,18 @@ FactoryGirl.define do
       end
     end
 
+    trait :remote_mirror do
+      transient do
+        sync_time Gitlab::Mirror::HOURLY
+        url "http://foo.com"
+        enabled true
+      end
+
+      after(:create) do |project, evaluator|
+        project.remote_mirrors.create!(url: evaluator.url, enabled: evaluator.enabled, sync_time: evaluator.sync_time)
+      end
+    end
+
     trait :read_only_repository do
       repository_read_only true
     end

spec/workers/update_all_mirrors_worker_spec.rb

@@ -7,6 +7,16 @@ describe UpdateAllMirrorsWorker do
   end
 
   describe '#perform' do
+    PROJECT_COUNT_WITH_TIME = { DateTime.now.beginning_of_hour + 15.minutes => 1,
+                                DateTime.now.beginning_of_hour => 2,
+                                DateTime.now.beginning_of_day => 3
+                              }
+
+    let!(:mirror1) { create(:empty_project, :mirror, sync_time: Gitlab::Mirror::FIFTEEN) }
+    let!(:mirror2) { create(:empty_project, :mirror, sync_time: Gitlab::Mirror::HOURLY) }
+    let!(:mirror3) { create(:empty_project, :mirror, sync_time: Gitlab::Mirror::DAYLY) }
+    let(:mirrors) { Project.where(mirror: true, sync_time: Gitlab::Mirror.sync_times) }
+
     it 'fails stuck mirrors' do
       worker = described_class.new
 
@@ -15,17 +25,25 @@ describe UpdateAllMirrorsWorker do
       worker.perform
     end
 
-    it 'enqueue a job on all mirrored Projects' do
-      worker = described_class.new
+    PROJECT_COUNT_WITH_TIME.each do |time, project_count|
+      describe "at #{time}" do
+        before do
+          allow(DateTime).to receive(:now).and_return(time)
+        end
 
-      mirror = create(:empty_project, :mirror)
-      create(:empty_project)
+        it 'enqueues a job on mirrored Projects' do
+          worker = described_class.new
 
-      expect(worker).to receive(:rand).with(30.minutes).and_return(10)
-      expect(RepositoryUpdateMirrorDispatchWorker).to receive(:perform_in).with(10, mirror.id)
+          expect(mirrors.count).to eq(project_count)
+          mirrors.each do |mirror|
+            expect(worker).to receive(:rand).with((mirror.sync_time / 2).minutes).and_return(mirror.sync_time / 2)
+            expect(RepositoryUpdateMirrorDispatchWorker).to receive(:perform_in).with(mirror.sync_time / 2, mirror.id)
+          end
 
           worker.perform
         end
+      end
+    end
 
     it 'does not execute if cannot get the lease' do
       allow_any_instance_of(Gitlab::ExclusiveLease)

spec/workers/update_all_remote_mirrors_worker.rb

+require 'rails_helper'
+
+describe UpdateAllRemoteMirrorsWorker do
+  describe "#perform" do
+    PROJECT_COUNT_WITH_TIME = { DateTime.now.beginning_of_hour + 15.minutes => 1,
+                                DateTime.now.beginning_of_hour => 2,
+                                DateTime.now.beginning_of_day => 3
+                              }
+
+    let!(:mirror1) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::FIFTEEN) }
+    let!(:mirror2) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::HOURLY) }
+    let!(:mirror3) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::DAYLY) }
+    let(:mirrors) { RemoteMirror.where(sync_time: Gitlab::Mirror.sync_times) }
+
+    it 'fails stuck mirrors' do
+      worker = described_class.new
+
+      expect(worker).to receive(:fail_stuck_mirrors!)
+
+      worker.perform
+    end
+
+    PROJECT_COUNT_WITH_TIME.each do |time, project_count|
+      describe "at #{time}" do
+        before do
+          allow(DateTime).to receive(:now).and_return(time)
+        end
+
+        it 'enqueues a job on mirrored Projects' do
+          worker = described_class.new
+
+          expect(mirrors.count).to eq(project_count)
+          mirrors.each do |mirror|
+            expect(RepositoryUpdateRemoteMirrorWorker).to receive(:perform_async).with(mirror.id)
+          end
+
+          worker.perform
+        end
+      end
+    end
+  end
+end