Merge branch 'dz-mr-version-compare' into 'master'

Allow compare merge request versions

## What does this MR do?

Add new functionality to the merge request page. It allows you easily compare merge request versions in one click.  

## Are there points in the code the reviewer needs to double check?

## Why was this MR needed?

To improve code review experience. 

## Screenshots (if relevant)

See discussion

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] ~~API support added~~
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/13570

See merge request !6127

CHANGELOG

@@ -132,6 +132,7 @@ v 8.11.4
   - Fix issue boards leak private label names and descriptions
   - Fix broken gitlab:backup:restore because of bad permissions on repo storage !6098 (Dirk Hörner)
   - Remove gitorious. !5866
+  - Allow compare merge request versions
 
 v 8.11.3
   - Allow system info page to handle case where info is unavailable

app/assets/stylesheets/pages/merge_requests.scss

@@ -375,7 +375,7 @@
   }
 }
 
-.mr-version-switch {
+.mr-version-controls {
   background: $background-color;
   padding: $gl-btn-padding;
   color: $gl-placeholder-color;

app/controllers/projects/merge_requests_controller.rb

@@ -90,16 +90,27 @@ class Projects::MergeRequestsController < Projects::ApplicationController
         @merge_request.merge_request_diff
       end
 
+    @merge_request_diffs = @merge_request.merge_request_diffs.select_without_diff
+    @comparable_diffs = @merge_request_diffs.select { |diff| diff.id < @merge_request_diff.id }
+
+    if params[:start_sha].present?
+      @start_sha = params[:start_sha]
+      @start_version = @comparable_diffs.find { |diff| diff.head_commit_sha == @start_sha }
+
+      unless @start_version
+        render_404
+      end
+    end
+
     respond_to do |format|
       format.html { define_discussion_vars }
       format.json do
-        unless @merge_request_diff.latest?
-          # Disable comments if browsing older version of the diff
-          @diff_notes_disabled = true
+        if @start_sha
+          compared_diff_version
+        else
+          original_diff_version
         end
 
-        @diffs = @merge_request_diff.diffs(diff_options)
-
         render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") }
       end
     end
@@ -529,4 +540,14 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     params[:merge_request] ||= ActionController::Parameters.new(source_project: @project)
     @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute
   end
+
+  def compared_diff_version
+    @diff_notes_disabled = true
+    @diffs = @merge_request_diff.compare_with(@start_sha).diffs(diff_options)
+  end
+
+  def original_diff_version
+    @diff_notes_disabled = !@merge_request_diff.latest?
+    @diffs = @merge_request_diff.diffs(diff_options)
+  end
 end

app/helpers/git_helper.rb

@@ -2,4 +2,8 @@ module GitHelper
   def strip_gpg_signature(text)
     text.gsub(/-----BEGIN PGP SIGNATURE-----(.*)-----END PGP SIGNATURE-----/m, "")
   end
+
+  def short_sha(text)
+    Commit.truncate_sha(text)
+  end
 end

app/helpers/merge_requests_helper.rb

@@ -100,4 +100,14 @@ module MergeRequestsHelper
   def merge_request_button_visibility(merge_request, closed)
     return 'hidden' if merge_request.closed? == closed || (merge_request.merged? == closed && !merge_request.closed?) || merge_request.closed_without_fork?
   end
+
+  def merge_request_version_path(project, merge_request, merge_request_diff, start_sha = nil)
+    diffs_namespace_project_merge_request_path(
+      project.namespace, project, merge_request,
+      diff_id: merge_request_diff.id, start_sha: start_sha)
+  end
+
+  def version_index(merge_request_diff)
+    @merge_request_diffs.size - @merge_request_diffs.index(merge_request_diff)
+  end
 end

app/models/merge_request_diff.rb

@@ -152,6 +152,10 @@ class MergeRequestDiff < ActiveRecord::Base
     self == merge_request.merge_request_diff
   end
 
+  def compare_with(sha)
+    CompareService.new.execute(project, head_commit_sha, project, sha)
+  end
+
   private
 
   def dump_commits(commits)

app/views/projects/merge_requests/show/_versions.html.haml

-- merge_request_diffs = @merge_request.merge_request_diffs.select_without_diff
-
-- if merge_request_diffs.size > 1
-  .mr-version-switch
-    Version:
-    %span.dropdown.inline
+- if @merge_request_diffs.size > 1
+  .mr-version-controls
+    Changes between
+    %span.dropdown.inline.mr-version-dropdown
       %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} }
-        %strong.monospace<
+        %strong
           - if @merge_request_diff.latest?
-            #{"latest"}
+            latest version
           - else
-            #{@merge_request_diff.head_commit.short_id}
+            version #{version_index(@merge_request_diff)}
         %span.caret
       %ul.dropdown-menu.dropdown-menu-selectable
-        - merge_request_diffs.each do |merge_request_diff|
+        - @merge_request_diffs.each do |merge_request_diff|
           %li
-            = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, diff_id: merge_request_diff.id), class: ('is-active' if merge_request_diff == @merge_request_diff) do
-              %strong.monospace
-                #{merge_request_diff.head_commit.short_id}
-              %br
+            = link_to merge_request_version_path(@project, @merge_request, merge_request_diff), class: ('is-active' if merge_request_diff == @merge_request_diff) do
+              %strong
+                - if merge_request_diff.latest?
+                  latest version
+                - else
+                  version #{version_index(merge_request_diff)}
+              .monospace #{short_sha(merge_request_diff.head_commit_sha)}
               %small
                 #{number_with_delimiter(merge_request_diff.commits.count)} #{'commit'.pluralize(merge_request_diff.commits.count)},
                 = time_ago_with_tooltip(merge_request_diff.created_at)
 
-    - unless @merge_request_diff.latest?
-      %span.prepend-left-default
+    - if @merge_request_diff.base_commit_sha
+      and
+      %span.dropdown.inline.mr-version-compare-dropdown
+        %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} }
+          %strong
+            - if @start_sha
+              version #{version_index(@start_version)}
+            - else
+              #{@merge_request.target_branch}
+          %span.caret
+        %ul.dropdown-menu.dropdown-menu-selectable
+          - @comparable_diffs.each do |merge_request_diff|
+            %li
+              = link_to merge_request_version_path(@project, @merge_request, @merge_request_diff, merge_request_diff.head_commit_sha), class: ('is-active' if merge_request_diff == @start_version) do
+                %strong
+                  - if merge_request_diff.latest?
+                    latest version
+                  - else
+                    version #{version_index(merge_request_diff)}
+                .monospace #{short_sha(merge_request_diff.head_commit_sha)}
+                %small
+                  = time_ago_with_tooltip(merge_request_diff.created_at)
+          %li
+            = link_to merge_request_version_path(@project, @merge_request, @merge_request_diff), class: ('is-active' unless @start_sha) do
+              %strong
+                #{@merge_request.target_branch} (base)
+              .monospace #{short_sha(@merge_request_diff.base_commit_sha)}
+
+    - unless @merge_request_diff.latest? && !@start_sha
+      .prepend-top-10
         = icon('info-circle')
-        This version is not the latest one. Comments are disabled
-    .pull-right
-      %span.monospace
-        #{@merge_request_diff.base_commit.short_id}..#{@merge_request_diff.head_commit.short_id}
+        - if @start_sha
+          Comments are disabled because you're comparing two versions of this merge request.
+        - else
+          Comments are disabled because you're viewing an old version of this merge request.

doc/user/project/merge_requests/versions.md

@@ -12,6 +12,12 @@ can select an older one from version dropdown.
 
 ![Merge Request Versions](img/versions.png)
 
+You can also compare the merge request version with older one to see what is
+changed since then.
+
+Please note that comments are disabled while viewing outdated merge versions
+or comparing to versions other than base.
+
 ---
 
 >**Note:**

spec/features/merge_requests/merge_request_versions_spec.rb

@@ -11,8 +11,8 @@ feature 'Merge Request versions', js: true, feature: true do
   end
 
   it 'show the latest version of the diff' do
-    page.within '.mr-version-switch' do
-      expect(page).to have_content 'Version: latest'
+    page.within '.mr-version-dropdown' do
+      expect(page).to have_content 'latest version'
     end
 
     expect(page).to have_content '8 changed files'
@@ -20,18 +20,49 @@ feature 'Merge Request versions', js: true, feature: true do
 
   describe 'switch between versions' do
     before do
-      page.within '.mr-version-switch' do
+      page.within '.mr-version-dropdown' do
         find('.btn-link').click
-        click_link '6f6d7e7e'
+        click_link 'version 1'
       end
     end
 
     it 'should show older version' do
-      page.within '.mr-version-switch' do
-        expect(page).to have_content 'Version: 6f6d7e7e'
+      page.within '.mr-version-dropdown' do
+        expect(page).to have_content 'version 1'
       end
 
       expect(page).to have_content '5 changed files'
     end
+
+    it 'show the message about disabled comments' do
+      expect(page).to have_content 'Comments are disabled'
+    end
+  end
+
+  describe 'compare with older version' do
+    before do
+      page.within '.mr-version-compare-dropdown' do
+        find('.btn-link').click
+        click_link 'version 1'
+      end
+    end
+
+    it 'should has correct value in the compare dropdown' do
+      page.within '.mr-version-compare-dropdown' do
+        expect(page).to have_content 'version 1'
+      end
+    end
+
+    it 'show the message about disabled comments' do
+      expect(page).to have_content 'Comments are disabled'
+    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
+
+    it 'show diff between new and old version' do
+      expect(page).to have_content '4 changed files with 15 additions and 6 deletions'
+    end
   end
 end

spec/helpers/git_helper_spec.rb

+require 'spec_helper'
+
+describe GitHelper do
+  describe '#short_sha' do
+    let(:short_sha) { helper.short_sha('d4e043f6c20749a3ab3f4b8e23f2a8979f4b9100') }
+
+    it { expect(short_sha).to eq('d4e043f6') }
+  end
+end