Merge branch '44879' into 'master'

Add signature verification badge to compare view

See merge request gitlab-org/gitlab-ce!18245

app/assets/javascripts/gpg_badges.js

@@ -7,12 +7,12 @@ import { __ } from '~/locale';
 export default class GpgBadges {
   static fetch() {
     const badges = $('.js-loading-gpg-badge');
-    const form = $('.commits-search-form');
+    const tag = $('.js-signature-container');
 
     badges.html('<i class="fa fa-spinner fa-spin"></i>');
 
-    const params = parseQueryStringIntoObject(form.serialize());
-    return axios.get(form.data('signaturesPath'), { params })
+    const params = parseQueryStringIntoObject(tag.serialize());
+    return axios.get(tag.data('signaturesPath'), { params })
     .then(({ data }) => {
       data.signatures.forEach((signature) => {
         badges.filter(`[data-commit-sha="${signature.commit_sha}"]`).replaceWith(signature.html);

app/assets/javascripts/pages/projects/compare/show/index.js

 import Diff from '~/diff';
 import initChangesDropdown from '~/init_changes_dropdown';
+import GpgBadges from '~/gpg_badges';
 
 document.addEventListener('DOMContentLoaded', () => {
   new Diff(); // eslint-disable-line no-new
   const paddingTop = 16;
   initChangesDropdown(document.querySelector('.navbar-gitlab').offsetHeight - paddingTop);
+  GpgBadges.fetch();
 });

app/controllers/projects/compare_controller.rb

@@ -8,8 +8,11 @@ class Projects::CompareController < Projects::ApplicationController
   # Authorize
   before_action :require_non_empty_project
   before_action :authorize_download_code!
-  before_action :define_ref_vars, only: [:index, :show, :diff_for_path]
-  before_action :define_diff_vars, only: [:show, :diff_for_path]
+  # Defining ivars
+  before_action :define_diffs, only: [:show, :diff_for_path]
+  before_action :define_environment, only: [:show]
+  before_action :define_diff_notes_disabled, only: [:show, :diff_for_path]
+  before_action :define_commits, only: [:show, :diff_for_path, :signatures]
   before_action :merge_request, only: [:index, :show]
 
   def index
@@ -22,9 +25,9 @@ class Projects::CompareController < Projects::ApplicationController
   end
 
   def diff_for_path
-    return render_404 unless @compare
+    return render_404 unless compare
 
-    render_diff_for_path(@compare.diffs(diff_options))
+    render_diff_for_path(compare.diffs(diff_options))
   end
 
   def create
@@ -41,30 +44,60 @@ class Projects::CompareController < Projects::ApplicationController
     end
   end
 
+  def signatures
+    respond_to do |format|
+      format.json do
+        render json: {
+          signatures: @commits.select(&:has_signature?).map do |commit|
+            {
+              commit_sha: commit.sha,
+              html: view_to_html_string('projects/commit/_signature', signature: commit.signature)
+            }
+          end
+        }
+      end
+    end
+  end
+
   private
 
-  def define_ref_vars
-    @start_ref = Addressable::URI.unescape(params[:from])
+  def compare
+    return @compare if defined?(@compare)
+
+    @compare = CompareService.new(@project, head_ref).execute(@project, start_ref)
+  end
+
+  def start_ref
+    @start_ref ||= Addressable::URI.unescape(params[:from])
+  end
+
+  def head_ref
+    return @ref if defined?(@ref)
+
     @ref = @head_ref = Addressable::URI.unescape(params[:to])
   end
 
-  def define_diff_vars
-    @compare = CompareService.new(@project, @head_ref)
-      .execute(@project, @start_ref)
+  def define_commits
+    @commits = compare.present? ? prepare_commits_for_rendering(compare.commits) : []
+  end
 
-    if @compare
-      @commits = prepare_commits_for_rendering(@compare.commits)
-      @diffs = @compare.diffs(diff_options)
+  def define_diffs
+    @diffs = compare.present? ? compare.diffs(diff_options) : []
+  end
 
-      environment_params = @repository.branch_exists?(@head_ref) ? { ref: @head_ref } : { commit: @compare.commit }
+  def define_environment
+    if compare
+      environment_params = @repository.branch_exists?(head_ref) ? { ref: head_ref } : { commit: compare.commit }
       @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
-
-      @diff_notes_disabled = true
     end
   end
 
+  def define_diff_notes_disabled
+    @diff_notes_disabled = compare.present?
+  end
+
   def merge_request
     @merge_request ||= MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened
-      .find_by(source_project: @project, source_branch: @head_ref, target_branch: @start_ref)
+      .find_by(source_project: @project, source_branch: head_ref, target_branch: start_ref)
   end
 end

app/views/projects/commits/show.html.haml

@@ -24,7 +24,7 @@
               = link_to _("Create merge request"), create_mr_path(@repository.root_ref, @ref), class: 'btn btn-success'
 
           .control
-            = form_tag(project_commits_path(@project, @id), method: :get, class: 'commits-search-form', data: { 'signatures-path' => namespace_project_signatures_path }) do
+            = form_tag(project_commits_path(@project, @id), method: :get, class: 'commits-search-form js-signature-container', data: { 'signatures-path' => namespace_project_signatures_path }) do
               = search_field_tag :search, params[:search], { placeholder: _('Filter by commit message'), id: 'commits-search', class: 'form-control search-text-input input-short', spellcheck: false }
           .control
             = link_to project_commits_path(@project, @ref, rss_url_options), title: _("Commits feed"), class: 'btn' do

app/views/projects/compare/_form.html.haml

-= form_tag project_compare_index_path(@project), method: :post, class: 'form-inline js-requires-input' do
+= form_tag project_compare_index_path(@project), method: :post, class: 'form-inline js-requires-input js-signature-container', data: { 'signatures-path' => signatures_namespace_project_compare_index_path } do
   .clearfix
     - if params[:to] && params[:from]
       .compare-switch-container

changelogs/unreleased/44879.yml

+---
+title: Add the signature verfication badge to the compare view
+merge_request: 18245
+author: Marc Shaw
+type: added

config/routes/repository.rb

@@ -18,6 +18,7 @@ scope format: false do
   resources :compare, only: [:index, :create] do
     collection do
       get :diff_for_path
+      get :signatures
     end
   end
 

spec/controllers/projects/compare_controller_spec.rb

@@ -3,96 +3,99 @@ require 'spec_helper'
 describe Projects::CompareController do
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
-  let(:ref_from) { "improve%2Fawesome" }
-  let(:ref_to) { "feature" }
 
   before do
     sign_in(user)
     project.add_master(user)
   end
 
-  it 'compare shows some diffs' do
-    get(:show,
-        namespace_id: project.namespace,
-        project_id: project,
-        from: ref_from,
-        to: ref_to)
+  describe 'GET index' do
+    render_views
+
+    before do
+      get :index, namespace_id: project.namespace, project_id: project
+    end
 
-    expect(response).to be_success
-    expect(assigns(:diffs).diff_files.first).not_to be_nil
-    expect(assigns(:commits).length).to be >= 1
+    it 'returns successfully' do
+      expect(response).to be_success
+    end
   end
 
-  it 'compare shows some diffs with ignore whitespace change option' do
-    get(:show,
+  describe 'GET show' do
+    render_views
+
+    subject(:show_request) { get :show, request_params }
+
+    let(:request_params) do
+      {
         namespace_id: project.namespace,
         project_id: project,
-        from: '08f22f25',
-        to: '66eceea0',
-        w: 1)
-
-    expect(response).to be_success
-    diff_file = assigns(:diffs).diff_files.first
-    expect(diff_file).not_to be_nil
-    expect(assigns(:commits).length).to be >= 1
-    # without whitespace option, there are more than 2 diff_splits
-    diff_splits = diff_file.diff.diff.split("\n")
-    expect(diff_splits.length).to be <= 2
-  end
+        from: source_ref,
+        to: target_ref,
+        w: whitespace
+      }
+    end
 
-  describe 'non-existent refs' do
-    it 'uses invalid source ref' do
-      get(:show,
-          namespace_id: project.namespace,
-          project_id: project,
-          from: 'non-existent',
-          to: ref_to)
+    let(:whitespace) { nil }
 
-      expect(response).to be_success
-      expect(assigns(:diffs).diff_files.to_a).to eq([])
-      expect(assigns(:commits)).to eq([])
-    end
+    context 'when the refs exist' do
+      context 'when we set the white space param' do
+        let(:source_ref) { "08f22f25" }
+        let(:target_ref) { "66eceea0" }
+        let(:whitespace) { 1 }
 
-    it 'uses invalid target ref' do
-      get(:show,
-          namespace_id: project.namespace,
-          project_id: project,
-          from: ref_from,
-          to: 'non-existent')
+        it 'shows some diffs with ignore whitespace change option' do
+          show_request
 
-      expect(response).to be_success
-      expect(assigns(:diffs)).to eq(nil)
-      expect(assigns(:commits)).to eq(nil)
-    end
+          expect(response).to be_success
+          diff_file = assigns(:diffs).diff_files.first
+          expect(diff_file).not_to be_nil
+          expect(assigns(:commits).length).to be >= 1
+          # without whitespace option, there are more than 2 diff_splits
+          diff_splits = diff_file.diff.diff.split("\n")
+          expect(diff_splits.length).to be <= 2
+        end
+      end
+
+      context 'when we do not set the white space param' do
+        let(:source_ref) { "improve%2Fawesome" }
+        let(:target_ref) { "feature" }
+        let(:whitespace) { nil }
 
-    it 'redirects back to index when params[:from] is empty and preserves params[:to]' do
-      post(:create,
-           namespace_id: project.namespace,
-           project_id: project,
-           from: '',
-           to: 'master')
+        it 'sets the diffs and commits ivars' do
+          show_request
 
-      expect(response).to redirect_to(project_compare_index_path(project, to: 'master'))
+          expect(response).to be_success
+          expect(assigns(:diffs).diff_files.first).not_to be_nil
+          expect(assigns(:commits).length).to be >= 1
+        end
+      end
     end
 
-    it 'redirects back to index when params[:to] is empty and preserves params[:from]' do
-      post(:create,
-           namespace_id: project.namespace,
-           project_id: project,
-           from: 'master',
-           to: '')
+    context 'when the source ref does not exist' do
+      let(:source_ref) { 'non-existent-source-ref' }
+      let(:target_ref) { "feature" }
+
+      it 'sets empty diff and commit ivars' do
+        show_request
 
-      expect(response).to redirect_to(project_compare_index_path(project, from: 'master'))
+        expect(response).to be_success
+        expect(assigns(:diffs).diff_files.to_a).to eq([])
+        expect(assigns(:commits)).to eq([])
+      end
     end
 
-    it 'redirects back to index when params[:from] and params[:to] are empty' do
-      post(:create,
-           namespace_id: project.namespace,
-           project_id: project,
-           from: '',
-           to: '')
+    context 'when the target ref does not exist' do
+      let(:target_ref) { 'non-existent-target-ref' }
+      let(:source_ref) { "improve%2Fawesome" }
 
-      expect(response).to redirect_to(namespace_project_compare_index_path)
+      it 'sets empty diff and commit ivars' do
+        show_request
+
+        expect(response).to be_success
+        expect(assigns(:diffs)).to eq([])
+        expect(assigns(:commits)).to eq([])
+      end
     end
   end
 
@@ -107,12 +110,14 @@ describe Projects::CompareController do
     end
 
     let(:existing_path) { 'files/ruby/feature.rb' }
+    let(:source_ref) { "improve%2Fawesome" }
+    let(:target_ref) { "feature" }
 
-    context 'when the from and to refs exist' do
-      context 'when the user has access to the project' do
+    context 'when the source and target refs exist' do
+      context 'when the user has access target the project' do
         context 'when the path exists in the diff' do
           it 'disables diff notes' do
-            diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path)
+            diff_for_path(from: source_ref, to: target_ref, old_path: existing_path, new_path: existing_path)
 
             expect(assigns(:diff_notes_disabled)).to be_truthy
           end
@@ -123,13 +128,13 @@ describe Projects::CompareController do
               meth.call(diffs)
             end
 
-            diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path)
+            diff_for_path(from: source_ref, to: target_ref, old_path: existing_path, new_path: existing_path)
           end
         end
 
         context 'when the path does not exist in the diff' do
           before do
-            diff_for_path(from: ref_from, to: ref_to, old_path: existing_path.succ, new_path: existing_path.succ)
+            diff_for_path(from: source_ref, to: target_ref, old_path: existing_path.succ, new_path: existing_path.succ)
           end
 
           it 'returns a 404' do
@@ -138,10 +143,10 @@ describe Projects::CompareController do
         end
       end
 
-      context 'when the user does not have access to the project' do
+      context 'when the user does not have access target the project' do
         before do
           project.team.truncate
-          diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path)
+          diff_for_path(from: source_ref, to: target_ref, old_path: existing_path, new_path: existing_path)
         end
 
         it 'returns a 404' do
@@ -150,9 +155,9 @@ describe Projects::CompareController do
       end
     end
 
-    context 'when the from ref does not exist' do
+    context 'when the source ref does not exist' do
       before do
-        diff_for_path(from: ref_from.succ, to: ref_to, old_path: existing_path, new_path: existing_path)
+        diff_for_path(from: source_ref.succ, to: target_ref, old_path: existing_path, new_path: existing_path)
       end
 
       it 'returns a 404' do
@@ -160,9 +165,9 @@ describe Projects::CompareController do
       end
     end
 
-    context 'when the to ref does not exist' do
+    context 'when the target ref does not exist' do
       before do
-        diff_for_path(from: ref_from, to: ref_to.succ, old_path: existing_path, new_path: existing_path)
+        diff_for_path(from: source_ref, to: target_ref.succ, old_path: existing_path, new_path: existing_path)
       end
 
       it 'returns a 404' do
@@ -170,4 +175,153 @@ describe Projects::CompareController do
       end
     end
   end
+
+  describe 'POST create' do
+    subject(:create_request) { post :create, request_params }
+
+    let(:request_params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        from: source_ref,
+        to: target_ref
+      }
+    end
+
+    context 'when sending valid params' do
+      let(:source_ref) { "improve%2Fawesome" }
+      let(:target_ref) { "feature" }
+
+      it 'redirects back to show' do
+        create_request
+
+        expect(response).to redirect_to(project_compare_path(project, to: target_ref, from: source_ref))
+      end
+    end
+
+    context 'when sending invalid params' do
+      context 'when the source ref is empty and target ref is set' do
+        let(:source_ref) { '' }
+        let(:target_ref) { 'master' }
+
+        it 'redirects back to index and preserves the target ref' do
+          create_request
+
+          expect(response).to redirect_to(project_compare_index_path(project, to: target_ref))
+        end
+      end
+
+      context 'when the target ref is empty and source ref is set' do
+        let(:source_ref) { 'master' }
+        let(:target_ref) { '' }
+
+        it 'redirects back to index and preserves source ref' do
+          create_request
+
+          expect(response).to redirect_to(project_compare_index_path(project, from: source_ref))
+        end
+      end
+
+      context 'when the target and source ref are empty' do
+        let(:source_ref) { '' }
+        let(:target_ref) { '' }
+
+        it 'redirects back to index' do
+          create_request
+
+          expect(response).to redirect_to(namespace_project_compare_index_path)
+        end
+      end
+    end
+  end
+
+  describe 'GET signatures' do
+    subject(:signatures_request) { get :signatures, request_params }
+
+    let(:request_params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        from: source_ref,
+        to: target_ref,
+        format: :json
+      }
+    end
+
+    context 'when the source and target refs exist' do
+      let(:source_ref) { "improve%2Fawesome" }
+      let(:target_ref) { "feature" }
+
+      context 'when the user has access to the project' do
+        render_views
+
+        let(:signature_commit) { build(:commit, project: project, safe_message: "message", sha: 'signature_commit') }
+        let(:non_signature_commit) { build(:commit, project: project, safe_message: "message", sha: 'non_signature_commit') }
+
+        before do
+          escaped_source_ref = Addressable::URI.unescape(source_ref)
+          escaped_target_ref = Addressable::URI.unescape(target_ref)
+
+          compare_service = CompareService.new(project, escaped_target_ref)
+          compare = compare_service.execute(project, escaped_source_ref)
+
+          expect(CompareService).to receive(:new).with(project, escaped_target_ref).and_return(compare_service)
+          expect(compare_service).to receive(:execute).with(project, escaped_source_ref).and_return(compare)
+
+          expect(compare).to receive(:commits).and_return([signature_commit, non_signature_commit])
+          expect(non_signature_commit).to receive(:has_signature?).and_return(false)
+        end
+
+        it 'returns only the commit with a signature' do
+          signatures_request
+
+          expect(response).to have_gitlab_http_status(200)
+          parsed_body = JSON.parse(response.body)
+          signatures = parsed_body['signatures']
+
+          expect(signatures.size).to eq(1)
+          expect(signatures.first['commit_sha']).to eq(signature_commit.sha)
+          expect(signatures.first['html']).to be_present
+        end
+      end
+
+      context 'when the user does not have access to the project' do
+        before do
+          project.team.truncate
+        end
+
+        it 'returns a 404' do
+          signatures_request
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+    end
+
+    context 'when the source ref does not exist' do
+      let(:source_ref) { 'non-existent-ref-source' }
+      let(:target_ref) { "feature" }
+
+      it 'returns no signatures' do
+        signatures_request
+
+        expect(response).to have_gitlab_http_status(200)
+        parsed_body = JSON.parse(response.body)
+        expect(parsed_body['signatures']).to be_empty
+      end
+    end
+
+    context 'when the target ref does not exist' do
+      let(:target_ref) { 'non-existent-ref-target' }
+      let(:source_ref) { "improve%2Fawesome" }
+
+      it 'returns no signatures' do
+        signatures_request
+
+        expect(response).to have_gitlab_http_status(200)
+        parsed_body = JSON.parse(response.body)
+        expect(parsed_body['signatures']).to be_empty
+      end
+    end
+  end
 end

spec/javascripts/gpg_badges_spec.js

@@ -16,8 +16,8 @@ describe('GpgBadges', () => {
   beforeEach(() => {
     mock = new MockAdapter(axios);
     setFixtures(`
-      <form 
-        class="commits-search-form" data-signatures-path="/hello" action="/hello"
+      <form
+        class="commits-search-form js-signature-container" data-signatures-path="/hello" action="/hello"
         method="get">
         <input name="utf8" type="hidden" value="✓">
         <input type="search" name="search" id="commits-search"class="form-control search-text-input input-short">