Merge branch 'ee-54650-send-an-email-to-project-owners-when-a-mirror-update-fails' into 'master'

[EE] Send a notification email on mirror update errors

See merge request gitlab-org/gitlab-ee!8801

app/finders/remote_mirror_finder.rb

+# 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

app/mailers/emails/remote_mirrors.rb

+# 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

app/mailers/notify.rb

@@ -15,6 +15,7 @@ class Notify < BaseMailer
   include Emails::Pipelines
   include Emails::Members
   include Emails::AutoDevops
+  include Emails::RemoteMirrors
 
   helper MergeRequestsHelper
   helper DiffHelper

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

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)

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(*)
@@ -388,6 +392,25 @@ 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
 

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)

app/views/notify/remote_mirror_update_failed_email.html.haml

+%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;" }
+     
+%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
+

app/views/notify/remote_mirror_update_failed_email.text.erb

+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 %>

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

app/workers/remote_mirror_notification_worker.rb

+# 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

app/workers/repository_update_remote_mirror_worker.rb

@@ -15,7 +15,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?

changelogs/unreleased/remote-mirror-update-failed-notification.yml

+---
+title: Send a notification email to project maintainers when a mirror update fails
+merge_request: 23595
+author:
+type: added

config/sidekiq_queues.yml

@@ -84,6 +84,7 @@
   - [object_pool, 1]
   - [repository_cleanup, 1]
   - [delete_stored_files, 1]
+  - [remote_mirror_notification, 2]
 
   # EE-specific queues
   - [ldap_group_sync, 2]

spec/models/remote_mirror_spec.rb

 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')

spec/services/notification_service_spec.rb

@@ -2209,6 +2209,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)

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