Commit d4fec49a authored by Dominik Sander's avatar Dominik Sander

Fix merge request comments on files with multiple commits

Having a merge request with a comments on a line which is then changed
in a later commit prevented new comments from properly showing up in the
merge request show page.

 * `Note#set_diff` do not use stored the diff when creating a new note
   in merge requests (we can not be sure the diff did not changed since
   the last comment on that line)
 * Do not rely just on `outdated?` of the first note when displaying
   comments of a MR in the discussion tab, but partition the
   active/outdated notes and display them all
 * In the inline changes tab just select the active notes, so an
   outdated note does not prevent an active one from being rendered
 * Just show active comments in the side-by-side changes tab
parent 5f78601c
...@@ -88,6 +88,7 @@ v 7.10.0 (unreleased) ...@@ -88,6 +88,7 @@ v 7.10.0 (unreleased)
- Allow user to choose a public email to show on public profile - Allow user to choose a public email to show on public profile
- Remove truncation from issue titles on milestone page (Jason Blanchard) - Remove truncation from issue titles on milestone page (Jason Blanchard)
- Fix stuck Merge Request merging events from old installations (Ben Bodenmiller) - Fix stuck Merge Request merging events from old installations (Ben Bodenmiller)
- Fix merge request comments on files with multiple commits
v 7.9.3 v 7.9.3
- Contains no changes - Contains no changes
......
...@@ -101,7 +101,7 @@ module DiffHelper ...@@ -101,7 +101,7 @@ module DiffHelper
end end
def line_comments def line_comments
@line_comments ||= @line_notes.group_by(&:line_code) @line_comments ||= @line_notes.select(&:active?).group_by(&:line_code)
end end
def organize_comments(type_left, type_right, line_code_left, line_code_right) def organize_comments(type_left, type_right, line_code_left, line_code_right)
......
...@@ -354,7 +354,7 @@ class Note < ActiveRecord::Base ...@@ -354,7 +354,7 @@ class Note < ActiveRecord::Base
def set_diff def set_diff
# First lets find notes with same diff # First lets find notes with same diff
# before iterating over all mr diffs # before iterating over all mr diffs
diff = Note.where(noteable_id: self.noteable_id, noteable_type: self.noteable_type, line_code: self.line_code).last.try(:diff) diff = diff_for_line_code unless for_merge_request?
diff ||= find_diff diff ||= find_diff
self.st_diff = diff.to_hash if diff self.st_diff = diff.to_hash if diff
...@@ -364,6 +364,10 @@ class Note < ActiveRecord::Base ...@@ -364,6 +364,10 @@ class Note < ActiveRecord::Base
@diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map) @diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map)
end end
def diff_for_line_code
Note.where(noteable_id: noteable_id, noteable_type: noteable_type, line_code: line_code).last.try(:diff)
end
# Check if such line of code exists in merge request diff # Check if such line of code exists in merge request diff
# If exists - its active discussion # If exists - its active discussion
# If not - its outdated diff # If not - its outdated diff
......
...@@ -23,7 +23,7 @@ ...@@ -23,7 +23,7 @@
%td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw diff_line_content(line.text) %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw diff_line_content(line.text)
- if @reply_allowed - if @reply_allowed
- comments = @line_notes.select { |n| n.line_code == line_code }.sort_by(&:created_at) - comments = @line_notes.select { |n| n.line_code == line_code && n.active? }.sort_by(&:created_at)
- unless comments.empty? - unless comments.empty?
= render "projects/notes/diff_notes_with_reply", notes: comments, line: line.text = render "projects/notes/diff_notes_with_reply", notes: comments, line: line.text
......
...@@ -6,9 +6,8 @@ ...@@ -6,9 +6,8 @@
= image_tag avatar_icon(note.author_email), class: "avatar s40" = image_tag avatar_icon(note.author_email), class: "avatar s40"
.timeline-content .timeline-content
- if note.for_merge_request? - if note.for_merge_request?
- if note.outdated? - (active_notes, outdated_notes) = discussion_notes.partition(&:active?)
= render "projects/notes/discussions/outdated", discussion_notes: discussion_notes = render "projects/notes/discussions/active", discussion_notes: active_notes if active_notes.length > 0
- else = render "projects/notes/discussions/outdated", discussion_notes: outdated_notes if outdated_notes.length > 0
= render "projects/notes/discussions/active", discussion_notes: discussion_notes
- else - else
= render "projects/notes/discussions/commit", discussion_notes: discussion_notes = render "projects/notes/discussions/commit", discussion_notes: discussion_notes
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment