From b65cb237cee0b1a8dfdc21a09f2b181d0edf5bde Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= <alejorro70@gmail.com>
Date: Wed, 5 Dec 2018 12:22:52 -0300
Subject: Send a notification email on mirror update errors

The email is sent to project maintainers containing the last mirror
update error. This will allow maintainers to set alarms and react
accordingly.
---
 app/finders/remote_mirror_finder.rb                | 15 +++++++
 app/mailers/emails/remote_mirrors.rb               | 12 ++++++
 app/mailers/notify.rb                              |  1 +
 app/mailers/previews/notify_preview.rb             |  8 ++++
 app/models/remote_mirror.rb                        |  8 +++-
 app/services/notification_recipient_service.rb     | 23 +++++++++++
 app/services/notification_service.rb               | 32 ++++++++-------
 .../remote_mirror_update_failed_email.html.haml    | 46 ++++++++++++++++++++++
 .../remote_mirror_update_failed_email.text.erb     |  7 ++++
 app/workers/all_queues.yml                         |  1 +
 app/workers/remote_mirror_notification_worker.rb   | 15 +++++++
 .../repository_update_remote_mirror_worker.rb      |  2 +-
 .../remote-mirror-update-failed-notification.yml   |  5 +++
 config/sidekiq_queues.yml                          |  1 +
 spec/models/remote_mirror_spec.rb                  | 39 +++++++++++++++++-
 spec/services/notification_service_spec.rb         | 33 ++++++++++++++++
 .../repository_update_remote_mirror_worker_spec.rb | 13 ++++--
 17 files changed, 240 insertions(+), 21 deletions(-)
 create mode 100644 app/finders/remote_mirror_finder.rb
 create mode 100644 app/mailers/emails/remote_mirrors.rb
 create mode 100644 app/views/notify/remote_mirror_update_failed_email.html.haml
 create mode 100644 app/views/notify/remote_mirror_update_failed_email.text.erb
 create mode 100644 app/workers/remote_mirror_notification_worker.rb
 create mode 100644 changelogs/unreleased/remote-mirror-update-failed-notification.yml

diff --git a/app/finders/remote_mirror_finder.rb b/app/finders/remote_mirror_finder.rb
new file mode 100644
index 00000000000..420db0077aa
--- /dev/null
+++ b/app/finders/remote_mirror_finder.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class RemoteMirrorFinder
+  attr_accessor :params
+
+  def initialize(params)
+    @params = params
+  end
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def execute
+    RemoteMirror.find_by(id: params[:id])
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+end
diff --git a/app/mailers/emails/remote_mirrors.rb b/app/mailers/emails/remote_mirrors.rb
new file mode 100644
index 00000000000..2018eb7260b
--- /dev/null
+++ b/app/mailers/emails/remote_mirrors.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+module Emails
+  module RemoteMirrors
+    def remote_mirror_update_failed_email(remote_mirror_id, recipient_id)
+      @remote_mirror = RemoteMirrorFinder.new(id: remote_mirror_id).execute
+      @project = @remote_mirror.project
+
+      mail(to: recipient(recipient_id), subject: subject('Remote mirror update failed'))
+    end
+  end
+end
diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb
index 88ad4c3e893..099ad779aa5 100644
--- a/app/mailers/notify.rb
+++ b/app/mailers/notify.rb
@@ -13,6 +13,7 @@ class Notify < BaseMailer
   include Emails::Pipelines
   include Emails::Members
   include Emails::AutoDevops
+  include Emails::RemoteMirrors
 
   helper MergeRequestsHelper
   helper DiffHelper
diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb
index e7e8d96eca4..2ac4610967d 100644
--- a/app/mailers/previews/notify_preview.rb
+++ b/app/mailers/previews/notify_preview.rb
@@ -145,6 +145,10 @@ class NotifyPreview < ActionMailer::Preview
     Notify.autodevops_disabled_email(pipeline, user.email).message
   end
 
+  def remote_mirror_update_failed_email
+    Notify.remote_mirror_update_failed_email(remote_mirror.id, user.id).message
+  end
+
   private
 
   def project
@@ -167,6 +171,10 @@ class NotifyPreview < ActionMailer::Preview
     @pipeline = Ci::Pipeline.last
   end
 
+  def remote_mirror
+    @remote_mirror ||= RemoteMirror.last
+  end
+
   def user
     @user ||= User.last
   end
diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb
index b7b4d0f1be9..5a6895aefab 100644
--- a/app/models/remote_mirror.rb
+++ b/app/models/remote_mirror.rb
@@ -65,10 +65,14 @@ class RemoteMirror < ActiveRecord::Base
       )
     end
 
-    after_transition started: :failed do |remote_mirror, _|
+    after_transition started: :failed do |remote_mirror|
       Gitlab::Metrics.add_event(:remote_mirrors_failed)
 
       remote_mirror.update(last_update_at: Time.now)
+
+      remote_mirror.run_after_commit do
+        RemoteMirrorNotificationWorker.perform_async(remote_mirror.id)
+      end
     end
   end
 
@@ -135,8 +139,8 @@ class RemoteMirror < ActiveRecord::Base
   end
 
   def mark_as_failed(error_message)
-    update_fail
     update_column(:last_error, Gitlab::UrlSanitizer.sanitize(error_message))
+    update_fail
   end
 
   def url=(value)
diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 9c236d7f41d..68cdc69023a 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -24,6 +24,10 @@ module NotificationRecipientService
     Builder::MergeRequestUnmergeable.new(*args).notification_recipients
   end
 
+  def self.build_project_maintainers_recipients(*args)
+    Builder::ProjectMaintainers.new(*args).notification_recipients
+  end
+
   module Builder
     class Base
       def initialize(*)
@@ -380,5 +384,24 @@ module NotificationRecipientService
         nil
       end
     end
+
+    class ProjectMaintainers < Base
+      attr_reader :target
+
+      def initialize(target, action:)
+        @target = target
+        @action = action
+      end
+
+      def build!
+        return [] unless project
+
+        add_recipients(project.team.maintainers, :watch, nil)
+      end
+
+      def acting_user
+        nil
+      end
+    end
   end
 end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index e24ef7f9c87..ff035fea216 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -429,26 +429,26 @@ class NotificationService
   end
 
   def pages_domain_verification_succeeded(domain)
-    recipients_for_pages_domain(domain).each do |user|
-      mailer.pages_domain_verification_succeeded_email(domain, user).deliver_later
+    project_maintainers_recipients(domain, action: 'succeeded').each do |recipient|
+      mailer.pages_domain_verification_succeeded_email(domain, recipient.user).deliver_later
     end
   end
 
   def pages_domain_verification_failed(domain)
-    recipients_for_pages_domain(domain).each do |user|
-      mailer.pages_domain_verification_failed_email(domain, user).deliver_later
+    project_maintainers_recipients(domain, action: 'failed').each do |recipient|
+      mailer.pages_domain_verification_failed_email(domain, recipient.user).deliver_later
     end
   end
 
   def pages_domain_enabled(domain)
-    recipients_for_pages_domain(domain).each do |user|
-      mailer.pages_domain_enabled_email(domain, user).deliver_later
+    project_maintainers_recipients(domain, action: 'enabled').each do |recipient|
+      mailer.pages_domain_enabled_email(domain, recipient.user).deliver_later
     end
   end
 
   def pages_domain_disabled(domain)
-    recipients_for_pages_domain(domain).each do |user|
-      mailer.pages_domain_disabled_email(domain, user).deliver_later
+    project_maintainers_recipients(domain, action: 'disabled').each do |recipient|
+      mailer.pages_domain_disabled_email(domain, recipient.user).deliver_later
     end
   end
 
@@ -474,6 +474,14 @@ class NotificationService
     mailer.send(:repository_cleanup_failure_email, project, user, error).deliver_later
   end
 
+  def remote_mirror_update_failed(remote_mirror)
+    recipients = project_maintainers_recipients(remote_mirror, action: 'update_failed')
+
+    recipients.each do |recipient|
+      mailer.remote_mirror_update_failed_email(remote_mirror.id, recipient.user.id).deliver_later
+    end
+  end
+
   protected
 
   def new_resource_email(target, method)
@@ -569,12 +577,8 @@ class NotificationService
 
   private
 
-  def recipients_for_pages_domain(domain)
-    project = domain.project
-
-    return [] unless project
-
-    notifiable_users(project.team.maintainers, :watch, target: project)
+  def project_maintainers_recipients(target, action:)
+    NotificationRecipientService.build_project_maintainers_recipients(target, action: action)
   end
 
   def notifiable?(*args)
diff --git a/app/views/notify/remote_mirror_update_failed_email.html.haml b/app/views/notify/remote_mirror_update_failed_email.html.haml
new file mode 100644
index 00000000000..4fb0a4c5a8a
--- /dev/null
+++ b/app/views/notify/remote_mirror_update_failed_email.html.haml
@@ -0,0 +1,46 @@
+%tr.alert{ style: "font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif;" }
+  %td{ style: "padding:10px;border-radius:3px;font-size:14px;line-height:1.3;text-align:center;overflow:hidden;background-color:#d22f57;color:#ffffff;" }
+    %table.img{ border: "0", cellpadding: "0", cellspacing: "0", style: "border-collapse:collapse;margin:0 auto;" }
+      %tbody
+        %tr
+          %td{ style: "vertical-align:middle;color:#ffffff;text-align:center;padding-right:5px;line-height:1;" }
+            %img{ alt: "✖", height: "13", src: image_url('mailers/ci_pipeline_notif_v1/icon-x-red-inverted.gif'), style: "display:block;", width: "13" }/
+          %td{ style: "vertical-align:middle;color:#ffffff;text-align:center;" }
+            A remote mirror update has failed.
+%tr.spacer{ style: "font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif;" }
+  %td{ style: "height:18px;font-size:18px;line-height:18px;" }
+    &nbsp;
+%tr.section{ style: "font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif;" }
+  %td{ style: "padding:0 15px;border:1px solid #ededed;border-radius:3px;overflow:hidden;" }
+    %table.table-info{ border: "0", cellpadding: "0", cellspacing: "0", style: "width:100%;" }
+      %tbody{ style: "font-size:15px;line-height:1.4;color:#8c8c8c;" }
+        %tr
+          %td{ style: "font-weight:300;padding:14px 0;margin:0;" } Project
+          %td{ style: "font-weight:500;padding:14px 0;margin:0;color:#333333;width:75%;padding-left:5px;" }
+            - namespace_url = @project.group ? group_url(@project.group) : user_url(@project.namespace.owner)
+            %a.muted{ href: namespace_url, style: "color:#333333;text-decoration:none;" }
+              = @project.owner_name
+            \/
+            %a.muted{ href: project_url(@project), style: "color:#333333;text-decoration:none;" }
+              = @project.name
+        %tr
+          %td{ style: "font-weight:300;padding:14px 0;margin:0;border-top:1px solid #ededed;" } Remote mirror
+          %td{ style: "font-weight:500;padding:14px 0;margin:0;color:#333333;width:75%;padding-left:5px;border-top:1px solid #ededed;" }
+            = @remote_mirror.safe_url
+        %tr
+          %td{ style: "font-weight:300;padding:14px 0;margin:0;border-top:1px solid #ededed;" } Last update at
+          %td{ style: "font-weight:500;padding:14px 0;margin:0;color:#333333;width:75%;padding-left:5px;border-top:1px solid #ededed;" }
+            = @remote_mirror.last_update_at
+
+%tr.table-warning{ style: "font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif;" }
+  %td{ style: "border: 1px solid #ededed; border-bottom: 0; border-radius: 4px 4px 0 0; overflow: hidden; background-color: #fdf4f6; color: #d22852; font-size: 14px; line-height: 1.4; text-align: center; padding: 8px 16px;" }
+    Logs may contain sensitive data. Please consider before forwarding this email.
+%tr.section{ style: "font-family: 'Helvetica Neue',Helvetica,Arial,sans-serif;" }
+  %td{ style: "padding: 0 16px; border: 1px solid #ededed; border-radius: 4px; overflow: hidden; border-top: 0; border-radius: 0 0 4px 4px;" }
+    %table.builds{ border: "0", cellpadding: "0", cellspacing: "0", style: "width: 100%; border-collapse: collapse;" }
+      %tbody
+        %tr.build-log
+          %td{ colspan: "2", style: "padding: 0 0 16px;" }
+          %pre{ style: "font-family: Monaco,'Lucida Console','Courier New',Courier,monospace; background-color: #fafafa; border-radius: 4px; overflow: hidden; white-space: pre-wrap; word-break: break-all; font-size:13px; line-height: 1.4; padding: 16px 8px; color: #333333; margin: 0;" }
+            = @remote_mirror.last_error
+
diff --git a/app/views/notify/remote_mirror_update_failed_email.text.erb b/app/views/notify/remote_mirror_update_failed_email.text.erb
new file mode 100644
index 00000000000..c6f29f0ad1c
--- /dev/null
+++ b/app/views/notify/remote_mirror_update_failed_email.text.erb
@@ -0,0 +1,7 @@
+A remote mirror update has failed.
+
+Project: <%= @project.human_name %> ( <%= project_url(@project) %> )
+Remote mirror: <%= @remote_mirror.safe_url %>
+Last update at: <%= @remote_mirror.last_update_at %>
+Last error:
+<%= @remote_mirror.last_error %>
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index dfce00a10a1..bc26b3f8ef2 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -124,6 +124,7 @@
 - propagate_service_template
 - reactive_caching
 - rebase
+- remote_mirror_notification
 - repository_fork
 - repository_import
 - repository_remove_remote
diff --git a/app/workers/remote_mirror_notification_worker.rb b/app/workers/remote_mirror_notification_worker.rb
new file mode 100644
index 00000000000..70c2e857d09
--- /dev/null
+++ b/app/workers/remote_mirror_notification_worker.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class RemoteMirrorNotificationWorker
+  include ApplicationWorker
+
+  def perform(remote_mirror_id)
+    remote_mirror = RemoteMirrorFinder.new(id: remote_mirror_id).execute
+
+    # 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?
+
+    NotificationService.new.remote_mirror_update_failed(remote_mirror)
+  end
+end
diff --git a/app/workers/repository_update_remote_mirror_worker.rb b/app/workers/repository_update_remote_mirror_worker.rb
index 9d4e67deb9c..a06db4b08cc 100644
--- a/app/workers/repository_update_remote_mirror_worker.rb
+++ b/app/workers/repository_update_remote_mirror_worker.rb
@@ -16,7 +16,7 @@ class RepositoryUpdateRemoteMirrorWorker
   end
 
   def perform(remote_mirror_id, scheduled_time)
-    remote_mirror = RemoteMirror.find(remote_mirror_id)
+    remote_mirror = RemoteMirrorFinder.new(id: remote_mirror_id).execute
     return if remote_mirror.updated_since?(scheduled_time)
 
     raise UpdateAlreadyInProgressError if remote_mirror.update_in_progress?
diff --git a/changelogs/unreleased/remote-mirror-update-failed-notification.yml b/changelogs/unreleased/remote-mirror-update-failed-notification.yml
new file mode 100644
index 00000000000..50ec8624ae5
--- /dev/null
+++ b/changelogs/unreleased/remote-mirror-update-failed-notification.yml
@@ -0,0 +1,5 @@
+---
+title: Send a notification email to project maintainers when a mirror update fails
+merge_request: 23595
+author:
+type: added
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index 5985569bef4..3ee32678f34 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -84,3 +84,4 @@
   - [object_pool, 1]
   - [repository_cleanup, 1]
   - [delete_stored_files, 1]
+  - [remote_mirror_notification, 2]
diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb
index b12ca79847c..5d3c25062d5 100644
--- a/spec/models/remote_mirror_spec.rb
+++ b/spec/models/remote_mirror_spec.rb
@@ -1,6 +1,6 @@
 require 'rails_helper'
 
-describe RemoteMirror do
+describe RemoteMirror, :mailer do
   include GitHelpers
 
   describe 'URL validation' do
@@ -137,6 +137,43 @@ describe RemoteMirror do
     end
   end
 
