Merge branch '1979-1-3-fix' into 'master'

Ensure approver references are all present

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

ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb

@@ -10,10 +10,12 @@ module Gitlab
 
       class Approver < ActiveRecord::Base
         self.table_name = 'approvers'
+        belongs_to :user
       end
 
       class ApproverGroup < ActiveRecord::Base
         self.table_name = 'approver_groups'
+        belongs_to :group
       end
 
       class ApprovalMergeRequestRule < ActiveRecord::Base
@@ -56,11 +58,11 @@ module Gitlab
         has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
 
         def approver_ids
-          @approver_ids ||= Approver.where(target_type: 'MergeRequest', target_id: id).pluck('distinct user_id')
+          @approver_ids ||= Approver.where(target_type: 'MergeRequest', target_id: id).joins(:user).pluck('distinct user_id')
         end
 
         def approver_group_ids
-          @approver_group_ids ||= ApproverGroup.where(target_type: 'MergeRequest', target_id: id).pluck('distinct group_id')
+          @approver_group_ids ||= ApproverGroup.where(target_type: 'MergeRequest', target_id: id).joins(:group).pluck('distinct group_id')
         end
 
         def sync_code_owners_with_approvers
@@ -78,11 +80,11 @@ module Gitlab
         has_many :approval_rules, class_name: 'ApprovalProjectRule'
 
         def approver_ids
-          @approver_ids ||= Approver.where(target_type: 'Project', target_id: id).pluck('distinct user_id')
+          @approver_ids ||= Approver.where(target_type: 'Project', target_id: id).joins(:user).pluck('distinct user_id')
         end
 
         def approver_group_ids
-          @approver_group_ids ||= ApproverGroup.where(target_type: 'Project', target_id: id).pluck('distinct group_id')
+          @approver_group_ids ||= ApproverGroup.where(target_type: 'Project', target_id: id).joins(:group).pluck('distinct group_id')
         end
       end
 

ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb

@@ -74,6 +74,27 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
           expect(approval_rule.groups).to contain_exactly(group2)
         end
       end
+
+      context 'when approver has user_id which no longer exists' do
+        before do
+          create_member_in(user1, :old_schema)
+          create_member_in(user2, :old_schema)
+          create_member_in(group1, :old_schema)
+          create_member_in(group2, :old_schema)
+        end
+
+        it 'ignores broken reference when assigning' do
+          User.where(id: user1).delete_all
+          Group.where(id: group1).delete_all
+
+          described_class.new.perform(target_type, target.id)
+
+          approval_rule = target.approval_rules.first
+
+          expect(approval_rule.users).to contain_exactly(user2)
+          expect(approval_rule.groups).to contain_exactly(group2)
+        end
+      end
     end
 
     context 'merge request' do