From f8fabfcc6ff6be4e20e3cc9353be86b7cf53af47 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@selenight.nl>
Date: Sun, 30 Apr 2017 15:32:09 -0500
Subject: Allow commenting on older versions of the diff and comparisons
 between diff versions

---
 app/assets/stylesheets/pages/diff.scss             |  6 ------
 app/assets/stylesheets/pages/merge_requests.scss   |  1 -
 .../projects/merge_requests_controller.rb          | 15 ++++++++------
 app/helpers/notes_helper.rb                        | 24 +++++++++-------------
 app/models/concerns/discussion_on_diff.rb          |  1 +
 app/models/concerns/note_on_diff.rb                |  4 ++++
 app/models/diff_discussion.rb                      | 20 +++++++++++++++++-
 app/models/diff_note.rb                            |  7 ++++---
 app/models/legacy_diff_discussion.rb               | 18 ++++++++++++----
 app/models/merge_request.rb                        | 18 ++++++++++------
 app/models/note.rb                                 | 22 ++++++++++++--------
 app/views/discussions/_diff_with_notes.html.haml   |  2 +-
 app/views/discussions/_discussion.html.haml        |  2 +-
 app/views/projects/diffs/_line.html.haml           |  2 +-
 .../merge_requests/show/_versions.html.haml        | 14 +++++++------
 15 files changed, 97 insertions(+), 59 deletions(-)

(limited to 'app')

diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss
index 1b4694377b3..feefaad8a15 100644
--- a/app/assets/stylesheets/pages/diff.scss
+++ b/app/assets/stylesheets/pages/diff.scss
@@ -425,12 +425,6 @@
   float: right;
 }
 
-.diffs {
-  .content-block {
-    border-bottom: none;
-  }
-}
-
 .files-changed {
   border-bottom: none;
 }
diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss
index 6a419384a34..bca62b7fc31 100644
--- a/app/assets/stylesheets/pages/merge_requests.scss
+++ b/app/assets/stylesheets/pages/merge_requests.scss
@@ -511,7 +511,6 @@
 
 .mr-version-controls {
   background: $gray-light;
-  border-bottom: 1px solid $border-color;
   color: $gl-text-color;
 
   .mr-version-menus-container {
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 09dc8b38229..a63b7ff0bed 100755
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -120,7 +120,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       define_diff_comment_vars
     else
       build_merge_request
-      @diffs = @merge_request.diffs(diff_options)
+      @compare = @merge_request
+      @diffs = @compare.diffs(diff_options)
       @diff_notes_disabled = true
     end
 
@@ -584,12 +585,14 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       end
     end
 
-    @diffs =
+    @compare =
       if @start_sha
-        @merge_request_diff.compare_with(@start_sha).diffs(diff_options)
+        @merge_request_diff.compare_with(@start_sha)
       else
-        @merge_request_diff.diffs(diff_options)
+        @merge_request_diff
       end
+
+    @diffs = @compare.diffs(diff_options)
   end
 
   def define_diff_comment_vars
@@ -598,11 +601,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       noteable_id: @merge_request.id
     }
 
-    @diff_notes_disabled = !@merge_request_diff.latest? || @start_sha
+    @diff_notes_disabled = false
 
     @use_legacy_diff_notes = !@merge_request.has_complete_diff_refs?
 
-    @grouped_diff_discussions = @merge_request.grouped_diff_discussions(@merge_request_diff.diff_refs)
+    @grouped_diff_discussions = @merge_request.grouped_diff_discussions(@compare.diff_refs)
     @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes))
   end
 
diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index eab0738a368..08180883eb9 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -60,20 +60,16 @@ module NotesHelper
     note.project.team.human_max_access(note.author_id)
   end
 
-  def discussion_diff_path(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
-
-      diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, diff_id: diff_id, anchor: discussion.line_code)
+  def discussion_path(discussion)
+    if discussion.for_merge_request?
+      return unless discussion.diff_discussion?
+
+      version_params = discussion.merge_request_version_params
+      return unless version_params
+
+      path_params = version_params.merge(anchor: discussion.line_code)
+
+      diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, path_params)
     elsif discussion.for_commit?
       anchor = discussion.line_code if discussion.diff_discussion?
 
diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb
index 8ee42875670..a7bdf5587b2 100644
--- a/app/models/concerns/discussion_on_diff.rb
+++ b/app/models/concerns/discussion_on_diff.rb
@@ -11,6 +11,7 @@ module DiscussionOnDiff
               :diff_line,
               :for_line?,
               :active?,
+              :created_at_diff?,
 
               to: :first_note
 
diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb
index 6c27dd5aa5c..6359f7596b1 100644
--- a/app/models/concerns/note_on_diff.rb
+++ b/app/models/concerns/note_on_diff.rb
@@ -30,6 +30,10 @@ module NoteOnDiff
     raise NotImplementedError
   end
 
+  def created_at_diff?(diff_refs)
+    false
+  end
+
   private
 
   def noteable_diff_refs
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 6a6466b493b..d627fbe327f 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -10,7 +10,6 @@ class DiffDiscussion < Discussion
 
   delegate  :position,
             :original_position,
-            :latest_merge_request_diff,
 
             to: :first_note
 
@@ -18,6 +17,25 @@ class DiffDiscussion < Discussion
     false
   end
 
+  def merge_request_version_params
+    return unless for_merge_request?
+
+    if active?
+      {}
+    else
+      diff_refs = position.diff_refs
+
+      if diff = noteable.merge_request_diff_for(diff_refs)
+        { diff_id: diff.id }
+      elsif diff = noteable.merge_request_diff_for(diff_refs.head_sha)
+        {
+          diff_id: diff.id,
+          start_sha: diff_refs.start_sha
+        }
+      end
+    end
+  end
+
   def reply_attributes
     super.merge(
       original_position: original_position.to_json,
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index abe4518d62a..76c59199afd 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -65,10 +65,11 @@ class DiffNote < Note
     self.position.diff_refs == diff_refs
   end
 
-  def latest_merge_request_diff
-    return unless for_merge_request?
+  def created_at_diff?(diff_refs)
+    return false unless supported?
+    return true if for_commit?
 
-    self.noteable.merge_request_diff_for(self.position.diff_refs)
+    self.original_position.diff_refs == diff_refs
   end
 
   private
diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb
index e617ce36f56..3c1d34db5fa 100644
--- a/app/models/legacy_diff_discussion.rb
+++ b/app/models/legacy_diff_discussion.rb
@@ -9,14 +9,14 @@ class LegacyDiffDiscussion < Discussion
 
   memoized_values << :active
 
-  def legacy_diff_discussion?
-    true
-  end
-
   def self.note_class
     LegacyDiffNote
   end
 
+  def legacy_diff_discussion?
+    true
+  end
+
   def active?(*args)
     return @active if @active.present?
 
@@ -27,6 +27,16 @@ class LegacyDiffDiscussion < Discussion
     !active?
   end
 
+  def merge_request_version_params
+    return unless for_merge_request?
+
+    if active?
+      {}
+    else
+      nil
+    end
+  end
+
   def reply_attributes
     super.merge(line_code: line_code)
   end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 365fa4f1e70..12c5481cd6d 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -374,12 +374,18 @@ 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]
+  def merge_request_diff_for(diff_refs_or_sha)
+    @merge_request_diffs_by_diff_refs_or_sha ||= Hash.new do |h, diff_refs_or_sha|
+      diffs = merge_request_diffs.viewable.select_without_diff
+      h[diff_refs_or_sha] =
+        if diff_refs_or_sha.is_a?(Gitlab::Diff::DiffRefs)
+          diffs.find_by_diff_refs(diff_refs_or_sha)
+        else
+          diffs.find_by(head_commit_sha: diff_refs_or_sha)
+        end
+    end
+
+    @merge_request_diffs_by_diff_refs_or_sha[diff_refs_or_sha]
   end
 
   def reload_diff_if_branch_changed
diff --git a/app/models/note.rb b/app/models/note.rb
index e720bfba030..b06985b4a6f 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -115,11 +115,19 @@ class Note < ActiveRecord::Base
     end
 
     def grouped_diff_discussions(diff_refs = nil)
-      diff_notes.
-        fresh.
-        discussions.
-        select { |n| n.active?(diff_refs) }.
-        group_by(&:line_code)
+      groups = {}
+
+      diff_notes.fresh.discussions.each do |discussion|
+        if discussion.active?(diff_refs)
+          discussions = groups[discussion.line_code] ||= []
+        elsif diff_refs && discussion.created_at_diff?(diff_refs)
+          discussions = groups[discussion.original_line_code] ||= []
+        end
+
+        discussions << discussion if discussions
+      end
+
+      groups
     end
 
     def count_for_collection(ids, type)
@@ -141,10 +149,6 @@ 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
diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml
index 549364761e6..78c5b0c1dda 100644
--- a/app/views/discussions/_diff_with_notes.html.haml
+++ b/app/views/discussions/_diff_with_notes.html.haml
@@ -3,7 +3,7 @@
 
 .diff-file.file-holder
   .js-file-title.file-title
-    = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_file.content_commit, project: discussion.project, url: discussion_diff_path(discussion)
+    = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_file.content_commit, project: discussion.project, url: discussion_path(discussion)
 
   .diff-content.code.js-syntax-highlight
     %table
diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml
index 8440fb3d785..38e85168f40 100644
--- a/app/views/discussions/_discussion.html.haml
+++ b/app/views/discussions/_discussion.html.haml
@@ -20,7 +20,7 @@
             = discussion.author.to_reference
             started a discussion
 
-            - url = discussion_diff_path(discussion)
+            - url = discussion_path(discussion)
             - if discussion.for_commit? && @noteable != discussion.noteable
               on
               - commit = discussion.noteable
diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml
index 3e426ee9e7d..7439b8a66f7 100644
--- a/app/views/projects/diffs/_line.html.haml
+++ b/app/views/projects/diffs/_line.html.haml
@@ -35,6 +35,6 @@
       - else
         = diff_line_content(line.text)
 
-- if line_discussions
+- if line_discussions&.any?
   - discussion_expanded = local_assigns.fetch(:discussion_expanded, line_discussions.any?(&:expanded?))
   = render "discussions/diff_discussion", discussions: line_discussions, expanded: discussion_expanded
diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml
index 547be78992e..11b0c55be0b 100644
--- a/app/views/projects/merge_requests/show/_versions.html.haml
+++ b/app/views/projects/merge_requests/show/_versions.html.haml
@@ -35,7 +35,7 @@
         %span.dropdown.inline.mr-version-compare-dropdown
           %a.btn.btn-default.dropdown-toggle{ data: {toggle: :dropdown} }
             %span
-              - if @start_sha
+              - if @start_version
                 version #{version_index(@start_version)}
               - else
                 #{@merge_request.target_branch}
@@ -59,7 +59,7 @@
                       %small
                         = time_ago_with_tooltip(merge_request_diff.created_at)
                 %li
-                  = link_to merge_request_version_path(@project, @merge_request, @merge_request_diff), class: ('is-active' unless @start_sha) do
+                  = link_to merge_request_version_path(@project, @merge_request, @merge_request_diff), class: ('is-active' unless @start_version) do
                     %strong
                       #{@merge_request.target_branch} (base)
                     .monospace= short_sha(@merge_request_diff.base_commit_sha)
@@ -75,13 +75,15 @@
         = succeed '.' do
           %code= @merge_request.target_branch
 
-    - if @diff_notes_disabled
+    - if @start_version || !@merge_request_diff.latest?
       .comments-disabled-notif.content-block
         = icon('info-circle')
-        - if @start_sha
-          Comments are disabled because you're comparing two versions of this merge request.
+        Not all comments are displayed because you're
+        - if @start_version
+          comparing two versions
         - else
-          Discussions on this version of the merge request are displayed but comment creation is disabled.
+          viewing an old version
+        of this merge request.
 
         .pull-right
           = link_to 'Show latest version', diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm'
-- 
cgit v1.2.1