diff --git a/CHANGELOG b/CHANGELOG index 383ccc38c56..879fb98c26f 100644 --- a/CHANGELOG +++ b/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 diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 7fdd79fa8b9..2a44b95de64 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/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; diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 4f9ca0097a1..8895cb955bd 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/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 diff --git a/app/helpers/git_helper.rb b/app/helpers/git_helper.rb index 09684955233..8ab394384f3 100644 --- a/app/helpers/git_helper.rb +++ b/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 diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index a9e175c3f5c..8abe7865fed 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/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 diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 445179a4487..18c583add88 100644 --- a/app/models/merge_request_diff.rb +++ b/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) diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index 2da70ce7137..00287f2d245 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -1,31 +1,60 @@ -- 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. diff --git a/doc/user/project/merge_requests/versions.md b/doc/user/project/merge_requests/versions.md index 2b1c5ddd562..a6aa4b47835 100644 --- a/doc/user/project/merge_requests/versions.md +++ b/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:** diff --git a/spec/features/merge_requests/merge_request_versions_spec.rb b/spec/features/merge_requests/merge_request_versions_spec.rb index 577c910f11b..9e759de3752 100644 --- a/spec/features/merge_requests/merge_request_versions_spec.rb +++ b/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 diff --git a/spec/helpers/git_helper_spec.rb b/spec/helpers/git_helper_spec.rb new file mode 100644 index 00000000000..9b1ef1e05a2 --- /dev/null +++ b/spec/helpers/git_helper_spec.rb @@ -0,0 +1,9 @@ +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