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

Push to card specs and scenarios #580

Merged
merged 40 commits into from May 15, 2014

Conversation

Projects
None yet
7 participants
@matin
Member

matin commented Apr 29, 2014

These are the specifications and scenarios for push to card.

Scenarios still required:

  • Credit to a card with can_credit of false should fail

Open issues required to merge:

  • Card.type and Card.category defaults #607
  • Use bank_name instead of bank #608
  • Exceeding transaction limit should product a 409 instead of 400 #611
Show outdated Hide outdated fixtures/_models/card.json Outdated
Show outdated Hide outdated features/rest/cards.feature Outdated
@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Apr 30, 2014

Contributor

One thing I've been thinking about this: the Right Way (tm) to do this via hypermedia is to expose the link depending on the actions available, rather than booleans.

Semi-realistic mockup:

This way:

{
    "cards" [{
        "number": "444444444444",
        "can_debit": true,
        "can_credit": true
    }],
    "links": [{
        "cards.debit": "...",
        "cards.credit": "..."
    }]
}

vs

{
    "cards" [{
        "number": "444444444444"
    }],
    "links": [{
        "cards.debit": "..."
    }]
}

and

{
    "cards" [{
        "number": "444444444444"
    }],
    "links": [{
        "cards.debit": "...",
        "cards.credit": "..."
    }]
}

What do you think?

Contributor

steveklabnik commented Apr 30, 2014

One thing I've been thinking about this: the Right Way (tm) to do this via hypermedia is to expose the link depending on the actions available, rather than booleans.

Semi-realistic mockup:

This way:

{
    "cards" [{
        "number": "444444444444",
        "can_debit": true,
        "can_credit": true
    }],
    "links": [{
        "cards.debit": "...",
        "cards.credit": "..."
    }]
}

vs

{
    "cards" [{
        "number": "444444444444"
    }],
    "links": [{
        "cards.debit": "..."
    }]
}

and

{
    "cards" [{
        "number": "444444444444"
    }],
    "links": [{
        "cards.debit": "...",
        "cards.credit": "..."
    }]
}

What do you think?

@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin Apr 30, 2014

Member

@steveklabnik we talked about this before. You're right that this is technically the Right Way to represent this information from a hypermedia API standpoint, but it's not the best experience.

can_debit is also consistent with how we do it for Bank Accounts. Think about the code in each case:

if card.can_credit: ...

vs.

if hasattr(card, credit): ...

or

try card.credit(amount):
    ...
except AttributeError:
    ...

The first way makes more sense—at least to me.

Member

matin commented Apr 30, 2014

@steveklabnik we talked about this before. You're right that this is technically the Right Way to represent this information from a hypermedia API standpoint, but it's not the best experience.

can_debit is also consistent with how we do it for Bank Accounts. Think about the code in each case:

if card.can_credit: ...

vs.

if hasattr(card, credit): ...

or

try card.credit(amount):
    ...
except AttributeError:
    ...

The first way makes more sense—at least to me.

@msherry

This comment has been minimized.

Show comment
Hide comment
@msherry

msherry Apr 30, 2014

Contributor

Isn't that just an issue for the clients?

@property
def can_credit(self):
    return hasattr(self.credit)
Contributor

msherry commented Apr 30, 2014

Isn't that just an issue for the clients?

@property
def can_credit(self):
    return hasattr(self.credit)
@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin Apr 30, 2014

Member

@msherry You make a good point.

Would you do something similar for balanced.js?

Can you create a similar type of abstraction in Java?

Member

matin commented Apr 30, 2014

@msherry You make a good point.

Would you do something similar for balanced.js?

Can you create a similar type of abstraction in Java?

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Apr 30, 2014

Contributor

You should be able to do in any language.

Contributor

steveklabnik commented Apr 30, 2014

You should be able to do in any language.

@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin Apr 30, 2014

Member

@steveklabnik Shouldn't it be cards.credits (plural) instead of cards.credits? That link, from my understanding, would be used both for a fetch and create.

What would you do if a Bank Account has prior debits but can_debit changes to false?

Also ... should ≠ can

Member

matin commented Apr 30, 2014

@steveklabnik Shouldn't it be cards.credits (plural) instead of cards.credits? That link, from my understanding, would be used both for a fetch and create.

