masteringmodernpayments / book

Discussion group for Mastering Modern Payments
https://www.masteringmodernpayments.com
6 stars 2 forks source link

Discuss: Subscriptions #9

Open peterkeen opened 10 years ago

peterkeen commented 10 years ago
romanbsd commented 10 years ago

In this chapter you have the following code:

    begin
      Stripe::Plan.create(
        id: options[:stripe_id],
        amount: options[:amount],
        currency: 'usd',
        interval: options[:interval],
        name: options[:name],
      )
    rescue Stripe::StripeError => e
      subscription.errors[:base] << e.message
    end

However there's no subscription variable in scope. I guess that you copied the rescue clause from the previous example. However, changing subscription to plan here won't have much effect, as the call to save! below will wipe the errors object...

peterkeen commented 10 years ago

Thanks @romanbsd. I'll have that fixed in the next version. In the mean time, you could change it to plan like you said, and then change the save! to plan.save if plan.valid?.

abinofbrooklyn commented 10 years ago

In you subscription/index.html.erb

<% @plans.each do |plan| %>
  <%= link_to "#{plan.name} (#{plan.price})",
        new_subscription_path(@plan) %>
<% end %>

Shouldn't it be (#{plan.amount}) instead of (#{plan.price})?

peterkeen commented 10 years ago

Thanks. Yep, you're right. Really it should be formatted_price(plan.amount).

bannio commented 10 years ago

In your invoice.payment_succeeded webhook event for creating an InvoicePayment you seem to use invoice.amount but I can't see that attribute in the Stripe invoice object. I can't see invoice.items for that matter. What am I missing?

peterkeen commented 10 years ago

Yep, that's completely busted. According to the latest Stripe docs it looks like those should be invoice.total and invoice.lines.data.select { |line| line.type == 'subscription' }.first.id.

I'll have it fixed in the next update.

On Tue, Nov 4, 2014 at 11:43 AM, Owen Bannister notifications@github.com wrote:

In your invoice.payment_succeeded webhook event for creating an InvoicePayment you seem to use invoice.amount but I can't see that attribute in the Stripe invoice object. I can't see invoice.items for that matter. What am I missing?

— Reply to this email directly or view it on GitHub https://github.com/masteringmodernpayments/book/issues/9#issuecomment-61670741 .

marcjeanson commented 10 years ago

Why loop over the invoice.lines to get the subscription id, when you can just do: invoice.subscription?

DanielKehoe commented 10 years ago

This should be a link: "Andrew Culver is the author of [Koudoku][subscriptions-koudoku]".

Same for: "According to [Patrick McKenzie][subscriptions-patio11-rainy-day]".

Also: "Stripe has a feature they call [Invoices][subscriptions-stripe-invoices]".

striking commented 10 years ago

Is your SubscriptionsController missing a close parenthesis?

