Only call the resolved service if we resolve all the active notes

MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/79142
Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/350397

We are looping over the 'active' discussions, active is when
the discussion is on an 'active' diff (not outdated), regardless of it is resolved or
not. We then were calling the ResolvedNotificationService, which only
checks if the discussions are resolved. But these two are mututaly
exclusive, one can be active _and_ resolved.

So in the case that we have only one active resolved discussion which
doesn't get outdated, the ResolvedNotificationService will be called
each time this method is called, and this service will handle it as if
we have just resolved all the discussions, when in fact we may not have
resolved any discussions when calling the UpdateDiffPositionService.

So we now keep track of if all the active discussions are resolved before
calling the UpdateDiffPositionService, and then compare this to after
the update service is called, and depending on this, we may or may not
call the ResolvedNotification service.

If we go from:
Before Update Service call | After Update service call | Call Resolved
False | True | True
False | False | False
True | True | False

We only want to call the ResolvedNotificationService if we have go from
having unresolved active, to all the active being resolved, otherwise there is no need
to call it as the check in ResolvedNotification will be false. And when
all the active discussions are already resolved, the notification
service will already have been called, so no need to call it again.

Changelog: fixed

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
-      end
+    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,36 +3616,67 @@ 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
 
-      it 'calls MergeRequests::ResolvedDiscussionNotificationService' do
-        expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService)
-          .to receive(:execute).with(subject)
+      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: 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 }
 
-        subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs,
-                                                 new_diff_refs: commit.diff_refs,
-                                                 current_user: subject.author)
+        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