Allow self-approvals in fallback approval rules

Allow authors of a MR to approve a MR if fallback approval rules are
in play.

Issue https://gitlab.com/gitlab-org/gitlab-ee/issues/7532

ee/app/models/approval_state.rb

@@ -133,15 +133,17 @@ class ApprovalState
 
   def can_approve?(user)
     return false unless user
-    # The check below considers authors being able to approve the MR.
-    # That is, they're included/excluded from that list accordingly.
     return true if unactioned_approvers.include?(user)
-    # We can safely unauthorize author and committers if it reaches this guard clause.
-    return false if merge_request.author == user
-    return false if merge_request.committers.include?(user)
+    return false unless any_approver_allowed?
     return false unless user.can?(:update_merge_request, merge_request)
+    # Users can only approve once.
+    return false if approvals.where(user: user).any?
+    # At this point, follow self-approval rules. Otherwise authors must
+    # have been in the list of unactioned_approvers to have been approved.
+    return committers_can_approve? if merge_request.committers.include?(user)
+    return authors_can_approve? if merge_request.author == user
 
-    any_approver_allowed? && merge_request.approvals.where(user: user).empty?
+    true
   end
 
   def has_approved?(user)
@@ -154,6 +156,10 @@ class ApprovalState
     project.merge_requests_author_approval?
   end
 
+  def committers_can_approve?
+    !project.merge_requests_disable_committers_approval?
+  end
+
   # TODO: remove after #1979 is closed
   # This is a temporary method for backward compatibility
   # before introduction of approval rules.

ee/spec/models/approvable_for_rule_spec.rb

@@ -43,26 +43,36 @@ describe ApprovableForRule do
     end
 
     context 'when the user is the author' do
-      context 'and user is an approver' do
+      context 'and author is an approver' do
         before do
           create(:approver, target: merge_request, user: author)
         end
 
-        it 'return true when authors can approve' do
+        it 'returns true when authors can approve' do
           project.update(merge_requests_author_approval: true)
 
           expect(merge_request.can_approve?(author)).to be true
         end
 
-        it 'return false when authors cannot approve' do
+        it 'returns false when authors cannot approve' do
           project.update(merge_requests_author_approval: false)
 
           expect(merge_request.can_approve?(author)).to be false
         end
       end
 
-      it 'returns false when user is not an approver' do
-        expect(merge_request.can_approve?(author)).to be false
+      context 'and author is not an approver' do
+        it 'returns true when authors can approve' do
+          project.update(merge_requests_author_approval: true)
+
+          expect(merge_request.can_approve?(author)).to be true
+        end
+
+        it 'returns false when authors cannot approve' do
+          project.update(merge_requests_author_approval: false)
+
+          expect(merge_request.can_approve?(author)).to be false
+        end
       end
     end
 
@@ -73,7 +83,7 @@ describe ApprovableForRule do
         project.add_developer(user)
       end
 
-      context 'and user is an approver' do
+      context 'and committer is an approver' do
         before do
           create(:approver, target: merge_request, user: user)
         end
@@ -91,8 +101,18 @@ describe ApprovableForRule do
         end
       end
 
-      it 'returns false when user is not an approver' do
-        expect(merge_request.can_approve?(user)).to be false
+      context 'and committer is not an approver' do
+        it 'return true when committers can approve' do
+          project.update(merge_requests_disable_committers_approval: false)
+
+          expect(merge_request.can_approve?(user)).to be true
+        end
+
+        it 'return false when committers cannot approve' do
+          project.update(merge_requests_disable_committers_approval: true)
+
+          expect(merge_request.can_approve?(user)).to be false
+        end
       end
     end
 

ee/spec/models/approval_state_spec.rb

@@ -442,8 +442,8 @@ describe ApprovalState do
         end
       end
 
-      def create_project_member(role)
-        user = create(:user)
+      def create_project_member(role, user_attrs = {})
+        user = create(:user, user_attrs)
         project.add_user(user, role)
         user
       end
@@ -453,11 +453,168 @@ describe ApprovalState do
       let(:author) { create_project_member(:developer) }
       let(:approver) { create_project_member(:developer) }
       let(:approver2) { create_project_member(:developer) }
+      let(:committer) { create_project_member(:developer, email: merge_request.commits.without_merge_commits.first.committer_email) }
       let(:developer) { create_project_member(:developer) }
       let(:other_developer) { create_project_member(:developer) }
       let(:reporter) { create_project_member(:reporter) }
       let(:stranger) { create(:user) }
 
