Skip to content
This repository has been archived by the owner on May 10, 2018. It is now read-only.

Commit

Permalink
Move checking for build_pushes? and build_pull_requests? to Approval
Browse files Browse the repository at this point in the history
Rejecting a pull request or a push in receive service results in not
creating a request object. This is not good when pushes or pull requests
are disabled in settings, because we have no way to tell what really
happend. For example: someone disables pushes in repo settings, another
person pushes, request nor build is not created. We can assume that it's
because of the settings, but currently we would need to check logs in
order to be sure.

This commit moves this check to Approval, which will also make it easier
to communicate a reason to the user.
  • Loading branch information
drogus committed Dec 23, 2013
1 parent 74c44d2 commit f70dd18
Show file tree
Hide file tree
Showing 7 changed files with 70 additions and 56 deletions.
19 changes: 18 additions & 1 deletion lib/travis/model/request/approval.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,26 @@ def initialize(request)
@commit = request.commit
end

def settings
repository.settings
end

delegate :build_pushes?, :build_pull_requests?, to: :settings

def accepted?
commit.present? &&
!repository.private? &&
(!excluded_repository? || included_repository?) &&
!skipped?
!skipped? &&
enabled_in_settings?
end

def enabled_in_settings?
request.pull_request? ? build_pull_requests? : build_pushes?
end

def disabled_in_settings?
!enabled_in_settings?
end

def branch_accepted?
Expand Down Expand Up @@ -46,6 +61,8 @@ def message
'excluded repository'
elsif skipped?
'skipped through commit message'
elsif disabled_in_settings?
request.pull_request? ? 'pull requests disabled' : 'pushes disabled'
elsif github_pages?
'github pages branch'
elsif request.config.blank?
Expand Down
6 changes: 1 addition & 5 deletions lib/travis/requests/services/receive/pull_request.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,8 @@
require 'travis/requests/services/receive/settings_support'

module Travis
module Requests
module Services
class Receive < Travis::Services::Base
class PullRequest
include SettingsSupport

attr_reader :event

def initialize(event)
Expand All @@ -29,7 +25,7 @@ def validate!
end

def disabled?
Travis::Features.feature_deactivated?(:pull_requests) || !repository_settings.build_pull_requests?
Travis::Features.feature_deactivated?(:pull_requests)
end

def head_change?
Expand Down
5 changes: 1 addition & 4 deletions lib/travis/requests/services/receive/push.rb
Original file line number Diff line number Diff line change
@@ -1,19 +1,16 @@
require 'travis/requests/services/receive/settings_support'

module Travis
module Requests
module Services
class Receive < Travis::Services::Base
class Push
include SettingsSupport
attr_reader :event

def initialize(event)
@event = event
end

def accept?
repository_settings.build_pushes?
true
end

def validate!
Expand Down
26 changes: 0 additions & 26 deletions lib/travis/requests/services/receive/settings_support.rb

This file was deleted.

52 changes: 50 additions & 2 deletions spec/travis/model/request/approval_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@

let(:approval) { Request::Approval.new(request) }

before do
approval.stubs(:build_pull_requests?).returns(true)
approval.stubs(:build_pushes?).returns(true)
end

describe 'config_accepted?' do
it 'approves the build when .travis.yml is missing, but builds with .travis.yml are allowed' do
request.config['.result'] = 'not_found'
Expand Down Expand Up @@ -44,7 +49,7 @@
end

it 'does not accept a request that does not have a commit' do
request.stubs(:commit).returns(nil)
approval.stubs(:commit).returns(nil)
approval.should_not be_accepted
end

Expand Down Expand Up @@ -74,15 +79,32 @@
request.stubs(:config).returns('branches' => { 'only' => ['gh_pages'] })
approval.should be_accepted
end

it 'does not accept a request when it is disabled in settings' do
approval.stubs(:enabled_in_settings?).returns(false)
approval.should_not be_accepted
end
end

describe 'approved?' do
xit 'should be specified'
end

describe 'message' do
it 'returns "pull requests disabled" if pull requests are disabled' do
approval.stubs(:enabled_in_settings?).returns(false)
request.stubs(:pull_request?).returns(true)
approval.message.should == 'pull requests disabled'
end

it 'returns "pushes disabled" if pushes are disabled' do
approval.stubs(:enabled_in_settings?).returns(false)
request.stubs(:pull_request?).returns(false)
approval.message.should == 'pushes disabled'
end

it 'returns "missing commit" if the commit is missing' do
request.stubs(:commit).returns(nil)
approval.stubs(:commit).returns(nil)
approval.message.should == 'missing commit'
end

Expand Down Expand Up @@ -197,4 +219,30 @@
approval.send(:excluded_repository?).should be_false
end
end

describe 'enabled_in_settings?' do
it 'returns true if pull requests are enabled and a request is a pull request' do
request.stubs(:pull_request?).returns(true)
approval.stubs(:build_pull_requests?).returns(true)
approval.enabled_in_settings?.should be_true
end

it 'returns true if pushes are enabled and a request is a push' do
request.stubs(:pull_request?).returns(false)
approval.stubs(:build_pushes?).returns(true)
approval.enabled_in_settings?.should be_true
end

it 'returns false if pull requests are disabled and a request is a pull request' do
request.stubs(:pull_request?).returns(true)
approval.stubs(:build_pull_requests?).returns(false)
approval.enabled_in_settings?.should be_false
end

it 'returns false if pushes are disabled and a request is a push' do
request.stubs(:pull_request?).returns(false)
approval.stubs(:build_pushes?).returns(false)
approval.enabled_in_settings?.should be_false
end
end
end
9 changes: 0 additions & 9 deletions spec/travis/requests/services/receive/pull_request_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,6 @@
Travis::Features.disable_for_all(:pull_requests)
payload.accept?.should be_false
end

it 'rejects when the feature is disabled for the repository' do
id = payload.event['repository']['id']
settings = mock('settings')
repo = stub('repository', settings: settings)
payload.expects(:run_service).with(:find_repo, github_id: id).returns(repo)
settings.expects(:build_pull_requests?).returns(false)
payload.accept?.should be_false
end
end

describe 'given action is "reopened"' do
Expand Down
9 changes: 0 additions & 9 deletions spec/travis/requests/services/receive/push_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,6 @@
end
end

it 'does not accept the request if pushes are disabled in the settings' do
id = payload.event['repository']['id']
settings = mock('settings')
repo = stub('repository', settings: settings)
payload.expects(:run_service).with(:find_repo, github_id: id).returns(repo)
settings.expects(:build_pushes?).returns(false)
payload.accept?.should be_false
end

describe 'commit' do
it 'returns all attributes required for a Commit' do
payload.commit.should == {
Expand Down

0 comments on commit f70dd18

Please sign in to comment.