Merge branch 'ee-dm-commit-diff-discussions-in-mr-context' into 'master'

Port of !12148 to EE

See merge request gitlab-org/gitlab-ee!3657

app/assets/javascripts/diff_notes/diff_notes_bundle.js

@@ -17,7 +17,8 @@ import './components/diff_note_avatars';
 import './components/new_issue_for_discussion';
 
 $(() => {
-  const projectPath = document.querySelector('.merge-request').dataset.projectPath;
+  const projectPathHolder = document.querySelector('.merge-request') || document.querySelector('.commit-box');
+  const projectPath = projectPathHolder.dataset.projectPath;
   const COMPONENT_SELECTOR = 'resolve-btn, resolve-discussion-btn, jump-to-discussion, comment-and-resolve-btn, new-issue-for-discussion-btn';
 
   window.gl = window.gl || {};

app/assets/javascripts/diff_notes/services/resolve.js

@@ -43,7 +43,7 @@ class ResolveServiceClass {
           discussion.resolveAllNotes(resolvedBy);
         }
 
-        gl.mrWidget.checkStatus();
+        if (gl.mrWidget) gl.mrWidget.checkStatus();
         discussion.updateHeadline(data);
       })
       .catch(() => new Flash('An error occurred when trying to resolve a discussion. Please try again.'));

app/controllers/projects/commit_controller.rb

@@ -134,6 +134,23 @@ class Projects::CommitController < Projects::ApplicationController
     @grouped_diff_discussions = commit.grouped_diff_discussions
     @discussions = commit.discussions
 
+    if merge_request_iid = params[:merge_request_iid]
+      @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).find_by(iid: merge_request_iid)
+
+      if @merge_request
+        @new_diff_note_attrs.merge!(
+          noteable_type: 'MergeRequest',
+          noteable_id: @merge_request.id
+        )
+
+        merge_request_commit_notes = @merge_request.notes.where(commit_id: @commit.id).inc_relations_for_view
+        merge_request_commit_diff_discussions = merge_request_commit_notes.grouped_diff_discussions(@commit.diff_refs)
+        @grouped_diff_discussions.merge!(merge_request_commit_diff_discussions) do |line_code, left, right|
+          left + right
+        end
+      end
+    end
+
     @notes = (@grouped_diff_discussions.values.flatten + @discussions).flat_map(&:notes)
     @notes = prepare_notes_for_rendering(@notes, @commit)
   end

app/controllers/projects/merge_requests/application_controller.rb

@@ -30,7 +30,6 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
       :task_num,
       :title,
       :discussion_locked,
-
       label_ids: []
     ]
   end

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -4,6 +4,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   include RendersNotes
 
   before_action :apply_diff_view_cookie!
+  before_action :commit
   before_action :define_diff_vars
   before_action :define_diff_comment_vars
 
@@ -20,18 +21,33 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   private
 
   def define_diff_vars
+    @merge_request_diffs = @merge_request.merge_request_diffs.viewable.order_id_desc
+    @compare = commit || find_merge_request_diff_compare
+    return render_404 unless @compare
+
+    @diffs = @compare.diffs(diff_options)
+  end
+
+  def commit
+    return nil unless commit_id = params[:commit_id].presence
+    return nil unless @merge_request.all_commits.exists?(sha: commit_id)
+
+    @commit ||= @project.commit(commit_id)
+  end
+
+  def find_merge_request_diff_compare
     @merge_request_diff =
-      if params[:diff_id]
-        @merge_request.merge_request_diffs.viewable.find(params[:diff_id])
+      if diff_id = params[:diff_id].presence
+        @merge_request.merge_request_diffs.viewable.find_by(id: diff_id)
       else
         @merge_request.merge_request_diff
       end
 
-    @merge_request_diffs = @merge_request.merge_request_diffs.viewable.order_id_desc
+    return unless @merge_request_diff
+
     @comparable_diffs = @merge_request_diffs.select { |diff| diff.id < @merge_request_diff.id }
 
-    if params[:start_sha].present?
-      @start_sha = params[:start_sha]
+    if @start_sha = params[:start_sha].presence
       @start_version = @comparable_diffs.find { |diff| diff.head_commit_sha == @start_sha }
 
       unless @start_version
@@ -40,20 +56,18 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
       end
     end
 
-    @compare =
-      if @start_sha
-        @merge_request_diff.compare_with(@start_sha)
-      else
-        @merge_request_diff
-      end
-
-    @diffs = @compare.diffs(diff_options)
+    if @start_sha
+      @merge_request_diff.compare_with(@start_sha)
+    else
+      @merge_request_diff
+    end
   end
 
   def define_diff_comment_vars
     @new_diff_note_attrs = {
       noteable_type: 'MergeRequest',
-      noteable_id: @merge_request.id
+      noteable_id: @merge_request.id,
+      commit_id: @commit&.id
     }
 
     @diff_notes_disabled = false

app/controllers/projects/merge_requests_controller.rb

@@ -9,11 +9,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   prepend ::EE::Projects::MergeRequestsController
 
   skip_before_action :merge_request, only: [:index, :bulk_update]
-
   before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort]
-
   before_action :set_issuables_index, only: [:index]
-
   before_action :authenticate_user!, only: [:assign_related_issues]
 
   def index

app/helpers/commits_helper.rb

@@ -228,4 +228,12 @@ module CommitsHelper
       [commits, 0]
     end
   end
+
+  def commit_path(project, commit, merge_request: nil)
+    if merge_request&.persisted?
+      diffs_project_merge_request_path(project, merge_request, commit_id: commit.id)
+    else
+      project_commit_path(project, commit)
+    end
+  end
 end

app/helpers/merge_requests_helper.rb

@@ -140,6 +140,30 @@ module MergeRequestsHelper
     }.merge(merge_params_ee(merge_request))
   end
 
