Memoize some queries in `Approvable`

ee/app/models/concerns/approvable.rb

 module Approvable
+  include Gitlab::Utils::StrongMemoize
+
   def requires_approve?
     approvals_required.nonzero?
   end
@@ -19,7 +21,7 @@ module Approvable
   end
 
   def approvals_required
-    @approvals_required ||= approvals_before_merge || target_project.approvals_before_merge
+    approvals_before_merge || target_project.approvals_before_merge
   end
 
   def approvals_before_merge
@@ -66,7 +68,9 @@ module Approvable
   # Users in the list of approvers who have not already approved this MR.
   #
   def approvers_left
-    User.where(id: all_approvers_including_groups.map(&:id)).where.not(id: approvals.select(:user_id))
+    strong_memoize(:approvers_left) do
+      User.where(id: all_approvers_including_groups.map(&:id)).where.not(id: approved_by_users.select(:id))
+    end
   end
 
   # The list of approvers from either this MR (if they've been set on the MR) or the
@@ -87,14 +91,16 @@ module Approvable
   end
 
   def all_approvers_including_groups
-    approvers = []
+    strong_memoize(:all_approvers_including_groups) do
+      approvers = []
 
-    # Approvers from direct assignment
-    approvers << approvers_from_users
+      # Approvers from direct assignment
+      approvers << approvers_from_users
 
-    approvers << approvers_from_groups
+      approvers << approvers_from_groups
 
-    approvers.flatten
+      approvers.flatten
+    end
   end
 
   def approvers_from_users
@@ -157,4 +163,12 @@ module Approvable
       approver_groups.find_or_initialize_by(group_id: group_id, target_id: id)
     end
   end
+
+  def reset_approval_cache!
+    approvals(true)
+    approved_by_users(true)
+
+    clear_memoization(:approvers_left)
+    clear_memoization(:all_approvers_including_groups)
+  end
 end

ee/app/services/merge_requests/approval_service.rb

@@ -4,6 +4,8 @@ module MergeRequests
       approval = merge_request.approvals.new(user: current_user)
 
       if approval.save
+        merge_request.reset_approval_cache!
+
         create_approval_note(merge_request)
         mark_pending_todos_as_done(merge_request)
 

ee/app/services/merge_requests/remove_approval_service.rb

@@ -9,9 +9,7 @@ module MergeRequests
       currently_approved = merge_request.approved?
 
       if approval.destroy_all
-        # bust the cache here, otherwise will show results from
-        # before the deletion
-        merge_request.approvals(true)
+        merge_request.reset_approval_cache!
 
         create_note(merge_request)
 

ee/app/views/shared/issuable/_approvals.html.haml

@@ -12,7 +12,7 @@
     Approvers
   .col-sm-10
     - if can_update_approvers
-      = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', email_user: true, skip_users: issuable.all_approvers_including_groups << ineligible_approver, project: issuable.target_project)
+      = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', email_user: true, skip_users: issuable.all_approvers_including_groups + [ineligible_approver], project: issuable.target_project)
       .help-block
         This merge request must be approved by these users.
         You can override the project settings by setting your own list of approvers.

ee/spec/models/approvable_spec.rb

+require 'spec_helper'
+
+describe Approvable do
+  let(:merge_request) { create(:merge_request, :with_approver) }
+
+  describe '#approvers_left' do
+    it 'only queries once' do
+      merge_request
+
+      expect(User).to receive(:where).and_call_original.once
+
+      3.times { merge_request.approvers_left }
+    end
+  end
+
+  describe '#reset_approval_cache!' do
+    it 'clears the cache of approvers left' do
+      user_can_approve = merge_request.approvers_left.first
+      merge_request.approvals.create!(user: user_can_approve)
+
+      merge_request.reset_approval_cache!
+
+      expect(merge_request.approvers_left).to be_empty
+    end
+  end
+end

ee/spec/services/merge_requests/approval_service_spec.rb

@@ -44,6 +44,12 @@ describe MergeRequests::ApprovalService do
         expect(todo.reload).to be_done
       end
 
+      it 'resets the cache for approvals' do
+        expect(merge_request).to receive(:reset_approval_cache!)
+
+        service.execute(merge_request)
+      end
+
       context 'with remaining approvals' do
         it 'does not fire a webhook' do
           expect(merge_request).to receive(:approvals_left).and_return(5)

ee/spec/services/merge_requests/remove_approval_service_spec.rb

@@ -34,6 +34,12 @@ describe MergeRequests::RemoveApprovalService do
 
         execute!
       end
+
+      it 'resets the cache for approvals' do
+        expect(merge_request).to receive(:reset_approval_cache!)
+
+        execute!
+      end
     end
 
     context 'with an approved merge request' do

spec/models/merge_request_spec.rb

@@ -692,10 +692,17 @@ describe MergeRequest do
     let(:author) { create(:user) }
     let(:merge_request) { create(:merge_request, source_project: project, author: author) }
 
+    def reloaded_merge_request
+      merge_request.reload
+      merge_request.reset_approval_cache!
+
+      merge_request
+    end
+
     it "includes approvers set on the MR" do
       expect do
         create(:approver, user: create(:user), target: merge_request)
-      end.to change { merge_request.reload.number_of_potential_approvers }.by(1)
+      end.to change { reloaded_merge_request.number_of_potential_approvers }.by(1)
     end
 
     it "includes approvers from group" do
@@ -703,7 +710,7 @@ describe MergeRequest do
 
       expect do
         create(:approver_group, group: group, target: merge_request)
-      end.to change { merge_request.reload.number_of_potential_approvers }.by(1)
+      end.to change { reloaded_merge_request.number_of_potential_approvers }.by(1)
     end
 
     it "includes project members with developer access and up" do
@@ -718,7 +725,7 @@ describe MergeRequest do
         # Add this user as both someone with access, and an explicit approver,
         # to ensure they aren't double-counted.
         create(:approver, user: developer, target: merge_request)
-      end.to change { merge_request.reload.number_of_potential_approvers }.by(2)
+      end.to change { reloaded_merge_request.number_of_potential_approvers }.by(2)
     end
 
     it "excludes users who have already approved the MR" do
@@ -726,14 +733,14 @@ describe MergeRequest do
         approver = create(:user)
         create(:approver, user: approver, target: merge_request)
         create(:approval, user: approver, merge_request: merge_request)
-      end.not_to change { merge_request.reload.number_of_potential_approvers }
+      end.not_to change { reloaded_merge_request.number_of_potential_approvers }
     end
 
     it "excludes the MR author" do
       expect do
         create(:approver, user: create(:user), target: merge_request)
         create(:approver, user: author, target: merge_request)
-      end.to change { merge_request.reload.number_of_potential_approvers }.by(1)
+      end.to change { reloaded_merge_request.number_of_potential_approvers }.by(1)
     end
 
     it "excludes blocked users" do
@@ -742,7 +749,7 @@ describe MergeRequest do
       project.add_developer(developer)
       project.add_developer(blocked_developer)
 
-      expect(merge_request.reload.number_of_potential_approvers).to eq(2)
+      expect(reloaded_merge_request.number_of_potential_approvers).to eq(2)
     end
 
     context "when the project is part of a group" do
@@ -760,7 +767,7 @@ describe MergeRequest do
           group.add_master(create(:user))
           blocked_developer = create(:user).tap { |u| u.block! }
           group.add_developer(blocked_developer)
-        end.to change { merge_request.reload.number_of_potential_approvers }.by(2)
+        end.to change { reloaded_merge_request.number_of_potential_approvers }.by(2)
       end
     end
   end