balanced / balanced-api

Balanced API specification.
221 stars 72 forks source link

Crediting a card with `can_credit` of `false` should still create a Credit #624

Open steveklabnik opened 10 years ago

steveklabnik commented 10 years ago

I was investigating this failure:

https://travis-ci.org/balanced/balanced-api/builds/25547319#L66

I realized that it was failing due to the stateful nature of our test: I try to query the credits, but the last response is an error. So, while thinking about how to determine which credit would go with this call, I inserted a call to /credits in the step definition:

diff --git a/features/step_definitions/credits.rb b/features/step_definitions/credits.rb
index 7b6f618..4c47af0 100644
--- a/features/step_definitions/credits.rb
+++ b/features/step_definitions/credits.rb
@@ -14,7 +14,11 @@ Given(/^I have more than two credits$/) do
 end

 Then(/^the credit was successfully created$/) do
-  href = @client["credits"]["href"]
+@client.get("/credits")
+puts @client.last_body
   step "I GET to #{href}"
   step "I should get a 200 OK status code"
 end

This prints

{"credits"=>[], "meta"=>{"last"=>"/credits?limit=10&offset=0", "next"=>nil, "href"=>"/credits?limit=10&offset=0", "limit"=>10, "offset"=>0, "previous"=>nil, 
"total"=>0, "first"=>"/credits?limit=10&offset=0"}, "links"=>{}}

So there are no credits, which means that this feature does not currently work. Thoughts?

matin commented 10 years ago

I'm not sure this line should be there at all: But the credit was successfully created

We create the transaction as a side-effect for exceeding the transaction limit because you can't know the transaction limit of a network or bank at the time of the transaction. I'm treating a transaction limit by Balanced in the same way.

I'm checking how a Debit to a Bank Account with can_debit of false handles this to make sure we're being consistent.

Regardless, can you make errors like this more readable?

matin commented 10 years ago

Here's how we do it today:

havaa:~ matin$ curl -u xxx: \
  https://api.balancedpayments.com/bank_acconts/BA2kMU3oXVc1zHzFrcCE97p6/debits \
  -d amount=100
{
  "errors": [
    {
      "status": "Conflict",
      "category_code": "funding-source-not-debitable",
      "additional": null,
      "status_code": 409,
      "category_type": "logical",
      "extras": {},
      "request_id": "OHM56b2076edf9e11e3be3f02b12035401b",
      "description": "Funding instrument cannot be debited. Your request id is OHM56b2076edf9e11e3be3f02b12035401b."
    }
  ],
  "links": {
    "debits.customer": "/customers/{debits.customer}",
    "debits.dispute": "/disputes/{debits.dispute}",
    "debits.source": "/resources/{debits.source}",
    "debits.order": "/orders/{debits.order}",
    "debits.refunds": "/debits/{debits.id}/refunds",
    "debits.events": "/debits/{debits.id}/events"
  },
  "debits": [
    {
      "status": "failed",
      "description": null,
      "links": {
        "customer": "AC5iBVquYrgxEkJ3lDkLY3MG",
        "source": "BA2kMU3oXVc1zHzFrcCE97p6",
        "order": null,
        "dispute": null
      },
      "updated_at": "2014-05-19T21:41:30.519070Z",
      "created_at": "2014-05-19T21:41:30.403807Z",
      "transaction_number": "W367-232-8160",
      "failure_reason": "Funding instrument cannot be debited.",
      "currency": "USD",
      "amount": 100,
      "failure_reason_code": "funding-source-not-debitable",
      "meta": {},
      "href": "/debits/WD2DDtV3OpYfHB1n1rIlKkp4",
      "appears_on_statement_as": "BAL*Matin Tamizi",
      "id": "WD2DDtV3OpYfHB1n1rIlKkp4"
    }
  ]
}

The scenario is correct. The API is wrong.

@cieplak Can you fix this?

@steveklabnik Can you make the error message more clear?

steveklabnik commented 10 years ago

In order to improve it, I would have to replace every array access in the test suite like this:

old

@client['credits']['id]

new

throw "no credits where credits were expected" @client['credits']
@client['credits']['id']

Now do that for every layer of nesting. And I'm not sure that error is any better, to be honest.

steveklabnik commented 10 years ago

@matin also, you had explicitly asked me to test for the existence of the credit afterwards, because it is part of the expected behavior. I wasn't testing this side effect previously.

cieplak commented 10 years ago

@matin what is wrong about the API here? that the /events endpoint is eventually consistent?

matin commented 10 years ago

re Credit being created: You're right. As I indicated in my prior comment, the scenario is correct and is consistent with current behavior with bank accounts.

TBH, I've never had much success with Ruby stack traces. It's one of the reasons I switched to Python :fire: :wink:. Makes sense if there's no better way to address it.

matin commented 10 years ago

@cieplak What do those two things have to do with each other?

cieplak commented 10 years ago

image

@matin there are credits, they just havent been ingested into elasticsearch. the credits index (/credits) is backed by es, but if you query the api for the credit id (/credits/:credit_id), it will hit the database which is always consistent

mjallday commented 10 years ago

@cieplak are you sure about that? i'm pretty sure only /transactions and /search are backed by es.

matin commented 10 years ago

@cieplak How is this any different than Scenario: Pushing money to a card cannot exceed $2,500 or attempting to debit a bank account with can_debit of false?

matin commented 10 years ago

ping @cieplak