Commit 9dffd0ab authored by Douwe Maan's avatar Douwe Maan

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

Allow commenting on individual commits inside an MR

Closes #31847

See merge request gitlab-org/gitlab-ce!12148
parents ff6a3ae7 daf9357a
......@@ -16,7 +16,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 || {};
......
......@@ -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.'));
......
......@@ -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
......
......@@ -28,7 +28,6 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
:task_num,
:title,
:discussion_locked,
label_ids: []
]
end
......
......@@ -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
......
......@@ -7,11 +7,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
include IssuableCollections
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
......
......@@ -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
......@@ -101,6 +101,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)
{}
end
......
# 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)
......
......@@ -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
......
......@@ -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
......
......@@ -17,6 +17,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?
......@@ -135,6 +136,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)
......
......@@ -11,6 +11,7 @@ class Discussion
:author,
:noteable,
:commit_id,
:for_commit?,
:for_merge_request?,
......
......@@ -649,6 +649,7 @@ class MergeRequest < ActiveRecord::Base
.to_sql
Note.from("(#{union}) #{Note.table_name}")
.includes(:noteable)
end
alias_method :discussion_notes, :related_notes
......@@ -925,21 +926,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
......
......@@ -230,16 +230,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
......@@ -401,6 +403,10 @@ class Note < ActiveRecord::Base
noteable_object&.touch
end
def banzai_render_context(field)
super.merge(noteable: noteable)
end
private
def keep_around_commit
......
......@@ -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
......
......@@ -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
......
......@@ -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.
......@@ -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"
......
- ref = local_assigns.fetch(:ref)
- 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)
- 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 }
- 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
= cache(cache_key, expires_in: 1.day) do
%li.commit.flex-row.js-toggle-container{ id: "commit-#{commit.short_id}" }
......@@ -11,7 +22,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
&middot;
= commit.short_id
......@@ -31,8 +42,7 @@
- commit_text = _('%{commit_author_link} committed %{commit_timeago}') % { commit_author_link: commit_author_link, commit_timeago: commit_timeago }
#{ commit_text.html_safe }
.commit-actions.hidden-xs
.commit-actions.flex-row.hidden-xs
- if request.xhr?
= render partial: 'projects/commit/signature', object: commit.signature
- else
......@@ -41,6 +51,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"
- 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
......
......@@ -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
- 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
- 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
- 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?
= 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'
%p
Nothing to merge from
%strong= @merge_request.source_branch
into
%strong= @merge_request.target_branch
= 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
- 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
- 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'
......@@ -8,7 +8,7 @@
= webpack_bundle_tag('common_vue')
= webpack_bundle_tag('diff_notes')
.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 } }
......@@ -38,21 +38,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 }
......
---
title: Make diff notes created on a commit in a merge request to persist a rebase.
merge_request: 12148
author:
type: added
......@@ -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
......@@ -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