Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix sub-resource query params bug #22

Merged
merged 8 commits into from

3 participants

@timcraft

I'm trying to use code that looks like this:

gocardless.merchant.bills(source_id: subscription_id)

This is currently broken. The existing implementation of the sub resource methods in GoCardless::Merchant looks correct, but it isn't the code being called. Instead it's using the singleton methods defined in GoCardless::Resource#initialize, which aren't merging the query params correctly.

This pull request fixes the bug, adds a little bit of refactoring, and removes the unnecessary methods from GoCardless::Merchant.

@tomblomfield

Thanks for the contribution @timcraft - we'll have a look at this pull request today and hopefully get it merged.

@timcraft

Cheers Tom. I've tested it against the sandbox, here's some code for reproducing the bug:

require 'gocardless'

GoCardless.environment = :sandbox
GoCardless.account_details = {...}

p GoCardless.client.merchant.bills(source_id: 'xxx')

Filtering the bills resource with a dummy source_id/subscription_id parameter should return an empty array, but it doesn't (assuming the account has some bills).

@hmarr
Owner

I'm looking at this now, Tim. Generally looks great, just pulling down and trying it out.

@hmarr
Owner

Confirmed it does fix the issue, merging. Thanks @timcraft!

@hmarr hmarr merged commit 07a28e9 into gocardless:master
@hmarr
Owner

Included in v1.3.2, just pushed to rubygems /cc @timcraft.

@timcraft

Awesome, thanks :smile:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 30, 2012
  1. @timcraft

    Add failing spec

    timcraft authored
    The functionality defined in GoCardless::Resource#initialize for magically
    creating methods for sub resources is not merging query parameters correctly.
  2. @timcraft
  3. @timcraft
  4. @timcraft
  5. @timcraft
  6. @timcraft

    Eliminate params variable

    timcraft authored
  7. @timcraft

    Remove sub-resource methods from GoCardless::Merchant

    timcraft authored
    These methods cannot be called directly because of the singleton methods
    defined by GoCardless::Resource#initialize.
  8. @timcraft
This page is out of date. Refresh to see the latest.
View
35 lib/gocardless/merchant.rb
@@ -11,40 +11,5 @@ class Merchant < Resource
:pending_balance,
:next_payout_amount
date_accessor :created_at, :next_payout_date
-
- def subscriptions(params = {})
- path = "/merchants/#{self.id}/subscriptions"
- @client.api_get(path, params).map do |attrs|
- GoCardless::Subscription.new_with_client(@client, attrs)
- end
- end
-
- def pre_authorizations(params = {})
- path = "/merchants/#{self.id}/pre_authorizations"
- @client.api_get(path, params).map do |attrs|
- GoCardless::PreAuthorization.new_with_client(@client, attrs)
- end
- end
-
- def users(params = {})
- path = "/merchants/#{self.id}/users"
- @client.api_get(path, params).map do |attrs|
- GoCardless::User.new_with_client(@client, attrs)
- end
- end
-
- def bills(params = {})
- path = "/merchants/#{self.id}/bills"
- @client.api_get(path, params).map do |attrs|
- GoCardless::Bill.new_with_client(@client, attrs)
- end
- end
-
- def payments(params = {})
- path = "/merchants/#{self.id}/payments"
- @client.api_get(path, params).map do |attrs|
- GoCardless::Payment.new_with_client(@client, attrs)
- end
- end
end
end
View
51 lib/gocardless/resource.rb
@@ -3,36 +3,31 @@
module GoCardless
class Resource
def initialize(hash = {})
- # Handle sub resources
- sub_resource_uris = hash.delete('sub_resource_uris')
- unless sub_resource_uris.nil?
- # Need to define a method for each sub resource
- sub_resource_uris.each do |name,uri|
- uri = URI.parse(uri)
-
- # Convert the query string to a hash
- default_query = if uri.query.nil? || uri.query == ''
- nil
- else
- Hash[CGI.parse(uri.query).map { |k,v| [k,v.first] }]
- end
-
- # Strip api prefix from path
- path = uri.path.sub(%r{^/api/v\d+}, '')
+ # Define an object singleton method for each sub resource
+ hash.delete('sub_resource_uris') { [] }.each do |name, uri|
+ uri = URI.parse(uri)
+
+ # Convert the query string to a params hash
+ default_query = if uri.query.nil? || uri.query.empty?
+ {}
+ else
+ Hash[CGI.parse(uri.query).map { |k,v| [k,v.first] }]
+ end
- # Modify the instance's metaclass to add the method
- metaclass = class << self; self; end
- metaclass.send(:define_method, name) do |*args|
- # 'name' will be something like 'bills', convert it to Bill and
- # look up the resource class with that name
- class_name = Utils.camelize(Utils.singularize(name.to_s))
- klass = GoCardless.const_get(class_name)
+ # Strip api prefix from path
+ path = uri.path.sub(%r{^/api/v\d+}, '')
+
+ # Modify the instance's metaclass to add the method
+ metaclass = class << self; self; end
+ metaclass.send(:define_method, name) do |*args|
+ # 'name' will be something like 'bills', convert it to Bill and
+ # look up the resource class with that name
+ class_name = Utils.camelize(Utils.singularize(name.to_s))
+ klass = GoCardless.const_get(class_name)
+ query = default_query.merge(args.first || {})
+ client.api_get(path, query).map do |attrs|
# Convert the results to instances of the looked-up class
- params = args.first || {}
- query = default_query.nil? ? nil : default_query.merge(params)
- client.api_get(path, query).map do |attrs|
- klass.new(attrs).tap { |m| m.client = client }
- end
+ klass.new_with_client(client, attrs)
end
end
end
View
31 spec/merchant_spec.rb
@@ -1,31 +0,0 @@
-require 'spec_helper'
-
-describe GoCardless::Merchant do
- before :each do
- @app_id = 'abc'
- @app_secret = 'xyz'
- @client = GoCardless::Client.new(:app_id => @app_id, :app_secret => @app_secret)
- @client.access_token = 'TOKEN123 manage_merchant:123'
- @redirect_uri = 'http://test.com/cb'
- end
-
- index_methods = [:subscriptions, :pre_authorizations, :users, :payments, :bills]
-
- index_methods.each do |method|
- it "##{method} works correctly" do
- merchant = GoCardless::Merchant.new_with_client(@client)
-
- data = [{:id => 1}, {:id => 2}]
- stub_get(@client, data)
-
- merchant.send(method).should be_a Array
- merchant.send(method).length.should == 2
- merchant.send(method).zip(data).each do |obj,attrs|
- class_name = GoCardless::Utils.camelize(method.to_s).sub(/s$/, '')
- obj.class.to_s.should == "GoCardless::#{class_name}"
- attrs.each { |k,v| obj.send(k).should == v }
- end
- end
- end
-end
-
View
14 spec/resource_spec.rb
@@ -376,7 +376,7 @@ class UpdatableResource < GoCardless::Resource
r.bills
end
- it "adds provided params to query string params" do
+ it "adds provided params to existing query string params" do
client = mock()
params = { 'merchant_id' => '1', :amount => '10.00' }
client.expects(:api_get).with(anything, params).returns([])
@@ -384,6 +384,18 @@ class UpdatableResource < GoCardless::Resource
r.bills(:amount => '10.00')
end
+ it "adds provided params when there are no existing query string params" do
+ client = mock()
+ params = { :source_id => 'xxx' }
+ client.expects(:api_get).with(anything, params).returns([])
+ r = @test_resource.new_with_client(client, {
+ 'sub_resource_uris' => {
+ 'bills' => 'https://test.com/merchants/1/bills'
+ }
+ })
+ r.bills(:source_id => 'xxx')
+ end
+
it "return instances of the correct resource class" do
client = stub(:api_get => [{:id => 1}])
r = @test_resource.new_with_client(client, @attrs)
Something went wrong with that request. Please try again.