adding view and feature specs

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)
+            data_attrs[:target] = '#notes'
+            method(:project_merge_request_path)
           when :commits
-      commits_project_merge_request_path(@project, @merge_request)
+            method(:commits_project_merge_request_path)
           when :pipelines
-      pipelines_project_merge_request_path(@project, @merge_request)
+            method(:pipelines_project_merge_request_path)
           when :diffs
-      diffs_project_merge_request_path(@project, @merge_request)
+            method(:diffs_project_merge_request_path)
           else
             raise "Cannot create tab #{tab}."
           end
 
-    link_to(url, data: data_attrs, &block)
+    link_to(url[merge_request.project, merge_request], data: data_attrs, &block)
   end
 
   def merge_params_ee(merge_request)

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

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

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 }

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

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
                                                     })

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

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

spec/factories/notes.rb

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

spec/features/merge_requests/versions_spec.rb

@@ -6,12 +6,35 @@ 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
 
+  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
+
+  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'
@@ -20,6 +43,12 @@ feature 'Merge Request versions', :js do
       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
     before do
       page.within '.mr-version-dropdown' do
@@ -62,24 +91,10 @@ feature 'Merge Request versions', :js do
       expect(page).to have_css(".diffs .notes[data-discussion-id='#{outdated_diff_note.discussion_id}']")
     end
 
-    it 'allows commenting' do
-      diff_file_selector = ".diff-file[id='7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44']"
-      line_code = '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44_2_2'
-
-      page.within(diff_file_selector) do
-        find(".line_holder[id='#{line_code}'] td:nth-of-type(1)").hover
-        find(".line_holder[id='#{line_code}'] button").click
-
-        page.within("form[data-line-code='#{line_code}']") do
-          fill_in "note[note]", with: "Typo, please fix"
-          find(".js-comment-button").click
-        end
-
-        wait_for_requests
-
-        expect(page).to have_content("Typo, please fix")
-      end
-    end
+    it_behaves_like 'allows commenting',
+                    file_id: '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44',
+                    line_code: '2_2',
+                    comment: 'Typo, please fix.'
   end
 
   describe 'compare with older version' do
@@ -132,25 +147,6 @@ feature 'Merge Request versions', :js do
       expect(page).to have_css(".diffs .notes[data-discussion-id='#{outdated_diff_note.discussion_id}']")
     end
 
-    it 'allows commenting' do
-      diff_file_selector = ".diff-file[id='7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44']"
-      line_code = '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44_4_4'
-
-      page.within(diff_file_selector) do
-        find(".line_holder[id='#{line_code}'] td:nth-of-type(1)").hover
-        find(".line_holder[id='#{line_code}'] button").click
-
-        page.within("form[data-line-code='#{line_code}']") do
-          fill_in "note[note]", with: "Typo, please fix"
-          find(".js-comment-button").click
-        end
-
-        wait_for_requests
-
-        expect(page).to have_content("Typo, please fix")
-      end
-    end
-
     it 'show diff between new and old version' do
       expect(page).to have_content '4 changed files with 15 additions and 6 deletions'
     end
@@ -162,6 +158,11 @@ feature 'Merge Request versions', :js do
       end
       expect(page).to have_content '8 changed files'
     end
+
+    it_behaves_like 'allows commenting',
+                    file_id: '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44',
+                    line_code: '4_4',
+                    comment: 'Typo, please fix.'
   end
 
   describe 'compare with same version' do
@@ -210,4 +211,24 @@ feature 'Merge Request versions', :js do
       expect(page).to have_content '0 changed files'
     end
   end
+
+  describe 'scoped in a commit' do
+    let(:params) { { commit_id: '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' } }
+
+    before do
+      wait_for_requests
+    end
+
+    it 'should only show diffs from the commit' do
+      diff_commit_ids = find_all('.diff-file [data-commit-id]').map {|diff| diff['data-commit-id']}
+
+      expect(diff_commit_ids).not_to be_empty
+      expect(diff_commit_ids).to all(eq(params[:commit_id]))
+    end
+
+    it_behaves_like 'allows commenting',
+                    file_id: '2f6fcd96b88b36ce98c38da085c795a27d92a3dd',
+                    line_code: '6_6',
+                    comment: 'Typo, please fix.'
+  end
 end

spec/helpers/merge_requests_helper_spec.rb

 require 'spec_helper'
 
 describe MergeRequestsHelper do
+  include ActionView::Helpers::UrlHelper
   include ProjectForksHelper
+
   describe 'ci_build_details_path' do
     let(:project) { create(:project) }
     let(:merge_request) { MergeRequest.new }
@@ -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

spec/lib/banzai/filter/commit_reference_filter_spec.rb

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

spec/models/diff_note_spec.rb

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

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

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

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

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

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

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