Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nested flag #326

Merged
merged 2 commits into from
Jul 21, 2017
Merged

Add nested flag #326

merged 2 commits into from
Jul 21, 2017

Conversation

g30rg
Copy link

@g30rg g30rg commented Jul 21, 2017

it is my first contribution to this repo, but it looks like creating a test in one commit (for the use case i raised bug #325 for) and fix it the follow up commit seems suitable.

all tests pass for me

please let me know whether i need to or can improve this contribution in any way to get this PR accepted

thanks!

@drewish
Copy link

drewish commented Jul 21, 2017

The test case is super helpful. Yeah definitely looks like a serialization bug. I'd like to simplify down the test case a little bit to focus on the issue:

  public function testCreateSubscripionWithExistingAccountXml() {
    $this->client->addResponse('GET', "/accounts/abcdef1234567890", 'accounts/create-201.xml');
    $account = Recurly_Account::get('abcdef1234567890', $this->client);

    $subscription = new Recurly_Subscription();
    $subscription->plan_code = 'gold';
    $subscription->quantity = 1;
    $subscription->currency = 'USD';
    $subscription->account = $account;

    $this->assertEquals("<?xml version=\"1.0\"?>\n<subscription><account><account_code>abcdef1234567890</account_code></account><plan_code>gold</plan_code><quantity>1</quantity><currency>USD</currency><subscription_add_ons></subscription_add_ons></subscription>\n", $subscription->xml());
  }

I'm going to go ahead and merge your code then clean up the test and get a release out. Thanks a ton for finding and fixing this.

@drewish drewish merged commit ddf4006 into recurly:master Jul 21, 2017
@drewish drewish mentioned this pull request Jul 21, 2017
@g30rg g30rg deleted the add_nested_flag branch July 21, 2017 19:17
@g30rg
Copy link
Author

g30rg commented Jul 21, 2017

i'm glad i was able to help! my bad, the account creation indeed is rather superfluous here. thanks for correcting the test-case and thanks for merging the fix!

@drewish
Copy link

drewish commented Jul 21, 2017

2.8.2 is out so let me know if you have any further issues. Thanks again.

g30rg added a commit to g30rg/recurly-client-php that referenced this pull request Jul 24, 2017
@g30rg g30rg mentioned this pull request Jul 24, 2017
@bhelx bhelx added the V2 V2 Client label Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V2 V2 Client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants