Commit e4eba908 authored by Douwe Maan's avatar Douwe Maan Committed by micael.bergeron

Allow commenting on individual commits inside an MR

parent 17542a78
...@@ -16,7 +16,8 @@ import './components/diff_note_avatars'; ...@@ -16,7 +16,8 @@ import './components/diff_note_avatars';
import './components/new_issue_for_discussion'; 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'; const COMPONENT_SELECTOR = 'resolve-btn, resolve-discussion-btn, jump-to-discussion, comment-and-resolve-btn, new-issue-for-discussion-btn';
window.gl = window.gl || {}; window.gl = window.gl || {};
......
...@@ -43,7 +43,7 @@ class ResolveServiceClass { ...@@ -43,7 +43,7 @@ class ResolveServiceClass {
discussion.resolveAllNotes(resolvedBy); discussion.resolveAllNotes(resolvedBy);
} }
gl.mrWidget.checkStatus(); if (gl.mrWidget) gl.mrWidget.checkStatus();
discussion.updateHeadline(data); discussion.updateHeadline(data);
}) })
.catch(() => new Flash('An error occurred when trying to resolve a discussion. Please try again.')); .catch(() => new Flash('An error occurred when trying to resolve a discussion. Please try again.'));
......
...@@ -134,6 +134,23 @@ def define_note_vars ...@@ -134,6 +134,23 @@ def define_note_vars
@grouped_diff_discussions = commit.grouped_diff_discussions @grouped_diff_discussions = commit.grouped_diff_discussions
@discussions = commit.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 = (@grouped_diff_discussions.values.flatten + @discussions).flat_map(&:notes)
@notes = prepare_notes_for_rendering(@notes, @commit) @notes = prepare_notes_for_rendering(@notes, @commit)
end end
......
...@@ -20,18 +20,33 @@ def diff_for_path ...@@ -20,18 +20,33 @@ def diff_for_path
private private
def define_diff_vars def define_diff_vars
@merge_request_diffs = @merge_request.merge_request_diffs.viewable.select_without_diff.order_id_desc
if commit_id = params[:commit_id].presence
@commit = @merge_request.target_project.commit(commit_id)
@compare = @commit
else
@compare = find_merge_request_diff_compare
end
return render_404 unless @compare
@diffs = @compare.diffs(diff_options)
end
def find_merge_request_diff_compare
@merge_request_diff = @merge_request_diff =
if params[:diff_id] if diff_id = params[:diff_id].presence
@merge_request.merge_request_diffs.viewable.find(params[:diff_id]) @merge_request.merge_request_diffs.viewable.find_by(id: diff_id)
else else
@merge_request.merge_request_diff @merge_request.merge_request_diff
end 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 } @comparable_diffs = @merge_request_diffs.select { |diff| diff.id < @merge_request_diff.id }
if params[:start_sha].present? if @start_sha = params[:start_sha].presence
@start_sha = params[:start_sha]
@start_version = @comparable_diffs.find { |diff| diff.head_commit_sha == @start_sha } @start_version = @comparable_diffs.find { |diff| diff.head_commit_sha == @start_sha }
unless @start_version unless @start_version
...@@ -40,20 +55,18 @@ def define_diff_vars ...@@ -40,20 +55,18 @@ def define_diff_vars
end end
end end
@compare = if @start_sha
if @start_sha @merge_request_diff.compare_with(@start_sha)
@merge_request_diff.compare_with(@start_sha) else
else @merge_request_diff
@merge_request_diff end
end
@diffs = @compare.diffs(diff_options)
end end
def define_diff_comment_vars def define_diff_comment_vars
@new_diff_note_attrs = { @new_diff_note_attrs = {
noteable_type: 'MergeRequest', noteable_type: 'MergeRequest',
noteable_id: @merge_request.id noteable_id: @merge_request.id,
commit_id: @commit&.id
} }
@diff_notes_disabled = false @diff_notes_disabled = false
......
...@@ -228,4 +228,12 @@ def limited_commits(commits) ...@@ -228,4 +228,12 @@ def limited_commits(commits)
[commits, 0] [commits, 0]
end end
end end
def commit_path(project, commit, merge_request: nil)
if merge_request&.persisted?
diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, commit_id: commit.id)
else
namespace_project_commit_path(project.namespace, project, commit.id)
end
end
end end
...@@ -32,6 +32,10 @@ def file_new_path ...@@ -32,6 +32,10 @@ def file_new_path
first_note.position.new_path first_note.position.new_path
end 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 # Returns an array of at most 16 highlighted lines above a diff note
def truncated_diff_lines(highlight: true) def truncated_diff_lines(highlight: true)
lines = highlight ? highlighted_diff_lines : diff_lines lines = highlight ? highlighted_diff_lines : diff_lines
......
...@@ -24,7 +24,11 @@ def merge_request_version_params ...@@ -24,7 +24,11 @@ def merge_request_version_params
return unless for_merge_request? return unless for_merge_request?
return {} if active? 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 end
def reply_attributes def reply_attributes
......
...@@ -17,6 +17,7 @@ class DiffNote < Note ...@@ -17,6 +17,7 @@ class DiffNote < Note
validates :noteable_type, inclusion: { in: NOTEABLE_TYPES } validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
validate :positions_complete validate :positions_complete
validate :verify_supported validate :verify_supported
validate :diff_refs_match_commit, if: :for_commit?
before_validation :set_original_position, on: :create before_validation :set_original_position, on: :create
before_validation :update_position, on: :create, if: :on_text? before_validation :update_position, on: :create, if: :on_text?
...@@ -135,6 +136,12 @@ def positions_complete ...@@ -135,6 +136,12 @@ def positions_complete
errors.add(:position, "is invalid") errors.add(:position, "is invalid")
end 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 def keep_around_commits
project.repository.keep_around(self.original_position.base_sha) project.repository.keep_around(self.original_position.base_sha)
project.repository.keep_around(self.original_position.start_sha) project.repository.keep_around(self.original_position.start_sha)
......
...@@ -11,6 +11,7 @@ class Discussion ...@@ -11,6 +11,7 @@ class Discussion
:author, :author,
:noteable, :noteable,
:commit_id,
:for_commit?, :for_commit?,
:for_merge_request?, :for_merge_request?,
......
...@@ -230,10 +230,14 @@ def skip_project_check? ...@@ -230,10 +230,14 @@ def skip_project_check?
for_personal_snippet? for_personal_snippet?
end end
def commit
project.commit(commit_id) if commit_id.present?
end
# override to return commits, which are not active record # override to return commits, which are not active record
def noteable def noteable
if for_commit? if for_commit?
@commit ||= project.commit(commit_id) @commit ||= commit
else else
super super
end end
......
...@@ -23,7 +23,7 @@ def add_commits(noteable, project, author, new_commits, existing_commits = [], o ...@@ -23,7 +23,7 @@ def add_commits(noteable, project, author, new_commits, existing_commits = [], o
body = "added #{commits_text}\n\n" body = "added #{commits_text}\n\n"
body << existing_commit_summary(noteable, existing_commits, oldrev) body << existing_commit_summary(noteable, existing_commits, oldrev)
body << new_commit_summary(new_commits).join("\n") body << new_commit_summary(noteable, new_commits).join("\n")
body << "\n\n[Compare with previous version](#{diff_comparison_url(noteable, project, oldrev)})" body << "\n\n[Compare with previous version](#{diff_comparison_url(noteable, project, oldrev)})"
create_note(NoteSummary.new(noteable, project, author, body, action: 'commit', commit_count: total_count)) create_note(NoteSummary.new(noteable, project, author, body, action: 'commit', commit_count: total_count))
...@@ -486,9 +486,9 @@ def cross_reference_exists?(noteable, mentioner) ...@@ -486,9 +486,9 @@ def cross_reference_exists?(noteable, mentioner)
# new_commits - Array of new Commit objects # new_commits - Array of new Commit objects
# #
# Returns an Array of Strings # Returns an Array of Strings
def new_commit_summary(new_commits) def new_commit_summary(merge_request, new_commits)
new_commits.collect do |commit| new_commits.collect do |commit|
"* #{commit.short_id} - #{escape_html(commit.title)}" "* [#{commit.short_id}](#{merge_request_commit_url(merge_request, commit)}) - #{escape_html(commit.title)}"
end end
end end
...@@ -668,4 +668,13 @@ def diff_comparison_url(merge_request, project, oldrev) ...@@ -668,4 +668,13 @@ def diff_comparison_url(merge_request, project, oldrev)
start_sha: oldrev start_sha: oldrev
) )
end end
def merge_request_commit_url(merge_request, commit)
url_helpers.diffs_namespace_project_merge_request_url(
project.namespace,
project,
merge_request.iid,
commit_id: commit.id
)
end
end end
...@@ -32,9 +32,17 @@ ...@@ -32,9 +32,17 @@
- elsif discussion.diff_discussion? - elsif discussion.diff_discussion?
on on
= conditional_link_to url.present?, url do = conditional_link_to url.present?, url do
- unless discussion.active? - if discussion.on_merge_request_commit?
an old version of - unless discussion.active?
the diff 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") = time_ago_with_tooltip(discussion.created_at, placement: "bottom", html_class: "note-created-ago")
= render "discussions/headline", discussion: discussion = render "discussions/headline", discussion: discussion
......
...@@ -47,7 +47,7 @@ ...@@ -47,7 +47,7 @@
%li= link_to s_("DownloadCommit|Email Patches"), project_commit_path(@project, @commit, format: :patch) %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) %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 %h3.commit-title
= markdown(@commit.title, pipeline: :single_line, author: @commit.author) = markdown(@commit.title, pipeline: :single_line, author: @commit.author)
- if @commit.description.present? - if @commit.description.present?
...@@ -80,3 +80,13 @@ ...@@ -80,3 +80,13 @@
- if last_pipeline.duration - if last_pipeline.duration
in in
= time_interval_in_words last_pipeline.duration = 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, namespace_project_merge_request_path(@project.namespace, @project, @merge_request)
Comments created here will be created in the context of that merge request.
...@@ -6,6 +6,9 @@ ...@@ -6,6 +6,9 @@
- @content_class = limited_container_width - @content_class = limited_container_width
- page_title "#{@commit.title} (#{@commit.short_id})", "Commits" - page_title "#{@commit.title} (#{@commit.short_id})", "Commits"
- page_description @commit.description - 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] } .container-fluid{ class: [limited_container_width, container_class] }
= render "commit_box" = render "commit_box"
......
- ref = local_assigns.fetch(:ref) - view_details = local_assigns.fetch(:view_details, false)
- merge_request = local_assigns.fetch(:merge_request, nil)
- cache_key = [project.full_path, commit.id, current_application_settings, @path.presence, current_controller?(:commits), I18n.locale] - 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)
- if @note_counts
- note_count = @note_counts.fetch(commit.id, 0)
- else
- notes = commit.notes
- note_count = notes.user.count
- cache_key = [project.full_path, commit.id, current_application_settings, note_count, @path.presence, current_controller?(:commits), merge_request, I18n.locale]
- cache_key.push(commit.status(ref)) if commit.status(ref) - cache_key.push(commit.status(ref)) if commit.status(ref)
= cache(cache_key, expires_in: 1.day) do = cache(cache_key, expires_in: 1.day) do
...@@ -11,7 +21,7 @@ ...@@ -11,7 +21,7 @@
.commit-detail .commit-detail
.commit-content .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 %span.commit-row-message.visible-xs-inline
&middot; &middot;
= commit.short_id = commit.short_id
...@@ -31,8 +41,7 @@ ...@@ -31,8 +41,7 @@
- commit_text = _('%{commit_author_link} committed %{commit_timeago}') % { commit_author_link: commit_author_link, commit_timeago: commit_timeago } - commit_text = _('%{commit_author_link} committed %{commit_timeago}') % { commit_author_link: commit_author_link, commit_timeago: commit_timeago }
#{ commit_text.html_safe } #{ commit_text.html_safe }
.commit-actions.flex-row.hidden-xs
.commit-actions.hidden-xs
- if request.xhr? - if request.xhr?
= render partial: 'projects/commit/signature', object: commit.signature = render partial: 'projects/commit/signature', object: commit.signature
- else - else
...@@ -41,6 +50,9 @@ ...@@ -41,6 +50,9 @@
- if commit.status(ref) - if commit.status(ref)
= render_commit_status(commit, ref: 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")) = clipboard_button(text: commit.id, title: _("Copy commit SHA to clipboard"))
= link_to_browse_code(project, commit) = link_to_browse_code(project, commit)
- if view_details && merge_request
= link_to "View details", namespace_project_commit_path(project.namespace, 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, hidden = limited_commits(@commits)
- commits.chunk { |c| c.committed_date.in_time_zone.to_date }.each do |day, commits| - commits.chunk { |c| c.committed_date.in_time_zone.to_date }.each do |day, commits|
...@@ -8,7 +11,7 @@ ...@@ -8,7 +11,7 @@
%li.commits-row{ data: { day: day } } %li.commits-row{ data: { day: day } }
%ul.content-list.commit-list.flex-list %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 - if hidden > 0
%li.alert.alert-warning %li.alert.alert-warning
......
...@@ -5,4 +5,4 @@ ...@@ -5,4 +5,4 @@
= custom_icon ('illustration_no_commits') = custom_icon ('illustration_no_commits')
- else - else
%ol#commits-list.list-unstyled %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 namespace_project_compare_path(@project.namespace, @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 @merge_request_diff.collected? || @merge_request_diff.overflow? = render 'projects/merge_requests/diffs/version_controls'
= render 'projects/merge_requests/diffs/versions' = render 'projects/merge_requests/diffs/different_base'
= render "projects/diffs/diffs", diffs: @diffs, environment: @environment, merge_request: true = render 'projects/merge_requests/diffs/not_all_comments_displayed'
- elsif @merge_request_diff.empty? = render 'projects/merge_requests/diffs/commit_widget'
- if @merge_request_diff&.empty?
.nothing-here-block .nothing-here-block
= image_tag 'illustrations/merge_request_changes_empty.svg' = image_tag 'illustrations/merge_request_changes_empty.svg'
%p %p
...@@ -9,5 +11,8 @@ ...@@ -9,5 +11,8 @@
%strong= @merge_request.source_branch %strong= @merge_request.source_branch
into into
%strong= @merge_request.target_branch %strong= @merge_request.target_branch
%p= link_to 'Create commit', project_new_blob_path(@project, @merge_request.source_branch), class: 'btn btn-save' %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
= icon('info-circle')
Not all comments are displayed because you're
= succeed '.' do
- if @commit
viewing only the changes in commit
= link_to @commit.short_id, diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, commit_id: @commit.id), class: "commit-sha"
- elsif @start_version
comparing two versions of the diff
- else
viewing an old version of the diff
.pull-right
= link_to diffs_namespace_project_merge_request_path(@project.namespace, @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-controls
.mr-version-menus-container.content-block .mr-version-menus-container.content-block
Changes between Changes between
...@@ -71,27 +71,3 @@ ...@@ -71,27 +71,3 @@
(base) (base)
%div %div
%strong.commit-sha= short_sha(@merge_request_diff.base_commit_sha) %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
=<