+      context 'when there are no approval rules' do
+        before do
+          project.update!(approvals_before_merge: 1)
+        end
+
+        it_behaves_like 'a MR that all members with write access can approve'
+
+        it 'has fallback rules apply' do
+          expect(subject.use_fallback?).to be_truthy
+        end
+
+        it 'requires one approval' do
+          expect(subject.approvals_left).to eq(1)
+        end
+
+        context 'when authors are authorized to approve their own MRs' do
+          before do
+            project.update!(merge_requests_author_approval: true)
+          end
+
+          it 'allows the author to approve the MR if within the approvers list' do
+            expect(subject.can_approve?(author)).to be_truthy
+          end
+
+          it 'allows the author to approve the MR if not within the approvers list' do
+            allow(subject).to receive(:approvers).and_return([])
+
+            expect(subject.can_approve?(author)).to be_truthy
+          end
+
+          context 'when the author has approved the MR already' do
+            before do
+              create(:approval, user: author, merge_request: merge_request)
+            end
+
+            it 'does not allow the author to approve the MR again' do
+              expect(subject.can_approve?(author)).to be_falsey
+            end
+          end
+        end
+
+        context 'when authors are not authorized to approve their own MRs' do
+          before do
+            project.update!(merge_requests_author_approval: false)
+          end
+
+          it 'allows the author to approve the MR if within the approvers list' do
+            allow(subject).to receive(:approvers).and_return([author])
+
+            expect(subject.can_approve?(author)).to be_truthy
+          end
+
+          it 'does not allow the author to approve the MR if not within the approvers list' do
+            allow(subject).to receive(:approvers).and_return([])
+
+            expect(subject.can_approve?(author)).to be_falsey
+          end
+        end
+
+        context 'when committers are authorized to approve their own MRs' do
+          before do
+            project.update!(merge_requests_disable_committers_approval: false)
+          end
+
+          it 'allows the committer to approve the MR if within the approvers list' do
+            allow(subject).to receive(:approvers).and_return([committer])
+
+            expect(subject.can_approve?(committer)).to be_truthy
+          end
+
+          it 'allows the committer to approve the MR if not within the approvers list' do
+            allow(subject).to receive(:approvers).and_return([])
+
+            expect(subject.can_approve?(committer)).to be_truthy
+          end
+
+          context 'when the committer has approved the MR already' do
+            before do
+              create(:approval, user: committer, merge_request: merge_request)
+            end
+
+            it 'does not allow the committer to approve the MR again' do
+              expect(subject.can_approve?(committer)).to be_falsey
+            end
+          end
+        end
+
+        context 'when committers are not authorized to approve their own MRs' do
+          before do
+            project.update!(merge_requests_disable_committers_approval: true)
+          end
+
+          it 'allows the committer to approve the MR if within the approvers list' do
+            allow(subject).to receive(:approvers).and_return([committer])
+
+            expect(subject.can_approve?(committer)).to be_truthy
+          end
+
+          it 'does not allow the committer to approve the MR if not within the approvers list' do
+            allow(subject).to receive(:approvers).and_return([])
+
+            expect(subject.can_approve?(committer)).to be_falsey
+          end
+        end
+
+        context 'when the user is both an author and a committer' do
+          let(:user) { committer }
+
+          before do
+            merge_request.update!(author: committer)
+          end
+
+          context 'when authors are authorized to approve their own MRs, but not committers' do
+            before do
+              project.update!(
+                merge_requests_author_approval: true,
+                merge_requests_disable_committers_approval: true
+              )
+            end
+
+            it 'allows the user to approve the MR if within the approvers list' do
+              allow(subject).to receive(:approvers).and_return([user])
+
+              expect(subject.can_approve?(user)).to be_truthy
+            end
+
+            it 'does not allow the user to approve the MR if not within the approvers list' do
+              allow(subject).to receive(:approvers).and_return([])
+
+              expect(subject.can_approve?(user)).to be_falsey
+            end
+          end
+
+          context 'when committers are authorized to approve their own MRs, but not authors' do
+            before do
+              project.update!(
+                merge_requests_author_approval: false,
+                merge_requests_disable_committers_approval: false
+              )
+            end
+
+            it 'allows the user to approve the MR if within the approvers list' do
+              allow(subject).to receive(:approvers).and_return([user])
+
+              expect(subject.can_approve?(user)).to be_truthy
+            end
+
+            it 'allows the user to approve the MR if not within the approvers list' do
+              allow(subject).to receive(:approvers).and_return([])
+
+              expect(subject.can_approve?(user)).to be_truthy
+            end
+          end
+        end
+      end
+
       context 'when there is one approver required' do
         let!(:rule) { create_rule(approvals_required: 1) }