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

Flexible/Metered Billing API support #460

Merged
merged 1 commit into from
Apr 11, 2018

Conversation

alexander-stripe
Copy link
Contributor

Implement support for flexible billing options on plans and creating usage records.

@alexander-stripe
Copy link
Contributor Author

The failing tests also seems unrelated to the changes. Might this be a similar problem as in stripe/stripe-ruby#634 and stripe/stripe-mock#56 ?

cc @brandur-stripe

@brandur-stripe
Copy link
Contributor

@alexander-stripe I think at least one of the test failures is related to your changes:

There was 1 error:
1) Stripe\UsageRecordTest::testIsCreatable
Error: Class 'Stripe\UsageRecord' not found
/home/travis/build/stripe/stripe-php/tests/Stripe/UsageRecordTest.php:15
--
There was 1 failure:
1) Stripe\AccountTest::testCanDeleteExternalAccount
Failed asserting that Stripe\StripeObject Object (...) is an instance of class "Stripe\BankAccount".
/home/travis/build/stripe/stripe-php/tests/Stripe/AccountTest.php:160
FAILURES!
Tests: 331, Assertions: 709, Errors: 1, Failures: 1.

Would you mind taking a quick peek at that failing account test while you're at it? You may just have to change Stripe\BankAccount to Stripe\Card or something.

@@ -43,6 +43,58 @@ public function testIsCreatable()
$this->assertInstanceOf("Stripe\\Plan", $resource);
}

public function testIsCreatableMetered()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure these tests are needed -- there is already a test for the create method and the library itself doesn't care about the request parameters, so the only thing being tested here is that the parameters provided in the tests pass stripe-mock's validation.

@brandur-stripe wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to you @ob-stripe — it doesn't hurt I guess, but agreed that it's redundant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not have the tests then, to keep things consistent in the test suite.

public static function create($params = null, $options = null)
{
self::_validateParams($params);
$subscription_item = $params['subscription_item'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe do the same thing that you did in stripe-java? I.e. add a check and throw \Stripe\Error\InvalidRequest exception if the parameter is missing.

@alexander-stripe alexander-stripe force-pushed the alexander/flexible-billing branch from 4d0e8c9 to 5e0e3c7 Compare April 10, 2018 21:55
@alexander-stripe alexander-stripe force-pushed the alexander/flexible-billing branch from 5e0e3c7 to 67e8c4f Compare April 11, 2018 20:30
@brandur-stripe
Copy link
Contributor

Feedback addressed! Pulling this in.

@brandur-stripe brandur-stripe merged commit 1a8adb9 into master Apr 11, 2018
@brandur-stripe brandur-stripe deleted the alexander/flexible-billing branch April 11, 2018 20:55
@brandur-stripe
Copy link
Contributor

Released as 6.6.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants