Resolve outdated diff discussions on push

4 files changed, 149 insertions, 71 deletions

diff --git a/spec/features/merge_requests/resolve_outdated_diff_discussions.rb b/spec/features/merge_requests/resolve_outdated_diff_discussions.rb
index fd9b2c95210..55a82bdf2b9 100644
--- a/spec/features/merge_requests/resolve_outdated_diff_discussions.rb
+++ b/spec/features/merge_requests/resolve_outdated_diff_discussions.rb
@@ -1,80 +1,78 @@
 require 'spec_helper'
 
 feature 'Resolve outdated diff discussions', js: true do
-  let(:merge_request) { create(:merge_request, importing: true) }
-  let(:project) { merge_request.source_project }
+  let(:project) { create(:project, :repository, :public) }
 
-  let!(:outdated_discussion) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: outdated_position).to_discussion }
-  let!(:active_discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion }
+  let(:merge_request) do
+    create(:merge_request, source_project: project, source_branch: 'csv', target_branch: 'master')
+  end
+
+  let(:outdated_diff_refs) { project.commit('926c6595b263b2a40da6b17f3e3b7ea08344fad6').diff_refs }
+  let(:current_diff_refs) { merge_request.diff_refs }
 
   let(:outdated_position) do
     Gitlab::Diff::Position.new(
-      old_path: "files/ruby/popen.rb",
-      new_path: "files/ruby/popen.rb",
+      old_path: 'files/csv/Book1.csv',
+      new_path: 'files/csv/Book1.csv',
       old_line: nil,
       new_line: 9,
       diff_refs: outdated_diff_refs
     )
   end
 
-  let(:outdated_diff_refs) { project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e").diff_refs }
-
-  before do
-    sign_in(create(:admin))
+  let(:current_position) do
+    Gitlab::Diff::Position.new(
+      old_path: 'files/csv/Book1.csv',
+      new_path: 'files/csv/Book1.csv',
+      old_line: nil,
+      new_line: 1,
+      diff_refs: current_diff_refs
+    )
   end
 
-  context 'when project.resolve_outdated_diff_discussions == true' do
-    before do
-      project.update_column(:resolve_outdated_diff_discussions, true)
-    end
+  let!(:outdated_discussion) do
+    create(:diff_note_on_merge_request,
+           project: project,
+           noteable: merge_request,
+           position: outdated_position).to_discussion
+  end
 
-    context 'with unresolved outdated discussions' do
-      it 'does not show outdated discussion' do
-        visit_merge_request(merge_request)
-        within(".discussion[data-discussion-id='#{outdated_discussion.id}']") do
-          expect(page).to have_css('.discussion-body .hide .js-toggle-content', visible: false)
-          expect(page).to have_content('Automatically resolved')
-        end
-      end
-    end
+  let!(:current_discussion) do
+    create(:diff_note_on_merge_request,
+           noteable: merge_request,
+           project: project,
+           position: current_position).to_discussion
+  end
 
-    context 'with unresolved active discussions' do
-      it 'shows active discussion' do
-        visit_merge_request(merge_request)
-        within(".discussion[data-discussion-id='#{active_discussion.id}']") do
-          expect(page).to have_css('.discussion-body .hide .js-toggle-content', visible: true)
-          expect(page).not_to have_content('Automatically resolved')
-        end
-      end
-    end
+  before do
+    sign_in(merge_request.author)
   end
 
-  context 'when project.resolve_outdated_diff_discussions == false' do
+  context 'when a discussion was resolved by a push' do
     before do
-      project.update_column(:resolve_outdated_diff_discussions, false)
+      project.update!(resolve_outdated_diff_discussions: true)
+
+      merge_request.update_diff_discussion_positions(
+        old_diff_refs: outdated_diff_refs,
+        new_diff_refs: current_diff_refs,
+        current_user: merge_request.author
+      )
+
+      visit project_merge_request_path(project, merge_request)
     end
 
-    context 'with unresolved outdated discussions' do
-      it 'shows outdated discussion' do
-        visit_merge_request(merge_request)
-        within(".discussion[data-discussion-id='#{outdated_discussion.id}']") do
-          expect(page).to have_css('.discussion-body .hide .js-toggle-content', visible: true)
-          expect(page).not_to have_content('Automatically resolved')
-        end
+    it 'shows that as automatically resolved' do
+      within(".discussion[data-discussion-id='#{outdated_discussion.id}']") do
+        expect(page).to have_css('.discussion-body', visible: false)
+        expect(page).to have_content('Automatically resolved')
       end
     end
 
-    context 'with unresolved active discussions' do
-      it 'shows active discussion' do
-        visit_merge_request(merge_request)
-        within(".discussion[data-discussion-id='#{active_discussion.id}']") do
-          expect(page).to have_css('.discussion-body .hide .js-toggle-content', visible: true)
-          expect(page).not_to have_content('Automatically resolved')
-        end
+    it 'does not show that for active discussions' do
+      within(".discussion[data-discussion-id='#{current_discussion.id}']") do
+        expect(page).to have_css('.discussion-body', visible: true)
+        expect(page).not_to have_content('Automatically resolved')
       end
     end
   end
-  def visit_merge_request(merge_request)
-    visit project_merge_request_path(project, merge_request)
-  end
 end
diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb
index 9921ca1af33..68540dd4e59 100644
--- a/spec/helpers/notes_helper_spec.rb
+++ b/spec/helpers/notes_helper_spec.rb
@@ -231,7 +231,7 @@ describe NotesHelper do
     end
   end
 
-  describe '#form_resurces' do
+  describe '#form_resources' do
     it 'returns note for personal snippet' do
       @snippet = create(:personal_snippet)
       @note = create(:note_on_personal_snippet)
@@ -266,4 +266,22 @@ describe NotesHelper do
       expect(noteable_note_url(note)).to match("/#{project.namespace.path}/#{project.path}/issues/#{issue.iid}##{dom_id(note)}")
     end
   end
+
+  describe '#discussion_resolved_intro' do
+    context 'when the discussion was resolved by a push' do
+      let(:discussion) { double(:discussion, resolved_by_push?: true) }
+
+      it 'returns "Automatically resolved"' do
+        expect(discussion_resolved_intro(discussion)).to eq('Automatically resolved')
+      end
+    end
+
+    context 'when the discussion was not resolved by a push' do
+      let(:discussion) { double(:discussion, resolved_by_push?: false) }
+
+      it 'returns "Resolved"' do
+        expect(discussion_resolved_intro(discussion)).to eq('Resolved')
+      end
+    end
+  end
 end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index f5d079c27c4..d80d5657c42 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1262,7 +1262,6 @@ describe MergeRequest do
 
   describe "#reload_diff" do
     let(:discussion) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject).to_discussion }
-
     let(:commit) { subject.project.commit(sample_commit.id) }
 
     it "does not change existing merge request diff" do
@@ -1280,9 +1279,19 @@ describe MergeRequest do
       subject.reload_diff
     end
 
-    it "updates diff discussion positions" do
-      old_diff_refs = subject.diff_refs
+    it "calls update_diff_discussion_positions" do
+      expect(subject).to receive(:update_diff_discussion_positions)
+
+      subject.reload_diff
+    end
+  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 }
+
+    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(
@@ -1293,7 +1302,9 @@ describe MergeRequest do
 
         subject.merge_request_diff(true)
       end
+    end
 
+    it "updates diff discussion positions" do
       expect(Discussions::UpdateDiffPositionService).to receive(:new).with(
         subject.project,
         subject.author,
@@ -1305,7 +1316,26 @@ describe MergeRequest do
       expect_any_instance_of(Discussions::UpdateDiffPositionService).to receive(:execute).with(discussion).and_call_original
       expect_any_instance_of(DiffNote).to receive(:save).once
 
-      subject.reload_diff(subject.author)
+      subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs,
+                                               new_diff_refs: commit.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)
+
+        subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs,
+                                                 new_diff_refs: commit.diff_refs,
+                                                 current_user: subject.author)
+      end
     end
   end
 
diff --git a/spec/services/discussions/update_diff_position_service_spec.rb b/spec/services/discussions/update_diff_position_service_spec.rb
index c239494298b..82b156f5ebe 100644
--- a/spec/services/discussions/update_diff_position_service_spec.rb
+++ b/spec/services/discussions/update_diff_position_service_spec.rb
@@ -150,21 +150,7 @@ describe Discussions::UpdateDiffPositionService do
       )
     end
 
-    context "when the diff line is the same" do
-      let(:line) { 16 }
-
-      it "updates the position" do
-        subject.execute(discussion)
-
-        expect(discussion.original_position).to eq(old_position)
-        expect(discussion.position).not_to eq(old_position)
-        expect(discussion.position.new_line).to eq(22)
-      end
-    end
-
-    context "when the diff line has changed" do
-      let(:line) { 9 }
-
+    shared_examples 'outdated diff note' do
       it "doesn't update the position" do
         subject.execute(discussion)
 
@@ -189,5 +175,51 @@ describe Discussions::UpdateDiffPositionService do
         subject.execute(discussion)
       end
     end
+
+    context "when the diff line is the same" do
+      let(:line) { 16 }
+
+      it "updates the position" do
+        subject.execute(discussion)
+
+        expect(discussion.original_position).to eq(old_position)
+        expect(discussion.position).not_to eq(old_position)
+        expect(discussion.position.new_line).to eq(22)
+      end
+
+      context 'when the resolve_outdated_diff_discussions setting is set' do
+        before do
+          project.update!(resolve_outdated_diff_discussions: true)
+        end
+
+        it 'does not resolve the discussion' do
+          subject.execute(discussion)
+
+          expect(discussion).not_to be_resolved
+          expect(discussion).not_to be_resolved_by_push
+        end
+      end
+    end
+
+    context "when the diff line has changed" do
+      let(:line) { 9 }
+
+      include_examples 'outdated diff note'
+
+      context 'when the resolve_outdated_diff_discussions setting is set' do
+        before do
+          project.update!(resolve_outdated_diff_discussions: true)
+        end
+
+        it 'sets resolves the discussion and sets resolved_by_push' do
+          subject.execute(discussion)
+
+          expect(discussion).to be_resolved
+          expect(discussion).to be_resolved_by_push
+        end
+
+        include_examples 'outdated diff note'
+      end
+    end
   end
 end