summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2017-12-07 16:49:04 +0000
committerDouwe Maan <douwe@gitlab.com>2017-12-07 16:49:04 +0000
commit9dffd0ab6b2e9f5b0db55230d8991f50a01f7669 (patch)
tree52e1870616006021d7cd33a53cf20eb28b1251e7
parentff6a3ae705bead7cc345cab3530cb82a0c74b043 (diff)
parentdaf9357aa92283e6cdd0d1e0cade65c8e2294540 (diff)
downloadgitlab-ce-9dffd0ab6b2e9f5b0db55230d8991f50a01f7669.tar.gz
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
-rw-r--r--app/assets/javascripts/diff_notes/diff_notes_bundle.js3
-rw-r--r--app/assets/javascripts/diff_notes/services/resolve.js2
-rw-r--r--app/controllers/projects/commit_controller.rb17
-rw-r--r--app/controllers/projects/merge_requests/application_controller.rb1
-rw-r--r--app/controllers/projects/merge_requests/diffs_controller.rb42
-rw-r--r--app/controllers/projects/merge_requests_controller.rb3
-rw-r--r--app/helpers/commits_helper.rb8
-rw-r--r--app/helpers/merge_requests_helper.rb24
-rw-r--r--app/models/commit.rb3
-rw-r--r--app/models/concerns/discussion_on_diff.rb4
-rw-r--r--app/models/diff_discussion.rb6
-rw-r--r--app/models/diff_note.rb7
-rw-r--r--app/models/discussion.rb1
-rw-r--r--app/models/merge_request.rb27
-rw-r--r--app/models/note.rb20
-rw-r--r--app/services/merge_requests/refresh_service.rb2
-rw-r--r--app/views/discussions/_discussion.html.haml14
-rw-r--r--app/views/projects/commit/_commit_box.html.haml12
-rw-r--r--app/views/projects/commit/show.html.haml3
-rw-r--r--app/views/projects/commits/_commit.html.haml29
-rw-r--r--app/views/projects/commits/_commits.html.haml7
-rw-r--r--app/views/projects/merge_requests/_commits.html.haml2
-rw-r--r--app/views/projects/merge_requests/diffs/_commit_widget.html.haml5
-rw-r--r--app/views/projects/merge_requests/diffs/_different_base.html.haml11
-rw-r--r--app/views/projects/merge_requests/diffs/_diffs.html.haml25
-rw-r--r--app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml17
-rw-r--r--app/views/projects/merge_requests/diffs/_version_controls.html.haml (renamed from app/views/projects/merge_requests/diffs/_versions.html.haml)26
-rw-r--r--app/views/projects/merge_requests/show.html.haml10
-rw-r--r--changelogs/unreleased/dm-commit-diff-discussions-in-mr-context.yml5
-rw-r--r--lib/banzai/filter/commit_reference_filter.rb34
-rw-r--r--lib/banzai/object_renderer.rb12
-rw-r--r--lib/gitlab/diff/diff_refs.rb22
-rw-r--r--lib/gitlab/git.rb12
-rw-r--r--lib/gitlab/git/commit.rb1
-rw-r--r--spec/controllers/projects/commit_controller_spec.rb18
-rw-r--r--spec/controllers/projects/merge_requests/diffs_controller_spec.rb3
-rw-r--r--spec/factories/notes.rb10
-rw-r--r--spec/features/merge_requests/versions_spec.rb105
-rw-r--r--spec/helpers/merge_requests_helper_spec.rb17
-rw-r--r--spec/lib/banzai/filter/commit_reference_filter_spec.rb12
-rw-r--r--spec/lib/gitlab/git_spec.rb25
-rw-r--r--spec/models/diff_note_spec.rb29
-rw-r--r--spec/models/merge_request_spec.rb2
-rw-r--r--spec/services/system_note_service_spec.rb4
-rw-r--r--spec/views/projects/commit/show.html.haml_spec.rb22
-rw-r--r--spec/views/projects/merge_requests/_commits.html.haml_spec.rb4
-rw-r--r--spec/views/projects/merge_requests/diffs/_diffs.html.haml_spec.rb36
47 files changed, 511 insertions, 193 deletions
diff --git a/app/assets/javascripts/diff_notes/diff_notes_bundle.js b/app/assets/javascripts/diff_notes/diff_notes_bundle.js
index 0863c3406bd..e0422057090 100644
--- a/app/assets/javascripts/diff_notes/diff_notes_bundle.js
+++ b/app/assets/javascripts/diff_notes/diff_notes_bundle.js
@@ -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 || {};
diff --git a/app/assets/javascripts/diff_notes/services/resolve.js b/app/assets/javascripts/diff_notes/services/resolve.js
index 6eae54f830b..96fe23640af 100644
--- a/app/assets/javascripts/diff_notes/services/resolve.js
+++ b/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.'));
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index 6ff96a3f295..2e7344b1cad 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/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
diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb
index 1269759fc2b..793ae03fb88 100644
--- a/app/controllers/projects/merge_requests/application_controller.rb
+++ b/app/controllers/projects/merge_requests/application_controller.rb
@@ -28,7 +28,6 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
:task_num,
:title,
:discussion_locked,
-
label_ids: []
]
end
diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb
index 9f966889995..fe8525a488c 100644
--- a/app/controllers/projects/merge_requests/diffs_controller.rb
+++ b/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
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 37acd1c9787..e7b3b73024b 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -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
diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb
index f68e2cd3afa..2d304f7eb91 100644
--- a/app/helpers/commits_helper.rb
+++ b/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
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index 5b2c58d193d..ce57422f45d 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -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
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 6b28d290f99..307e4fcedfe 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -1,3 +1,4 @@
+# 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)
diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb
index f5cbb3becad..4b4d519f3df 100644
--- a/app/models/concerns/discussion_on_diff.rb
+++ b/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
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 6eba87da1a1..4a65738214b 100644
--- a/app/models/diff_discussion.rb
+++ b/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
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index ae5f138a920..b53d44cda95 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -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)
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index 437df923d2d..92482a1a875 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -11,6 +11,7 @@ class Discussion
:author,
:noteable,
+ :commit_id,
:for_commit?,
:for_merge_request?,
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 949d42f865c..422f138c4ea 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -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
diff --git a/app/models/note.rb b/app/models/note.rb
index 733bbbc013f..c4c2ab8e67d 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -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
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index 434dda89db0..9f05535d4d4 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/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
diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml
index 0f03163a2e8..205320ed87c 100644
--- a/app/views/discussions/_discussion.html.haml
+++ b/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
diff --git a/app/views/projects/commit/_commit_box.html.haml b/app/views/projects/commit/_commit_box.html.haml
index 5f607c2ab25..09934c09865 100644
--- a/app/views/projects/commit/_commit_box.html.haml
+++ b/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.
diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml
index abb292f8f27..2890e9d2b65 100644
--- a/app/views/projects/commit/show.html.haml
+++ b/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"
diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml
index 1b91a94a9f8..618a6355d23 100644
--- a/app/views/projects/commits/_commit.html.haml
+++ b/app/views/projects/commits/_commit.html.haml
@@ -1,7 +1,18 @@
-- 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"
diff --git a/app/views/projects/commits/_commits.html.haml b/app/views/projects/commits/_commits.html.haml
index d14897428d0..ac6852751be 100644
--- a/app/views/projects/commits/_commits.html.haml
+++ b/app/views/projects/commits/_commits.html.haml
@@ -1,4 +1,7 @@
-- 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
diff --git a/app/views/projects/merge_requests/_commits.html.haml b/app/views/projects/merge_requests/_commits.html.haml
index 11793919ff7..b414518b597 100644
--- a/app/views/projects/merge_requests/_commits.html.haml
+++ b/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
diff --git a/app/views/projects/merge_requests/diffs/_commit_widget.html.haml b/app/views/projects/merge_requests/diffs/_commit_widget.html.haml
new file mode 100644
index 00000000000..2e5594f8cbe
--- /dev/null
+++ b/app/views/projects/merge_requests/diffs/_commit_widget.html.haml
@@ -0,0 +1,5 @@
+- 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
diff --git a/app/views/projects/merge_requests/diffs/_different_base.html.haml b/app/views/projects/merge_requests/diffs/_different_base.html.haml
new file mode 100644
index 00000000000..0e57066f9c9
--- /dev/null
+++ b/app/views/projects/merge_requests/diffs/_different_base.html.haml
@@ -0,0 +1,11 @@
+- 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
diff --git a/app/views/projects/merge_requests/diffs/_diffs.html.haml b/app/views/projects/merge_requests/diffs/_diffs.html.haml
index 3d7a8f9d870..60c91024b23 100644
--- a/app/views/projects/merge_requests/diffs/_diffs.html.haml
+++ b/app/views/projects/merge_requests/diffs/_diffs.html.haml
@@ -1,13 +1,18 @@
-- 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
diff --git a/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml b/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml
new file mode 100644
index 00000000000..529fbb8547a
--- /dev/null
+++ b/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml
@@ -0,0 +1,17 @@
+- 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
diff --git a/app/views/projects/merge_requests/diffs/_versions.html.haml b/app/views/projects/merge_requests/diffs/_version_controls.html.haml
index 9f7152b9824..1c26f0405d2 100644
--- a/app/views/projects/merge_requests/diffs/_versions.html.haml
+++ b/app/views/projects/merge_requests/diffs/_version_controls.html.haml
@@ -1,4 +1,4 @@
-- 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'
diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml
index d88e3d794d3..abff702fd9d 100644
--- a/app/views/projects/merge_requests/show.html.haml
+++ b/app/views/projects/merge_requests/show.html.haml
@@ -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 }
diff --git a/changelogs/unreleased/dm-commit-diff-discussions-in-mr-context.yml b/changelogs/unreleased/dm-commit-diff-discussions-in-mr-context.yml
new file mode 100644
index 00000000000..1f8b42ea21f
--- /dev/null
+++ b/changelogs/unreleased/dm-commit-diff-discussions-in-mr-context.yml
@@ -0,0 +1,5 @@
+---
+title: Make diff notes created on a commit in a merge request to persist a rebase.
+merge_request: 12148
+author:
+type: added
diff --git a/lib/banzai/filter/commit_reference_filter.rb b/lib/banzai/filter/commit_reference_filter.rb
index 714e0319025..eedb95197aa 100644
--- a/lib/banzai/filter/commit_reference_filter.rb
+++ b/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
diff --git a/lib/banzai/object_renderer.rb b/lib/banzai/object_renderer.rb
index ecb3affbba5..2691be81623 100644
--- a/lib/banzai/object_renderer.rb
+++ b/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
diff --git a/lib/gitlab/diff/diff_refs.rb b/lib/gitlab/diff/diff_refs.rb
index c98eefbce25..88e0db830f6 100644
--- a/lib/gitlab/diff/diff_refs.rb
+++ b/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
diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb
index 1f31cdbc96d..1f7c35cafaa 100644
--- a/lib/gitlab/git.rb
+++ b/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
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index 8900e2d7afe..e90b158fb34 100644
--- a/lib/gitlab/git/commit.rb
+++ b/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,
diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb
index fd90c0d8bad..694c64ae1ad 100644
--- a/spec/controllers/projects/commit_controller_spec.rb
+++ b/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
diff --git a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb b/spec/controllers/projects/merge_requests/diffs_controller_spec.rb
index 18a70bec103..ba97ccfbbd4 100644
--- a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb
+++ b/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
diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index ab4ae123429..471bfb3213a 100644
--- a/spec/factories/notes.rb
+++ b/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
diff --git a/spec/features/merge_requests/versions_spec.rb b/spec/features/merge_requests/versions_spec.rb
index 29f95039af8..482f2e51c8b 100644
--- a/spec/features/merge_requests/versions_spec.rb
+++ b/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
diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb
index fd7900c32f4..3008528e60c 100644
--- a/spec/helpers/merge_requests_helper_spec.rb
+++ b/spec/helpers/merge_requests_helper_spec.rb
@@ -1,7 +1,9 @@
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 }
@@ -41,4 +43,19 @@ describe MergeRequestsHelper do
it { is_expected.to eq([source_title, target_title]) }
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
diff --git a/spec/lib/banzai/filter/commit_reference_filter_spec.rb b/spec/lib/banzai/filter/commit_reference_filter_spec.rb
index 702fcac0c6f..080a5f57da9 100644
--- a/spec/lib/banzai/filter/commit_reference_filter_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/git_spec.rb b/spec/lib/gitlab/git_spec.rb
index 494dfe0e595..ce15057dd7d 100644
--- a/spec/lib/gitlab/git_spec.rb
+++ b/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
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index 8389d5c5430..4d0b3245a13 100644
--- a/spec/models/diff_note_spec.rb
+++ b/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
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 71fbb82184c..30a5a3bbff7 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -967,7 +967,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
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index a918383ecd2..47412110b4b 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -692,9 +692,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
diff --git a/spec/views/projects/commit/show.html.haml_spec.rb b/spec/views/projects/commit/show.html.haml_spec.rb
index 32c95c6bb0d..a9c32122600 100644
--- a/spec/views/projects/commit/show.html.haml_spec.rb
+++ b/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
diff --git a/spec/views/projects/merge_requests/_commits.html.haml_spec.rb b/spec/views/projects/merge_requests/_commits.html.haml_spec.rb
index efed2e02a1b..3ca67114558 100644
--- a/spec/views/projects/merge_requests/_commits.html.haml_spec.rb
+++ b/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
diff --git a/spec/views/projects/merge_requests/diffs/_diffs.html.haml_spec.rb b/spec/views/projects/merge_requests/diffs/_diffs.html.haml_spec.rb
new file mode 100644
index 00000000000..e7c40421f1f
--- /dev/null
+++ b/spec/views/projects/merge_requests/diffs/_diffs.html.haml_spec.rb
@@ -0,0 +1,36 @@
+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