+  def tab_link_for(merge_request, tab, options = {}, &block)
+    data_attrs = {
+      action: tab.to_s,
+      target: "##{tab}",
+      toggle: options.fetch(:force_link, false) ? '' : 'tab'
+    }
+
+    url = case tab
+          when :show
+            data_attrs[:target] = '#notes'
+            method(:project_merge_request_path)
+          when :commits
+            method(:commits_project_merge_request_path)
+          when :pipelines
+            method(:pipelines_project_merge_request_path)
+          when :diffs
+            method(:diffs_project_merge_request_path)
+          else
+            raise "Cannot create tab #{tab}."
+          end
+
+    link_to(url[merge_request.project, merge_request], data: data_attrs, &block)
+  end
+
   def merge_params_ee(merge_request)
     { squash: merge_request.squash }
   end

app/models/commit.rb

+# coding: utf-8
 class Commit
   extend ActiveModel::Naming
   extend Gitlab::Cache::RequestCache
@@ -25,7 +26,7 @@ class Commit
   DIFF_HARD_LIMIT_FILES = 1000
   DIFF_HARD_LIMIT_LINES = 50000
 
-  MIN_SHA_LENGTH = 7
+  MIN_SHA_LENGTH = Gitlab::Git::Commit::MIN_SHA_LENGTH
   COMMIT_SHA_PATTERN = /\h{#{MIN_SHA_LENGTH},40}/.freeze
 
   def banzai_render_context(field)

app/models/concerns/discussion_on_diff.rb

@@ -32,6 +32,10 @@ module DiscussionOnDiff
     first_note.position.new_path
   end
 
+  def on_merge_request_commit?
+    for_merge_request? && commit_id.present?
+  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

app/models/diff_discussion.rb

@@ -24,7 +24,11 @@ class DiffDiscussion < Discussion
     return unless for_merge_request?
     return {} if active?
 
-    noteable.version_params_for(position.diff_refs)
+    if on_merge_request_commit?
+      { commit_id: commit_id }
+    else
+      noteable.version_params_for(position.diff_refs)
+    end
   end
 
   def reply_attributes

app/models/diff_note.rb

@@ -22,6 +22,7 @@ class DiffNote < Note
   validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
   validate :positions_complete
   validate :verify_supported
+  validate :diff_refs_match_commit, if: :for_commit?
 
   before_validation :set_original_position, on: :create
   before_validation :update_position, on: :create, if: :on_text?
@@ -140,6 +141,12 @@ class DiffNote < Note
     errors.add(:position, "is invalid")
   end
 
+  def diff_refs_match_commit
+    return if self.original_position.diff_refs == self.commit.diff_refs
+
+    errors.add(:commit_id, 'does not match the diff refs')
+  end
+
   def keep_around_commits
     project.repository.keep_around(self.original_position.base_sha)
     project.repository.keep_around(self.original_position.start_sha)

app/models/discussion.rb

@@ -11,6 +11,7 @@ class Discussion
             :author,
 
             :noteable,
+            :commit_id,
             :for_commit?,
             :for_merge_request?,
 

app/models/merge_request.rb

@@ -667,6 +667,7 @@ class MergeRequest < ActiveRecord::Base
       .to_sql
 
     Note.from("(#{union}) #{Note.table_name}")
+      .includes(:noteable)
   end
 
   alias_method :discussion_notes, :related_notes
@@ -942,21 +943,27 @@ class MergeRequest < ActiveRecord::Base
       .order(id: :desc)
   end
 
-  # Note that this could also return SHA from now dangling commits
-  #
-  def all_commit_shas
-    return commit_shas unless persisted?
-
-    diffs_relation = merge_request_diffs
-
+  def all_commits
     # MySQL doesn't support LIMIT in a subquery.
-    diffs_relation = diffs_relation.recent if Gitlab::Database.postgresql?
+    diffs_relation = if Gitlab::Database.postgresql?
+                       merge_request_diffs.recent
+                     else
+                       merge_request_diffs
+                     end
 
     MergeRequestDiffCommit
       .where(merge_request_diff: diffs_relation)
       .limit(10_000)
-      .pluck('sha')
-      .uniq
+  end
+
+  # Note that this could also return SHA from now dangling commits
+  #
+  def all_commit_shas
+    @all_commit_shas ||= begin
+      return commit_shas unless persisted?
+
+      all_commits.pluck(:sha).uniq
+    end
   end
 
   def merge_commit

app/models/note.rb

@@ -236,16 +236,18 @@ class Note < ActiveRecord::Base
     for_personal_snippet?
   end
 
+  def commit
+    @commit ||= project.commit(commit_id) if commit_id.present?
+  end
+
   # override to return commits, which are not active record
   def noteable
-    if for_commit?
-      @commit ||= project.commit(commit_id)
-    else
-      super
-    end
-  # Temp fix to prevent app crash
-  # if note commit id doesn't exist
+    return commit if for_commit?
+
+    super
   rescue
+    # Temp fix to prevent app crash
+    # if note commit id doesn't exist
     nil
   end
 
@@ -375,6 +377,42 @@ class Note < ActiveRecord::Base
     Gitlab::EtagCaching::Store.new.touch(key)
   end
 
+  def touch(*args)
+    # We're not using an explicit transaction here because this would in all
+    # cases result in all future queries going to the primary, even if no writes
+    # are performed.
+    #
+    # We touch the noteable first so its SELECT query can run before our writes,
+    # ensuring it runs on a secondary (if no prior write took place).
+    touch_noteable
+    super
+  end
+
+  # By default Rails will issue an "SELECT *" for the relation, which is
+  # overkill for just updating the timestamps. To work around this we manually
+  # touch the data so we can SELECT only the columns we need.
+  def touch_noteable
+    # Commits are not stored in the DB so we can't touch them.
+    return if for_commit?
+
+    assoc = association(:noteable)
+
+    noteable_object =
+      if assoc.loaded?
+        noteable
+      else
+        # If the object is not loaded (e.g. when notes are loaded async) we
+        # _only_ want the data we actually need.
+        assoc.scope.select(:id, :updated_at).take
+      end
+
+    noteable_object&.touch
+  end
+
+  def banzai_render_context(field)
+    super.merge(noteable: noteable)
+  end
+
   private
 
   def keep_around_commit

app/services/merge_requests/refresh_service.rb

@@ -6,7 +6,7 @@ module MergeRequests
       @oldrev, @newrev = oldrev, newrev
       @branch_name = Gitlab::Git.ref_name(ref)
 
-      find_new_commits
+      Gitlab::GitalyClient.allow_n_plus_1_calls(&method(:find_new_commits))
       # Be sure to close outstanding MRs before reloading them to avoid generating an
       # empty diff during a manual merge
       close_merge_requests

app/views/discussions/_discussion.html.haml

@@ -32,9 +32,17 @@
             - elsif discussion.diff_discussion?
               on
               = conditional_link_to url.present?, url do
-                - unless discussion.active?
-                  an old version of
-                the diff
+                - if discussion.on_merge_request_commit?
+                  - unless discussion.active?
+                    an outdated change in
+                  commit
+
+                  %span.commit-sha= Commit.truncate_sha(discussion.commit_id)
+                - else
+                  - unless discussion.active?
+                    an old version of
+                  the diff
+
 
             = time_ago_with_tooltip(discussion.created_at, placement: "bottom", html_class: "note-created-ago")
           = render "discussions/headline", discussion: discussion

app/views/projects/commit/_commit_box.html.haml

@@ -47,7 +47,7 @@
           %li= link_to s_("DownloadCommit|Email Patches"), project_commit_path(@project, @commit, format: :patch)
         %li= link_to s_("DownloadCommit|Plain Diff"),    project_commit_path(@project, @commit, format: :diff)
 
-.commit-box
+.commit-box{ data: { project_path: project_path(@project) } }
   %h3.commit-title
     = markdown(@commit.title, pipeline: :single_line, author: @commit.author)
   - if @commit.description.present?
@@ -80,3 +80,13 @@
       - if last_pipeline.duration
         in
         = time_interval_in_words last_pipeline.duration
+
+  - if @merge_request
+    .well-segment
+      = icon('info-circle fw')
+
+      This commit is part of merge request
+      = succeed '.' do
+        = link_to @merge_request.to_reference, diffs_project_merge_request_path(@project, @merge_request, commit_id: @commit.id)
+
+      Comments created here will be created in the context of that merge request.

app/views/projects/commit/show.html.haml

@@ -6,6 +6,9 @@
 - @content_class = limited_container_width
 - page_title        "#{@commit.title} (#{@commit.short_id})", "Commits"
 - page_description  @commit.description
+- content_for :page_specific_javascripts do
+  = page_specific_javascript_bundle_tag('common_vue')
+  = page_specific_javascript_bundle_tag('diff_notes')
 
 .container-fluid{ class: [limited_container_width, container_class] }
   = render "commit_box"

app/views/projects/commits/_commit.html.haml

-- ref = local_assigns.fetch(:ref)
+- view_details  = local_assigns.fetch(:view_details, false)
+- merge_request = local_assigns.fetch(:merge_request, nil)
+- project       = local_assigns.fetch(:project) { merge_request&.project }
+- ref           = local_assigns.fetch(:ref) { merge_request&.source_branch }
 
-- cache_key = [project.full_path, commit.id, current_application_settings, @path.presence, current_controller?(:commits), I18n.locale]
-- cache_key.push(commit.status(ref)) if commit.status(ref)
+- link = commit_path(project, commit, merge_request: merge_request)
+- cache_key = [project.full_path,
+               commit.id,
+               current_application_settings,
+               @path.presence,
+               current_controller?(:commits),
+               merge_request&.iid,
+               view_details,
+               commit.status(ref),
+               I18n.locale].compact
 
 -# EE-only
 - show_project_name = local_assigns.fetch(:show_project_name, false)
@@ -15,7 +26,7 @@
 
     .commit-detail
       .commit-content
-        = link_to_markdown_field(commit, :title, project_commit_path(project, commit.id), class: "commit-row-message item-title")
+        = link_to_markdown_field(commit, :title, link, class: "commit-row-message item-title")
         %span.commit-row-message.visible-xs-inline
           ·
           = commit.short_id
@@ -38,8 +49,7 @@
           %span.project_namespace
             = project.name_with_namespace
 
-
-      .commit-actions.hidden-xs
+      .commit-actions.flex-row.hidden-xs
         - if request.xhr?
           = render partial: 'projects/commit/signature', object: commit.signature
         - else
@@ -48,6 +58,9 @@
         - if commit.status(ref)
           = render_commit_status(commit, ref: ref)
 
-        = link_to commit.short_id, project_commit_path(project, commit), class: "commit-sha btn btn-transparent btn-link"
+        = link_to commit.short_id, link, class: "commit-sha btn btn-transparent btn-link"
         = clipboard_button(text: commit.id, title: _("Copy commit SHA to clipboard"))
         = link_to_browse_code(project, commit)
+
+        - if view_details && merge_request
+          = link_to "View details", project_commit_path(project, commit.id, merge_request_iid: merge_request.iid), class: "btn btn-default"

app/views/projects/commits/_commits.html.haml

-- ref = local_assigns.fetch(:ref)
+- merge_request = local_assigns.fetch(:merge_request, nil)
+- project = local_assigns.fetch(:project) { merge_request&.project }
+- ref = local_assigns.fetch(:ref) { merge_request&.source_branch }
+
 - commits, hidden = limited_commits(@commits)
 
 - commits.chunk { |c| c.committed_date.in_time_zone.to_date }.each do |day, commits|
@@ -8,7 +11,7 @@
 
   %li.commits-row{ data: { day: day } }
     %ul.content-list.commit-list.flex-list
-      = render partial: 'projects/commits/commit', collection: commits, locals: { project: project, ref: ref }
+      = render partial: 'projects/commits/commit', collection: commits, locals: { project: project, ref: ref, merge_request: merge_request }
 
 - if hidden > 0
   %li.alert.alert-warning

app/views/projects/merge_requests/_commits.html.haml

@@ -5,4 +5,4 @@
     = custom_icon ('illustration_no_commits')
 - else
   %ol#commits-list.list-unstyled
-    = render "projects/commits/commits", project: @merge_request.source_project, ref: @merge_request.source_branch
+    = render "projects/commits/commits", merge_request: @merge_request

app/views/projects/merge_requests/diffs/_commit_widget.html.haml

+- if @commit
+  .info-well.hidden-xs.prepend-top-default
+    .well-segment
+      %ul.blob-commit-info
+        = render 'projects/commits/commit', commit: @commit, merge_request: @merge_request, view_details: true

app/views/projects/merge_requests/diffs/_different_base.html.haml

+- if @merge_request_diff && different_base?(@start_version, @merge_request_diff)
+  .mr-version-controls
+    .content-block
+      = icon('info-circle')
+      Selected versions have different base commits.
+      Changes will include
+      = link_to project_compare_path(@project, from: @start_version.base_commit_sha, to: @merge_request_diff.base_commit_sha) do
+        new commits
+      from
+      = succeed '.' do
+        %code.ref-name= @merge_request.target_branch

app/views/projects/merge_requests/diffs/_diffs.html.haml

-- if @merge_request_diff.collected? || @merge_request_diff.overflow?
-  = render 'projects/merge_requests/diffs/versions'
-  = render "projects/diffs/diffs", diffs: @diffs, environment: @environment, merge_request: true
-- elsif @merge_request_diff.empty?
-  .nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch}
+= render 'projects/merge_requests/diffs/version_controls'
+= render 'projects/merge_requests/diffs/different_base'
+= render 'projects/merge_requests/diffs/not_all_comments_displayed'
+= render 'projects/merge_requests/diffs/commit_widget'
+
+- if @merge_request_diff&.empty?
+  .nothing-here-block
+    = image_tag 'illustrations/merge_request_changes_empty.svg'
+    = succeed '.' do
+      No changes between
+      %span.ref-name= @merge_request.source_branch
+      and
+      %span.ref-name= @merge_request.target_branch
+      %p= link_to 'Create commit', project_new_blob_path(@project, @merge_request.source_branch), class: 'btn btn-save'
+- else
+  - diff_viewable = @merge_request_diff ? @merge_request_diff.collected? || @merge_request_diff.overflow? : true
+  - if diff_viewable
+    = render "projects/diffs/diffs", diffs: @diffs, environment: @environment, merge_request: true

