Merge branch '3892-cleanup-projects-where-user-is-mirror-user' into 'master'

Resolve "User::DestroyService should cleanup projects where user is mirror_user"

Closes #3892

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

app/mailers/emails/projects.rb

 module Emails
   module Projects
+    prepend Emails::EE::Projects
+
     def project_was_moved_email(project_id, user_id, old_path_with_namespace)
       @current_user = @user = User.find user_id
       @project = Project.find project_id
@@ -38,13 +40,5 @@ module Emails
            reply_to:  @message.reply_to,
            subject:   @message.subject)
     end
-
-    def mirror_was_hard_failed_email(project_id, user_id)
-      @project = Project.find(project_id)
-      user = User.find(user_id)
-
-      mail(to: user.notification_email,
-           subject: subject('Repository mirroring paused'))
-    end
   end
 end

app/models/project_team.rb

@@ -87,6 +87,15 @@ class ProjectTeam
     @masters ||= fetch_members(Gitlab::Access::MASTER)
   end
 
+  def owners
+    @owners ||=
+      if group
+        group.owners
+      else
+        [project.owner]
+      end
+  end
+
   def import(source_project, current_user = nil)
     target_project = project
 

app/services/users/destroy_service.rb

 module Users
   class DestroyService
+    prepend ::EE::Users::DestroyService
+
     attr_accessor :current_user
 
     def initialize(current_user)
@@ -49,11 +51,7 @@ module Users
         ::Projects::DestroyService.new(project, current_user, skip_repo: project.legacy_storage?).execute
       end
 
-      Project.includes(group: :owners).where(mirror_user: user).find_each do |project|
-        if project.group.present?
-          project.update(mirror_user: project.group.owners.first)
-        end
-      end
+      yield(user) if block_given?
 
       MigrateToGhostUserService.new(user).execute unless options[:hard_delete]
 

app/workers/delete_user_worker.rb

@@ -5,7 +5,7 @@ class DeleteUserWorker
     delete_user  = User.find(delete_user_id)
     current_user = User.find(current_user_id)
 
-    Users::DestroyService.new(current_user).execute(delete_user, options.symbolize_keys)
+    ::Users::DestroyService.new(current_user).execute(delete_user, options.symbolize_keys)
   rescue Gitlab::Access::AccessDeniedError => e
     Rails.logger.warn("User could not be destroyed: #{e}")
   end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180307012445) do
+ActiveRecord::Schema.define(version: 20180307164427) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"

ee/app/mailers/emails/ee/projects.rb

+module Emails
+  module EE
+    module Projects
+      def mirror_was_hard_failed_email(project_id, user_id)
+        @project = Project.find(project_id)
+        user = User.find(user_id)
+
+        mail(to: user.notification_email,
+             subject: subject('Repository mirroring paused'))
+      end
+
+      def project_mirror_user_changed_email(new_mirror_user_id, deleted_user_name, project_id)
+        @project = Project.find(project_id)
+        @deleted_user_name = deleted_user_name
+        new_mirror_user = User.find(new_mirror_user_id)
+
+        mail(to: new_mirror_user.notification_email,
+             subject: subject('Mirror user changed'))
+      end
+    end
+  end
+end

ee/app/services/ee/notification_service.rb

@@ -34,5 +34,9 @@ module EE
         mailer.mirror_was_hard_failed_email(project.id, recipient.user.id).deliver_later
       end
     end
+
+    def project_mirror_user_changed(new_mirror_user, deleted_user_name, project)
+      mailer.project_mirror_user_changed_email(new_mirror_user.id, deleted_user_name, project.id).deliver_later
+    end
   end
 end

ee/app/services/ee/users/destroy_service.rb

+module EE
+  module Users
+    module DestroyService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute(user, options = {})
+        super(user, options) do |delete_user|
+          mirror_cleanup(delete_user)
+        end
+      end
+
+      def mirror_cleanup(user)
+        user_mirrors = ::Project.where(mirror_user: user)
+
+        user_mirrors.find_each do |mirror|
+          new_mirror_user = first_mirror_owner(user, mirror)
+
+          mirror.update_attributes(mirror_user: new_mirror_user)
+          ::NotificationService.new.project_mirror_user_changed(new_mirror_user, user.name, mirror)
+        end
+      end
+
+      private
+
+      def first_mirror_owner(user, mirror)
+        mirror_owners = mirror.team.owners
+        mirror_owners -= [user]
+
+        mirror_owners.first
+      end
+    end
+  end
+end

ee/app/views/notify/project_mirror_user_changed_email.html.haml

+%p
+  The mirror user for #{@project.full_path} has been changed from #{@deleted_user_name} to yourself because their account was deleted.
+
+%p
+  You can change this setting from the #{link_to("repository settings page", project_settings_repository_path(@project))}.

ee/app/views/notify/project_mirror_user_changed_email.text.erb