What would you do if a Bank Account has prior debits but can_debit changes to false?

Also ... should ≠ can

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Apr 30, 2014

Contributor

We currently use the singular everywhere already, I think.

Do bank accounts change their ability to debit and be debited? Isn't that a feature of the kind of account?

By "should" I meant "I can't think of a counterexample." Ruby, Python, C++, Java, PHP, C#.... it would be a pretty serious deficiency.

Contributor

steveklabnik commented Apr 30, 2014

We currently use the singular everywhere already, I think.

Do bank accounts change their ability to debit and be debited? Isn't that a feature of the kind of account?

By "should" I meant "I can't think of a counterexample." Ruby, Python, C++, Java, PHP, C#.... it would be a pretty serious deficiency.

@msherry

This comment has been minimized.

Show comment
Hide comment
@msherry

msherry Apr 30, 2014

Contributor

@steveklabnik They do. You can't debit a bank account that hasn't been verified, for instance.

Contributor

msherry commented Apr 30, 2014

@steveklabnik They do. You can't debit a bank account that hasn't been verified, for instance.

@remear

This comment has been minimized.

Show comment
Hide comment
@remear

remear Apr 30, 2014

Contributor

Java uses ResourceField to simulate object properties like other languages. I'd really have to do some thinking on how one could be set based on the existence of a link. So, I'll have to go with maybe, but I'm not fond of the idea. Of course, there could be a class method, but then it would be card.can_debit() instead of card.can_debit, which might confuse users.

Contributor

remear commented Apr 30, 2014

Java uses ResourceField to simulate object properties like other languages. I'd really have to do some thinking on how one could be set based on the existence of a link. So, I'll have to go with maybe, but I'm not fond of the idea. Of course, there could be a class method, but then it would be card.can_debit() instead of card.can_debit, which might confuse users.

@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin Apr 30, 2014

Member

@steveklabnik Do we now ...

from https://docs.balancedpayments.com/1.1/api/cards/#create-a-card-direct

{
    "cards": [
        {
            "address": {
                "city": null, 
                "country_code": null, 
                "line1": null, 
                "line2": null, 
                "postal_code": null, 
                "state": null
            }, 
            "avs_postal_match": null, 
            "avs_result": null, 
            "avs_street_match": null, 
            "brand": "MasterCard", 
            "created_at": "2014-04-29T00:46:42.022131Z", 
            "cvv": "xxx", 
            "cvv_match": "yes", 
            "cvv_result": "Match", 
            "expiration_month": 12, 
            "expiration_year": 2020, 
            "fingerprint": "fc4ccd5de54f42a5e75f76fbfde60948440c7a382ee7d21b2bc509ab9cfed788", 
            "href": "/cards/CC5H1ZZk43sPUu0f9lIxbhgF", 
            "id": "CC5H1ZZk43sPUu0f9lIxbhgF", 
            "is_verified": true, 
            "links": {
                "customer": null
            }, 
            "meta": {}, 
            "name": null, 
            "number": "xxxxxxxxxxxx5100", 
            "updated_at": "2014-04-29T00:46:42.022134Z"
        }
    ], 
    "links": {
        "cards.card_holds": "/cards/{cards.id}/card_holds", 
        "cards.customer": "/customers/{cards.customer}", 
        "cards.debits": "/cards/{cards.id}/debits"
    }
}
Member

matin commented Apr 30, 2014

@steveklabnik Do we now ...

from https://docs.balancedpayments.com/1.1/api/cards/#create-a-card-direct

{
    "cards": [
        {
            "address": {
                "city": null, 
                "country_code": null, 
                "line1": null, 
                "line2": null, 
                "postal_code": null, 
                "state": null
            }, 
            "avs_postal_match": null, 
            "avs_result": null, 
            "avs_street_match": null, 
            "brand": "MasterCard", 
            "created_at": "2014-04-29T00:46:42.022131Z", 
            "cvv": "xxx", 
            "cvv_match": "yes", 
            "cvv_result": "Match", 
            "expiration_month": 12, 
            "expiration_year": 2020, 
            "fingerprint": "fc4ccd5de54f42a5e75f76fbfde60948440c7a382ee7d21b2bc509ab9cfed788", 
            "href": "/cards/CC5H1ZZk43sPUu0f9lIxbhgF", 
            "id": "CC5H1ZZk43sPUu0f9lIxbhgF", 
            "is_verified": true, 
            "links": {
                "customer": null
            }, 
            "meta": {}, 
            "name": null, 
            "number": "xxxxxxxxxxxx5100", 
            "updated_at": "2014-04-29T00:46:42.022134Z"
        }
    ], 
    "links": {
        "cards.card_holds": "/cards/{cards.id}/card_holds", 
        "cards.customer": "/customers/{cards.customer}", 
        "cards.debits": "/cards/{cards.id}/debits"
    }
}
@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Apr 30, 2014