+  describe '#mark_as_failed' do
+    let(:remote_mirror) { create(:remote_mirror) }
+    let(:error_message) { 'http://user:pass@test.com/root/repoC.git/' }
+    let(:sanitized_error_message) { 'http://*****:*****@test.com/root/repoC.git/' }
+
+    subject do
+      remote_mirror.update_start
+      remote_mirror.mark_as_failed(error_message)
+    end
+
+    it 'sets the update_status to failed' do
+      subject
+
+      expect(remote_mirror.reload.update_status).to eq('failed')
+    end
+
+    it 'saves the sanitized error' do
+      subject
+
+      expect(remote_mirror.last_error).to eq(sanitized_error_message)
+    end
+
+    context 'notifications' do
+      let(:user) { create(:user) }
+
+      before do
+        remote_mirror.project.add_maintainer(user)
+      end
+
+      it 'notifies the project maintainers' do
+        perform_enqueued_jobs { subject }
+
+        should_email(user)
+      end
+    end
+  end
+
   context 'when remote mirror gets destroyed' do
     it 'removes remote' do
       mirror = create_mirror(url: 'http://foo:bar@test.com')
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 0f6c2604984..68ac3a00ab0 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -2167,6 +2167,39 @@ describe NotificationService, :mailer do
     end
   end
 
+  context 'Remote mirror notifications' do
+    describe '#remote_mirror_update_failed' do
+      let(:project) { create(:project) }
+      let(:remote_mirror) { create(:remote_mirror, project: project) }
+      let(:u_blocked) { create(:user, :blocked) }
+      let(:u_silence) { create_user_with_notification(:disabled, 'silent-maintainer', project) }
+      let(:u_owner)   { project.owner }
+      let(:u_maintainer1) { create(:user) }
+      let(:u_maintainer2) { create(:user) }
+      let(:u_developer) { create(:user) }
+
+      before do
+        project.add_maintainer(u_blocked)
+        project.add_maintainer(u_silence)
+        project.add_maintainer(u_maintainer1)
+        project.add_maintainer(u_maintainer2)
+        project.add_developer(u_developer)
+
+        # Mock remote update
+        allow(project.repository).to receive(:async_remove_remote)
+        allow(project.repository).to receive(:add_remote)
+
+        reset_delivered_emails!
+      end
+
+      it 'emails current watching maintainers' do
+        notification.remote_mirror_update_failed(remote_mirror)
+
+        should_only_email(u_maintainer1, u_maintainer2, u_owner)
+      end
+    end
+  end
+
   def build_team(project)
     @u_watcher               = create_global_setting_for(create(:user), :watch)
     @u_participating         = create_global_setting_for(create(:user), :participating)
diff --git a/spec/workers/repository_update_remote_mirror_worker_spec.rb b/spec/workers/repository_update_remote_mirror_worker_spec.rb
index 4f1ad2474f5..d73b0b53713 100644
--- a/spec/workers/repository_update_remote_mirror_worker_spec.rb
+++ b/spec/workers/repository_update_remote_mirror_worker_spec.rb
@@ -25,12 +25,19 @@ describe RepositoryUpdateRemoteMirrorWorker do
       it 'sets status as failed when update remote mirror service executes with errors' do
         error_message = 'fail!'
 
-        expect_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_return(status: :error, message: error_message)
+        expect_next_instance_of(Projects::UpdateRemoteMirrorService) do |service|
+          expect(service).to receive(:execute).with(remote_mirror).and_return(status: :error, message: error_message)
+        end
+
+        # Mock the finder so that it returns an object we can set expectations on
+        expect_next_instance_of(RemoteMirrorFinder) do |finder|
+          expect(finder).to receive(:execute).and_return(remote_mirror)
+        end
+        expect(remote_mirror).to receive(:mark_as_failed).with(error_message)
+
         expect do
           subject.perform(remote_mirror.id, Time.now)
         end.to raise_error(RepositoryUpdateRemoteMirrorWorker::UpdateError, error_message)
-
-        expect(remote_mirror.reload.update_status).to eq('failed')
       end
 
       it 'does nothing if last_update_started_at is higher than the time the job was scheduled in' do
-- 
cgit v1.2.1