Merge branch 'dm-outdated-diff-link' into 'master'

Link to outdated diff in older MR version from outdated diff discussion

Closes #27865

See merge request !10572

app/assets/stylesheets/pages/merge_requests.scss

@@ -527,6 +527,8 @@
   }
 
   .comments-disabled-notif {
+    line-height: 28px;
+
     .btn {
       margin-left: 5px;
     }

app/controllers/projects/compare_controller.rb

@@ -61,7 +61,6 @@ class Projects::CompareController < Projects::ApplicationController
       @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
 
       @diff_notes_disabled = true
-      @grouped_diff_discussions = {}
     end
   end
 

app/controllers/projects/merge_requests_controller.rb

@@ -16,7 +16,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :define_show_vars, only: [:show, :diffs, :commits, :conflicts, :conflict_for_path, :builds, :pipelines]
   before_action :define_widget_vars, only: [:merge, :cancel_merge_when_pipeline_succeeds, :merge_check]
   before_action :define_commit_vars, only: [:diffs]
-  before_action :define_diff_comment_vars, only: [:diffs]
   before_action :ensure_ref_fetched, only: [:show, :diffs, :commits, :builds, :conflicts, :conflict_for_path, :pipelines]
   before_action :close_merge_request_without_source_project, only: [:show, :diffs, :commits, :builds, :pipelines]
   before_action :apply_diff_view_cookie!, only: [:new_diffs]
@@ -101,34 +100,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     respond_to do |format|
       format.html { define_discussion_vars }
       format.json do
-        @merge_request_diff =
+        define_diff_vars
-          if params[:diff_id]
+        define_diff_comment_vars
-            @merge_request.merge_request_diffs.viewable.find(params[:diff_id])
-          else
-            @merge_request.merge_request_diff
-          end
-
-        @merge_request_diffs = @merge_request.merge_request_diffs.viewable.select_without_diff
-        @comparable_diffs = @merge_request_diffs.select { |diff| diff.id < @merge_request_diff.id }
-
-        if params[:start_sha].present?
-          @start_sha = params[:start_sha]
-          @start_version = @comparable_diffs.find { |diff| diff.head_commit_sha == @start_sha }
-
-          unless @start_version
-            @start_sha = @merge_request_diff.head_commit_sha
-            @start_version = @merge_request_diff
-          end
-        end
 
         @environment = @merge_request.environments_for(current_user).last
 
-        if @start_sha
-          compared_diff_version
-        else
-          original_diff_version
-        end
-
         render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") }
       end
     end
@@ -140,16 +116,17 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def diff_for_path
     if params[:id]
       merge_request
+      define_diff_vars
       define_diff_comment_vars
     else
       build_merge_request
+      @diffs = @merge_request.diffs(diff_options)
       @diff_notes_disabled = true
-      @grouped_diff_discussions = {}
     end
 
     define_commit_vars
 
-    render_diff_for_path(@merge_request.diffs(diff_options))
+    render_diff_for_path(@diffs)
   end
 
   def commits
@@ -586,15 +563,46 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @base_commit = @merge_request.diff_base_commit || @merge_request.likely_diff_base_commit
   end
 
+  def define_diff_vars
+    @merge_request_diff =
+      if params[:diff_id]
+        @merge_request.merge_request_diffs.viewable.find(params[:diff_id])
+      else
+        @merge_request.merge_request_diff
+      end
+
+    @merge_request_diffs = @merge_request.merge_request_diffs.viewable.select_without_diff
+    @comparable_diffs = @merge_request_diffs.select { |diff| diff.id < @merge_request_diff.id }
+
+    if params[:start_sha].present?
+      @start_sha = params[:start_sha]
+      @start_version = @comparable_diffs.find { |diff| diff.head_commit_sha == @start_sha }
+
+      unless @start_version
+        @start_sha = @merge_request_diff.head_commit_sha
+        @start_version = @merge_request_diff
+      end
+    end
+
+    @diffs =
+      if @start_sha
+        @merge_request_diff.compare_with(@start_sha).diffs(diff_options)
+      else
+        @merge_request_diff.diffs(diff_options)
+      end
+  end
+
   def define_diff_comment_vars
     @new_diff_note_attrs = {
       noteable_type: 'MergeRequest',
       noteable_id: @merge_request.id
     }
 
+    @diff_notes_disabled = !@merge_request_diff.latest? || @start_sha
+
     @use_legacy_diff_notes = !@merge_request.has_complete_diff_refs?
 
-    @grouped_diff_discussions = @merge_request.grouped_diff_discussions
+    @grouped_diff_discussions = @merge_request.grouped_diff_discussions(@merge_request_diff.diff_refs)
     @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes))
   end
 
