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:  - #{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 +  and  %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