From fba9a87c3956640fc49f520daba28984f732cde7 Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Tue, 7 Jun 2016 18:52:15 +0200 Subject: [PATCH 1/4] Add settings to API V3 This adds /repo/{repository.id}/settings endpoints for reading and updating repo settings. Main points: 1. Sets up Settings as a first class resource instead of as an attribute of Repository 2. Adds new meta-programmed method to Query for accessing all prefixed params as a hash. --- lib/travis/api/v3/access_control/generic.rb | 4 + lib/travis/api/v3/models/settings.rb | 29 ++++ lib/travis/api/v3/queries/settings.rb | 15 +++ lib/travis/api/v3/query.rb | 10 ++ lib/travis/api/v3/renderer/settings.rb | 18 +++ lib/travis/api/v3/routes.rb | 6 + lib/travis/api/v3/routes/dsl.rb | 4 + lib/travis/api/v3/services.rb | 1 + .../api/v3/services/repository/settings.rb | 0 lib/travis/api/v3/services/settings/find.rb | 9 ++ lib/travis/api/v3/services/settings/update.rb | 11 ++ spec/v3/services/settings_spec.rb | 127 ++++++++++++++++++ 12 files changed, 234 insertions(+) create mode 100644 lib/travis/api/v3/models/settings.rb create mode 100644 lib/travis/api/v3/queries/settings.rb create mode 100644 lib/travis/api/v3/renderer/settings.rb create mode 100644 lib/travis/api/v3/services/repository/settings.rb create mode 100644 lib/travis/api/v3/services/settings/find.rb create mode 100644 lib/travis/api/v3/services/settings/update.rb create mode 100644 spec/v3/services/settings_spec.rb diff --git a/lib/travis/api/v3/access_control/generic.rb b/lib/travis/api/v3/access_control/generic.rb index 0344b35b..92b68e1e 100644 --- a/lib/travis/api/v3/access_control/generic.rb +++ b/lib/travis/api/v3/access_control/generic.rb @@ -92,6 +92,10 @@ module Travis::API::V3 private_repository_visible?(repository) end + def settings_visible?(settings) + repository_visible?(settings.repository) + end + def private_repository_visible?(repository) false end diff --git a/lib/travis/api/v3/models/settings.rb b/lib/travis/api/v3/models/settings.rb new file mode 100644 index 00000000..626ea097 --- /dev/null +++ b/lib/travis/api/v3/models/settings.rb @@ -0,0 +1,29 @@ +module Travis::API::V3 + class Models::Settings + attr_reader :repository + + def initialize(repository) + @repository = repository + end + + def to_h + defaults.merge(repository.settings || {}) + end + + def update(settings = {}) + settings = defaults.merge(settings) + repository.update_attributes(settings: JSON.generate(settings)) + end + + private + + def defaults + { + 'builds_only_with_travis_yml' => false, + 'build_pushes' => true, + 'build_pull_requests' => true, + 'maximum_number_of_builds' => 0 + } + end + end +end diff --git a/lib/travis/api/v3/queries/settings.rb b/lib/travis/api/v3/queries/settings.rb new file mode 100644 index 00000000..ce5ea94e --- /dev/null +++ b/lib/travis/api/v3/queries/settings.rb @@ -0,0 +1,15 @@ +module Travis::API::V3 + class Queries::Settings < Query + params :builds_only_with_travis_yml, :build_pushes, :build_pull_requests, :maximum_number_of_builds, prefix: :settings + + def find(repository) + Models::Settings.new(repository) + end + + def update(repository) + settings = find(repository) + settings.update(settings_params) + settings + end + end +end diff --git a/lib/travis/api/v3/query.rb b/lib/travis/api/v3/query.rb index a9812e0c..af4e5006 100644 --- a/lib/travis/api/v3/query.rb +++ b/lib/travis/api/v3/query.rb @@ -32,6 +32,15 @@ module Travis::API::V3 end RUBY + @@prefixed_params_accessor = <<-RUBY + def %s_params + @%s ||= begin + params = @params.select { |key, _| key.start_with?('%s.'.freeze) } + Hash[params.map { |key, value| [key.split('.'.freeze).last, value] }] + end + end + RUBY + def self.type name[/[^:]+$/].underscore end @@ -48,6 +57,7 @@ module Travis::API::V3 check_type: check_type }) end + class_eval(@@prefixed_params_accessor % { prefix: prefix }) end def self.prefix(input) diff --git a/lib/travis/api/v3/renderer/settings.rb b/lib/travis/api/v3/renderer/settings.rb new file mode 100644 index 00000000..87ad2b8c --- /dev/null +++ b/lib/travis/api/v3/renderer/settings.rb @@ -0,0 +1,18 @@ +module Travis::API::V3 + module Renderer::Settings + extend self + + AVAILABLE_ATTRIBUTES = [:settings] + + def available_attributes + AVAILABLE_ATTRIBUTES + end + + def render(settings, **) + { + :@type => 'settings'.freeze, + :settings => settings.to_h + } + end + end +end diff --git a/lib/travis/api/v3/routes.rb b/lib/travis/api/v3/routes.rb index d22edce9..585dc1d8 100644 --- a/lib/travis/api/v3/routes.rb +++ b/lib/travis/api/v3/routes.rb @@ -117,6 +117,12 @@ module Travis::API::V3 get :find post :create end + + resource :settings do + route '/settings' + get :find + patch :update + end end resource :user do diff --git a/lib/travis/api/v3/routes/dsl.rb b/lib/travis/api/v3/routes/dsl.rb index 9a98ada0..a0210ce2 100644 --- a/lib/travis/api/v3/routes/dsl.rb +++ b/lib/travis/api/v3/routes/dsl.rb @@ -60,6 +60,10 @@ module Travis::API::V3 current_resource.add_service('POST'.freeze, *args) end + def patch(*args) + current_resource.add_service('PATCH'.freeze, *args) + end + def delete(*args) current_resource.add_service('DELETE'.freeze, *args) end diff --git a/lib/travis/api/v3/services.rb b/lib/travis/api/v3/services.rb index 637ea337..32b30b7c 100644 --- a/lib/travis/api/v3/services.rb +++ b/lib/travis/api/v3/services.rb @@ -20,6 +20,7 @@ module Travis::API::V3 Repositories = Module.new { extend Services } Repository = Module.new { extend Services } Requests = Module.new { extend Services } + Settings = Module.new { extend Services } User = Module.new { extend Services } def result_type diff --git a/lib/travis/api/v3/services/repository/settings.rb b/lib/travis/api/v3/services/repository/settings.rb new file mode 100644 index 00000000..e69de29b diff --git a/lib/travis/api/v3/services/settings/find.rb b/lib/travis/api/v3/services/settings/find.rb new file mode 100644 index 00000000..9ab54fec --- /dev/null +++ b/lib/travis/api/v3/services/settings/find.rb @@ -0,0 +1,9 @@ +module Travis::API::V3 + class Services::Settings::Find < Service + def run! + raise LoginRequired unless access_control.logged_in? or access_control.full_access? + raise NotFound unless repo = find(:repository) + find(:settings, repo) + end + end +end diff --git a/lib/travis/api/v3/services/settings/update.rb b/lib/travis/api/v3/services/settings/update.rb new file mode 100644 index 00000000..c2780e20 --- /dev/null +++ b/lib/travis/api/v3/services/settings/update.rb @@ -0,0 +1,11 @@ +module Travis::API::V3 + class Services::Settings::Update < Service + params :builds_only_with_travis_yml, :build_pushes, :build_pull_requests, :maximum_number_of_builds, prefix: :settings + + def run! + raise LoginRequired unless access_control.logged_in? or access_control.full_access? + raise NotFound unless repository = find(:repository) + query.update(repository) + end + end +end diff --git a/spec/v3/services/settings_spec.rb b/spec/v3/services/settings_spec.rb new file mode 100644 index 00000000..d5a3bd09 --- /dev/null +++ b/spec/v3/services/settings_spec.rb @@ -0,0 +1,127 @@ +require 'spec_helper' + +describe Travis::API::V3::Services::Settings do + let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } + let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } + let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } + let(:json_headers) { { 'CONTENT_TYPE' => 'application/json' } } + + describe :Find do + describe 'not authenticated' do + before { get("/v3/repo/#{repo.id}/settings") } + example { expect(last_response.status).to eq(403) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'error', + 'error_type' => 'login_required', + 'error_message' => 'login required' + ) + end + end + + describe 'authenticated, missing repo' do + before { get('/v3/repo/9999999999/settings', {}, auth_headers) } + + example { expect(last_response.status).to eq(404) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'error', + 'error_type' => 'not_found', + 'error_message' => 'repository not found (or insufficient access)', + 'resource_type' => 'repository' + ) + end + end + + describe 'authenticated, existing repo, repo has no settings' do + before { get("/v3/repo/#{repo.id}/settings", {}, auth_headers) } + + example { expect(last_response.status).to eq(200) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'settings', + 'settings' => { + 'builds_only_with_travis_yml' => false, + 'build_pushes' => true, + 'build_pull_requests' => true, + 'maximum_number_of_builds' => 0 + } + ) + end + end + + describe 'authenticated, existing repo, repo has some settings' do + before do + repo.update_attributes(settings: JSON.dump('build_pushes' => false)) + get("/v3/repo/#{repo.id}/settings", {}, auth_headers) + end + + example { expect(last_response.status).to eq(200) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'settings', + 'settings' => { + 'builds_only_with_travis_yml' => false, + 'build_pushes' => false, + 'build_pull_requests' => true, + 'maximum_number_of_builds' => 0 + } + ) + end + end + end + + describe :Update do + describe 'not authenticated' do + before do + patch("/v3/repo/#{repo.id}/settings", JSON.dump(build_pushes: false), json_headers) + end + + example { expect(last_response.status).to eq(403) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'error', + 'error_type' => 'login_required', + 'error_message' => 'login required' + ) + end + end + + describe 'authenticated, missing repo' do + before do + patch('/v3/repo/9999999999/settings', JSON.dump(build_pushes: false), json_headers.merge(auth_headers)) + end + + example { expect(last_response.status).to eq(404) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'error', + 'error_type' => 'not_found', + 'error_message' => 'repository not found (or insufficient access)', + 'resource_type' => 'repository' + ) + end + end + + describe 'authenticated, existing repo' do + let(:params) { JSON.dump('settings.build_pushes' => false) } + + before do + patch("/v3/repo/#{repo.id}/settings", params, json_headers.merge(auth_headers)) + end + + example { expect(last_response.status).to eq(200) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'settings', + 'settings' => { + 'builds_only_with_travis_yml' => false, + 'build_pushes' => false, + 'build_pull_requests' => true, + 'maximum_number_of_builds' => 0 + } + ) + end + end + end +end From daf534edb7bb0b82df9ea5cc4d81d2def3ede45d Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Wed, 8 Jun 2016 17:13:05 +0200 Subject: [PATCH 2/4] Don't overwrite settings with defaults Also changes the way the JSON fields are set to force ActiveRecord to recognise the changes. Is there a better way? --- lib/travis/api/v3/models/settings.rb | 6 ++++-- spec/v3/services/settings_spec.rb | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/travis/api/v3/models/settings.rb b/lib/travis/api/v3/models/settings.rb index 626ea097..48824816 100644 --- a/lib/travis/api/v3/models/settings.rb +++ b/lib/travis/api/v3/models/settings.rb @@ -11,8 +11,10 @@ module Travis::API::V3 end def update(settings = {}) - settings = defaults.merge(settings) - repository.update_attributes(settings: JSON.generate(settings)) + settings = to_h.merge(settings) + repository.settings.clear + settings.each { |k, v| repository.settings[k] = v } + repository.save! end private diff --git a/spec/v3/services/settings_spec.rb b/spec/v3/services/settings_spec.rb index d5a3bd09..c193a7ec 100644 --- a/spec/v3/services/settings_spec.rb +++ b/spec/v3/services/settings_spec.rb @@ -107,6 +107,7 @@ describe Travis::API::V3::Services::Settings do let(:params) { JSON.dump('settings.build_pushes' => false) } before do + repo.update_attributes(settings: JSON.dump('maximum_number_of_builds' => 20)) patch("/v3/repo/#{repo.id}/settings", params, json_headers.merge(auth_headers)) end @@ -118,7 +119,7 @@ describe Travis::API::V3::Services::Settings do 'builds_only_with_travis_yml' => false, 'build_pushes' => false, 'build_pull_requests' => true, - 'maximum_number_of_builds' => 0 + 'maximum_number_of_builds' => 20 } ) end From eb0eab596703bc320ee7f67e6c2aac62f4628c9f Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Wed, 8 Jun 2016 17:18:24 +0200 Subject: [PATCH 3/4] Move settings to top level of response To better accommodate nested responses. --- lib/travis/api/v3/renderer/settings.rb | 6 ++---- spec/v3/services/settings_spec.rb | 30 +++++++++++--------------- 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/lib/travis/api/v3/renderer/settings.rb b/lib/travis/api/v3/renderer/settings.rb index 87ad2b8c..b27a8aa7 100644 --- a/lib/travis/api/v3/renderer/settings.rb +++ b/lib/travis/api/v3/renderer/settings.rb @@ -9,10 +9,8 @@ module Travis::API::V3 end def render(settings, **) - { - :@type => 'settings'.freeze, - :settings => settings.to_h - } + response = { '@type' => 'settings'.freeze } + response.merge(settings.to_h) end end end diff --git a/spec/v3/services/settings_spec.rb b/spec/v3/services/settings_spec.rb index c193a7ec..f1c93ff9 100644 --- a/spec/v3/services/settings_spec.rb +++ b/spec/v3/services/settings_spec.rb @@ -40,12 +40,10 @@ describe Travis::API::V3::Services::Settings do example do expect(JSON.load(body)).to eq( '@type' => 'settings', - 'settings' => { - 'builds_only_with_travis_yml' => false, - 'build_pushes' => true, - 'build_pull_requests' => true, - 'maximum_number_of_builds' => 0 - } + 'builds_only_with_travis_yml' => false, + 'build_pushes' => true, + 'build_pull_requests' => true, + 'maximum_number_of_builds' => 0 ) end end @@ -60,12 +58,10 @@ describe Travis::API::V3::Services::Settings do example do expect(JSON.load(body)).to eq( '@type' => 'settings', - 'settings' => { - 'builds_only_with_travis_yml' => false, - 'build_pushes' => false, - 'build_pull_requests' => true, - 'maximum_number_of_builds' => 0 - } + 'builds_only_with_travis_yml' => false, + 'build_pushes' => false, + 'build_pull_requests' => true, + 'maximum_number_of_builds' => 0 ) end end @@ -115,12 +111,10 @@ describe Travis::API::V3::Services::Settings do example do expect(JSON.load(body)).to eq( '@type' => 'settings', - 'settings' => { - 'builds_only_with_travis_yml' => false, - 'build_pushes' => false, - 'build_pull_requests' => true, - 'maximum_number_of_builds' => 20 - } + 'builds_only_with_travis_yml' => false, + 'build_pushes' => false, + 'build_pull_requests' => true, + 'maximum_number_of_builds' => 20 ) end end From 5fee54b91b2e6f599b0933422b5e40288880ad57 Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Thu, 9 Jun 2016 11:29:17 +0200 Subject: [PATCH 4/4] Fewer object allocations :) --- lib/travis/api/v3/models/settings.rb | 20 ++++++++------------ lib/travis/api/v3/renderer/settings.rb | 3 +-- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/lib/travis/api/v3/models/settings.rb b/lib/travis/api/v3/models/settings.rb index 48824816..00949495 100644 --- a/lib/travis/api/v3/models/settings.rb +++ b/lib/travis/api/v3/models/settings.rb @@ -1,5 +1,12 @@ module Travis::API::V3 class Models::Settings + DEFAULTS = { + 'builds_only_with_travis_yml' => false, + 'build_pushes' => true, + 'build_pull_requests' => true, + 'maximum_number_of_builds' => 0 + }.freeze + attr_reader :repository def initialize(repository) @@ -7,7 +14,7 @@ module Travis::API::V3 end def to_h - defaults.merge(repository.settings || {}) + DEFAULTS.merge(repository.settings || {}) end def update(settings = {}) @@ -16,16 +23,5 @@ module Travis::API::V3 settings.each { |k, v| repository.settings[k] = v } repository.save! end - - private - - def defaults - { - 'builds_only_with_travis_yml' => false, - 'build_pushes' => true, - 'build_pull_requests' => true, - 'maximum_number_of_builds' => 0 - } - end end end diff --git a/lib/travis/api/v3/renderer/settings.rb b/lib/travis/api/v3/renderer/settings.rb index b27a8aa7..0eb31152 100644 --- a/lib/travis/api/v3/renderer/settings.rb +++ b/lib/travis/api/v3/renderer/settings.rb @@ -9,8 +9,7 @@ module Travis::API::V3 end def render(settings, **) - response = { '@type' => 'settings'.freeze } - response.merge(settings.to_h) + { '@type' => 'settings'.freeze }.merge!(settings.to_h) end end end