+The mirror user for <%= @project.full_path %> has been changed from <%= @deleted_user_name %> to yourself because their account was deleted.
+
+You can change this setting from the repository settings page in <%= project_settings_repository_url(@project) %>.

ee/changelogs/unreleased/3892-cleanup-projects-where-user-is-mirror-user.yml

+---
+title: Mirror owners now get assigned as mirror users when the assigned mirror users disable their accounts
+merge_request: 4827
+author:
+type: fixed

ee/db/migrate/20180307164427_disable_mirroring_for_projects_with_invalid_mirror_users.rb

+class DisableMirroringForProjectsWithInvalidMirrorUsers < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def up
+    execute <<~SQL
+      UPDATE projects
+      SET mirror = FALSE, mirror_user_id = NULL
+      WHERE mirror = true AND
+        NOT EXISTS (SELECT 1 FROM users WHERE users.id = projects.mirror_user_id)
+    SQL
+  end
+
+  def down
+  end
+end

ee/spec/services/ee/notification_service_spec.rb

@@ -185,4 +185,16 @@ describe EE::NotificationService, :mailer do
       end
     end
   end
+
+  context 'mirror user changed' do
+    it 'sends email' do
+      mirror_user = create(:user)
+      project = create(:project, :mirror, mirror_user_id: mirror_user.id)
+      new_mirror_user = project.team.owners.first
+
+      expect(Notify).to receive(:project_mirror_user_changed_email).with(new_mirror_user.id, mirror_user.name, project.id).and_call_original
+
+      subject.project_mirror_user_changed(new_mirror_user, mirror_user.name, project)
+    end
+  end
 end

ee/spec/services/ee/users/destroy_service_spec.rb

+require 'spec_helper'
+
+describe Users::DestroyService do
+  context 'when project is a mirror' do
+    it 'assigns mirror_user to a project owner' do
+      mirror_user = create(:user)
+      project = create(:project, :mirror, mirror_user_id: mirror_user.id)
+      new_mirror_user = project.team.owners.first
+
+      expect_any_instance_of(EE::NotificationService).to receive(:project_mirror_user_changed).with(new_mirror_user, mirror_user.name, project)
+
+      expect do
+        described_class.new(mirror_user).execute(mirror_user)
+      end.to change { project.reload.mirror_user }.from(mirror_user).to(new_mirror_user)
+    end
+  end
+end

spec/mailers/notify_spec.rb

@@ -1434,6 +1434,23 @@ describe Notify do
     end
   end
 
+  describe 'mirror user changed' do
+    let(:mirror_user) { create(:user) }
+    let(:project) { create(:project, :mirror, mirror_user_id: mirror_user.id) }
+    let(:new_mirror_user) { project.team.owners.first }
+
+    subject { described_class.project_mirror_user_changed_email(new_mirror_user.id, mirror_user.name, project.id) }
+
+    it_behaves_like 'an email sent from GitLab'
+    it_behaves_like 'it should not have Gmail Actions links'
+    it_behaves_like "a user cannot unsubscribe through footer link"
+
+    it 'has the correct subject and body' do
+      is_expected.to have_subject("#{project.name} | Mirror user changed")
+      is_expected.to have_html_escaped_body_text(project.full_path)
+    end
+  end
+
   describe 'admin notification' do
     let(:example_site_path) { root_path }
     let(:user) { create(:user) }

spec/services/users/destroy_service_spec.rb

@@ -167,24 +167,6 @@ describe Users::DestroyService do
       end
     end
 
-    context "when the user was the mirror_user for a group project" do
-      let(:group_owner) { create(:user) }
-      let(:mirror_user) { create(:user) }
-      let(:group)       { create(:group) }
-
-      before do
-        group.add_owner(group_owner)
-        group.add_master(mirror_user)
-      end
-
-      it 'updates the mirror_user to one of the group owners' do
-        project = create(:project, namespace_id: group.id, creator: group_owner, mirror_user: mirror_user)
-        service.execute(mirror_user)
-
-        expect(project.reload.mirror_user).to eq group_owner
-      end
-    end
-
     describe "user personal's repository removal" do
       before do
         Sidekiq::Testing.inline! { service.execute(user) }

spec/workers/delete_user_worker_spec.rb

@@ -5,14 +5,14 @@ describe DeleteUserWorker do
   let!(:current_user) { create(:user) }
 
   it "calls the DeleteUserWorker with the params it was given" do
-    expect_any_instance_of(Users::DestroyService).to receive(:execute)
+    expect_any_instance_of(EE::Users::DestroyService).to receive(:execute)
                                                       .with(user, {})
 
     described_class.new.perform(current_user.id, user.id)
   end
 
   it "uses symbolized keys" do
-    expect_any_instance_of(Users::DestroyService).to receive(:execute)
+    expect_any_instance_of(EE::Users::DestroyService).to receive(:execute)
                                                       .with(user, test: "test")
 
     described_class.new.perform(current_user.id, user.id, "test" => "test")