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.

Sign up for GitHub

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

Jump to bottom

Fixed method post_subscription() w.r.t checking ref of $subscription #68

Closed
wants to merge 4 commits into from
Closed

Fixed method post_subscription() w.r.t checking ref of $subscription #68

wants to merge 4 commits into from

Conversation

manwar
Copy link
Contributor

@manwar manwar commented Dec 6, 2015

Hi Luke, please review the above change.

Many Thanks.
Best Regards,
Mohammad S Anwar

@andrewsolomon
Copy link
Collaborator

Well spotted @manwar! To make sure the code is now doing exactly what we expect and doesn't go missing through a clumsy merge, could you please write a unit test exploiting this fix?

Thanks

Andrew

andrewsolomon
andrewsolomon requested changes Apr 29, 2017
View reviewed changes
Copy link
Collaborator

@andrewsolomon andrewsolomon left a comment

Choose a reason for hiding this comment

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

please add a unit test

@manwar
Copy link
Contributor Author

manwar commented Apr 30, 2017

Hi @andrewsolomon

Please review the proposed changes.

Many Thanks.
Best Regards,
Mohammad S Anwar

andrewsolomon
andrewsolomon reviewed Apr 30, 2017
View reviewed changes
lib/Net/Stripe.pm
return $self->_post("customers/$customer/subscriptions/" . $subscription->id, $subscription);
} elsif (defined($subscription) && !ref($subscription)) {
return $self->_post("customers/$customer/subscriptions/" . $subscription, _defined_arguments(\%args));
if (defined($subscription)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 That's nice and clear!

andrewsolomon
andrewsolomon reviewed Apr 30, 2017
View reviewed changes
t/live.t
@@ -201,7 +201,7 @@ Charges: {
);
} 'Created a charge object with metadata';
isa_ok $charge, 'Net::Stripe::Charge';
ok defined($charge->metadata), "charge has metadata";
ok defined($charge->metadata), "charge has metadata";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for white space clean-up, but in the future please do that as a separate commit so that reverting this change (in case of unexpected problems) won't collide with other code making logical changes in this area.

andrewsolomon
andrewsolomon reviewed Apr 30, 2017
View reviewed changes
lib/Net/Stripe.pm
return $self->_post("customers/$customer/subscriptions/" . $subscription->id, $subscription);
}
else {
die 'subscription should be of type Net::Stripe::Subscription';
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is tested! 👍

andrewsolomon
andrewsolomon reviewed Apr 30, 2017
View reviewed changes
lib/Net/Stripe.pm
if (defined($subscription)) {
if (ref($subscription)) {
if (ref($subscription) eq 'Net::Stripe::Subscription') {
return $self->_post("customers/$customer/subscriptions/" . $subscription->id, $subscription);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't tested 👎

andrewsolomon
andrewsolomon reviewed Apr 30, 2017
View reviewed changes
lib/Net/Stripe.pm
}
}
else {
return $self->_post("customers/$customer/subscriptions/" . $subscription, _defined_arguments(\%args));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't tested 👎

andrewsolomon
andrewsolomon reviewed Apr 30, 2017
View reviewed changes
t/live.t
subscription => $subs_list
);
};
ok $@, "subscription should be of type Net::Stripe::Subscription";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good test, but only of the failure case. Would it be possible to add tests for the two cases with the 👎 above?

@manwar
Copy link
Contributor Author

manwar commented Apr 30, 2017

Hi @andrewsolomon

Added two more test cases as requested.
Please review the changes.

Many Thanks.
Best Regards,
Mohammad S Anwar

@andrewsolomon
Copy link
Collaborator

Thanks @manwar but for some reason this test is failing when I apply your change to master

ok 140 - subscription should be of type Net::Stripe::Subscription
Error: invalid_request_error - Received unknown parameters: id, amount, currency, interval, interval_count, name On parameter: id
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 255 just after 140.
Error: invalid_request_error - Received unknown parameters: id, amount, currency, interval, interval_count, name On parameter: id
Dubious, test returned 255 (wstat 65280, 0xff00)

Would you be so kind as to create a pull request from a new fork of this repo?

Many thanks,

Andrew

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewsolomon andrewsolomon andrewsolomon requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@manwar @andrewsolomon