Skip to content

Change subscription #21

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

Open
wants to merge 13 commits into
base: development
Choose a base branch
from
Open

Change subscription #21

wants to merge 13 commits into from

Conversation

idelahoz
Copy link
Contributor

@idelahoz idelahoz commented May 8, 2015

This PR adds a settings page for the team, Right now the settings page only has an option for changing a team's plan.

Asana task: https://app.asana.com/0/30343922347920/33619659511433

The page has a credit card form, is the user changes to a paid plan a no credit card is entered, an error will be returned.

@begedin
Copy link
Contributor

begedin commented May 13, 2015

@idelahoz I merged all of my stuff, so I also merged development into your branch to catch it up with the current state of the repo.

You'll notice there's a new route - team.manage as well as a new nav link in the team sidebar that links to it. Since you added a team.settings route for basically the same thing, feel free to merge the two. Doesn't really matter if it's "settings" or "manage".

Keep in mind, I made my route only available to team owners. I'm guessing you'll want to do the same, but I'm not sure.

callback();
};
Stripe.card.createToken($form, stripeCallback);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this work here?

return new Ember.RSVP.Promise(function(resolve) {
  Stripe.card.createToken($form, function(status, response) {
    controller.set('cardToken', response.id);
    resolve();
  });
});

Then in the createPlan action, you could have

 this.createStripeToken().then(this.requestPlanChange)

or

var controller = this;
controller.createStripeToken().then(function() {
  this.requestPlanChange
});

, whichever works.

Also, if you move the form and related logic to a component, I think it would be more nicely abstracted, though I think something like that is big enough for a refactor task of it's own.

},
changePlan: function(){
if(this.get('currentPlanIsPaid')){
this.createStripeToken().then(this.requestPlanChange.bind(this));
Copy link

Choose a reason for hiding this comment

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

Let us stay away from using bind. It makes the code more confusing. Also I don't think phantomjs supports it.

@venkatd
Copy link

venkatd commented May 19, 2015

@idelahoz

},
changePlan: function(){
var controller = this;
if(this.get('currentPlanIsPaid')){
Copy link

Choose a reason for hiding this comment

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

This doesn't follow the what do I want principle because you just want to be able to call requestPlanChange directly.

@begedin
Copy link
Contributor

begedin commented May 22, 2015

@idelahoz I merged my stuff and then merged development in to here to get it up to date and modified some tests to make them pass.

Only slight problem - there's a console warning that stripe.js gets loaded more than once. Not sure if you encountered it or if you want to try and get rid of it at this point or later.

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

Successfully merging this pull request may close these issues.

3 participants