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

Deal better with plan downgrades. #1382

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Deal better with plan downgrades. #1382

wants to merge 1 commit into from

Conversation

dracos
Copy link
Member

@dracos dracos commented Sep 6, 2018

See https://github.com/mysociety/better-cities/issues/19 for full discussion. I keep asking myself if I've missed something obvious but it doesn't seem so. Perhaps you will spot/think of something.
If the logic seem okay, will copy to MapIt and add tests there, probably easier place for them.

@dracos dracos requested a review from struan September 6, 2018 07:51
@dracos dracos force-pushed the api-downgrade branch 2 times, most recently from 847fd42 to c18166e Compare September 6, 2018 08:01
Copy link
Member

@struan struan left a comment

Choose a reason for hiding this comment

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

It could probably do with a bit more documentation but this looks good to me.

if ($form_data['coupon']) {
$this->stripe->coupon = $form_data['coupon'];
} elseif ($this->stripe->discount) {
$this->stripe->deleteDiscount();
}

Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to have a comment in here with either an explanation of what this is for or a link to one because trying to derive it all from code isn't easy :)

} else {
# An upgrade higher than where they started the month - upgrade
# back to the original without proration, then upgrade to new
# plan from there, removing the old maximum_plan.
Copy link
Member

Choose a reason for hiding this comment

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

Would be useful to mention in here that this is because stripe doesn't handle this case so need to do two step.

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.

None yet

2 participants