@@ -678,16 +686,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params.merge(diff_options: diff_options)).execute
   end
 
-  def compared_diff_version
-    @diff_notes_disabled = true
-    @diffs = @merge_request_diff.compare_with(@start_sha).diffs(diff_options)
-  end
-
-  def original_diff_version
-    @diff_notes_disabled = !@merge_request_diff.latest?
-    @diffs = @merge_request_diff.diffs(diff_options)
-  end
-
   def close_merge_request_without_source_project
     if !@merge_request.source_project && @merge_request.open?
       @merge_request.close

app/helpers/diff_helper.rb

@@ -62,6 +62,8 @@ module DiffHelper
   end
 
   def parallel_diff_discussions(left, right, diff_file)
+    return unless @grouped_diff_discussions
+    
     discussions_left = discussions_right = nil
 
     if left && (left.unchanged? || left.removed?)

app/helpers/notes_helper.rb

@@ -61,12 +61,23 @@ module NotesHelper
   end
 
   def discussion_diff_path(discussion)
-    return unless discussion.diff_discussion?
+    if discussion.for_merge_request? && discussion.diff_discussion?
+      if discussion.active?
+        # Without a diff ID, the link always points to the latest diff version
+        diff_id = nil
+      elsif merge_request_diff = discussion.latest_merge_request_diff
+        diff_id = merge_request_diff.id
+      else
+        # If the discussion is not active, and we cannot find the latest
+        # merge request diff for this discussion, we return no path at all.
+        return
+      end
 
-    if discussion.for_merge_request? && discussion.active?
+      diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, diff_id: diff_id, anchor: discussion.line_code)
-      diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code)
     elsif discussion.for_commit?
-      namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code)
+      anchor = discussion.line_code if discussion.diff_discussion?
+
+      namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: anchor)
     end
   end
 end

app/models/concerns/discussion_on_diff.rb

@@ -5,8 +5,6 @@ module DiscussionOnDiff
   included do
     NUMBER_OF_TRUNCATED_DIFF_LINES = 16
 
-    memoized_values << :active
-
     delegate  :line_code,
               :original_line_code,
               :diff_file,
@@ -29,12 +27,6 @@ module DiscussionOnDiff
     true
   end
 
-  def active?
-    return @active if @active.present?
-
-    @active = first_note.active?
-  end
-
   # Returns an array of at most 16 highlighted lines above a diff note
   def truncated_diff_lines(highlight: true)
     lines = highlight ? highlighted_diff_lines : diff_lines

app/models/concerns/note_on_diff.rb

@@ -25,4 +25,18 @@ module NoteOnDiff
   def diff_attributes
     raise NotImplementedError
   end
+
+  def active?(diff_refs = nil)
+    raise NotImplementedError
+  end
+
+  private
+
+  def noteable_diff_refs
+    if noteable.respond_to?(:diff_sha_refs)
+      noteable.diff_sha_refs
+    else
+      noteable.diff_refs
+    end
+  end
 end

app/models/concerns/noteable.rb

@@ -36,10 +36,10 @@ module Noteable
       .discussions(self)
   end
 
-  def grouped_diff_discussions
+  def grouped_diff_discussions(*args)
     # Doesn't use `discussion_notes`, because this may include commit diff notes
     # besides MR diff notes, that we do no want to display on the MR Changes tab.
-    notes.inc_relations_for_view.grouped_diff_discussions
+    notes.inc_relations_for_view.grouped_diff_discussions(*args)
   end
 
   def resolvable_discussions

app/models/diff_discussion.rb

@@ -10,6 +10,7 @@ class DiffDiscussion < Discussion
 
   delegate  :position,
             :original_position,
+            :latest_merge_request_diff,
 
             to: :first_note
 

app/models/diff_note.rb

@@ -65,20 +65,18 @@ class DiffNote < Note
     self.position.diff_refs == diff_refs
   end
 
+  def latest_merge_request_diff
+    return unless for_merge_request?
+
+    self.noteable.merge_request_diff_for(self.position.diff_refs)
+  end
+
   private
 
   def supported?
     for_commit? || self.noteable.has_complete_diff_refs?
   end
 
-  def noteable_diff_refs
-    if noteable.respond_to?(:diff_sha_refs)
-      noteable.diff_sha_refs
-    else
-      noteable.diff_refs
-    end
-  end
-
   def set_original_position
     self.original_position = self.position.dup unless self.original_position&.complete?
   end

app/models/legacy_diff_discussion.rb

@@ -7,6 +7,8 @@
 class LegacyDiffDiscussion < Discussion
   include DiscussionOnDiff
 
+  memoized_values << :active
+
   def legacy_diff_discussion?
     true
   end
@@ -15,6 +17,12 @@ class LegacyDiffDiscussion < Discussion
     LegacyDiffNote
   end
 
+  def active?(*args)
+    return @active if @active.present?
+
+    @active = first_note.active?(*args)
+  end
+
   def collapsed?
     !active?
   end

app/models/legacy_diff_note.rb

@@ -56,11 +56,12 @@ class LegacyDiffNote < Note
   #
   # If the note's current diff cannot be matched in the MergeRequest's current
   # diff, it's considered inactive.
-  def active?
+  def active?(diff_refs = nil)
     return @active if defined?(@active)
     return true if for_commit?
     return true unless diff_line
     return false unless noteable
+    return false if diff_refs && diff_refs != noteable_diff_refs
 
     noteable_diff = find_noteable_diff
 

app/models/merge_request.rb

@@ -366,6 +366,14 @@ class MergeRequest < ActiveRecord::Base
     merge_request_diff(true)
   end
 
+  def merge_request_diff_for(diff_refs)
+    @merge_request_diffs_by_diff_refs ||= Hash.new do |h, diff_refs|
+      h[diff_refs] = merge_request_diffs.viewable.select_without_diff.find_by_diff_refs(diff_refs)
+    end
+
+    @merge_request_diffs_by_diff_refs[diff_refs]
+  end
+
   def reload_diff_if_branch_changed
     if source_branch_changed? || target_branch_changed?
       reload_diff

app/models/merge_request_diff.rb

@@ -31,6 +31,10 @@ class MergeRequestDiff < ActiveRecord::Base
   # It allows you to override variables like head_commit_sha before getting diff.
   after_create :save_git_content, unless: :importing?
 
+  def self.find_by_diff_refs(diff_refs)
+    find_by(start_commit_sha: diff_refs.start_sha, head_commit_sha: diff_refs.head_sha, base_commit_sha: diff_refs.base_sha)
+  end
+
   def self.select_without_diff
     select(column_names - ['st_diffs'])
   end
@@ -130,6 +134,12 @@ class MergeRequestDiff < ActiveRecord::Base
     st_commits.map { |commit| commit[:id] }
   end
 
+  def diff_refs=(new_diff_refs)
+    self.base_commit_sha = new_diff_refs&.base_sha
+    self.start_commit_sha = new_diff_refs&.start_sha
+    self.head_commit_sha = new_diff_refs&.head_sha
+  end
+
   def diff_refs
     return unless start_commit_sha || base_commit_sha
 

app/models/note.rb

@@ -113,11 +113,11 @@ class Note < ActiveRecord::Base
       Discussion.build(notes)
     end
 
-    def grouped_diff_discussions
+    def grouped_diff_discussions(diff_refs = nil)
       diff_notes.
         fresh.
         discussions.
-        select(&:active?).
+        select { |n| n.active?(diff_refs) }.
         group_by(&:line_code)
     end
 
