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

Add charge parameter to subscription termination functions #374

Merged
merged 1 commit into from
Oct 19, 2018
Merged

Add charge parameter to subscription termination functions #374

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions lib/recurly/subscription.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,34 +127,39 @@ public function updateAtRenewal() {
/**
* Terminate the subscription immediately and issue a full refund of the last renewal
*
* @param bool $charge If true, unbilled usage is billed on final invoice, else negative usage record created to zero out final invoice.
* @throws Recurly_Error
*/
public function terminateAndRefund() {
$this->terminate('full');
public function terminateAndRefund($charge = true) {
Copy link

Choose a reason for hiding this comment

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

I'm wondering if there's other ways we could pass in this parameter... having unnamed Boolean parameters doesn't make for very readable code. Having to pass in an array with the options is more verbose but we'd be able to have a understandable bit of code:

$subscription->terminateAndRefund(array('charge_for_unbilled_usage' => false))

vs

  $subscription->terminateWithoutRefund(false);

Copy link
Author

Choose a reason for hiding this comment

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

I did some research, and named parameters have apparently been a subject of debate in the PHP world. Since they don't exist at the moment, we basically have two options:

  1. Do your suggestion of passing in an options array. This has the benefit of making it more explicit, but you lose the ability to specify the type of the option, so you have to do some validation within the function. At that point, would we throw an exception if charge_for_unbilled_usage was not a boolean? Would we even check for the type, or would we just check for its presence and if it's not falsey, assume that it's meant to be true? It's hard because the default by the API is true and not false, so I'm not a huge fan of this solution even though it's better than what I have now.

  2. We could create class constants that evaluate to true or false so that people can pass in something that makes it more clear. So the API would look like

$subscription->terminateWithoutRefund(Recurly_Subscription::CHARGE_FOR_UNBILLED_USAGE);

and

$subscription->terminateWithoutRefund(Recurly_Subscription::NO_CHARGE_FOR_UNBILLED_USAGE);

I'm open to suggestions on the naming of these class constants should we go this route.

The nice thing about option number 2 is that it leaves it up to the programmer as to whether they want to use our class constants or not, but the validation of what's being passed in is still super simple.

Oh, and there's always the secret option number 3, which is just forget about the 3 existing terminateAnd... methods, and make the terminate() method public, allowing people to pass in what they like as the refundType and charge variables. But this feels like moving backwards.

Copy link

Choose a reason for hiding this comment

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

Your point about secret option 3 actually hints at another option, we could just make that part of the function signatures: terminateAndPartialRefundIgnoreUnbilledUsage... that could get too crazy but is an option.

Copy link
Author

Choose a reason for hiding this comment

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

Well, the benefit of doing that would be that we wouldn't break anything for anyone, no one would have mysterious unreadable booleans being passed into the method, and people have to explicitly choose to go against the default...

I don't think including those would be any crazier than option number 2, but I like that it keeps with the pattern we already had.

Shall I go ahead and change over to that pattern?

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 prefer keeping it simple and leaving it as a simple boolean parameter. The documentation should suffice for the small percentage of people who do not want to use the default of true. If we believe we may expand beyond this one parameter then we might want to consider another option.

$this->terminate('full', $charge);
}
/**
* Terminate the subscription immediately and issue a prorated/partial refund of the last renewal
*
* @param bool $charge If true, unbilled usage is billed on final invoice, else negative usage record created to zero out final invoice.
* @throws Recurly_Error
*/
public function terminateAndPartialRefund() {
$this->terminate('partial');
public function terminateAndPartialRefund($charge = true) {
$this->terminate('partial', $charge);
}
/**
* Terminate the subscription immediately without a refund
*
* @param bool $charge If true, unbilled usage is billed on final invoice, else negative usage record created to zero out final invoice.
* @throws Recurly_Error
*/
public function terminateWithoutRefund() {
$this->terminate('none');
public function terminateWithoutRefund($charge = true) {
$this->terminate('none', $charge);
}

/**
* @param $refundType Types include none, partial, or full refund processed on the subscription charges
* @param string $refundType Types include none, partial, or full refund processed on the subscription charges
* @param bool $charge If true, unbilled usage is billed on final invoice, else negative usage record created to zero out final invoice.
* @throws Recurly_Error
*/
private function terminate($refundType) {
$this->_save(Recurly_Client::PUT, $this->uri() . '/terminate?refund=' . $refundType);
private function terminate($refundType, $charge) {
$chargeString = ($charge) ? 'true' : 'false';
$this->_save(Recurly_Client::PUT, $this->uri() . '/terminate?refund=' . $refundType . '&charge=' . $chargeString);
}

/**
Expand Down