From d4fec49abc0fceec11f970c0699dfe71ee185290 Mon Sep 17 00:00:00 2001 From: Dominik Sander Date: Fri, 3 Apr 2015 16:46:23 +0200 Subject: 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 --- CHANGELOG | 1 + app/helpers/diff_helper.rb | 2 +- app/models/note.rb | 6 +++++- app/views/projects/diffs/_text_file.html.haml | 2 +- app/views/projects/notes/_discussion.html.haml | 7 +++---- 5 files changed, 11 insertions(+), 7 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index e8f367c332b..0487cc64745 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -88,6 +88,7 @@ v 7.10.0 (unreleased) - Allow user to choose a public email to show on public profile - Remove truncation from issue titles on milestone page (Jason Blanchard) - Fix stuck Merge Request merging events from old installations (Ben Bodenmiller) + - Fix merge request comments on files with multiple commits v 7.9.3 - Contains no changes diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index b56f21c7a18..4f42972a4dd 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -101,7 +101,7 @@ module DiffHelper end def line_comments - @line_comments ||= @line_notes.group_by(&:line_code) + @line_comments ||= @line_notes.select(&:active?).group_by(&:line_code) end def organize_comments(type_left, type_right, line_code_left, line_code_right) diff --git a/app/models/note.rb b/app/models/note.rb index fdab4517df3..2cf3fac2def 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -354,7 +354,7 @@ class Note < ActiveRecord::Base def set_diff # First lets find notes with same diff # 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 self.st_diff = diff.to_hash if diff @@ -364,6 +364,10 @@ class Note < ActiveRecord::Base @diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map) 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 # If exists - its active discussion # If not - its outdated diff diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index e691db9c08e..e6dfbfd6511 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -23,7 +23,7 @@ %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw diff_line_content(line.text) - 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? = render "projects/notes/diff_notes_with_reply", notes: comments, line: line.text diff --git a/app/views/projects/notes/_discussion.html.haml b/app/views/projects/notes/_discussion.html.haml index 3561ca49f81..b8068835b3a 100644 --- a/app/views/projects/notes/_discussion.html.haml +++ b/app/views/projects/notes/_discussion.html.haml @@ -6,9 +6,8 @@ = image_tag avatar_icon(note.author_email), class: "avatar s40" .timeline-content - if note.for_merge_request? - - if note.outdated? - = render "projects/notes/discussions/outdated", discussion_notes: discussion_notes - - else - = render "projects/notes/discussions/active", discussion_notes: discussion_notes + - (active_notes, outdated_notes) = discussion_notes.partition(&:active?) + = render "projects/notes/discussions/active", discussion_notes: active_notes if active_notes.length > 0 + = render "projects/notes/discussions/outdated", discussion_notes: outdated_notes if outdated_notes.length > 0 - else = render "projects/notes/discussions/commit", discussion_notes: discussion_notes -- cgit v1.2.1