Resolve outdated diff discussions on push

app/helpers/notes_helper.rb

@@ -146,4 +146,8 @@ module NotesHelper
       autocomplete: autocomplete
     }
   end
+
+  def discussion_resolved_intro(discussion)
+    discussion.resolved_by_push? ? 'Automatically resolved' : 'Resolved'
+  end
 end

app/models/concerns/resolvable_discussion.rb

@@ -24,6 +24,7 @@ module ResolvableDiscussion
 
     delegate  :resolved_at,
               :resolved_by,
+              :resolved_by_push?,
 
               to: :last_resolved_note,
               allow_nil: true

app/models/concerns/resolvable_note.rb

@@ -51,22 +51,30 @@ module ResolvableNote
   end
 
   # If you update this method remember to also update `.resolve!`
-  def resolve!(current_user)
-    return unless resolvable?
-    return if resolved?
+  def resolve_without_save(current_user, resolved_by_push: false)
+    return false unless resolvable?
+    return false if resolved?
 
     self.resolved_at = Time.now
     self.resolved_by = current_user
-    save!
+    self.resolved_by_push = resolved_by_push
   end
 
   # If you update this method remember to also update `.unresolve!`
-  def unresolve!
-    return unless resolvable?
-    return unless resolved?
+  def unresolve_without_save(current_user)
+    return false unless resolvable?
+    return false unless resolved?
 
     self.resolved_at = nil
     self.resolved_by = nil
-    save!
+  end
+
+  def resolve!(current_user, resolved_by_push: false)
+    resolve_without_save(current_user, resolved_by_push: resolved_by_push) &&
+      save!
+  end
+
+  def unresolve!
+    unresolve_without_save(current_user) && save
   end
 end

app/models/merge_request.rb

@@ -918,6 +918,12 @@ class MergeRequest < ActiveRecord::Base
     active_diff_discussions.each do |discussion|
       service.execute(discussion)
     end
+
+    if project.resolve_outdated_diff_discussions?
+      MergeRequests::ResolvedDiscussionNotificationService
+        .new(project, current_user)
+        .execute(self)
+    end
   end
 
   def keep_around_commit

app/services/discussions/update_diff_position_service.rb

@@ -10,6 +10,10 @@ module Discussions
       discussion.notes.each do |note|
         if outdated
           note.change_position = position
+
+          if project.resolve_outdated_diff_discussions?
+            note.resolve_without_save(current_user, resolved_by_push: true)
+          end
         else
           note.position = position
           note.change_position = nil

app/views/discussions/_headline.html.haml

 - if discussion.resolved?
-  - if project.resolve_outdated_diff_discussions
-    .discussion-headline-light.js-discussion-headline
-      Automatically resolved
-      - if discussion.resolved_by
-        by
-        = link_to_member(@project, discussion.resolved_by, avatar: false)
-        with a push
-      = time_ago_with_tooltip(discussion.resolved_at, placement: "bottom")
-  - else
-    .discussion-headline-light.js-discussion-headline
-      Resolved
-      - if discussion.resolved_by
-        by
-        = link_to_member(@project, discussion.resolved_by, avatar: false)
-      = time_ago_with_tooltip(discussion.resolved_at, placement: "bottom")
+  .discussion-headline-light.js-discussion-headline
+    = discussion_resolved_intro(discussion)
+    - if discussion.resolved_by
+      by
+      = link_to_member(@project, discussion.resolved_by, avatar: false)
+    - if discussion.resolved_by_push?
+      with a push
+    = time_ago_with_tooltip(discussion.resolved_at, placement: "bottom")
 - elsif discussion.updated?
   .discussion-headline-light.js-discussion-headline
     Last updated

app/views/projects/_merge_request_merge_settings.html.haml

@@ -16,7 +16,7 @@
   .checkbox
     = form.label :resolve_outdated_diff_discussions do
       = form.check_box :resolve_outdated_diff_discussions
-      %strong Automatically resolve merge request diffs discussions on lines changed with a push
+      %strong Automatically resolve merge request diff discussions when they become outdated
   .checkbox
     = form.label :printing_merge_request_link_enabled do
       = form.check_box :printing_merge_request_link_enabled

db/migrate/20170905112933_add_resolved_by_push_to_notes.rb

+class AddResolvedByPushToNotes < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :notes, :resolved_by_push, :boolean
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170901071411) do
+ActiveRecord::Schema.define(version: 20170905112933) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1002,6 +1002,7 @@ ActiveRecord::Schema.define(version: 20170901071411) do
     t.text "note_html"
     t.integer "cached_markdown_version"
     t.text "change_position"
+    t.boolean "resolved_by_push"
   end
 
   add_index "notes", ["author_id"], name: "index_notes_on_author_id", using: :btree

doc/user/discussions/index.md

@@ -116,17 +116,22 @@ are resolved.
 
 ![Only allow merge if all the discussions are resolved message](img/only_allow_merge_if_all_discussions_are_resolved_msg.png)
 
-### Automatically resolve merge request diffs discussions on lines changed with a push
+### Automatically resolve merge request diff discussions when they become outdated
 
-You can automatically resolve merge request diff discussions on lines modified with a new commit.
+> [Introduced][ce-14053] in GitLab 10.0.
 
-Navigate to your project's settings page, select the
-**Automatically resolve merge request diffs discussions on lines changed with a push** check box and hit
+You can automatically resolve merge request diff discussions on lines modified
+with a new push.
+
+Navigate to your project's settings page, select the **Automatically resolve
+merge request diffs discussions on lines changed with a push** check box and hit
 **Save** for the changes to take effect.
 
-![Automatically resolve merge request diffs discussions on lines changed with a push](img/automatically_resolve_outdated_diff_discussions.png)
+![Automatically resolve merge request diff discussions when they become outdated](img/automatically_resolve_outdated_discussions.png)
 
-From now on, any discussions on an outdated diff will be resolved by default. 
+From now on, any discussions on a diff will be resolved by default if a push
+makes that diff section outdated. Discussions on lines that don't change and
+top-level resolvable discussions are not automatically resolved.
 
 ## Threaded discussions
 
@@ -153,6 +158,7 @@ comments in greater detail.
 [ce-7527]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7527
 [ce-7180]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7180
 [ce-8266]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8266
+[ce-14053]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14053
 [resolve-discussion-button]: img/resolve_discussion_button.png
 [resolve-comment-button]: img/resolve_comment_button.png
 [discussion-view]: img/discussion_view.png

spec/features/merge_requests/resolve_outdated_diff_discussions.rb

 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

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

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
 

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