Only send one notification for failed remote mirror

Retries in Sidekiq and in the remote mirror scheduler can cause repeated
attempts in quick succession if the sync fails. Each failure will then
send an e-mail to all project maintainers, which can spam users
unnecessarily.

Modify the logic to send one notification the first time the mirror
fails by setting `error_notification_sent` to `true` and reset the
flag after a successful sync.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/56222

app/models/remote_mirror.rb

@@ -61,7 +61,10 @@ class RemoteMirror < ActiveRecord::Base
       timestamp = Time.now
       remote_mirror.update!(
-        last_update_at: timestamp, last_successful_update_at: timestamp, last_error: nil
+        last_update_at: timestamp,
+        last_successful_update_at: timestamp,
+        last_error: nil,
+        error_notification_sent: false
       )
     end
@@ -179,6 +182,10 @@ class RemoteMirror < ActiveRecord::Base
     project.repository.add_remote(remote_name, remote_url)
   end
+  def after_sent_notification
+    update_column(:error_notification_sent, true)
+  end
+
   private
   def store_credentials
@@ -221,7 +228,8 @@ class RemoteMirror < ActiveRecord::Base
       last_error: nil,
       last_update_at: nil,
       last_successful_update_at: nil,
-      update_status: 'finished'
+      update_status: 'finished',
+      error_notification_sent: false
     )
   end

app/workers/remote_mirror_notification_worker.rb

@@ -9,7 +9,10 @@ class RemoteMirrorNotificationWorker
     # We check again if there's an error because a newer run since this job was
     # fired could've completed successfully.
     return unless remote_mirror && remote_mirror.last_error.present?
+    return if remote_mirror.error_notification_sent?
     NotificationService.new.remote_mirror_update_failed(remote_mirror)
+
+    remote_mirror.after_sent_notification
   end
 end

db/migrate/20190115054216_add_error_notification_sent_to_remote_mirrors.rb

+# frozen_string_literal: true
+
+class AddErrorNotificationSentToRemoteMirrors < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :remote_mirrors, :error_notification_sent, :boolean
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20190103140724) do
+ActiveRecord::Schema.define(version: 20190115054216) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1849,6 +1849,7 @@ ActiveRecord::Schema.define(version: 20190103140724) do
     t.string "encrypted_credentials_salt"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.boolean "error_notification_sent"
     t.index ["last_successful_update_at"], name: "index_remote_mirrors_on_last_successful_update_at", using: :btree
     t.index ["project_id"], name: "index_remote_mirrors_on_project_id", using: :btree
   end

spec/models/remote_mirror_spec.rb

@@ -303,6 +303,25 @@ describe RemoteMirror, :mailer do
     end
   end
+  context '#url=' do
+    let(:remote_mirror) { create(:project, :repository, :remote_mirror).remote_mirrors.first }
+
+    it 'resets all the columns when URL changes' do
+      remote_mirror.update(last_error: Time.now,
+                           last_update_at: Time.now,
+                           last_successful_update_at: Time.now,
+                           update_status: 'started',
+                           error_notification_sent: true)
+
+      expect { remote_mirror.update_attribute(:url, 'http://new.example.com') }
+        .to change { remote_mirror.last_error }.to(nil)
+        .and change { remote_mirror.last_update_at }.to(nil)
+        .and change { remote_mirror.last_successful_update_at }.to(nil)
+        .and change { remote_mirror.update_status }.to('finished')
+        .and change { remote_mirror.error_notification_sent }.to(false)
+    end
+  end
+
   context '#updated_since?' do
     let(:remote_mirror) { create(:project, :repository, :remote_mirror).remote_mirrors.first }
     let(:timestamp) { Time.now - 5.minutes }

spec/workers/remote_mirror_notification_worker_spec.rb

+require 'spec_helper'
+
+describe RemoteMirrorNotificationWorker, :mailer do
+  set(:project) { create(:project, :repository, :remote_mirror) }
+  set(:mirror) { project.remote_mirrors.first }
+
+  describe '#execute' do
+    it 'calls NotificationService#remote_mirror_update_failed when the mirror exists' do
+      mirror.update_column(:last_error, "There was a problem fetching")
+
+      expect(NotificationService).to receive_message_chain(:new, :remote_mirror_update_failed)
+
+      subject.perform(mirror.id)
+
+      expect(mirror.reload.error_notification_sent?).to be_truthy
+    end
+
+    it 'does nothing when the mirror has no errors' do
+      expect(NotificationService).not_to receive(:new)
+
+      subject.perform(mirror.id)
+    end
+
+    it 'does nothing when the mirror does not exist' do
+      expect(NotificationService).not_to receive(:new)
+
+      subject.perform(RemoteMirror.maximum(:id).to_i.succ)
+    end
+
+    it 'does nothing when a notification has already been sent' do
+      mirror.update_columns(last_error: "There was a problem fetching",
+                            error_notification_sent: true)
+
+      expect(NotificationService).not_to receive(:new)
+
+      subject.perform(mirror.id)
+    end
+  end
+end

spec/workers/repository_update_remote_mirror_worker_spec.rb

@@ -22,6 +22,13 @@ describe RepositoryUpdateRemoteMirrorWorker do
         expect { subject.perform(remote_mirror.id, Time.now) }.to change { remote_mirror.reload.update_status }.to('finished')
       end
+      it 'resets the notification flag upon success' do
+        expect_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_return(status: :success)
+        remote_mirror.update_column(:error_notification_sent, true)
+
+        expect { subject.perform(remote_mirror.id, Time.now) }.to change { remote_mirror.reload.error_notification_sent }.to(false)
+      end
+
       it 'sets status as failed when update remote mirror service executes with errors' do
         error_message = 'fail!'