Merge branch...

Merge branch 'AshleyDumaine/gitlab-ce-36994-toggle-for-automatically-collapsing-outdated-diff-comments' into 'master'

Add setting to collapse outdated diff comments on push

Closes #36994

See merge request !14053

app/controllers/projects_controller.rb

@@ -323,6 +323,7 @@ class ProjectsController < Projects::ApplicationController
       :build_allow_git_fetch,
       :build_coverage_regex,
       :build_timeout_in_minutes,
+      :resolve_outdated_diff_discussions,
       :container_registry_enabled,
       :default_branch,
       :description,

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,34 @@ 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
+
+    true
   end
 
   # If you update this method remember to also update `.unresolve!`
-  def unresolve!
-    return unless resolvable?
-    return unless resolved?
+  def unresolve_without_save
+    return false unless resolvable?
+    return false unless resolved?
 
     self.resolved_at = nil
     self.resolved_by = nil
-    save!
+
+    true
+  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 && 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/models/project.rb

@@ -37,6 +37,7 @@ class Project < ActiveRecord::Base
 
   default_value_for :archived, false
   default_value_for :visibility_level, gitlab_config_features.visibility_level
+  default_value_for :resolve_outdated_diff_discussions, false
   default_value_for :container_registry_enabled, gitlab_config_features.container_registry
   default_value_for(:repository_storage) { current_application_settings.pick_repository_storage }
   default_value_for(:shared_runners_enabled) { current_application_settings.shared_runners_enabled }

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?
   .discussion-headline-light.js-discussion-headline
-    Resolved
+    = 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

app/views/projects/_merge_request_merge_settings.html.haml

@@ -13,6 +13,10 @@
     = form.label :only_allow_merge_if_all_discussions_are_resolved do
       = form.check_box :only_allow_merge_if_all_discussions_are_resolved
       %strong Only allow merge requests to be merged if all discussions are resolved
+  .checkbox
+    = form.label :resolve_outdated_diff_discussions do
+      = form.check_box :resolve_outdated_diff_discussions
+      %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

changelogs/unreleased/36994-toggle-for-automatically-collapsing-outdated-diff-comments.yml

+---
+title: Add repository toggle for automatically resolving outdated diff discussions
+merge_request: 14053
+author: AshleyDumaine
+type: added

db/migrate/20170825154015_resolve_outdated_diff_discussions.rb

+class ResolveOutdatedDiffDiscussions < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column(:projects, :resolve_outdated_diff_discussions, :boolean)
+  end
+end

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
@@ -1219,6 +1220,7 @@ ActiveRecord::Schema.define(version: 20170901071411) do
     t.string "ci_config_path"
     t.text "delete_error"
     t.integer "storage_version", limit: 2
+    t.boolean "resolve_outdated_diff_discussions"
   end
 
   add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree

doc/api/projects.md

@@ -70,6 +70,7 @@ Parameters:
     "jobs_enabled": true,
     "wiki_enabled": true,
     "snippets_enabled": false,
+    "resolve_outdated_diff_discussions": false,
     "container_registry_enabled": false,
     "created_at": "2013-09-30T13:46:02Z",
     "last_activity_at": "2013-09-30T13:46:02Z",
@@ -137,6 +138,7 @@ Parameters:
     "jobs_enabled": true,
     "wiki_enabled": true,
     "snippets_enabled": false,
+    "resolve_outdated_diff_discussions": false,
     "container_registry_enabled": false,
     "created_at": "2013-09-30T13:46:02Z",
     "last_activity_at": "2013-09-30T13:46:02Z",
@@ -246,6 +248,7 @@ Parameters:
     "jobs_enabled": true,
     "wiki_enabled": true,
     "snippets_enabled": false,
+    "resolve_outdated_diff_discussions": false,
     "container_registry_enabled": false,
     "created_at": "2013-09-30T13:46:02Z",
     "last_activity_at": "2013-09-30T13:46:02Z",
@@ -313,6 +316,7 @@ Parameters:
     "jobs_enabled": true,
     "wiki_enabled": true,
     "snippets_enabled": false,
+    "resolve_outdated_diff_discussions": false,
     "container_registry_enabled": false,
     "created_at": "2013-09-30T13:46:02Z",
     "last_activity_at": "2013-09-30T13:46:02Z",
