Merge branch 'reload_diff_in_update_service_part_2' into 'master'

Only call the resolved notification service if we have resolved all the notes

See merge request gitlab-org/gitlab!79142

app/models/concerns/resolvable_discussion.rb

@@ -99,6 +99,12 @@ module ResolvableDiscussion
     update { |notes| notes.unresolve! }
   end
 
+  def clear_memoized_values
+    self.class.memoized_values.each do |name|
+      clear_memoization(name)
+    end
+  end
+
   private
 
   def update
@@ -110,8 +116,6 @@ module ResolvableDiscussion
     # Set the notes array to the updated notes
     @notes = notes_relation.fresh.to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
-    self.class.memoized_values.each do |name|
-      clear_memoization(name)
-    end
+    clear_memoized_values
   end
 end

app/models/merge_request.rb

@@ -1754,6 +1754,8 @@ class MergeRequest < ApplicationRecord
 
     paths = active_diff_discussions.flat_map { |n| n.diff_file.paths }.uniq
 
+    active_discussions_resolved = active_diff_discussions.all?(&:resolved?)
+
     service = Discussions::UpdateDiffPositionService.new(
       self.project,
       current_user,
@@ -1764,9 +1766,15 @@ class MergeRequest < ApplicationRecord
 
     active_diff_discussions.each do |discussion|
       service.execute(discussion)
+      discussion.clear_memoized_values
     end
 
-    if project.resolve_outdated_diff_discussions?
+    # If they were all already resolved, this method will have already been called.
+    # If they all don't get resolved, we don't need to call the method
+    # If they go from unresolved -> resolved, then we call the method
+    if !active_discussions_resolved &&
+        active_diff_discussions.all?(&:resolved?) &&
+        project.resolve_outdated_diff_discussions?
       MergeRequests::ResolvedDiscussionNotificationService
         .new(project: project, current_user: current_user)
         .execute(self)

spec/models/concerns/resolvable_discussion_spec.rb

@@ -584,4 +584,14 @@ RSpec.describe Discussion, ResolvableDiscussion do
       expect(subject.last_resolved_note).to eq(second_note)
     end
   end
+
+  describe '#clear_memoized_values' do
+    it 'resets the memoized values' do
+      described_class.memoized_values.each do |memo|
+        subject.instance_variable_set("@#{memo}", 'memoized')
+        expect { subject.clear_memoized_values }.to change { subject.instance_variable_get("@#{memo}") }
+          .from('memoized').to(nil)
+      end
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -3577,21 +3577,38 @@ RSpec.describe MergeRequest, factory_default: :keep do
   end
 
   describe '#update_diff_discussion_positions' do
-    let(:discussion) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject).to_discussion }
-    let(:commit) { subject.project.commit(sample_commit.id) }
-    let(:old_diff_refs) { subject.diff_refs }
+    subject { create(:merge_request, source_project: project) }
 
-    before do
-      # Update merge_request_diff so that #diff_refs will return commit.diff_refs
-      allow(subject).to receive(:create_merge_request_diff) do
-        subject.merge_request_diffs.create!(
-          base_commit_sha: commit.parent_id,
-          start_commit_sha: commit.parent_id,
-          head_commit_sha: commit.sha
+    let(:project) { create(:project, :repository) }
+    let(:create_commit) { project.commit("913c66a37b4a45b9769037c55c2d238bd0942d2e") }
+    let(:modify_commit) { project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e") }
+    let(:edit_commit) { project.commit("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") }
+    let(:discussion) { create(:diff_note_on_merge_request, noteable: subject, project: project, position: old_position).to_discussion }
+    let(:path) { "files/ruby/popen.rb" }
+    let(:new_line) { 9 }
+
+    let(:old_diff_refs) do
+      Gitlab::Diff::DiffRefs.new(
+        base_sha: create_commit.parent_id,
+        head_sha: modify_commit.sha
       )
+    end
 
-        subject.reload_merge_request_diff
+    let(:new_diff_refs) do
+      Gitlab::Diff::DiffRefs.new(
+        base_sha: create_commit.parent_id,
+        head_sha: edit_commit.sha
+      )
     end
+
+    let(:old_position) do
+      Gitlab::Diff::Position.new(
+        old_path: path,
+        new_path: path,
+        old_line: nil,
+        new_line: new_line,
+        diff_refs: old_diff_refs
+      )
     end
 
     it "updates diff discussion positions" do
@@ -3599,38 +3616,69 @@ RSpec.describe MergeRequest, factory_default: :keep do
         subject.project,
         subject.author,
         old_diff_refs: old_diff_refs,
-        new_diff_refs: commit.diff_refs,
+        new_diff_refs: new_diff_refs,
         paths: discussion.position.paths
       ).and_call_original
 
       expect_any_instance_of(Discussions::UpdateDiffPositionService).to receive(:execute).with(discussion).and_call_original
-      expect_any_instance_of(DiffNote).to receive(:save).once
 
       subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs,
-                                               new_diff_refs: commit.diff_refs,
+                                               new_diff_refs: new_diff_refs,
                                                current_user: subject.author)
     end
 
-    context 'when resolve_outdated_diff_discussions is set' do
-      let(:project) { create(:project, :repository) }
+    it 'does not call the resolve method' do
+      expect(MergeRequests::ResolvedDiscussionNotificationService).not_to receive(:new)
 
-      subject { create(:merge_request, source_project: project) }
+      subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs,
+                                               new_diff_refs: new_diff_refs,
+                                               current_user: subject.author)
+    end
 
+    context 'when resolve_outdated_diff_discussions is set' do
       before do
         discussion
 
         subject.project.update!(resolve_outdated_diff_discussions: true)
       end
 
+      context 'when the active discussion is resolved in the update' do
         it 'calls MergeRequests::ResolvedDiscussionNotificationService' do
           expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService)
             .to receive(:execute).with(subject)
 
           subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs,
-                                                 new_diff_refs: commit.diff_refs,
+                                                   new_diff_refs: new_diff_refs,
                                                    current_user: subject.author)
         end
       end
+
+      context 'when the active discussion does not have resolved in the update' do
+        let(:new_line) { 16 }
+
+        it 'does not call the resolve method' do
+          expect(MergeRequests::ResolvedDiscussionNotificationService).not_to receive(:new)
+
+          subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs,
+                                                   new_diff_refs: new_diff_refs,
+                                                   current_user: subject.author)
+        end
+      end
+
+      context 'when the active discussion was already resolved' do
+        before do
+          discussion.resolve!(subject.author)
+        end
+
+        it 'does not call the resolve method' do
+          expect(MergeRequests::ResolvedDiscussionNotificationService).not_to receive(:new)
+
+          subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs,
+                                                   new_diff_refs: new_diff_refs,
+                                                   current_user: subject.author)
+        end
+      end
+    end
   end
 
   describe '#branch_merge_base_commit' do