Improve merge request version feature

* Use version numbers instead of sha as more user-friendly
* Make it clear that we compare later version (left) with older one (right)
* Improve wording of version switch panel

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

6 files changed, 42 insertions, 41 deletions

diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss
index dd3e79b37af..2a44b95de64 100644
--- a/app/assets/stylesheets/pages/merge_requests.scss
+++ b/app/assets/stylesheets/pages/merge_requests.scss
@@ -383,10 +383,6 @@
   a.btn-link {
     color: $gl-dark-link-color;
   }
-
-  .compare-dots {
-    margin: 0 $btn-side-margin;
-  }
 }
 
 .merge-request-details {
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index e385cc20148..b86915e8470 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -95,7 +95,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     if params[:start_sha].present?
       @start_sha = params[:start_sha]
-      validate_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|
@@ -554,10 +558,4 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     @diffs = @merge_request_diff.diffs(diff_options)
   end
-
-  def validate_start_sha
-    unless @comparable_diffs.map(&:head_commit_sha).include?(@start_sha)
-      render_404
-    end
-  end
 end
diff --git a/app/helpers/git_helper.rb b/app/helpers/git_helper.rb
index 9d7982e169c..8ab394384f3 100644
--- a/app/helpers/git_helper.rb
+++ b/app/helpers/git_helper.rb
@@ -4,6 +4,6 @@ module GitHelper
   end
 
   def short_sha(text)
-    text[0...8]
+    Commit.truncate_sha(text)
   end
 end
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index 08ceaf9b4ca..8abe7865fed 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -106,4 +106,8 @@ module MergeRequestsHelper
       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
diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml
index 894736247e7..e8cb7f92c23 100644
--- a/app/views/projects/merge_requests/show/_versions.html.haml
+++ b/app/views/projects/merge_requests/show/_versions.html.haml
@@ -1,53 +1,56 @@
 - if @merge_request_diffs.size > 1
   .mr-version-controls
-    Version
+    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:&nbsp;
-          #{short_sha(@merge_request_diff.head_commit_sha)}
+            latest version
+          - else
+            version #{version_index(@merge_request_diff)}
         %span.caret
       %ul.dropdown-menu.dropdown-menu-selectable
-        - @merge_request_diffs.each_with_index do |merge_request_diff, i|
+        - @merge_request_diffs.each do |merge_request_diff|
           %li
             = link_to merge_request_version_path(@project, @merge_request, merge_request_diff), class: ('is-active' if merge_request_diff == @merge_request_diff) do
-              %strong.monospace
-                - if i.zero?
-                  Latest:
+              %strong
+                - if merge_request_diff.latest?
+                  latest version
                 - else
-                  #{@merge_request_diffs.size - i}.
-                #{short_sha(merge_request_diff.head_commit_sha)}
-              %br
+                  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)
 
     - if @merge_request_diff.base_commit_sha
-      %span.compare-dots ...
-
-      Compared with
+      &nbsp;and&nbsp;
       %span.dropdown.inline.mr-version-compare-dropdown
         %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} }
-          %strong.monospace<
+          %strong
             - if @start_sha
-              #{short_sha(@start_sha)}
+              version #{version_index(@start_version)}
             - else
-              Base: #{short_sha(@merge_request_diff.base_commit_sha)}
+              #{@merge_request.target_branch}
           %span.caret
         %ul.dropdown-menu.dropdown-menu-selectable
-          - @comparable_diffs.each_with_index do |merge_request_diff, i|
+          - @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.head_commit_sha == @start_sha) do
-                %strong.monospace
-                  #{@comparable_diffs.size - i}. #{short_sha(merge_request_diff.head_commit_sha)}
-                %br
+              = 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.monospace
-                Base: #{short_sha(@merge_request_diff.base_commit_sha)}
+              %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
diff --git a/spec/features/merge_requests/merge_request_versions_spec.rb b/spec/features/merge_requests/merge_request_versions_spec.rb
index 7ccf4e8e8f2..9e759de3752 100644
--- a/spec/features/merge_requests/merge_request_versions_spec.rb
+++ b/spec/features/merge_requests/merge_request_versions_spec.rb
@@ -12,7 +12,7 @@ feature 'Merge Request versions', js: true, feature: true do
 
   it 'show the latest version of the diff' do
     page.within '.mr-version-dropdown' do
-      expect(page).to have_content 'Latest: 5937ac0a'
+      expect(page).to have_content 'latest version'
     end
 
     expect(page).to have_content '8 changed files'
@@ -22,13 +22,13 @@ feature 'Merge Request versions', js: true, feature: true do
     before 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-dropdown' do
-        expect(page).to have_content '6f6d7e7e'
+        expect(page).to have_content 'version 1'
       end
 
       expect(page).to have_content '5 changed files'
@@ -43,13 +43,13 @@ feature 'Merge Request versions', js: true, feature: true do
     before do
       page.within '.mr-version-compare-dropdown' do
         find('.btn-link').click
-        click_link '6f6d7e7e'
+        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 '6f6d7e7e'
+        expect(page).to have_content 'version 1'
       end
     end