Contributor

@matin Cool! I just mis-remembered. I don't care at all, I was just trying to be consistent.

@remear you construct the object based on the JSON that comes back, right? So why wouldn't you be able to?

Contributor

steveklabnik commented Apr 30, 2014

@matin Cool! I just mis-remembered. I don't care at all, I was just trying to be consistent.

@remear you construct the object based on the JSON that comes back, right? So why wouldn't you be able to?

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Apr 30, 2014

Contributor

@remear

there could be a class method, but then it would be card.can_debit() instead of card.can_debit, which might confuse users.

Ahh, I think this is the difference. Are parenthesis confusing? I don't understand this, but maybe that's just me.

Contributor

steveklabnik commented Apr 30, 2014

@remear

there could be a class method, but then it would be card.can_debit() instead of card.can_debit, which might confuse users.

Ahh, I think this is the difference. Are parenthesis confusing? I don't understand this, but maybe that's just me.

Show outdated Hide outdated features/rest/credits.feature Outdated
Show outdated Hide outdated features/rest/credits.feature Outdated
Show outdated Hide outdated features/rest/credits.feature Outdated

@steveklabnik steveklabnik referenced this pull request Apr 30, 2014

Closed

Test numbers #589

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 1, 2014

Contributor

One question about this interface is that can_credit implies that a credit will succeed. It's a statement of possibility, not a guarantee about debit-ability. :/

@msherry @mjallday @mahmoudimus are there scenarios in which can_{debit,credit} transitions from true to false? @msherry , your earlier example was from false to true.

Contributor

steveklabnik commented May 1, 2014

One question about this interface is that can_credit implies that a credit will succeed. It's a statement of possibility, not a guarantee about debit-ability. :/

@msherry @mjallday @mahmoudimus are there scenarios in which can_{debit,credit} transitions from true to false? @msherry , your earlier example was from false to true.

@msherry

This comment has been minimized.

Show comment
Hide comment
@msherry

msherry May 1, 2014

Contributor

@steveklabnik Sure, if we receive a NOC (Notice of Change) from the RDFI, we can no longer credit/debit that bank account. If a card is lost or stolen, this would likely apply there, as well.

Contributor

msherry commented May 1, 2014

@steveklabnik Sure, if we receive a NOC (Notice of Change) from the RDFI, we can no longer credit/debit that bank account. If a card is lost or stolen, this would likely apply there, as well.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 1, 2014

Contributor

Seems legit.

Contributor

steveklabnik commented May 1, 2014

Seems legit.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 1, 2014

Contributor

After exploring all these options, seems like can_debit and can_credit should be there.

Contributor

steveklabnik commented May 1, 2014

After exploring all these options, seems like can_debit and can_credit should be there.

@steveklabnik steveklabnik referenced this pull request May 2, 2014

Closed

Push to card page #195

Show outdated Hide outdated fixtures/_models/token.json Outdated

@matin matin closed this May 12, 2014

@matin matin reopened this May 12, 2014

@kyungmin kyungmin referenced this pull request May 12, 2014

Merged

Push to card dashboard changes #1226

3 of 4 tasks complete
@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin May 15, 2014

Member

@mjallday Rebased master.

Member

matin commented May 15, 2014

@mjallday Rebased master.

@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin May 15, 2014

Member

Going to merge 😄. The only failing scenarios are unrelated to push to card.

Member

matin commented May 15, 2014

Going to merge 😄. The only failing scenarios are unrelated to push to card.

matin added a commit that referenced this pull request May 15, 2014

Merge pull request #580 from matin/master
Push to card specs and scenarios

@matin matin merged commit 0b0f857 into balanced:master May 15, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment