From 360b94ceba146935a40b02f39ed3d833eaea134a Mon Sep 17 00:00:00 2001
From: "micael.bergeron" <micaelbergeron@gmail.com>
Date: Fri, 1 Dec 2017 14:08:30 -0500
Subject: [PATCH] adding view and feature specs
---
app/helpers/merge_requests_helper.rb | 32 +++---
app/models/merge_request.rb | 9 +-
.../_not_all_comments_displayed.html.haml | 2 +-
.../projects/merge_requests/show.html.haml | 8 +-
lib/banzai/object_renderer.rb | 1 +
.../projects/commit_controller_spec.rb | 3 +-
.../merge_requests/diffs_controller_spec.rb | 3 +-
spec/factories/notes.rb | 10 +-
spec/features/merge_requests/versions_spec.rb | 105 +++++++++++-------
spec/helpers/merge_requests_helper_spec.rb | 17 +++
.../filter/commit_reference_filter_spec.rb | 12 ++
spec/models/diff_note_spec.rb | 29 ++---
.../projects/commit/show.html.haml_spec.rb | 22 +++-
.../merge_requests/_commits.html.haml_spec.rb | 4 +-
.../diffs/_diffs.html.haml_spec.rb | 36 ++++++
15 files changed, 201 insertions(+), 92 deletions(-)
create mode 100644 spec/views/projects/merge_requests/diffs/_diffs.html.haml_spec.rb
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index 004aaeb2c56..ce57422f45d 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -101,28 +101,28 @@ module MergeRequestsHelper
}.merge(merge_params_ee(merge_request))
end
- def tab_link_for(tab, options={}, &block)
+ def tab_link_for(merge_request, tab, options = {}, &block)
data_attrs = {
action: tab.to_s,
- target: "##{tab.to_s}",
+ target: "##{tab}",
toggle: options.fetch(:force_link, false) ? '' : 'tab'
}
url = case tab
- when :show
- data_attrs.merge!(target: '#notes')
- project_merge_request_path(@project, @merge_request)
- when :commits
- commits_project_merge_request_path(@project, @merge_request)
- when :pipelines
- pipelines_project_merge_request_path(@project, @merge_request)
- when :diffs
- diffs_project_merge_request_path(@project, @merge_request)
- else
- raise "Cannot create tab #{tab}."
- end
-
- link_to(url, data: data_attrs, &block)
+ 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)
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 2dad036639a..422f138c4ea 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -927,10 +927,12 @@ class MergeRequest < ActiveRecord::Base
end
def all_commits
- diffs_relation = merge_request_diffs
-
# 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)
@@ -942,6 +944,7 @@ class MergeRequest < ActiveRecord::Base
def all_commit_shas
@all_commit_shas ||= begin
return commit_shas unless persisted?
+
all_commits.pluck(:sha).uniq
end
end
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
index faabba5fc35..529fbb8547a 100644
--- 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
@@ -12,6 +12,6 @@
- else
viewing an old version of the diff
.pull-right
- = link_to diffs_project_merge_request_path(@project, @merge_request), class: 'btn btn-sm' do
+ = 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/show.html.haml b/app/views/projects/merge_requests/show.html.haml
index 0d7abe8137f..abff702fd9d 100644
--- a/app/views/projects/merge_requests/show.html.haml
+++ b/app/views/projects/merge_requests/show.html.haml
@@ -38,21 +38,21 @@
.nav-links.scrolling-tabs
%ul.merge-request-tabs
%li.notes-tab
- = tab_link_for :show, force_link: @commit.present? 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
- = tab_link_for :commits do
+ = tab_link_for @merge_request, :commits do
Commits
%span.badge= @commits_count
- if @pipelines.any?
%li.pipelines-tab
- = tab_link_for :pipelines do
+ = tab_link_for @merge_request, :pipelines do
Pipelines
%span.badge.js-pipelines-mr-count= @pipelines.size
%li.diffs-tab
- = tab_link_for :diffs 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/lib/banzai/object_renderer.rb b/lib/banzai/object_renderer.rb
index 0bf9a8d66bc..2691be81623 100644
--- a/lib/banzai/object_renderer.rb
+++ b/lib/banzai/object_renderer.rb
@@ -86,6 +86,7 @@ module Banzai
def save_options
return {} unless @redaction_context[:xhtml]
+
{ save_with: Nokogiri::XML::Node::SaveOptions::AS_XHTML }
end
end
diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb
index fa8533bc3d9..694c64ae1ad 100644
--- a/spec/controllers/projects/commit_controller_spec.rb
+++ b/spec/controllers/projects/commit_controller_spec.rb
@@ -140,7 +140,8 @@ describe Projects::CommitController do
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',
+ expect(assigns(:new_diff_note_attrs)).to eq({
+ noteable_type: 'MergeRequest',
noteable_id: merge_request.id,
commit_id: commit.id
})
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/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/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
--
2.30.9