@@ -411,6 +415,7 @@ Parameters:
   "jobs_enabled": true,
   "wiki_enabled": true,
   "snippets_enabled": false,
+  "resolve_outdated_diff_discussions": false,
   "container_registry_enabled": false,
   "created_at": "2013-09-30T13:46:02Z",
   "last_activity_at": "2013-09-30T13:46:02Z",
@@ -537,6 +542,7 @@ Parameters:
 | `jobs_enabled` | boolean | no | Enable jobs for this project |
 | `wiki_enabled` | boolean | no | Enable wiki for this project |
 | `snippets_enabled` | boolean | no | Enable snippets for this project |
+| `resolve_outdated_diff_discussions` | boolean | no | Automatically resolve merge request diffs discussions on lines changed with a push |
 | `container_registry_enabled` | boolean | no | Enable container registry for this project |
 | `shared_runners_enabled` | boolean | no | Enable shared runners for this project |
 | `visibility` | string | no | See [project visibility level](#project-visibility-level) |
@@ -574,6 +580,7 @@ Parameters:
 | `jobs_enabled` | boolean | no | Enable jobs for this project |
 | `wiki_enabled` | boolean | no | Enable wiki for this project |
 | `snippets_enabled` | boolean | no | Enable snippets for this project |
+| `resolve_outdated_diff_discussions` | boolean | no | Automatically resolve merge request diffs discussions on lines changed with a push |
 | `container_registry_enabled` | boolean | no | Enable container registry for this project |
 | `shared_runners_enabled` | boolean | no | Enable shared runners for this project |
 | `visibility` | string | no | See [project visibility level](#project-visibility-level) |
@@ -610,6 +617,7 @@ Parameters:
 | `jobs_enabled` | boolean | no | Enable jobs for this project |
 | `wiki_enabled` | boolean | no | Enable wiki for this project |
 | `snippets_enabled` | boolean | no | Enable snippets for this project |
+| `resolve_outdated_diff_discussions` | boolean | no | Automatically resolve merge request diffs discussions on lines changed with a push |
 | `container_registry_enabled` | boolean | no | Enable container registry for this project |
 | `shared_runners_enabled` | boolean | no | Enable shared runners for this project |
 | `visibility` | string | no | See [project visibility level](#project-visibility-level) |
@@ -683,6 +691,7 @@ Example response:
   "jobs_enabled": true,
   "wiki_enabled": true,
   "snippets_enabled": false,
+  "resolve_outdated_diff_discussions": false,
   "container_registry_enabled": false,
   "created_at": "2013-09-30T13:46:02Z",
   "last_activity_at": "2013-09-30T13:46:02Z",
@@ -758,6 +767,7 @@ Example response:
   "jobs_enabled": true,
   "wiki_enabled": true,
   "snippets_enabled": false,
+  "resolve_outdated_diff_discussions": false,
   "container_registry_enabled": false,
   "created_at": "2013-09-30T13:46:02Z",
   "last_activity_at": "2013-09-30T13:46:02Z",
@@ -839,6 +849,7 @@ Example response:
   "jobs_enabled": true,
   "wiki_enabled": true,
   "snippets_enabled": false,
+  "resolve_outdated_diff_discussions": false,
   "container_registry_enabled": false,
   "created_at": "2013-09-30T13:46:02Z",
   "last_activity_at": "2013-09-30T13:46:02Z",
@@ -932,6 +943,7 @@ Example response:
   "jobs_enabled": true,
   "wiki_enabled": true,
   "snippets_enabled": false,
+  "resolve_outdated_diff_discussions": false,
   "container_registry_enabled": false,
   "created_at": "2013-09-30T13:46:02Z",
   "last_activity_at": "2013-09-30T13:46:02Z",

doc/user/discussions/index.md

@@ -116,6 +116,23 @@ 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 diff discussions when they become outdated
+
+> [Introduced][ce-14053] in GitLab 10.0.
+
+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 diff discussions when they become outdated](img/automatically_resolve_outdated_discussions.png)
+
+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
 
 > [Introduced][ce-7527] in GitLab 9.1.
@@ -141,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

lib/api/entities.rb

@@ -119,6 +119,7 @@ module API
       expose :archived?, as: :archived
       expose :visibility
       expose :owner, using: Entities::UserBasic, unless: ->(project, options) { project.group }
+      expose :resolve_outdated_diff_discussions
       expose :container_registry_enabled
 
       # Expose old field names with the new permissions methods to keep API compatible

lib/api/projects.rb

@@ -16,6 +16,7 @@ module API
         optional :jobs_enabled, type: Boolean, desc: 'Flag indication if jobs are enabled'
         optional :snippets_enabled, type: Boolean, desc: 'Flag indication if snippets are enabled'
         optional :shared_runners_enabled, type: Boolean, desc: 'Flag indication if shared runners are enabled for that project'
+        optional :resolve_outdated_diff_discussions, type: Boolean, desc: 'Automatically resolve merge request diffs discussions on lines changed with a push'
         optional :container_registry_enabled, type: Boolean, desc: 'Flag indication if the container registry is enabled for that project'
         optional :lfs_enabled, type: Boolean, desc: 'Flag indication if Git LFS is enabled for that project'
         optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The visibility of the project.'
@@ -236,6 +237,7 @@ module API
         at_least_one_of_ce =
           [
             :jobs_enabled,
+            :resolve_outdated_diff_discussions,
             :container_registry_enabled,
             :default_branch,
             :description,

lib/api/v3/entities.rb

@@ -64,6 +64,7 @@ module API
         expose :owner, using: ::API::Entities::UserBasic, unless: ->(project, options) { project.group }
         expose :name, :name_with_namespace
         expose :path, :path_with_namespace
+        expose :resolve_outdated_diff_discussions
         expose :container_registry_enabled
 
         # Expose old field names with the new permissions methods to keep API compatible

lib/api/v3/projects.rb

@@ -18,6 +18,7 @@ module API
           optional :builds_enabled, type: Boolean, desc: 'Flag indication if builds are enabled'
           optional :snippets_enabled, type: Boolean, desc: 'Flag indication if snippets are enabled'
           optional :shared_runners_enabled, type: Boolean, desc: 'Flag indication if shared runners are enabled for that project'
+          optional :resolve_outdated_diff_discussions, type: Boolean, desc: 'Automatically resolve merge request diffs discussions on lines changed with a push'
           optional :container_registry_enabled, type: Boolean, desc: 'Flag indication if the container registry is enabled for that project'
           optional :lfs_enabled, type: Boolean, desc: 'Flag indication if Git LFS is enabled for that project'
           optional :public, type: Boolean, desc: 'Create a public project. The same as visibility_level = 20.'
@@ -296,9 +297,9 @@ module API
           use :optional_params
           at_least_one_of :name, :description, :issues_enabled, :merge_requests_enabled,
             :wiki_enabled, :builds_enabled, :snippets_enabled,
-            :shared_runners_enabled, :container_registry_enabled,
-            :lfs_enabled, :public, :visibility_level, :public_builds,
-            :request_access_enabled, :only_allow_merge_if_build_succeeds,
+            :shared_runners_enabled, :resolve_outdated_diff_discussions,
+            :container_registry_enabled, :lfs_enabled, :public, :visibility_level,
+            :public_builds, :request_access_enabled, :only_allow_merge_if_build_succeeds,
             :only_allow_merge_if_all_discussions_are_resolved, :path,
             :default_branch
         end

spec/features/merge_requests/resolve_outdated_diff_discussions.rb

+require 'spec_helper'
+
+feature 'Resolve outdated diff discussions', js: true do
+  let(:project) { create(:project, :repository, :public) }
+
+  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/csv/Book1.csv',
+      new_path: 'files/csv/Book1.csv',
+      old_line: nil,
+      new_line: 9,
+      diff_refs: outdated_diff_refs
+    )
+  end
+
+  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
+
+  let!(:outdated_discussion) do
+    create(:diff_note_on_merge_request,
+           project: project,
+           noteable: merge_request,
+           position: outdated_position).to_discussion
+  end
+
+  let!(:current_discussion) do
+    create(:diff_note_on_merge_request,
+           noteable: merge_request,
+           project: project,
+           position: current_position).to_discussion
+  end
+
+  before do
+    sign_in(merge_request.author)
+  end
+
+  context 'when a discussion was resolved by a push' do
+    before do
+      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
+
+    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
+
+    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
+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/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb

@@ -214,7 +214,7 @@ end
 # The background migration relies on a temporary table, hence we're migrating
 # to a specific version of the database where said table is still present.
 #
-describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads, :migration, schema: 20170608152748 do
+describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads, :migration, schema: 20170825154015 do
   let(:migration) { described_class.new }
   let(:project) { create(:project_empty_repo) }
   let(:author) { create(:user) }

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -65,6 +65,7 @@ Note:
 - change_position
 - resolved_at
 - resolved_by_id
+- resolved_by_push
 - discussion_id
 - original_discussion_id
 LabelLink:
@@ -398,6 +399,7 @@ Project:
 - public_builds
 - last_repository_check_failed
 - last_repository_check_at
+- collapse_outdated_diff_comments
 - container_registry_enabled
 - only_allow_merge_if_pipeline_succeeds
 - has_external_issue_tracker
@@ -406,6 +408,7 @@ Project:
 - only_allow_merge_if_all_discussions_are_resolved
 - auto_cancel_pending_pipelines
 - printing_merge_request_link_enabled
+- resolve_outdated_diff_discussions
 - build_allow_git_fetch
 - last_repository_updated_at
 - ci_config_path

spec/models/concerns/resolvable_note_spec.rb

@@ -189,8 +189,8 @@ describe Note, ResolvableNote do
         allow(subject).to receive(:resolvable?).and_return(false)
       end
 
-      it "returns nil" do
-        expect(subject.resolve!(current_user)).to be_nil
+      it "returns false" do
+        expect(subject.resolve!(current_user)).to be_falsey
       end
 
       it "doesn't set resolved_at" do
@@ -224,8 +224,8 @@ describe Note, ResolvableNote do
           subject.resolve!(user)
         end
 
-        it "returns nil" do
-          expect(subject.resolve!(current_user)).to be_nil
+        it "returns false" do
+          expect(subject.resolve!(current_user)).to be_falsey
         end
 
         it "doesn't change resolved_at" do
@@ -279,8 +279,8 @@ describe Note, ResolvableNote do
         allow(subject).to receive(:resolvable?).and_return(false)
       end
 
-      it "returns nil" do
-        expect(subject.unresolve!).to be_nil
+      it "returns false" do
+        expect(subject.unresolve!).to be_falsey
       end
     end
 
@@ -320,8 +320,8 @@ describe Note, ResolvableNote do
       end
 
       context "when not resolved" do
-        it "returns nil" do
-          expect(subject.unresolve!).to be_nil
+        it "returns false" do
+          expect(subject.unresolve!).to be_falsey
         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/requests/api/projects_spec.rb

@@ -414,6 +414,7 @@ describe API::Projects do
         jobs_enabled: false,
         merge_requests_enabled: false,
         wiki_enabled: false,
+        resolve_outdated_diff_discussions: false,
         only_allow_merge_if_pipeline_succeeds: false,
         request_access_enabled: true,
         only_allow_merge_if_all_discussions_are_resolved: false,
@@ -477,20 +478,40 @@ describe API::Projects do
       expect(json_response['avatar_url']).to eq("http://localhost/uploads/-/system/project/avatar/#{project_id}/banana_sample.gif")
     end
 
+    it 'sets a project as allowing outdated diff discussions to automatically resolve' do
+      project = attributes_for(:project, resolve_outdated_diff_discussions: false)
+
+      post api('/projects', user), project
+
+      expect(json_response['resolve_outdated_diff_discussions']).to be_falsey
+    end
+
+    it 'sets a project as allowing outdated diff discussions to automatically resolve if resolve_outdated_diff_discussions' do
+      project = attributes_for(:project, resolve_outdated_diff_discussions: true)
+
+      post api('/projects', user), project
+
+      expect(json_response['resolve_outdated_diff_discussions']).to be_truthy
+    end
+
     it 'sets a project as allowing merge even if build fails' do
-      project = attributes_for(:project, { only_allow_merge_if_pipeline_succeeds: false })
+      project = attributes_for(:project, only_allow_merge_if_pipeline_succeeds: false)
+
       post api('/projects', user), project
+
       expect(json_response['only_allow_merge_if_pipeline_succeeds']).to be_falsey
     end
 
     it 'sets a project as allowing merge only if merge_when_pipeline_succeeds' do
-      project = attributes_for(:project, { only_allow_merge_if_pipeline_succeeds: true })
+      project = attributes_for(:project, only_allow_merge_if_pipeline_succeeds: true)
+
       post api('/projects', user), project
+
       expect(json_response['only_allow_merge_if_pipeline_succeeds']).to be_truthy
     end
 
     it 'sets a project as allowing merge even if discussions are unresolved' do
-      project = attributes_for(:project, { only_allow_merge_if_all_discussions_are_resolved: false })
+      project = attributes_for(:project, only_allow_merge_if_all_discussions_are_resolved: false)
 
       post api('/projects', user), project
 
@@ -506,7 +527,7 @@ describe API::Projects do
     end
 
     it 'sets a project as allowing merge only if all discussions are resolved' do
-      project = attributes_for(:project, { only_allow_merge_if_all_discussions_are_resolved: true })
+      project = attributes_for(:project, only_allow_merge_if_all_discussions_are_resolved: true)
 
       post api('/projects', user), project
 
@@ -514,7 +535,7 @@ describe API::Projects do
     end
 
     it 'ignores import_url when it is nil' do
-      project = attributes_for(:project, { import_url: nil })
+      project = attributes_for(:project, import_url: nil)
 
       post api('/projects', user), project
 
@@ -642,20 +663,36 @@ describe API::Projects do
       expect(json_response['visibility']).to eq('private')
     end
 
+    it 'sets a project as allowing outdated diff discussions to automatically resolve' do
+      project = attributes_for(:project, resolve_outdated_diff_discussions: false)
+
+      post api("/projects/user/#{user.id}", admin), project
+
+      expect(json_response['resolve_outdated_diff_discussions']).to be_falsey
+    end
+
+    it 'sets a project as allowing outdated diff discussions to automatically resolve' do
+      project = attributes_for(:project, resolve_outdated_diff_discussions: true)
+
+      post api("/projects/user/#{user.id}", admin), project
+
+      expect(json_response['resolve_outdated_diff_discussions']).to be_truthy
+    end
+
     it 'sets a project as allowing merge even if build fails' do
-      project = attributes_for(:project, { only_allow_merge_if_pipeline_succeeds: false })
+      project = attributes_for(:project, only_allow_merge_if_pipeline_succeeds: false)
       post api("/projects/user/#{user.id}", admin), project
       expect(json_response['only_allow_merge_if_pipeline_succeeds']).to be_falsey
     end
 
-    it 'sets a project as allowing merge only if merge_when_pipeline_succeeds' do
-      project = attributes_for(:project, { only_allow_merge_if_pipeline_succeeds: true })
+    it 'sets a project as allowing merge only if pipeline succeeds' do
+      project = attributes_for(:project, only_allow_merge_if_pipeline_succeeds: true)
       post api("/projects/user/#{user.id}", admin), project
       expect(json_response['only_allow_merge_if_pipeline_succeeds']).to be_truthy
     end
 
     it 'sets a project as allowing merge even if discussions are unresolved' do
-      project = attributes_for(:project, { only_allow_merge_if_all_discussions_are_resolved: false })
+      project = attributes_for(:project, only_allow_merge_if_all_discussions_are_resolved: false)
 
       post api("/projects/user/#{user.id}", admin), project
 
@@ -663,7 +700,7 @@ describe API::Projects do
     end
 
     it 'sets a project as allowing merge only if all discussions are resolved' do
-      project = attributes_for(:project, { only_allow_merge_if_all_discussions_are_resolved: true })
+      project = attributes_for(:project, only_allow_merge_if_all_discussions_are_resolved: true)
 
       post api("/projects/user/#{user.id}", admin), project
 
@@ -732,6 +769,7 @@ describe API::Projects do
         expect(json_response['wiki_enabled']).to be_present
         expect(json_response['jobs_enabled']).to be_present
         expect(json_response['snippets_enabled']).to be_present
+        expect(json_response['resolve_outdated_diff_discussions']).to eq(project.resolve_outdated_diff_discussions)
         expect(json_response['container_registry_enabled']).to be_present
         expect(json_response['created_at']).to be_present
         expect(json_response['last_activity_at']).to be_present

spec/requests/api/v3/projects_spec.rb

@@ -687,6 +687,7 @@ describe API::V3::Projects do
         expect(json_response['wiki_enabled']).to be_present
         expect(json_response['builds_enabled']).to be_present
         expect(json_response['snippets_enabled']).to be_present
+        expect(json_response['resolve_outdated_diff_discussions']).to eq(project.resolve_outdated_diff_discussions)
         expect(json_response['container_registry_enabled']).to be_present
         expect(json_response['created_at']).to be_present
         expect(json_response['last_activity_at']).to be_present

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