Merge branch 'display-mr-in-commit-page' into 'master'

Display related merge requests in commit detail page

Closes #2383

See merge request gitlab-org/gitlab-ce!13713

app/assets/javascripts/commit_merge_requests.js

+/* global Flash */
+
+import axios from './lib/utils/axios_utils';
+import { n__, s__ } from './locale';
+
+export function getHeaderText(childElementCount, mergeRequestCount) {
+  if (childElementCount === 0) {
+    return `${mergeRequestCount} ${n__('merge request', 'merge requests', mergeRequestCount)}`;
+  }
+  return ',';
+}
+
+export function createHeader(childElementCount, mergeRequestCount) {
+  const headerText = getHeaderText(childElementCount, mergeRequestCount);
+
+  return $('<span />', {
+    class: 'append-right-5',
+    text: headerText,
+  });
+}
+
+export function createLink(mergeRequest) {
+  return $('<a />', {
+    class: 'append-right-5',
+    href: mergeRequest.path,
+    text: `!${mergeRequest.iid}`,
+  });
+}
+
+export function createTitle(mergeRequest) {
+  return $('<span />', {
+    text: mergeRequest.title,
+  });
+}
+
+export function createItem(mergeRequest) {
+  const $item = $('<span />');
+  const $link = createLink(mergeRequest);
+  const $title = createTitle(mergeRequest);
+  $item.append($link);
+  $item.append($title);
+
+  return $item;
+}
+
+export function createContent(mergeRequests) {
+  const $content = $('<span />');
+
+  if (mergeRequests.length === 0) {
+    $content.text(s__('Commits|No related merge requests found'));
+  } else {
+    mergeRequests.forEach((mergeRequest) => {
+      const $header = createHeader($content.children().length, mergeRequests.length);
+      const $item = createItem(mergeRequest);
+      $content.append($header);
+      $content.append($item);
+    });
+  }
+
+  return $content;
+}
+
+export function fetchCommitMergeRequests() {
+  const $container = $('.merge-requests');
+
+  axios.get($container.data('projectCommitPath'))
+    .then((response) => {
+      const $content = createContent(response.data);
+
+      $container.html($content);
+    })
+    .catch(() => Flash(s__('Commits|An error occurred while fetching merge requests data.')));
+}

app/assets/javascripts/dispatcher.js

@@ -68,6 +68,7 @@ import Diff from './diff';
 import ProjectLabelSubscription from './project_label_subscription';
 import SearchAutocomplete from './search_autocomplete';
 import Activities from './activities';