app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml

+- if @commit || @start_version || (@merge_request_diff && !@merge_request_diff.latest?)
+  .mr-version-controls
+    .content-block.comments-disabled-notif.clearfix
+      = icon('info-circle')
+      = succeed '.' do
+        - if @commit
+          Only comments from the following commit are shown below
+        - else
+          Not all comments are displayed because you're
+          - if @start_version
+            comparing two versions of the diff
+          - else
+            viewing an old version of the diff
+      .pull-right
+        = link_to diffs_project_merge_request_path(@merge_request.project, @merge_request), class: 'btn btn-sm' do
+          Show latest version
+          = "of the diff" if @commit

app/views/projects/merge_requests/diffs/_versions.html.haml → app/views/projects/merge_requests/diffs/_version_controls.html.haml

-- if @merge_request_diffs.size > 1
+- if @merge_request_diff && @merge_request_diffs.size > 1
   .mr-version-controls
     .mr-version-menus-container.content-block
       Changes between
@@ -71,27 +71,3 @@
                         (base)
                     %div
                       %strong.commit-sha= short_sha(@merge_request_diff.base_commit_sha)
-
-    - if different_base?(@start_version, @merge_request_diff)
-      .content-block
-        = icon('info-circle')
-        Selected versions have different base commits.
-        Changes will include
-        = link_to project_compare_path(@project, from: @start_version.base_commit_sha, to: @merge_request_diff.base_commit_sha) do
-          new commits
-        from
-        = succeed '.' do
-          %code= @merge_request.target_branch
-
-    - if @start_version || !@merge_request_diff.latest?
-      .comments-disabled-notif.content-block
-        = icon('info-circle')
-        Not all comments are displayed because you're
-        - if @start_version
-          comparing two versions
-        - else
-          viewing an old version
-        of the diff.
-
-        .pull-right
-          = link_to 'Show latest version', diffs_project_merge_request_path(@project, @merge_request), class: 'btn btn-sm'