@@ -140,6 +140,10 @@ class Note < ActiveRecord::Base
     true
   end
 
+  def latest_merge_request_diff
+    nil
+  end
+
   def max_attachment_size
     current_application_settings.max_attachment_size.megabytes.to_i
   end

app/views/discussions/_discussion.html.haml

@@ -20,21 +20,22 @@
             = discussion.author.to_reference
             started a discussion
 
+            - url = discussion_diff_path(discussion)
             - if discussion.for_commit? && @noteable != discussion.noteable
               on
               - commit = discussion.noteable
               - if commit
                 commit
-                - anchor = discussion.line_code if discussion.diff_discussion?
+                = link_to commit.short_id, url, class: 'monospace'
-                = link_to commit.short_id, namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: anchor), class: 'monospace'
               - else
                 a deleted commit
             - elsif discussion.diff_discussion?
               on
-              - if discussion.active?
+              = conditional_link_to url.present?, url do
-                = link_to 'the diff', discussion_diff_path(discussion)
+                - if discussion.active?
-              - else
+                  the diff
-                an outdated diff
+                - else
+                  an outdated diff
 
             = time_ago_with_tooltip(discussion.created_at, placement: "bottom", html_class: "note-created-ago")
           = render "discussions/headline", discussion: discussion

app/views/projects/diffs/_parallel_view.html.haml

@@ -5,8 +5,7 @@
       - left = line[:left]
       - right = line[:right]
       - last_line = right.new_pos if right
-      - unless @diff_notes_disabled
+      - discussions_left, discussions_right = parallel_diff_discussions(left, right, diff_file)
-        - discussions_left, discussions_right = parallel_diff_discussions(left, right, diff_file)
       %tr.line_holder.parallel
         - if left
           - case left.type

app/views/projects/diffs/_text_file.html.haml

@@ -4,11 +4,10 @@
     %a.show-suppressed-diff.js-show-suppressed-diff Changes suppressed. Click to show.
 
 %table.text-file.code.js-syntax-highlight{ data: diff_view_data, class: too_big ? 'hide' : '' }
-  - discussions = @grouped_diff_discussions unless @diff_notes_disabled
   = render partial: "projects/diffs/line",
     collection: diff_file.highlighted_diff_lines,
     as: :line,
-    locals: { diff_file: diff_file, discussions: discussions }
+    locals: { diff_file: diff_file, discussions: @grouped_diff_discussions }
 
   - if !diff_file.new_file && !diff_file.deleted_file && diff_file.highlighted_diff_lines.any?
     - last_line = diff_file.highlighted_diff_lines.last

app/views/projects/merge_requests/show/_versions.html.haml

@@ -72,13 +72,16 @@
         = link_to namespace_project_compare_path(@project.namespace, @project, from: @start_version.base_commit_sha, to: @merge_request_diff.base_commit_sha) do
           new commits
         from
-        %code= @merge_request.target_branch
+        = succeed '.' do
+          %code= @merge_request.target_branch
 
-    - unless @merge_request_diff.latest? && !@start_sha
+    - if @diff_notes_disabled
       .comments-disabled-notif.content-block
         = icon('info-circle')
         - if @start_sha
           Comments are disabled because you're comparing two versions of this merge request.
         - else
-          Comments are disabled because you're viewing an old version of this merge request.
+          Discussions on this version of the merge request are displayed but comment creation is disabled.
-        = link_to 'Show latest version', diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm'
+
+        .pull-right
+          = link_to 'Show latest version', diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm'

changelogs/unreleased/dm-link-discussion-to-outdated-diff.yml

@@ -0,0 +1,4 @@
+---
+title: Link to outdated diff in older MR version from outdated diff discussion
+merge_request:
+author:

lib/gitlab/diff/diff_refs.rb

@@ -18,6 +18,12 @@ module Gitlab
           head_sha == other.head_sha
       end
 
+      alias_method :eql?, :==
+
+      def hash
+        [base_sha, start_sha, head_sha].hash
+      end
+
       # There is only one case in which we will have `start_sha` and `head_sha`,
       # but not `base_sha`, which is when a diff is generated between an
       # orphaned branch and another branch, which means there _is_ no base, but

spec/factories/notes.rb

@@ -40,6 +40,7 @@ FactoryGirl.define do
 
       transient do
         line_number 14
+        diff_refs { noteable.try(:diff_refs) }
       end
 
       position do
@@ -48,7 +49,7 @@ FactoryGirl.define do
           new_path: "files/ruby/popen.rb",
           old_line: nil,
           new_line: line_number,
-          diff_refs: noteable.try(:diff_refs)
+          diff_refs: diff_refs
         )
       end
 

spec/features/merge_requests/discussion_spec.rb

@@ -0,0 +1,51 @@
+require 'spec_helper'
+
+feature 'Merge Request Discussions', feature: true do
+  before do
+    login_as :admin
+  end
+
+  context "Diff discussions" do
+    let(:merge_request) { create(:merge_request, importing: true) }
+    let(:project) { merge_request.source_project }
+    let!(:old_merge_request_diff) { merge_request.merge_request_diffs.create(diff_refs: outdated_diff_refs) }
+    let!(:new_merge_request_diff) { merge_request.merge_request_diffs.create }
+
+    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(:outdated_position) do
+      Gitlab::Diff::Position.new(
+        old_path: "files/ruby/popen.rb",
+        new_path: "files/ruby/popen.rb",
+        old_line: nil,
+        new_line: 9,
+        diff_refs: outdated_diff_refs
+      )
+    end
+
+    let(:outdated_diff_refs) { project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e").diff_refs }
+
+    before(:each) do
+      visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+    end
+
+    context 'active discussions' do
+      it 'shows a link to the diff' do
+        within(".discussion[data-discussion-id='#{active_discussion.id}']") do
+          path = diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, anchor: active_discussion.line_code)
+          expect(page).to have_link('the diff', href: path)
+        end
+      end
+    end
+
+    context 'outdated discussions' do
+      it 'shows a link to the outdated diff' do
+        within(".discussion[data-discussion-id='#{outdated_discussion.id}']") do
+          path = diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, diff_id: old_merge_request_diff.id, anchor: outdated_discussion.line_code)
+          expect(page).to have_link('an outdated diff', href: path)
+        end
+      end
+    end
+  end
+end

spec/features/merge_requests/versions_spec.rb

@@ -36,8 +36,23 @@ feature 'Merge Request versions', js: true, feature: true do
       expect(page).to have_content '5 changed files'
     end
 
-    it 'show the message about disabled comments' do
+    it 'show the message about disabled comment creation' do
-      expect(page).to have_content 'Comments are disabled'
+      expect(page).to have_content 'comment creation is disabled'
+    end
+
+    it 'shows comments that were last relevant at that version' do
+      position = Gitlab::Diff::Position.new(
+        old_path: ".gitmodules",
+        new_path: ".gitmodules",
+        old_line: nil,
+        new_line: 4,
+        diff_refs: merge_request_diff1.diff_refs
+      )
+      outdated_diff_note = create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position)
+      outdated_diff_note.position = outdated_diff_note.original_position
+      outdated_diff_note.save!
+
+      expect(page).to have_css(".diffs .notes[data-discussion-id='#{outdated_diff_note.discussion_id}']")
     end
   end
 

spec/models/diff_note_spec.rb

@@ -155,6 +155,23 @@ describe DiffNote, models: true do
     end
   end
 
+  describe '#latest_merge_request_diff' do
+    context 'when active' do
+      it 'returns the current merge request diff' do
+        expect(subject.latest_merge_request_diff).to eq(merge_request.merge_request_diff)
+      end
+    end
+
+    context 'when outdated' do
+      let!(:old_merge_request_diff) { merge_request.merge_request_diff }
+      let!(:new_merge_request_diff) { merge_request.merge_request_diffs.create(diff_refs: commit.diff_refs) }
+
+      it 'returns the latest merge request diff that this diff note applied to' do
+        expect(subject.latest_merge_request_diff).to eq(old_merge_request_diff)
+      end
+    end
+  end
+
   describe "creation" do
     describe "updating of position" do
       context "when noteable is a commit" do