From aacb4caf7d60508256b7dabd751802d3ed83f3e0 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 1 Sep 2016 17:02:53 +0300 Subject: [PATCH] Refactor code for merge request version compare feature Signed-off-by: Dmitriy Zaporozhets --- .../projects/merge_requests_controller.rb | 44 +++++++++---- .../merge_requests/show/_versions.html.haml | 65 +++++++++---------- 2 files changed, 61 insertions(+), 48 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 2fa55b83c0e..e385cc20148 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -90,18 +90,21 @@ 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] + validate_start_sha + 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 - end - - if params[:start_sha].present? - compare_diff_version + if @start_sha + compared_diff_version else - @diffs = @merge_request_diff.diffs(diff_options) + original_diff_version end render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } @@ -534,14 +537,27 @@ class Projects::MergeRequestsController < Projects::ApplicationController @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute end - def compare_diff_version - @compare = CompareService.new.execute(@project, @merge_request_diff.head_commit_sha, @project, params[:start_sha]) + def compared_diff_version + compare = CompareService.new.execute(@project, @merge_request_diff.head_commit_sha, @project, @start_sha) - if @compare - @commits = @compare.commits - @commit = @compare.commit - @diffs = @compare.diffs(diff_options) + if compare + @diffs = compare.diffs(diff_options) @diff_notes_disabled = true end end + + def original_diff_version + unless @merge_request_diff.latest? + # Disable comments if browsing older version of the diff + @diff_notes_disabled = true + end + + @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/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index bdedaa31e12..aa45a6cb18b 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -1,58 +1,55 @@ -- merge_request_diffs = @merge_request.merge_request_diffs.select_without_diff -- compareable_diffs = merge_request_diffs.select { |diff| diff.id < @merge_request_diff.id } - -- if merge_request_diffs.size > 1 +- if @merge_request_diffs.size > 1 .mr-version-controls Version %span.dropdown.inline.mr-version-dropdown %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} } %strong.monospace< - if @merge_request_diff.latest? - Latest: #{short_sha(@merge_request_diff.head_commit_sha)} - - else - #{short_sha(@merge_request_diff.head_commit_sha)} + Latest:  + #{short_sha(@merge_request_diff.head_commit_sha)} %span.caret %ul.dropdown-menu.dropdown-menu-selectable - - merge_request_diffs.each_with_index do |merge_request_diff, i| + - @merge_request_diffs.each_with_index do |merge_request_diff, i| %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 + = link_to mr_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: - else - #{merge_request_diffs.size - i}. - #{merge_request_diff.head_commit.short_id} + #{@merge_request_diffs.size - i}. + #{short_sha(merge_request_diff.head_commit_sha)} %br %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) - %span.compare-dots ... + - if @merge_request_diff.base_commit_sha + %span.compare-dots ... - Compared with - %span.dropdown.inline.mr-version-compare-dropdown - %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} } - %strong.monospace< - - if params[:start_sha].present? - #{short_sha(params[:start_sha])} - - else - Base: #{short_sha(@merge_request_diff.base_commit_sha)} - %span.caret - %ul.dropdown-menu.dropdown-menu-selectable - - compareable_diffs.each_with_index do |merge_request_diff, i| - %li - = link_to mr_version_path(@project, @merge_request, @merge_request_diff, merge_request_diff.head_commit_sha), class: ('is-active' if merge_request_diff.head_commit_sha == params[:start_sha]) do - %strong.monospace - #{compareable_diffs.size - i}. #{short_sha(merge_request_diff.head_commit_sha)} - %br - %small - = time_ago_with_tooltip(merge_request_diff.created_at) - %li - = link_to mr_version_path(@project, @merge_request, @merge_request_diff), class: ('is-active' unless params[:start_sha].present?) do - %strong.monospace + Compared with + %span.dropdown.inline.mr-version-compare-dropdown + %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} } + %strong.monospace< + - if @start_sha + #{short_sha(@start_sha)} + - else Base: #{short_sha(@merge_request_diff.base_commit_sha)} + %span.caret + %ul.dropdown-menu.dropdown-menu-selectable + - @comparable_diffs.each_with_index do |merge_request_diff, i| + %li + = link_to mr_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 + %small + = time_ago_with_tooltip(merge_request_diff.created_at) + %li + = link_to mr_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)} - - unless @merge_request_diff.latest? && params[:start_sha].blank? + - unless @merge_request_diff.latest? && !@start_sha .prepend-top-10 = icon('info-circle') Comments are disabled while viewing outdated merge versions or comparing to versions other than base.