app/views/projects/merge_requests/show.html.haml

@@ -9,7 +9,7 @@
   = webpack_bundle_tag('diff_notes')
   = webpack_bundle_tag('issuable')
 
-.merge-request{ 'data-mr-action': "#{j params[:tab].presence || 'show'}", 'data-url' => merge_request_path(@merge_request, format: :json), 'data-project-path' => project_path(@merge_request.project) }
+.merge-request{ data: { mr_action: j(params[:tab].presence || 'show'), url: merge_request_path(@merge_request, format: :json), project_path: project_path(@merge_request.project) } }
   = render "projects/merge_requests/mr_title"
 
   .merge-request-details.issuable-details{ data: { id: @merge_request.project.id } }
@@ -46,21 +46,21 @@
           .nav-links.scrolling-tabs
             %ul.merge-request-tabs
               %li.notes-tab
-                = link_to project_merge_request_path(@project, @merge_request), data: { target: 'div#notes', action: 'show', toggle: 'tab' } do
+                = tab_link_for @merge_request, :show, force_link: @commit.present? do
                   Discussion
                   %span.badge= @merge_request.related_notes.user.count
               - if @merge_request.source_project
                 %li.commits-tab
-                  = link_to commits_project_merge_request_path(@project, @merge_request), data: { target: 'div#commits', action: 'commits', toggle: 'tab' } do
+                  = tab_link_for @merge_request, :commits do
                     Commits
                     %span.badge= @commits_count
               - if @pipelines.any?
                 %li.pipelines-tab
-                  = link_to pipelines_project_merge_request_path(@project, @merge_request), data: { target: '#pipelines', action: 'pipelines', toggle: 'tab' } do
+                  = tab_link_for @merge_request, :pipelines do
                     Pipelines
                     %span.badge.js-pipelines-mr-count= @pipelines.size
               %li.diffs-tab
-                = link_to diffs_project_merge_request_path(@project, @merge_request), data: { target: 'div#diffs', action: 'diffs', toggle: 'tab' } do
+                = tab_link_for @merge_request, :diffs do
                   Changes
                   %span.badge= @merge_request.diff_size
         #resolve-count-app.line-resolve-all-container.prepend-top-10{ "v-cloak" => true }

changelogs/unreleased/dm-commit-diff-discussions-in-mr-context.yml

+---
+title: Make diff notes created on a commit in a merge request to persist a rebase.
+merge_request: 12148
+author:
+type: added

lib/banzai/filter/commit_reference_filter.rb

@@ -22,10 +22,30 @@ module Banzai
         end
       end
 
+      def referenced_merge_request_commit_shas
+        return [] unless noteable.is_a?(MergeRequest)
+
+        @referenced_merge_request_commit_shas ||= begin
+          referenced_shas = references_per_parent.values.reduce(:|).to_a
+          noteable.all_commit_shas.select do |sha|
+            referenced_shas.any? { |ref| Gitlab::Git.shas_eql?(sha, ref) }
+          end
+        end
+      end
+
       def url_for_object(commit, project)
         h = Gitlab::Routing.url_helpers
-        h.project_commit_url(project, commit,
-                                        only_path: context[:only_path])
+
+        if referenced_merge_request_commit_shas.include?(commit.id)
+          h.diffs_project_merge_request_url(project,
+                                            noteable,
+                                            commit_id: commit.id,
+                                            only_path: only_path?)
+        else
+          h.project_commit_url(project,
+                               commit,
+                               only_path: only_path?)
+        end
       end
 
       def object_link_text_extras(object, matches)
@@ -38,6 +58,16 @@ module Banzai
 
         extras
       end
+
+      private
+
+      def noteable
+        context[:noteable]
+      end
+
+      def only_path?
+        context[:only_path]
+      end
     end
   end
 end

lib/banzai/object_renderer.rb

@@ -17,11 +17,11 @@ module Banzai
 
     # project - A Project to use for redacting Markdown.
     # user - The user viewing the Markdown/HTML documents, if any.
-    # context - A Hash containing extra attributes to use during redaction
+    # redaction_context - A Hash containing extra attributes to use during redaction
     def initialize(project, user = nil, redaction_context = {})
       @project = project
       @user = user
-      @redaction_context = redaction_context
+      @redaction_context = base_context.merge(redaction_context)
     end
 
     # Renders and redacts an Array of objects.
@@ -73,19 +73,19 @@ module Banzai
 
     # Returns a Banzai context for the given object and attribute.
     def context_for(object, attribute)
-      base_context.merge(object.banzai_render_context(attribute))
+      @redaction_context.merge(object.banzai_render_context(attribute))
     end
 
     def base_context
-      @base_context ||= @redaction_context.merge(
+      {
         current_user: user,
         project: project,
         skip_redaction: true
-      )
+      }
     end
 
     def save_options
-      return {} unless base_context[:xhtml]
+      return {} unless @redaction_context[:xhtml]
 
       { save_with: Nokogiri::XML::Node::SaveOptions::AS_XHTML }
     end

lib/gitlab/diff/diff_refs.rb

@@ -13,9 +13,9 @@ module Gitlab
 
       def ==(other)
         other.is_a?(self.class) &&
-          shas_equal?(base_sha, other.base_sha) &&
-          shas_equal?(start_sha, other.start_sha) &&
-          shas_equal?(head_sha, other.head_sha)
+          Git.shas_eql?(base_sha, other.base_sha) &&
+          Git.shas_eql?(start_sha, other.start_sha) &&
+          Git.shas_eql?(head_sha, other.head_sha)
       end
 
       alias_method :eql?, :==
@@ -47,22 +47,6 @@ module Gitlab
           CompareService.new(project, head_sha).execute(project, start_sha, straight: straight)
         end
       end
-
-      private
-
-      def shas_equal?(sha1, sha2)
-        return true if sha1 == sha2
-        return false if sha1.nil? || sha2.nil?
-        return false unless sha1.class == sha2.class
-
-        length = [sha1.length, sha2.length].min
-
-        # If either of the shas is below the minimum length, we cannot be sure
-        # that they actually refer to the same commit because of hash collision.
-        return false if length < Commit::MIN_SHA_LENGTH
-
-        sha1[0, length] == sha2[0, length]
-      end
     end
   end
 end

lib/gitlab/git.rb

@@ -70,6 +70,18 @@ module Gitlab
       def diff_line_code(file_path, new_line_position, old_line_position)
         "#{Digest::SHA1.hexdigest(file_path)}_#{old_line_position}_#{new_line_position}"
       end
