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

Reduce simplest use case from two API calls to one #492

Open
steveklabnik opened this issue Jan 23, 2014 · 35 comments
Open

Reduce simplest use case from two API calls to one #492

steveklabnik opened this issue Jan 23, 2014 · 35 comments

Comments

@steveklabnik
Copy link
Contributor

TL;DR: Stripe offers the ability to charge a card in one step:

# Set your secret key: remember to change this to your live secret key in production
# See your keys here https://manage.stripe.com/account
Stripe.api_key = "sk_test_BQokikJOvBiI2HlWgH4olfQ2"

# Get the credit card details submitted by the form
token = params[:stripeToken]

# Create the charge on Stripe's servers - this will charge the user's card
begin
  charge = Stripe::Charge.create(
    :amount => 1000, # amount in cents, again
    :currency => "usd",
    :card => token,
    :description => "payinguser@example.com"
  )
rescue Stripe::CardError => e
  # The card has been declined
end

https://stripe.com/docs/tutorials/charges

We require two:

require 'balanced'
Balanced.configure('ak-test-3ndxkwi2d8Gb4E15emwEbwLkEE3K4naM')

card = Balanced::Card.fetch('/cards/CC8VE3J2LlBelNryyr0Rmuk')
card.debit(
  :description => 'Some descriptive text for the debit in the dashboard',
  :amount => 5000,
  :appears_on_statement_as => 'Statement text'
)

I'd like to make us only require one, if someone wants to simply charge a card for a one-off purchase.

@mahmoudimus
Copy link
Contributor

@steveklabnik how would this work in the ruby api?

# presumably done by some Rails app config
require 'balanced'
Balanced.configure('ak-test-3ndxkwi2d8Gb4E15emwEbwLkEE3K4naM')

....meanwhile...in the app code

# ... some code..
card = Balanced::Card.debit(
     :href => request_href, 
     :amount => 5000,
     :description => 'Some descriptive text for the debit in the dashboard',
     :appears_on_statement_as => 'Statement text'
)

Right?

That means we should modify https://github.com/balanced/balanced-api/blob/cukes/fixtures/_models/token.json#L4 to return a 'card.debits' href right?

@mjallday
Copy link
Contributor

If we allow passing a source param for the root debits.create we could do

import balanced
balanced.configure('xxx')
card_uri = '/cards/XXXX'
debit = balanced.Debit(source=card_uri, amount=100).save()

Currently the root debit method expects an untokenized card payload. This change would mean we don't need to update the spec etc.

@matthewfl
Copy link
Contributor

I feel that using the api client, you can write this as:

Balanced::Card.fetch('/cards/asdfasdf').debit(:amount => 1234)

And therefore from a visual perspective of the code it is not that bad

if you really want to debit a card in a single pass, then you can do:

http.post(url + '/debits', {amount: 1234})

@mahmoudimus
Copy link
Contributor

@mjallday no - since you have to know to append a /debits at the end of that.

@mjallday
Copy link
Contributor

@matthewfl latency.

@matthewfl
Copy link
Contributor

@mjallday the idea in rev1, was we were going to try and avoid having multiple routes for preforming actions.

if latency is an issue, you can just add '/debits' to the url and perform a post manually

@mahmoudimus
Copy link
Contributor

@matthewfl @mjallday in this case, for some reason, we are asking our customers to KNOW our URI structure. I disagree with this.

@matthewfl
Copy link
Contributor

@mahmoudimus I agree with this statement (about our url structure), however I feel as if our api should be fast enough where the double request issue is not issue.

@steveklabnik
Copy link
Contributor Author

the idea in rev1, was we were going to try and avoid having multiple routes for preforming actions.

This is not important, especially for a convenience method like this.

@steveklabnik
Copy link
Contributor Author

It is a marketing issue. Even if our API is fast enough, that's not a solution.—
Sent from Mailbox for iPhone

On Thu, Jan 23, 2014 at 11:50 AM, Matthew Francis-Landau
notifications@github.com wrote:

@mahmoudimus I agree with this statement (about our url structure), however I feel as if our api should be fast enough where the double request issue is not issue.

Reply to this email directly or view it on GitHub:
#492 (comment)

@matthewfl
Copy link
Contributor

@steveklabnik then we might as well add a method to all api clients that construct the url internally and preform a post request. This accomplishes having the api be clean and allowing people to preform the debit in a single action

@mjallday
Copy link
Contributor

@matthewfl that will mean people need to know about this if they want to write their own client.

@mahmoudimus
Copy link
Contributor

Why does this "dirty" up the API, @matthewfl ?

We're just adding a link that comes back (which is cheap). Any construction is a smell to me.

[Edit] Clarifying "this"

@matthewfl
Copy link
Contributor

When we wrote the first spec for this version of the api, we decided to leave out this feature as we looked at all the different ways to preform the same action in rev0 and decided we wanted to simplify it and minimize the size of the interface.

In my mind, I feel that doing Card.find(uri).debit(1234) should be the defacto way of preforming a debit. Now, I am not saying that the api client needs to do 2 api calls. In fact, if the api client is able to return some proxy object and optimize this into one api call, that is fine by me. However I do not think that there should be two different methods of creating a debit, where one is "faster."

@mahmoudimus
Copy link
Contributor

@matthewfl that's not true.

In this case, it's about features. If there is no way of asking our API to charge a card w/o verifying its state, that's upsetting.

Why are you FORCING me to get the card first even if I don't care?

@matthewfl
Copy link
Contributor

afaik, the Card.find method just returns some object representing a Card, in my mind, there is no requirement that the object has to make a request to our api.

basically I am +1 for making the making the api "faster" by allowing more direct, but I am -1 for increasing the size of the interface of the clients/api. This means that doing Card.find(uri).debit(1234) can do 100 requests or do 1 requests, and in using our api libraries, that should be unknown to me. Now if someone asks on say irc about doing a debit with one request, I would rather tell them that by chaining the request as Card.find(uri).debit(1234) will do the right then then saying we have a special method just for this specific use case.

@mahmoudimus
Copy link
Contributor

@matthewfl you should assume that we are not in charge of client libraries. You should be under the impression that you don't know how they're implemented as an API maintainer.

Forcing consumers to work around the API via client hacks puts a burden of keeping up to date on our endpoints (antipattern no. 1). It also opens up a vector of workarounds for the API -- asking consumers to work around our unfriendliness towards mobile or bandwidth constrained clients.

@matthewfl
Copy link
Contributor

and in using our api libraries

@steveklabnik
Copy link
Contributor Author

Right, this seems to be splitting into two things: what it does at the HTTP layer, and how we implement it in our own client libraries.

I'm really mostly concerned about the HTTP case.

@mahmoudimus mahmoudimus reopened this Jan 23, 2014
@mahmoudimus
Copy link
Contributor

Whoops, my bad about closing this issue. Hit the wrong button :(

@dougblack
Copy link

I was writing this before the distinction was made between HTTP layer/client library layer. Posting anyways in case anyone finds it useful. +1 for having an open debate on Github. Very cool.

In most of the Twilio client libraries we lazily load the objects. So, something like

client.account.accounts.get(id)

just updates the URI internally, meaning no HTTP requests are made until an action or property on that object is accessed. So, this would actually fire a POST and no GETs.

account = client.accounts.get(id)
account.update(friendly_name="new friendly name")

Not sure if this would work in your case or not, just adding another perspective.

@matthewfl
Copy link
Contributor

@steveklabnik I agree that there are two issues here, in my mind the HTTP case can be covered by appending /debits if need be. I guess we could add better support into the api if this is not sufficient, however I don't believe that's needed.

@dougblack thanks for your input, I was trying to suggest something similar above.

@steveklabnik
Copy link
Contributor Author

So.

When looking at the API, we do support this directly, but with the card number:

$ curl https://api.balancedpayments.com/debits \
> -H "Accept: application/vnd.api+json;revision=1.1" \
> -d source[number]=4111111111111111 \
> -d source[expiration_year]=2018 \
> -d source[expiration_month]=12 \
> -d amount=1234 \
> -u ak-test-1iY1ALQreeaiFZpJCYDCH2TKOvQnZyCYv: 
{
  "debits": [
    {
      "status": "succeeded",
      "description": null,
      "links": {
        "customer": "CU1eg5xoQsV3SBUKF78u5Qxa",
        "source": "CC474uCmdu2tnli3TleR5o3J",
        "order": null
      },
      "updated_at": "2014-01-23T21:15:49.015962Z",
      "created_at": "2014-01-23T21:15:47.700553Z",
      "transaction_number": "W597-190-0533",
      "failure_reason": null,
      "currency": "USD",
      "amount": 1234,
      "failure_reason_code": null,
      "meta": {},
      "href": "/debits/WD47f1QeCoxAJ5lKAy8kMAQ7",
      "appears_on_statement_as": "BAL*example.com",
      "id": "WD47f1QeCoxAJ5lKAy8kMAQ7"
    }
  ],
  "links": {
    "debits.source": "/resources/{debits.source}",
    "debits.customer": "/customers/{debits.customer}",
    "debits.order": "/orders/{debits.order}",
    "debits.refunds": "/debits/{debits.id}/refunds",
    "debits.events": "/debits/{debits.id}/events"
  }
}

Basically, what I would like is for this endpoint to also accept a token:

$ curl https://api.balancedpayments.com/debits \
> -H "Accept: application/vnd.api+json;revision=1.1" \
> -d token=fwef23rf3f \
> -d amount=1234 \
> -u ak-test-1iY1ALQreeaiFZpJCYDCH2TKOvQnZyCYv: 

That's it. It's not some kind of super significant massive change.

@matthewfl
Copy link
Contributor

@steveklabnik is there some reason why accepting tokens is less taboo then just adding /debits to the card href? Supposedly, in the past there have been issues with people passing url/id to endpoints, which was one thing we were trying to avoid in general with the api. I believe associating card to customers and orders are basically the only exceptions at this point.

@steveklabnik
Copy link
Contributor Author

Yes. We are trying to educate people against manually constructing URLs. Saying "we're hypermedia! Except in this one case, then you add a URL to something" doesn't make sense.

Secondarily, the only way you can get a card href is if you've made an API request to create the card. So it's only available in the two-call scenario.

@steveklabnik
Copy link
Contributor Author

I didn't know that Balanced.js returns a URI as well as the token. So yes, in that case, passing the token or the href is fine.

@mjallday
Copy link
Contributor

@steveklabnik - @mahmoud's response to that (which is the same as what I originally suggested I think) was:

no - since you have to know to append a /debits at the end of that.

essentially, how do you discover that the debits endpoint exists? in the python client it's hard coded, i'm guessing maybe that's not kosher?

@steveklabnik
Copy link
Contributor Author

I mean by passing it in as a source parameter to /debits, not by munging the URL.

essentially, how do you discover that the debits endpoint exists?

We basically would be moving from one known endpoint to two. I'm both pretty confident that maintaining /debits for this purpose is both really worthwhile as well as not likely to provide a major burden, so seems fine.

@mahmoudimus
Copy link
Contributor

@steveklabnik another thing is that we can send back a bookmarks schema based on the revision to seed the resources on configurations.

require 'balanced'
balanced.configure('.....')

# any further operation will get a side-loaded list of URL maps to seed the resource endpoints, solving the issue of /debits being hardcoded.
Balanced::Customer.find( ...  )  # side loads all URL map first for caching

@mahmoudimus
Copy link
Contributor

@matthewfl @steveklabnik what's the status with this?

@steveklabnik
Copy link
Contributor Author

I need to write a spec.—
Sent from Mailbox for iPhone

On Fri, Jan 24, 2014 at 7:10 PM, Mahmoud Abdelkader
notifications@github.com wrote:

@matthewfl @steveklabnik what's the status with this?

Reply to this email directly or view it on GitHub:
#492 (comment)

@matin
Copy link
Member

matin commented Apr 29, 2014

@steveklabnik is this issue this valid?

@zachsnow
Copy link

Wondering where this stands in the case that you would like to use the result of the postal code or cvv checks to decide whether or not to charge the card? I know it's straightforward to tokenize a card on the client and then immediately charge that card on the server using source (2 requests total). It would be great if the charge would be guaranteed to fail whenever the postal or cvv checks failed (for instance, with Stripe there is an option at the account level to always decline cards that fail these checks). As it stands the requirement to fetch the card to perform these checks makes it 3 requests to safely charge a card.

@msherry
Copy link
Contributor

msherry commented Jan 31, 2015

@zachsnow There is an equivalent option for balanced -- we can make AVS/CVV failures hard failures for your marketplace. Contact support@balancedpayments.com to enable this.

@zachsnow
Copy link

@msherry thank you so much for this information! If this isn't documented anywhere I'd highly recommend you do so in a very visible place (I didn't see it anyway, not sayin' much).

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

No branches or pull requests

8 participants