Improve various specs

Add test case for not allow author to approve

ee/app/services/approval_rules/finalize_service.rb

@@ -11,13 +11,15 @@ module ApprovalRules
     def execute
       return unless merge_request.merged?
 
-      if merge_request.approval_rules.regular.exists?
-        merge_group_members_into_users
-      else
-        copy_project_approval_rules
+      ActiveRecord::Base.transaction do
+        if merge_request.approval_rules.regular.exists?
+          merge_group_members_into_users
+        else
+          copy_project_approval_rules
+        end
+
+        merge_request.approval_rules.each(&:sync_approved_approvers)
       end
-
-      merge_request.approval_rules.each(&:sync_approved_approvers)
     end
 
     private
@@ -30,9 +32,12 @@ module ApprovalRules
 
     def copy_project_approval_rules
       merge_request.target_project.approval_rules.each do |project_rule|
-        rule = merge_request.approval_rules.create!(project_rule.attributes.slice('approvals_required', 'name'))
-        rule.users = project_rule.approvers
-        rule.groups = project_rule.groups.public_or_visible_to_user(merge_request.author)
+        users = project_rule.approvers
+        groups = project_rule.groups.public_or_visible_to_user(merge_request.author)
+
+        merge_request.approval_rules.create!(
+          project_rule.attributes.slice('approvals_required', 'name').merge(users: users, groups: groups)
+        )
       end
     end
   end

ee/spec/models/approval_merge_request_rule_spec.rb

 # frozen_string_literal: true
 
-require 'rails_helper'
+require 'spec_helper'
 
-RSpec.describe ApprovalMergeRequestRule, type: :model do
+describe ApprovalMergeRequestRule do
   let(:merge_request) { create(:merge_request) }
 
   subject { create(:approval_merge_request_rule, merge_request: merge_request) }
 
   describe '#project' do
     it 'returns project of MergeRequest' do
+      expect(subject.project).to be_present
       expect(subject.project).to eq(merge_request.project)
     end
   end
 
   describe '#approvers' do
-    context 'when project setting includes author' do
-      before do
-        merge_request.target_project.update(merge_requests_author_approval: true)
-
-        create(:group) do |group|
-          group.add_guest(merge_request.author)
-          subject.groups << group
-        end
+    before do
+      create(:group) do |group|
+        group.add_guest(merge_request.author)
+        subject.groups << group
       end
+    end
 
+    context 'when project merge_requests_author_approval is true' do
       it 'contains author' do
+        merge_request.project.update(merge_requests_author_approval: true)
+
         expect(subject.approvers).to contain_exactly(merge_request.author)
       end
     end
+
+    context 'when project merge_requests_author_approval is false' do
+      it 'contains author' do
+        merge_request.project.update(merge_requests_author_approval: false)
+
+        expect(subject.approvers).to be_empty
+      end
+    end
   end
 
   describe '#sync_approved_approvers' do
@@ -46,14 +55,14 @@ RSpec.describe ApprovalMergeRequestRule, type: :model do
       it 'does nothing' do
         subject.sync_approved_approvers
 
-        expect(subject.approved_approvers).to be_empty
+        expect(subject.approved_approvers.reload).to be_empty
       end
     end
 
     context 'when merged' do
       let(:merge_request) { create(:merged_merge_request) }
 
-      it 'updates mapping' do
+      it 'records approved approvers as approved_approvers association' do
         subject.sync_approved_approvers
 
         expect(subject.approved_approvers.reload).to contain_exactly(member1, member2)

ee/spec/models/concerns/approval_rule_like_spec.rb

 # frozen_string_literal: true
 
-require 'rails_helper'
+require 'spec_helper'
 
-RSpec.describe ApprovalRuleLike, type: :model do
-  let(:member1) { create(:user) }
-  let(:member2) { create(:user) }
-  let(:member3) { create(:user) }
+describe ApprovalRuleLike do
+  let(:user1) { create(:user) }
+  let(:user2) { create(:user) }
+  let(:user3) { create(:user) }
   let(:group1) { create(:group) }
   let(:group2) { create(:group) }
 
@@ -15,16 +15,17 @@ RSpec.describe ApprovalRuleLike, type: :model do
     describe '#add_member' do
       it 'adds as a member of the rule' do
         expect do
-          subject.add_member(member1)
-        end.to change { subject.users.count }.by(1)
+          subject.add_member(user1)
+          subject.add_member(group1)
+        end.to change { subject.users.count }.by(1).and change { subject.groups.count }.by(1)
       end
 
       it 'does nothing if already a member' do
-        subject.add_member(member1)
+        subject.add_member(user1)
 
         expect do
-          subject.add_member(member1)
-        end.not_to change { subject.users.count }
+          subject.add_member(user1)
+        end.not_to change { subject.users.count + subject.groups.count }
       end
     end
 
@@ -45,31 +46,31 @@ RSpec.describe ApprovalRuleLike, type: :model do
     end
 
     describe '#approvers' do
-      let(:group1_member1) { create(:user) }
-      let(:group2_member1) { create(:user) }
+      let(:group1_user) { create(:user) }
+      let(:group2_user) { create(:user) }
 
       before do
-        subject.users << member1
-        subject.users << member2
+        subject.users << user1
+        subject.users << user2
         subject.groups << group1
         subject.groups << group2
 
-        group1.add_guest(group1_member1)
-        group2.add_guest(group2_member1)
+        group1.add_guest(group1_user)
+        group2.add_guest(group2_user)
       end
 
       it 'contains users as direct members and group members' do
-        expect(subject.approvers).to contain_exactly(member1, member2, group1_member1, group2_member1)
+        expect(subject.approvers).to contain_exactly(user1, user2, group1_user, group2_user)
       end
 
       context 'when user is both a direct member and a group member' do
         before do
-          group1.add_guest(member1)
-          group2.add_guest(member2)
+          group1.add_guest(user1)
+          group2.add_guest(user2)
         end
 
         it 'contains only unique users' do
-          expect(subject.approvers).to contain_exactly(member1, member2, group1_member1, group2_member1)
+          expect(subject.approvers).to contain_exactly(user1, user2, group1_user, group2_user)
         end
       end
     end

ee/spec/services/approval_rules/finalize_service_spec.rb

@@ -7,26 +7,26 @@ describe ApprovalRules::FinalizeService do
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
 
   describe '#execute' do
-    let!(:member1) { create(:user) }
-    let!(:member2) { create(:user) }
-    let!(:member3) { create(:user) }
+    let!(:user1) { create(:user) }
+    let!(:user2) { create(:user) }
+    let!(:user3) { create(:user) }
     let!(:group1) { create(:group) }
     let!(:group2) { create(:group) }
-    let!(:group1_member) { create(:user) }
-    let!(:group2_member) { create(:user) }
-    let!(:approval1) { create(:approval, merge_request: merge_request, user: member1) }
-    let!(:approval2) { create(:approval, merge_request: merge_request, user: member3) }
-    let!(:approval3) { create(:approval, merge_request: merge_request, user: group1_member) }
-    let!(:approval4) { create(:approval, merge_request: merge_request, user: group2_member) }
+    let!(:group1_user) { create(:user) }
+    let!(:group2_user) { create(:user) }
+    let!(:approval1) { create(:approval, merge_request: merge_request, user: user1) }
+    let!(:approval2) { create(:approval, merge_request: merge_request, user: user3) }
+    let!(:approval3) { create(:approval, merge_request: merge_request, user: group1_user) }
+    let!(:approval4) { create(:approval, merge_request: merge_request, user: group2_user) }
     let!(:project_rule) { create(:approval_project_rule, project: project, name: 'foo', approvals_required: 12) }
 
     subject { described_class.new(merge_request) }
 
     before do
-      group1.add_guest(group1_member)
-      group2.add_guest(group2_member)
+      group1.add_guest(group1_user)
+      group2.add_guest(group2_user)
 
-      project_rule.users = [member1, member2]
+      project_rule.users = [user1, user2]
       project_rule.groups << group1
     end
 
@@ -38,7 +38,7 @@ describe ApprovalRules::FinalizeService do
       end
     end
 
-    context 'when project rule is not overwritten' do
+    context 'when there is no merge request rules' do
       it_behaves_like 'skipping when unmerged'
 
       context 'when merged' do
@@ -57,18 +57,18 @@ describe ApprovalRules::FinalizeService do
 
           expect(rule.name).to eq('foo')
           expect(rule.approvals_required).to eq(12)
-          expect(rule.users).to contain_exactly(member1, member2, group1_member)
+          expect(rule.users).to contain_exactly(user1, user2, group1_user)
           expect(rule.groups).to contain_exactly(group1)
 
-          expect(rule.approved_approvers).to contain_exactly(member1, group1_member)
+          expect(rule.approved_approvers).to contain_exactly(user1, group1_user)
         end
       end
     end
 
-    context 'when project rule is overwritten' do
+    context 'when there is a regular merge request rule' do
       before do
         rule = create(:approval_merge_request_rule, merge_request: merge_request, name: 'bar', approvals_required: 32)
-        rule.users = [member2, member3]
+        rule.users = [user2, user3]
         rule.groups << group2
       end
 
@@ -78,7 +78,7 @@ describe ApprovalRules::FinalizeService do
         let(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
 
         it 'does not copy project rules, and updates approval mapping with MR rules' do
-          expect(merge_request).not_to receive(:copy_project_approval_rules)
+          allow(subject).to receive(:copy_project_approval_rules)
 
           expect do
             subject.execute
@@ -88,10 +88,11 @@ describe ApprovalRules::FinalizeService do
 
           expect(rule.name).to eq('bar')
           expect(rule.approvals_required).to eq(32)
-          expect(rule.users).to contain_exactly(member2, member3, group2_member)
+          expect(rule.users).to contain_exactly(user2, user3, group2_user)
           expect(rule.groups).to contain_exactly(group2)
 
-          expect(rule.approved_approvers).to contain_exactly(member3, group2_member)
+          expect(rule.approved_approvers).to contain_exactly(user3, group2_user)
+          expect(subject).not_to have_received(:copy_project_approval_rules)
         end
       end
     end