+
+      def shas_eql?(sha1, sha2)
+        return false if sha1.nil? || sha2.nil?
+        return false unless sha1.class == sha2.class
+
+        # If either of the shas is below the minimum length, we cannot be sure
+        # that they actually refer to the same commit because of hash collision.
+        length = [sha1.length, sha2.length].min
+        return false if length < Gitlab::Git::Commit::MIN_SHA_LENGTH
+
+        sha1[0, length] == sha2[0, length]
+      end
     end
   end
 end

lib/gitlab/git/commit.rb

@@ -6,6 +6,7 @@ module Gitlab
 
       attr_accessor :raw_commit, :head
 
+      MIN_SHA_LENGTH = 7
       SERIALIZE_KEYS = [
         :id, :message, :parent_ids,
         :authored_date, :author_name, :author_email,

spec/controllers/projects/commit_controller_spec.rb

@@ -132,6 +132,22 @@ describe Projects::CommitController do
         expect(response).to be_success
       end
     end
+
+    context 'in the context of a merge_request' do
+      let(:merge_request) { create(:merge_request, source_project: project) }
+      let(:commit) { merge_request.commits.first }
+
+      it 'prepare diff notes in the context of the merge request' do
+        go(id: commit.id, merge_request_iid: merge_request.iid)
+
+        expect(assigns(:new_diff_note_attrs)).to eq({
+                                                      noteable_type: 'MergeRequest',
+                                                      noteable_id: merge_request.id,
+                                                      commit_id: commit.id
+                                                    })
+        expect(response).to be_ok
+      end
+    end
   end
 
   describe 'GET branches' do
@@ -323,7 +339,7 @@ describe Projects::CommitController do
 
     context 'when the commit does not exist' do
       before do
-        diff_for_path(id: commit.id.succ, old_path: existing_path, new_path: existing_path)
+        diff_for_path(id: commit.id.reverse, old_path: existing_path, new_path: existing_path)
       end
 
       it 'returns a 404' do

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -100,7 +100,8 @@ describe Projects::MergeRequests::DiffsController do
 
             expect(assigns(:diff_notes_disabled)).to be_falsey
             expect(assigns(:new_diff_note_attrs)).to eq(noteable_type: 'MergeRequest',
-                                                        noteable_id: merge_request.id)
+                                                        noteable_id: merge_request.id,
+                                                        commit_id: nil)
           end
 
           it 'only renders the diffs for the path given' do

spec/factories/notes.rb

@@ -63,13 +63,19 @@ FactoryGirl.define do
 
     factory :diff_note_on_commit, traits: [:on_commit], class: DiffNote do
       association :project, :repository
+
+      transient do
+        line_number 14
+        diff_refs { project.commit(commit_id).try(:diff_refs) }
+      end
+
       position do
         Gitlab::Diff::Position.new(
           old_path: "files/ruby/popen.rb",
           new_path: "files/ruby/popen.rb",
           old_line: nil,
-          new_line: 14,
-          diff_refs: project.commit(commit_id).try(:diff_refs)
+          new_line: line_number,
+          diff_refs: diff_refs
         )
       end
     end

spec/features/merge_requests/versions_spec.rb

@@ -6,18 +6,47 @@ feature 'Merge Request versions', :js do
   let!(:merge_request_diff1) { merge_request.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') }
   let!(:merge_request_diff2) { merge_request.merge_request_diffs.create(head_commit_sha: nil) }
   let!(:merge_request_diff3) { merge_request.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') }
+  let!(:params) { Hash.new }
 
   before do
     sign_in(create(:admin))
-    visit diffs_project_merge_request_path(project, merge_request)
+    visit diffs_project_merge_request_path(project, merge_request, params)
   end
 
-  it 'show the latest version of the diff' do
-    page.within '.mr-version-dropdown' do
-      expect(page).to have_content 'latest version'
+  shared_examples 'allows commenting' do |file_id:, line_code:, comment:|
+    it do
+      diff_file_selector = ".diff-file[id='#{file_id}']"
+      line_code = "#{file_id}_#{line_code}"
+
+      page.within(diff_file_selector) do
+        find(".line_holder[id='#{line_code}'] td:nth-of-type(1)").hover
+        find(".line_holder[id='#{line_code}'] button").click
+
+        page.within("form[data-line-code='#{line_code}']") do
+          fill_in "note[note]", with: comment
+          find(".js-comment-button").click
+        end
+
+        wait_for_requests
+
+        expect(page).to have_content(comment)
+      end
     end
+  end
 
-    expect(page).to have_content '8 changed files'
+  describe 'compare with the latest version' do
+    it 'show the latest version of the diff' do
+      page.within '.mr-version-dropdown' do
+        expect(page).to have_content 'latest version'
+      end
+
+      expect(page).to have_content '8 changed files'
+    end
+
+    it_behaves_like 'allows commenting',
+                    file_id: '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44',
+                    line_code: '1_1',
+                    comment: 'Typo, please fix.'
   end
 
   describe 'switch between versions' do
@@ -62,24 +91,10 @@ feature 'Merge Request versions', :js do
       expect(page).to have_css(".diffs .notes[data-discussion-id='#{outdated_diff_note.discussion_id}']")
     end
 
-    it 'allows commenting' do
-      diff_file_selector = ".diff-file[id='7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44']"
-      line_code = '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44_2_2'
-
-      page.within(diff_file_selector) do
-        find(".line_holder[id='#{line_code}'] td:nth-of-type(1)").hover
-        find(".line_holder[id='#{line_code}'] button").click
-
-        page.within("form[data-line-code='#{line_code}']") do
-          fill_in "note[note]", with: "Typo, please fix"
-          find(".js-comment-button").click
-        end
-
-        wait_for_requests
-
-        expect(page).to have_content("Typo, please fix")
-      end
-    end
+    it_behaves_like 'allows commenting',
+                    file_id: '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44',
+                    line_code: '2_2',
+                    comment: 'Typo, please fix.'
   end
 
   describe 'compare with older version' do
@@ -132,25 +147,6 @@ feature 'Merge Request versions', :js do
       expect(page).to have_css(".diffs .notes[data-discussion-id='#{outdated_diff_note.discussion_id}']")
     end
 
-    it 'allows commenting' do
-      diff_file_selector = ".diff-file[id='7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44']"
-      line_code = '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44_4_4'
-
-      page.within(diff_file_selector) do
-        find(".line_holder[id='#{line_code}'] td:nth-of-type(1)").hover
-        find(".line_holder[id='#{line_code}'] button").click
-
-        page.within("form[data-line-code='#{line_code}']") do
-          fill_in "note[note]", with: "Typo, please fix"
-          find(".js-comment-button").click
-        end
-
-        wait_for_requests
-
-        expect(page).to have_content("Typo, please fix")
-      end
-    end
-
     it 'show diff between new and old version' do
       expect(page).to have_content '4 changed files with 15 additions and 6 deletions'
     end
@@ -162,6 +158,11 @@ feature 'Merge Request versions', :js do
       end
       expect(page).to have_content '8 changed files'
     end
+
+    it_behaves_like 'allows commenting',
+                    file_id: '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44',
+                    line_code: '4_4',
+                    comment: 'Typo, please fix.'
   end
 
   describe 'compare with same version' do
@@ -210,4 +211,24 @@ feature 'Merge Request versions', :js do
       expect(page).to have_content '0 changed files'
     end
   end
+
+  describe 'scoped in a commit' do
+    let(:params) { { commit_id: '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' } }
+
+    before do
+      wait_for_requests
+    end
+
+    it 'should only show diffs from the commit' do
+      diff_commit_ids = find_all('.diff-file [data-commit-id]').map {|diff| diff['data-commit-id']}
+
+      expect(diff_commit_ids).not_to be_empty
+      expect(diff_commit_ids).to all(eq(params[:commit_id]))
+    end
+
+    it_behaves_like 'allows commenting',
+                    file_id: '2f6fcd96b88b36ce98c38da085c795a27d92a3dd',
+                    line_code: '6_6',
+                    comment: 'Typo, please fix.'
+  end
 end

spec/helpers/merge_requests_helper_spec.rb

 require 'spec_helper'
 
 describe MergeRequestsHelper do
+  include ActionView::Helpers::UrlHelper
   include ProjectForksHelper
+
   describe 'ci_build_details_path' do
     let(:project) { create(:project) }
     let(:merge_request) { MergeRequest.new }
@@ -55,4 +57,19 @@ describe MergeRequestsHelper do
       expect(render_items_list(%w(user user1 user2))).to eq("user, user1 and user2")
     end
   end
+
+  describe '#tab_link_for' do
+    let(:merge_request) { create(:merge_request, :simple) }
+    let(:options) { Hash.new }
+
+    subject { tab_link_for(merge_request, :show, options) { 'Discussion' } }
+
+    describe 'supports the :force_link option' do
+      let(:options) { { force_link: true } }
+
+      it 'removes the data-toggle attributes' do
+        is_expected.not_to match(/data-toggle="tab"/)
+      end
+    end
+  end
 end

spec/lib/banzai/filter/commit_reference_filter_spec.rb

@@ -92,6 +92,18 @@ describe Banzai::Filter::CommitReferenceFilter do
       expect(link).not_to match %r(https?://)
       expect(link).to eq urls.project_commit_url(project, reference, only_path: true)
     end
+
+    context "in merge request context" do
+      let(:noteable) { create(:merge_request, target_project: project, source_project: project) }
+      let(:commit) { noteable.commits.first }
+
+      it 'handles merge request contextual commit references' do
+        url = urls.diffs_project_merge_request_url(project, noteable, commit_id: commit.id)
+        doc = reference_filter("See #{reference}", noteable: noteable)
+
+        expect(doc.css('a').first[:href]).to eq(url)
+      end
+    end
   end
 
   context 'cross-project / cross-namespace complete reference' do

spec/lib/gitlab/git_spec.rb

@@ -38,4 +38,29 @@ describe Gitlab::Git do
       expect(described_class.ref_name(utf8_invalid_ref)).to eq("an_invalid_ref_å")
     end
   end
+
+  describe '.shas_eql?' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:sha1, :sha2, :result) do
+      sha           = RepoHelpers.sample_commit.id
+      short_sha     = sha[0, Gitlab::Git::Commit::MIN_SHA_LENGTH]
+      too_short_sha = sha[0, Gitlab::Git::Commit::MIN_SHA_LENGTH - 1]
+
+      [
+        [sha, sha,           true],
+        [sha, short_sha,     true],
+        [sha, sha.reverse,   false],
+        [sha, too_short_sha, false],
+        [sha, nil,           false]
+      ]
+    end
+
+    with_them do
+      it { expect(described_class.shas_eql?(sha1, sha2)).to eq(result) }
+      it 'is commutative' do
+        expect(described_class.shas_eql?(sha2, sha1)).to eq(result)
+      end
+    end
+  end
 end

spec/models/diff_note_spec.rb

@@ -9,13 +9,14 @@ describe DiffNote do
 
   let(:path) { "files/ruby/popen.rb" }
 
+  let(:diff_refs) { merge_request.diff_refs }
   let!(:position) do
     Gitlab::Diff::Position.new(
       old_path: path,
       new_path: path,
       old_line: nil,
       new_line: 14,
-      diff_refs: merge_request.diff_refs
+      diff_refs: diff_refs
     )
   end
 
@@ -25,7 +26,7 @@ describe DiffNote do
       new_path: path,
       old_line: 16,
       new_line: 22,
-      diff_refs: merge_request.diff_refs
+      diff_refs: diff_refs
     )
   end
 
@@ -158,25 +159,21 @@ describe DiffNote do
   describe "creation" do
     describe "updating of position" do
       context "when noteable is a commit" do
-        let(:diff_note) { create(:diff_note_on_commit, project: project, position: position) }
+        let(:diff_refs) { commit.diff_refs }
 
-        it "doesn't update the position" do
-          diff_note
+        subject { create(:diff_note_on_commit, project: project, position: position, commit_id: commit.id) }
 
-          expect(diff_note.original_position).to eq(position)
-          expect(diff_note.position).to eq(position)
+        it "doesn't update the position" do
+          is_expected.to have_attributes(original_position: position,
+                                         position: position)
         end
       end
 
       context "when noteable is a merge request" do
-        let(:diff_note) { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
-
         context "when the note is active" do
           it "doesn't update the position" do
-            diff_note
-
-            expect(diff_note.original_position).to eq(position)
-            expect(diff_note.position).to eq(position)
+            expect(subject.original_position).to eq(position)
+            expect(subject.position).to eq(position)
           end
         end
 
@@ -186,10 +183,8 @@ describe DiffNote do
           end
 
           it "updates the position" do
-            diff_note
-
-            expect(diff_note.original_position).to eq(position)
-            expect(diff_note.position).not_to eq(position)
+            expect(subject.original_position).to eq(position)
+            expect(subject.position).not_to eq(position)
           end
         end
       end

spec/models/merge_request_spec.rb

@@ -1149,7 +1149,7 @@ describe MergeRequest do
       end
 
       shared_examples 'returning all SHA' do
-        it 'returns all SHA from all merge_request_diffs' do
+        it 'returns all SHAs from all merge_request_diffs' do
           expect(subject.merge_request_diffs.size).to eq(2)
           expect(subject.all_commit_shas).to match_array(all_commit_shas)
         end

spec/services/system_note_service_spec.rb

@@ -694,9 +694,9 @@ describe SystemNoteService do
   describe '.new_commit_summary' do
     it 'escapes HTML titles' do
       commit = double(title: '<pre>This is a test</pre>', short_id: '12345678')
-      escaped = '* 12345678 - &lt;pre&gt;This is a test&lt;&#x2F;pre&gt;'
+      escaped = '&lt;pre&gt;This is a test&lt;&#x2F;pre&gt;'
 
-      expect(described_class.new_commit_summary([commit])).to eq([escaped])
+      expect(described_class.new_commit_summary([commit])).to all(match(%r[- #{escaped}]))
     end
   end
 

spec/views/projects/commit/show.html.haml_spec.rb

@@ -2,14 +2,15 @@ require 'spec_helper'
 
 describe 'projects/commit/show.html.haml' do
   let(:project) { create(:project, :repository) }
+  let(:commit) { project.commit }
 
   before do
     assign(:project, project)
     assign(:repository, project.repository)
-    assign(:commit, project.commit)
-    assign(:noteable, project.commit)
+    assign(:commit, commit)
+    assign(:noteable, commit)
     assign(:notes, [])
-    assign(:diffs, project.commit.diffs)
+    assign(:diffs, commit.diffs)
 
     allow(view).to receive(:current_user).and_return(nil)
     allow(view).to receive(:can?).and_return(false)
@@ -43,4 +44,19 @@ describe 'projects/commit/show.html.haml' do
       expect(rendered).not_to have_selector('.limit-container-width')
     end
   end
+
+  context 'in the context of a merge request' do
+    let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+    before do
+      assign(:merge_request, merge_request)
+      render
+    end
+
+    it 'shows that it is in the context of a merge request' do
+      merge_request_url = diffs_project_merge_request_url(project, merge_request, commit_id: commit.id)
+      expect(rendered).to have_content("This commit is part of merge request")
+      expect(rendered).to have_link(merge_request.to_reference, merge_request_url)
+    end
+  end
 end

spec/views/projects/merge_requests/_commits.html.haml_spec.rb

@@ -25,8 +25,8 @@ describe 'projects/merge_requests/_commits.html.haml' do
   it 'shows commits from source project' do
     render
 
-    commit = source_project.commit(merge_request.source_branch)
-    href = project_commit_path(source_project, commit)
+    commit = merge_request.commits.first # HEAD
+    href = diffs_project_merge_request_path(target_project, merge_request, commit_id: commit)
 
     expect(rendered).to have_link(Commit.truncate_sha(commit.sha), href: href)
   end

spec/views/projects/merge_requests/diffs/_diffs.html.haml_spec.rb

+require 'spec_helper'
+
+describe 'projects/merge_requests/diffs/_diffs.html.haml' do
+  include Devise::Test::ControllerHelpers
+
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :public, :repository) }
+  let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project, author: user) }
+
+  before do
+    allow(view).to receive(:url_for).and_return(controller.request.fullpath)
+
+    assign(:merge_request, merge_request)
+    assign(:environment, merge_request.environments_for(user).last)
+    assign(:diffs, merge_request.diffs)
+    assign(:merge_request_diffs, merge_request.diffs)
+    assign(:diff_notes_disabled, true) # disable note creation
+    assign(:use_legacy_diff_notes, false)
+    assign(:grouped_diff_discussions, {})
+    assign(:notes, [])
+  end
+
+  context 'for a commit' do
+    let(:commit) { merge_request.commits.last }
+
+    before do
+      assign(:commit, commit)
+    end
+
+    it "shows the commit scope" do
+      render
+
+      expect(rendered).to have_content "Only comments from the following commit are shown below"
+    end
+  end
+end