def create
    @plan = Plan.find(params[:plan_id]
    @subscription = CreateSubscription.call(
      @plan,
      params[:email_address],
      params[:stripeToken]
    )
    if @subscription.errors.blank?
      flash[:notice] = 'Thank you for your purchase!' +
        'Please click the link in the email we just sent ' +
        'you to get started.'
      redirect_to '/'
    else
      render :new
    end
  end
peterkeen commented 10 years ago

@striking Fixed, and I added a basic linter to my build process so stupid syntax errors don't creep in anymore.

weavermedia commented 10 years ago

CreatePlan paragraph text reads:

"If everything goes well, it then saves our new plan and returns it."

However the CreatePlan service object will currently save the plan to the database whether or not the creation of the plan on Stripe is successful.

class CreatePlan
  def self.call(options={})
    plan = Plan.new(options)

    if !plan.valid?
      return plan
    end

    begin
      Stripe::Plan.create(
        id: options[:stripe_id],
        amount: options[:amount],
        currency: 'usd',
        interval: options[:interval],
        name: options[:name],
      )
    rescue Stripe::StripeError => e
      plan.errors[:base] << e.message
    end

    plan.save

    return plan
  end
end

Shouldn't the content of the rescue block just return plan rather than update errors?

peterkeen commented 10 years ago

Yep, good catch. I'll have that in the next update. Thanks!

weavermedia commented 10 years ago

I'm writing a subscription feature for a multi-tenanted app where each tenant just has one plan (Small, Medium or Large, based on number of widgets for sale).

Although this is simpler than your feature in this chapter I'm having trouble paring the code down to fit.

For instance I don't have a Subscription model but I do have a SubscriptionsController which just has a create action that creates a Stripe customer via API and adds the plan_id to the tenant record. But with a @subscription object for the view I'm not able to pass errors back from the CreateCustomer service back to the controller.

Maybe it would be better to just add a subscribe action to my TenantsController?

Any tips?

peterkeen commented 10 years ago

@weavermedia I understand wanting to pare down the code in the book, but it's all there for a reason. The book talks about a Subscription model because it makes it possible to query things like subscription expiration dates or other subscription data within your database instead of calling out to Stripe every time you need to know.

That said, if you really want to skip putting subscription stuff in the database, I would suggest passing an OpenStruct back from CreateSubscription that looks like a model object, so you can pass errors and status back to the controller / view.

weavermedia commented 10 years ago

The code you have in the book is excellent and you make a good point for persisting subscription data, but for right now my needs are dead simple. I realise a more experienced developer would've been able to write a simpler implementation right away from your code.

Using an OpenStruct is a great idea for passing errors back to the controller. Thanks.

MarkMurphy commented 9 years ago

Currious...why go through the subscription to update the card? Why not directly through the stripe customer?

So instead of:

class ChangeSubscriptionCard
  def self.call(subscription, token)
    begin
      user = subscription.user
      customer = Stripe::Customer.retrieve(user.stripe_customer_id)
      stripe_sub = customer.subscriptions.retrieve(subscription.stripe_id)

      stripe_sub.card = token
      stripe_sub.save!
    rescue Stripe::StripeError => e
      subscription.errors[:base] << e.message
    end

    subscription
  end
end

this:

class ChangeCard
  def self.call(user, token)
    begin
      customer = Stripe::Customer.retrieve(user.stripe_customer_id)
      customer.card = token
      customer.save!
    rescue Stripe::StripeError => e
      user.errors[:base] << e.message
    end

    user
  end
end
MarkMurphy commented 9 years ago

I'd also like to see some examples in the Subscriptions controller for upgrading and downgrading which makes use of the ChangePlan service object. I'm curious to see how you would organize those actions in the controller i.e. would you combine them into one edit/update action or would you create separate upgrade and downgrade actions?

peterkeen commented 9 years ago

@MarkMurphy both good points. I'll work on addressing them in the next version, but in brief: you're right, updating the customer's card is more straight forward and is probably the right way to go. For changing plans, I would probably just have it as one action that can take a plan as a query param.

lgs commented 9 years ago

@peterkeen

Is "Complete Package" code book build with or without Payola?

I'm facing some difficulties fitting payola subscriptions with the book workflow, then should be great if the code is based on payola instead of plan stripe.

MarkMurphy commented 9 years ago

When you're creating the database tables for plans and subscriptions you should probably index the stripe_id columns

taylorscollon commented 9 years ago

Has anyone had luck implementing the upgrade service? I can't seem to get the controller right...

ciaranha commented 9 years ago

At the point where it says to create a plan from the console with: irb(main):001:0> CreatePlan.call(stripe_id: 'test_plan', name: 'Test Plan', amount: 500, interval: 'month', description: 'Test Plan', published: false)

My console is returning NameError: uninitialized constant CreatePlan from (irb):1 from /Users/ciaranhanrahan/.rvm/rubies/ruby-2.1.1/bin/irb:11:in

'.

Any ideas what I might have done wrong up to this point?

peterkeen commented 9 years ago

@Jaibles did you put the CreatePlan service in /app/services/create_plan.rb?

ciaranha commented 9 years ago

Yea I have screen shot 2015-01-27 at 15 42 01

class CreatePlan
  def self.call(options={})
    plan = Plan.new(options)

    if !plan.valid?
      return plan
    end

    begin
      Stripe::Plan.create(
        id: options[:stripe_id],
        amount: options[:amount],
        currency: 'usd',
        interval: options[:interval],
        name: options[:name],
      )
    rescue Stripe::StripeError => e
      plan.errors[:base] << e.message
      return plan
    end

    plan.save

    return plan
  end
end

(thanks for getting back so promptly!)

MarkMurphy commented 9 years ago

@Jaibles Did you add the services folder to your autoload paths in your rails config?

peterkeen commented 9 years ago

@MarkMurphy thanks, that was going to be my next question.

MarkMurphy commented 9 years ago

@Jaibles

# config/application.rb
...
class Application < Rails::Application
    ...

    # Custom directories with classes and modules you want to be autoloadable.
    config.autoload_paths += %W(#{config.root}/app/services)

end
MarkMurphy commented 9 years ago

@Jaibles what command are you using to start the console: irb or rails c the prompt looks like you are using irb which doesn't load your rails environment.

ciaranha commented 9 years ago

@MarkMurphy I've actually tried both. At first I was using rails c (which I now know I should be using) returns: CreatePlan.call(stripe_id: 'test_plan', name: 'Test Plan', amount: 500, interval: 'month', description: 'Test Plan', published: false) PG::UndefinedTable: ERROR: relation "plans" does not exist LINE 5: WHERE a.attrelid = '"plans"'::regclass ^ : SELECT a.attname, format_type(a.atttypid, a.atttypmod), pg_get_expr(d.adbin, d.adrelid), a.attnotnull, a.atttypid, a.atttypmod FROM pg_attribute a LEFT JOIN pg_attrdef d ON a.attrelid = d.adrelid AND a.attnum = d.adnum WHERE a.attrelid = '"plans"'::regclass AND a.attnum > 0 AND NOT a.attisdropped ORDER BY a.attnum

MarkMurphy commented 9 years ago

@Jaibles now you're getting somewhere. This is an altogether different error...I'd be happy to help but this conversation should be moved elsewhere at this point. How about creating a gist or posting on stack overflow. Post a link back here and I'll try to help you out.

ciaranha commented 9 years ago

Thanks @MarkMurphy really appreciate it https://gist.github.com/Jaibles/1a18ed57d9079aced43b

jonskirk commented 9 years ago

Hi - I'm having some trouble with my CreatePlan service ... I put it in app/services, added the path to the autoload_paths, and am able to run the call method from a test. I have Stripe set up, and was able to make the basic integration work no problem. However, running the call method in the service fails with this error:

uninitialized constant Stripe::Product

Any clues why it's not seeing Stripe?

MarkMurphy commented 9 years ago

@jonskirk can you create a gist with your service code and link back to it here.

jonskirk commented 9 years ago

@MarkMurphy - thanks for the quick response! Here's a gist with the service and the test that's failing:

https://gist.github.com/jonskirk/a11749b358833ec4234b

jonskirk commented 9 years ago

Ah, very sorry - simple cut/paste error. My model is called Product, not Plan, and I managed to change Stripe::Plan from the example to Stripe::Product ...

MarkMurphy commented 9 years ago

@jonskirk does that mean you've resolved the issue you were having?

jonskirk commented 9 years ago

@MarkMurphy yes, it does. I spent a while on it before posting, but it was one of those not seeing the wood for the forest issues ... Thanks!

anayib commented 9 years ago

Hi,

Has anyone integrated the subscription form with the devise model to give access to the app only to those users that have already payed?.

Im trying to integrate the devise signup form with what we have in the subscription chapter of the book.

any insights?

cheers

tyranni commented 9 years ago

Hi @everyone :) I set up the subscription thing following the book example and everything woks fine :). Customers provide their credit card and some other details like email address ... After that they are subscribed to a plan and get charged. All as expected. Now I would like to add a free plan which customers can subscribe to without any obligations. Could anyone please push me in the right direction? What do I have to change in the service objects to make it possible to subscribe to a free plan whithout providing a credit card? Only setting the amount of a plan to 0 (like the stripe docs tell us) or changing the custom form doesn't seem to be enough ;). Thanks for any hint and help!

p.s.: great book, pete! thank you!!!

anayib commented 9 years ago

Hi tyranii, sorry Im one steop behind and can not help you with that.

On the other hand I set all up and when I fill the form with testing credit cards numbers I always get this message: "This customer has no attached card"

And nothing else happens. Does anyone know why and what can I do to test if it its working well?. I got no redirect to another page neither a confirmation with the always accepted code: 4242424242424242

I really appreciate any help,

cheers,

tyranni commented 9 years ago

Hey anayib :) Thanks for answering. I solved my little problem in the meantime. The service objects were all ok and there was nothing which needed to be changed. It was 'Stripe.card.createToken($form, stripeResponseHandler);' in the jquery function which, of course, produced an error when there was no credit card provided through the custom form.

Now to your case. Could it be that you placed your form in a bootstrap modal / popover? Then disabling turbolinks should help. greetings :)

anayib commented 9 years ago

Hi tyranni,

Thanks for the suggestion. I just figure out how to keep turbolinks and not affect the js functionality. Instance of adding the traditional code to the subscription.js file, you can add this one (I just put at the beginning $(document).on("page:change" ... so that we dont have conflict with turbolinks. The code below:

$(document).on("page:change", function() { $('#payment-form').submit(function(event) { var $form = $(this);

$form.find('button').prop('disabled', true);

Stripe.card.createToken($form, stripeResponseHandler);

return false;

}); });

function stripeResponseHandler(status, response) { var $form = $('#payment-form'); if (response.error) { // Show the errors on the form $form.find('.payment-errors').text(response.error.message); $form.find('button').prop('disabled', false); } else { // response contains id and card, which contains additional card details var token = response.id; // Insert the token into the form so it gets submitted to the server $form.append($('').val(token)); // and submit $form.get(0).submit(); } };

anayib commented 9 years ago

Hi everyone:

Im having problems rendering all the plans I have already created for a stripe subscription. I already created 3 plans but the index view containing the following code just shows one plan app/views/subscriptions/index.html.erb . Below the code I put in the index view

<%@plans.each do |plan| %> <%= link_to "#{plan.name} (#{formatted_price(plan.amount)})", new_subscription_path(plan_id: plan.id) %> <% end %>

Does anyone has any idea about what I need to do to show all the plans I have already created?

2.1.5 :004 > Plan.count (0.8ms) SELECT COUNT(*) FROM "plans" => 3 2.1.5 :005 >

Thanks in advance

weavermedia commented 9 years ago

@anayib how are you setting @plans in your subscriptions controller index action?

Something like this?

# subscriptions_controller.rb
def index
  ...
  @plans = Plan.all
  ...
end

Try adding a simple debug line to to your index.html to show all the plans so you can see exactly what's been set for each one:

<%= @plans %>

anayib commented 9 years ago

@weavermedia thanks a lot for your response. Your comment makes me realise that it should be @plans= Plan.all in the controller and I had @plan = Plan.all

Problem solved! ...

Thanks!

maxrosecollins commented 9 years ago

Where can I find the sample code for this part of the book? When I download the code I have an app that looks completely different to the one described in the book, it has an admin section... no app/service folder, etc?

peterkeen commented 9 years ago

There is currently no example code to download for that part of the book. It's at the top of my MMP todo list.

On Thu, Feb 19, 2015 at 11:17 AM, Max Rose-Collins <notifications@github.com

wrote:

Where can I find the sample code for this part of the book? When I download the code I have an app that looks completely different to the one described in the book, it has an admin section... no app/service folder, etc?

— Reply to this email directly or view it on GitHub https://github.com/masteringmodernpayments/book/issues/9#issuecomment-75081106 .

maxrosecollins commented 9 years ago

okay no problem. What is the code you can download?

peterkeen commented 9 years ago

That is the application that sold MMPv1, from June 2013 through October 2014.

On Thu, Feb 19, 2015 at 11:22 AM, Max Rose-Collins <notifications@github.com

wrote:

okay no problem. What is the code you can download?

— Reply to this email directly or view it on GitHub https://github.com/masteringmodernpayments/book/issues/9#issuecomment-75082202 .

maxrosecollins commented 9 years ago

ah right okay. Thanks!