Merge branch 'improve-discussions' into 'master'

Improve discussions

Implement outdated discussion detection

app/assets/stylesheets/sections/notes.scss

@@ -43,38 +43,14 @@ ul.notes {
   }
 
   .discussion {
-    padding: 8px 0;
+    padding: 10px 0;
     overflow: hidden;
     display: block;
     position:relative;
+    border-bottom: 1px solid #EEE;
 
     .discussion-body {
       margin-left: 50px;
-
-      .diff-file,
-      .discussion-hidden,
-      .notes {
-        background-color: #F9F9F9;
-      }
-      .diff-file .notes {
-        /* reset */
-        background: inherit;
-        border: none;
-        @include box-shadow(none);
-
-      }
-      .discussion-hidden .note {
-        @extend .cgray;
-        padding: 8px;
-        text-align: center;
-      }
-      .notes .note {
-        border-color: #ddd;
-        padding: 8px;
-      }
-      .reply-btn {
-        margin-top: 8px;
-      }
     }
   }
 
@@ -137,10 +113,6 @@ ul.notes {
       vertical-align: top;
     }
   }
-
-  .reply-btn {
-    margin: 5px;
-  }
 }
 
 /**
@@ -376,3 +348,17 @@ ul.notes {
   margin-top: 5px;
   margin-bottom: 5px;
 }
+
+.discussion-body,
+.diff-file {
+  .notes .note {
+    border-color: #ddd;
+    padding: 10px 15px;
+  }
+
+  .discussion-reply-holder {
+    background: #f9f9f9;
+    padding: 10px 15px;
+    border-top: 1px solid #DDD;
+  }
+}

app/helpers/notes_helper.rb

@@ -15,12 +15,6 @@ module NotesHelper
     end
   end
 
-  def link_to_merge_request_diff_line_note(note)
-    if note.for_merge_request_diff_line? and note.diff
-      link_to "#{note.diff_file_name}:L#{note.diff_new_line}", diffs_project_merge_request_path(note.project, note.noteable, anchor: note.line_code)
-    end
-  end
-
   def note_timestamp(note)
     # Shows the created at time and the updated at time if different
     ts = "#{time_ago_with_tooltip(note.created_at, 'bottom', 'note_created_ago')}"

app/models/note.rb

@@ -179,10 +179,26 @@ class Note < ActiveRecord::Base
     @diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map)
   end
 
+  # Check if such line of code exists in merge request diff
+  # If exists - its active discussion
+  # If not - its outdated diff
   def active?
-    # TODO: determine if discussion is outdated
-    # according to recent MR diff or not
-    true
+    noteable.diffs.each do |mr_diff|
+      next unless mr_diff.new_path == self.diff.new_path
+
+      Gitlab::DiffParser.new(mr_diff.diff.lines.to_a, mr_diff.new_path).
+        each do |full_line, type, line_code, line_new, line_old|
+        if full_line == diff_line
+          return true
+        end
+      end
+    end
+
+    false
+  end
+
+  def outdated?
+    !active?
   end
 
   def diff_file_index

app/views/projects/notes/_diff_notes_with_reply.html.haml

 - note = notes.first # example note
 -# Check if line want not changed since comment was left
 - if !defined?(line) || line == note.diff_line
-  %tr.notes_holder.js-toggle-content
+  %tr.notes_holder
     %td.notes_line{ colspan: 2 }
       %span.btn.disabled
         %i.icon-comment
@@ -9,4 +9,5 @@
     %td.notes_content
       %ul.notes{ rel: note.discussion_id }
         = render notes
+      .discussion-reply-holder
         = link_to_reply_diff(note)

app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml

@@ -2,7 +2,7 @@
 - note2 = notes2.first # example note
 -# Check if line want not changed since comment was left
 /- if !defined?(line) || line == note.diff_line
-%tr.notes_holder.js-toggle-content
+%tr.notes_holder
   - if note1
     %td.notes_line
       %span.btn.disabled

app/views/projects/notes/_discussion.html.haml

 - note = discussion_notes.first
-.discussion.js-toggle-container{ class: note.discussion_id }
-  .discussion-header
-    .discussion-actions
-      = link_to "#", class: "js-toggle-button" do
-        %i.icon-chevron-up
-        Show/hide discussion
-    = image_tag avatar_icon(note.author_email), class: "avatar s32"
-    %div
-      = link_to_member(@project, note.author, avatar: false)
-      - if note.for_merge_request?
-        - if note.diff
-          started a discussion on this merge request diff
-          = link_to_merge_request_diff_line_note(note)
+- if note.for_merge_request?
+  - if note.outdated?
+    = render "projects/notes/discussions/outdated", discussion_notes: discussion_notes
   - else
-          started
-          %strong
-            %i.icon-remove
-            outdated
-          discussion on this merge request diff
-      - elsif note.for_commit?
-        started a discussion on commit
-        #{link_to note.noteable.short_id, project_commit_path(note.project, note.noteable)}
-        = link_to_commit_diff_line_note(note) if note.for_diff_line?
-      - else
-        %cite.cgray started a discussion
-    %div
-      - last_note = discussion_notes.last
-      last updated by
-      = link_to_member(@project, last_note.author, avatar: false)
-      %span.discussion-last-update
-        #{time_ago_with_tooltip(last_note.updated_at, 'bottom', 'discussion_updated_ago')}
-  .discussion-body.js-toggle-content
-    - if note.for_diff_line?
-      - if note.active?
-        = render "projects/notes/discussion_diff", discussion_notes: discussion_notes, note: note
-      - else
-        = link_to 'show outdated discussion', '#', class: 'js-show-outdated-discussion'
-        %div.hide.outdated-discussion
-          .notes{ rel: discussion_notes.first.discussion_id }
-            = render discussion_notes
-
-    - else
-      .notes{ rel: discussion_notes.first.discussion_id }
-        = render discussion_notes
-      = link_to_reply_diff(discussion_notes.first)
+    = render "projects/notes/discussions/active", discussion_notes: discussion_notes
+- else
+  = render "projects/notes/discussions/commit", discussion_notes: discussion_notes

app/views/projects/notes/discussions/_active.html.haml

+- note = discussion_notes.first
+.discussion.js-toggle-container{ class: note.discussion_id }
+  .discussion-header
+    .discussion-actions
+      = link_to "#", class: "js-toggle-button" do
+        %i.icon-chevron-up
+        Show/hide discussion
+    = image_tag avatar_icon(note.author_email), class: "avatar s32"
+    %div
+      = link_to_member(@project, note.author, avatar: false)
+      started a discussion
+      = link_to diffs_project_merge_request_path(note.project, note.noteable, anchor: note.line_code) do
+        %strong on the diff
+    .last-update.hide.js-toggle-content
+      - last_note = discussion_notes.last
+      last updated by
+      = link_to_member(@project, last_note.author, avatar: false)
+      %span.discussion-last-update
+        #{time_ago_with_tooltip(last_note.updated_at, 'bottom', 'discussion_updated_ago')}
+  .discussion-body.js-toggle-content
+    = render "projects/notes/discussion_diff", discussion_notes: discussion_notes, note: note

app/views/projects/notes/discussions/_commit.html.haml

+- note = discussion_notes.first
+.discussion.js-toggle-container{ class: note.discussion_id }
+  .discussion-header
+    .discussion-actions
+      = link_to "#", class: "js-toggle-button" do
+        %i.icon-chevron-up
+        Show/hide discussion
+    = image_tag avatar_icon(note.author_email), class: "avatar s32"
+    %div
+      = link_to_member(@project, note.author, avatar: false)
+      started a discussion on commit
+      = link_to(note.noteable.short_id, project_commit_path(note.project, note.noteable), class: 'monospace')
+    .last-update.hide.js-toggle-content
+      - last_note = discussion_notes.last
+      last updated by
+      = link_to_member(@project, last_note.author, avatar: false)
+      %span.discussion-last-update
+        #{time_ago_with_tooltip(last_note.updated_at, 'bottom', 'discussion_updated_ago')}
+  .discussion-body.js-toggle-content
+    - if note.for_diff_line?
+      = render "projects/notes/discussion_diff", discussion_notes: discussion_notes, note: note
+    - else
+      .panel.panel-default
+        .notes{ rel: discussion_notes.first.discussion_id }
+          = render discussion_notes
+        .discussion-reply-holder
+          = link_to_reply_diff(discussion_notes.first)
+

app/views/projects/notes/discussions/_outdated.html.haml

+- note = discussion_notes.first
+.discussion.js-toggle-container{ class: note.discussion_id }
+  .discussion-header
+    .discussion-actions
+      = link_to "#", class: "js-toggle-button" do
+        %i.icon-chevron-down
+        Show/hide discussion
+    = image_tag avatar_icon(note.author_email), class: "avatar s32"
+    %div
+      = link_to_member(@project, note.author, avatar: false)
+      started a discussion on the
+      %strong outdated diff
+    %div
+      - last_note = discussion_notes.last
+      last updated by
+      = link_to_member(@project, last_note.author, avatar: false)
+      %span.discussion-last-update
+        #{time_ago_with_tooltip(last_note.updated_at, 'bottom', 'discussion_updated_ago')}
+  .discussion-body.js-toggle-content.hide
+    = render "projects/notes/discussion_diff", discussion_notes: discussion_notes, note: note