Commenting on image diffs

6 files changed, 42 insertions, 6 deletions

diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb
index eee1a36ac6b..f5cbb3becad 100644
--- a/app/models/concerns/discussion_on_diff.rb
+++ b/app/models/concerns/discussion_on_diff.rb
@@ -28,6 +28,10 @@ module DiscussionOnDiff
     true
   end
 
+  def file_new_path
+    first_note.position.new_path
+  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
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 07c4846e2ac..6eba87da1a1 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -11,6 +11,8 @@ class DiffDiscussion < Discussion
   delegate  :position,
             :original_position,
             :change_position,
+            :on_text?,
+            :on_image?,
 
             to: :first_note
 
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index e9a60e6ce09..d88a92dc027 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -12,8 +12,8 @@ class DiffNote < Note
 
   validates :original_position, presence: true
   validates :position, presence: true
-  validates :diff_line, presence: true
-  validates :line_code, presence: true, line_code: true
+  validates :diff_line, presence: true, if: :on_text?
+  validates :line_code, presence: true, line_code: true, if: :on_text?
   validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
   validate :positions_complete
   validate :verify_supported
@@ -43,6 +43,14 @@ class DiffNote < Note
     end
   end
 
+  def on_text?
+    position.position_type == "text"
+  end
+
+  def on_image?
+    position.position_type == "image"
+  end
+
   def diff_file
     @diff_file ||= self.original_position.diff_file(self.project.repository)
   end
@@ -56,6 +64,8 @@ class DiffNote < Note
   end
 
   def original_line_code
+    return unless on_text?
+
     self.diff_file.line_code(self.diff_line)
   end
 
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index b80da7b246a..437df923d2d 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -66,6 +66,10 @@ class Discussion
     @context_noteable = context_noteable
   end
 
+  def on_image?
+    false
+  end
+
   def ==(other)
     other.class == self.class &&
       other.context_noteable == self.context_noteable &&
diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb
index 3c1d34db5fa..80fc6304fd4 100644
--- a/app/models/legacy_diff_discussion.rb
+++ b/app/models/legacy_diff_discussion.rb
@@ -17,6 +17,14 @@ class LegacyDiffDiscussion < Discussion
     true
   end
 
+  def on_image?
+    false
+  end
+
+  def on_text?
+    true
+  end
+
   def active?(*args)
     return @active if @active.present?
 
diff --git a/app/models/note.rb b/app/models/note.rb
index f44590e2144..ceded9f2aef 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -134,14 +134,22 @@ class Note < ActiveRecord::Base
       Discussion.build(notes)
     end
 
+    # Group diff discussions by line code or file path.
+    # It is not needed to group by line code when comment is
+    # on an image.
     def grouped_diff_discussions(diff_refs = nil)
       groups = {}
 
       diff_notes.fresh.discussions.each do |discussion|
-        line_code = discussion.line_code_in_diffs(diff_refs)
-
-        if line_code
-          discussions = groups[line_code] ||= []
+        group_key =
+          if discussion.on_image?
+            discussion.file_new_path
+          else
+            discussion.line_code_in_diffs(diff_refs)
+          end
+
+        if group_key
+          discussions = groups[group_key] ||= []
           discussions << discussion
         end
       end