+import { fetchCommitMergeRequests } from './commit_merge_requests';
 
 (function() {
   var Dispatcher;
@@ -311,6 +312,7 @@ import Activities from './activities';
           const stickyBarPaddingTop = 16;
           initChangesDropdown(document.querySelector('.navbar-gitlab').offsetHeight - stickyBarPaddingTop);
           $('.commit-info.branches').load(document.querySelector('.js-commit-box').dataset.commitPath);
+          fetchCommitMergeRequests();
           break;
         case 'projects:commit:pipelines':
           new MiniPipelineGraph({

app/controllers/projects/commit_controller.rb

@@ -12,7 +12,7 @@ class Projects::CommitController < Projects::ApplicationController
   before_action :authorize_download_code!
   before_action :authorize_read_pipeline!, only: [:pipelines]
   before_action :commit
-  before_action :define_commit_vars, only: [:show, :diff_for_path, :pipelines]
+  before_action :define_commit_vars, only: [:show, :diff_for_path, :pipelines, :merge_requests]
   before_action :define_note_vars, only: [:show, :diff_for_path]
   before_action :authorize_edit_tree!, only: [:revert, :cherry_pick]
 
@@ -52,6 +52,18 @@ class Projects::CommitController < Projects::ApplicationController
     end
   end
 
+  def merge_requests
+    @merge_requests = @commit.merge_requests.map do |mr|
+      { iid: mr.iid, path: merge_request_path(mr), title: mr.title }
+    end
+
+    respond_to do |format|
+      format.json do
+        render json: @merge_requests.to_json
+      end
+    end
+  end
+
   def branches
     # branch_names_contains/tag_names_contains can take a long time when there are thousands of
     # branches/tags - each `git branch --contains xxx` request can consume a cpu core.

app/models/commit.rb

@@ -238,6 +238,10 @@ class Commit
     notes.includes(:author)
   end
 
+  def merge_requests
+    @merge_requests ||= project.merge_requests.by_commit_sha(sha)
+  end
+
   def method_missing(method, *args, &block)
     @raw.__send__(method, *args, &block) # rubocop:disable GitlabSecurity/PublicSend
   end

app/models/merge_request.rb

@@ -140,7 +140,9 @@ class MergeRequest < ActiveRecord::Base
   scope :merged, -> { with_state(:merged) }
   scope :closed_and_merged, -> { with_states(:closed, :merged) }
   scope :from_source_branches, ->(branches) { where(source_branch: branches) }
-
+  scope :by_commit_sha, ->(sha) do
+    where('EXISTS (?)', MergeRequestDiff.select(1).where('merge_requests.latest_merge_request_diff_id = merge_request_diffs.id').by_commit_sha(sha)).reorder(nil)
+  end
   scope :join_project, -> { joins(:target_project) }
   scope :references_project, -> { references(:target_project) }
   scope :assigned, -> { where("assignee_id IS NOT NULL") }

app/models/merge_request_diff.rb

@@ -28,6 +28,9 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   scope :viewable, -> { without_state(:empty) }
+  scope :by_commit_sha, ->(sha) do
+    joins(:merge_request_diff_commits).where(merge_request_diff_commits: { sha: sha }).reorder(nil)
+  end
 
   scope :recent, -> { order(id: :desc).limit(100) }
 

app/views/projects/commit/_commit_box.html.haml

@@ -64,6 +64,12 @@
     .commit-info.branches
       %i.fa.fa-spinner.fa-spin
 
+  .well-segment.merge-request-info
+    .icon-container
+      = custom_icon('mr_bold')
+    %span.commit-info.merge-requests{ 'data-project-commit-path' => merge_requests_project_commit_path(@project, @commit.id, format: :json) }
+      = icon('spinner spin')
+
   - if @commit.last_pipeline
     - last_pipeline = @commit.last_pipeline
     .well-segment.pipeline-info

changelogs/unreleased/display-mr-in-commit-page.yml

+---
+title: Add link on commit page to merge request that introduced that commit
+merge_request: 13713
+author: Hiroyuki Sato
+type: added

config/routes/project.rb

@@ -50,6 +50,7 @@ constraints(ProjectUrlConstrainer.new) do
           post :revert
           post :cherry_pick
           get :diff_for_path
+          get :merge_requests
         end
       end
 

db/migrate/20170827123848_add_index_on_merge_request_diff_commit_sha.rb

+# rubocop:disable RemoveIndex
+
+class AddIndexOnMergeRequestDiffCommitSha < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :merge_request_diff_commits, :sha, length: Gitlab::Database.mysql? ? 20 : nil
+  end
+
+  def down
+    remove_index :merge_request_diff_commits, :sha if index_exists? :merge_request_diff_commits, :sha
+  end
+end

db/schema.rb

@@ -1013,6 +1013,7 @@ ActiveRecord::Schema.define(version: 20180105212544) do
   end
 
   add_index "merge_request_diff_commits", ["merge_request_diff_id", "relative_order"], name: "index_merge_request_diff_commits_on_mr_diff_id_and_order", unique: true, using: :btree
+  add_index "merge_request_diff_commits", ["sha"], name: "index_merge_request_diff_commits_on_sha", using: :btree
 
   create_table "merge_request_diff_files", id: false, force: :cascade do |t|
     t.integer "merge_request_diff_id", null: false

spec/javascripts/commit_merge_requests_spec.js

+import * as CommitMergeRequests from '~/commit_merge_requests';
+
+describe('CommitMergeRequests', () => {
+  describe('createContent', () => {
+    it('should return created content', () => {
+      const content1 = CommitMergeRequests.createContent([{ iid: 1, path: '/path1', title: 'foo' }, { iid: 2, path: '/path2', title: 'baz' }])[0];
+      expect(content1.tagName).toEqual('SPAN');
+      expect(content1.childElementCount).toEqual(4);
+
+      const content2 = CommitMergeRequests.createContent([])[0];
+      expect(content2.tagName).toEqual('SPAN');
+      expect(content2.childElementCount).toEqual(0);
+      expect(content2.innerText).toEqual('No related merge requests found');
+    });
+  });
+
+  describe('getHeaderText', () => {
+    it('should return header text', () => {
+      expect(CommitMergeRequests.getHeaderText(0, 1)).toEqual('1 merge request');
+      expect(CommitMergeRequests.getHeaderText(0, 2)).toEqual('2 merge requests');
+      expect(CommitMergeRequests.getHeaderText(1, 1)).toEqual(',');
+      expect(CommitMergeRequests.getHeaderText(1, 2)).toEqual(',');
+    });
+  });
+
+  describe('createHeader', () => {
+    it('should return created header', () => {
+      const header = CommitMergeRequests.createHeader(0, 1)[0];
+      expect(header.tagName).toEqual('SPAN');
+      expect(header.innerText).toEqual('1 merge request');
+    });
+  });
+
+  describe('createItem', () => {
+    it('should return created item', () => {
+      const item = CommitMergeRequests.createItem({ iid: 1, path: '/path', title: 'foo' })[0];
+      expect(item.tagName).toEqual('SPAN');
+      expect(item.childElementCount).toEqual(2);
+      expect(item.children[0].tagName).toEqual('A');
+      expect(item.children[1].tagName).toEqual('SPAN');
+    });
+  });
+
+  describe('createLink', () => {
+    it('should return created link', () => {
+      const link = CommitMergeRequests.createLink({ iid: 1, path: '/path', title: 'foo' })[0];
+      expect(link.tagName).toEqual('A');
+      expect(link.href).toMatch(/\/path$/);
+      expect(link.innerText).toEqual('!1');
+    });
+  });
+
+  describe('createTitle', () => {
+    it('should return created title', () => {
+      const title = CommitMergeRequests.createTitle({ iid: 1, path: '/path', title: 'foo' })[0];
+      expect(title.tagName).toEqual('SPAN');
+      expect(title.innerText).toEqual('foo');
+    });
+  });
+});

spec/models/commit_spec.rb

@@ -513,4 +513,17 @@ eos
       expect(described_class.valid_hash?('a' * 41)).to be false
     end
   end
+
+  describe '#merge_requests' do
+    let!(:project) { create(:project, :repository) }
+    let!(:merge_request1) { create(:merge_request, source_project: project, source_branch: 'master', target_branch: 'feature') }
+    let!(:merge_request2) { create(:merge_request, source_project: project, source_branch: 'merged-target', target_branch: 'feature') }
+    let(:commit1) { merge_request1.merge_request_diff.commits.last }
+    let(:commit2) { merge_request1.merge_request_diff.commits.first }
+
+    it 'returns merge_requests that introduced that commit' do
+      expect(commit1.merge_requests).to eq([merge_request1, merge_request2])
+      expect(commit2.merge_requests).to eq([merge_request1])
+    end
+  end
 end

spec/models/merge_request_diff_spec.rb

@@ -15,6 +15,28 @@ describe MergeRequestDiff do
     it { expect(subject.start_commit_sha).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9') }
   end
 
+  describe '.by_commit_sha' do
+    subject(:by_commit_sha) { described_class.by_commit_sha(sha) }
+
+    let!(:merge_request) { create(:merge_request, :with_diffs) }
+
+    context 'with sha contained in' do
+      let(:sha) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' }
+
+      it 'returns merge request diffs' do
+        expect(by_commit_sha).to eq([merge_request.merge_request_diff])
+      end
+    end
+
+    context 'with sha not contained in' do
+      let(:sha) { 'b83d6e3' }
+
+      it 'returns empty result' do
+        expect(by_commit_sha).to be_empty
+      end
+    end
+  end
+
   describe '#latest' do
     let!(:mr) { create(:merge_request, :with_diffs) }
     let!(:first_diff) { mr.merge_request_diff }

spec/models/merge_request_spec.rb

@@ -87,6 +87,39 @@ describe MergeRequest do
     it { is_expected.to respond_to(:merge_when_pipeline_succeeds) }
   end
 
+  describe '.by_commit_sha' do
+    subject(:by_commit_sha) { described_class.by_commit_sha(sha) }
+
+    let!(:merge_request) { create(:merge_request, :with_diffs) }
+
+    context 'with sha contained in latest merge request diff' do
+      let(:sha) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' }
+
+      it 'returns merge requests' do
+        expect(by_commit_sha).to eq([merge_request])
+      end
+    end
+
+    context 'with sha contained not in latest merge request diff' do
+      let(:sha) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' }
+
+      it 'returns empty requests' do
+        latest_merge_request_diff = merge_request.merge_request_diffs.create
+        latest_merge_request_diff.merge_request_diff_commits.where(sha: 'b83d6e391c22777fca1ed3012fce84f633d7fed0').delete_all
+
+        expect(by_commit_sha).to be_empty
+      end
+    end
+
+    context 'with sha not contained in' do
+      let(:sha) { 'b83d6e3' }
+
+      it 'returns empty result' do
+        expect(by_commit_sha).to be_empty
+      end
+    end
+  end
+
   describe '.in_projects' do
     it 'returns the merge requests for a set of projects' do
       expect(described_class.in_projects(Project.all)).to eq([subject])