Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Start recording ref in exchanges table #4361

Merged
merged 10 commits into from
Mar 17, 2017
Merged

Start recording ref in exchanges table #4361

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f10e1c7
Added ref parameter to the record_exchange def
kaguillera Mar 2, 2017
cdaf7a2
Fixed the test that were failing due to changes.
kaguillera Mar 2, 2017
4bafcf9
Fixed the make_exchange in harness that uses record_exchange
kaguillera Mar 2, 2017
54f70d1
Change ref to Null if try...except fails in create_card_hold
kaguillera Mar 3, 2017
9445dda
Attempt to resolve github file conflict
kaguillera Mar 17, 2017
b14e1e3
Moved fake_data.py location as per master current state.
kaguillera Mar 17, 2017
6e8ed29
Merge branch 'master' into record-ref-in-exchanges
mattbk Mar 17, 2017
c604b2a
Merge branch 'record-ref-in-exchanges'
mattbk Mar 17, 2017
f3cf8af
Merge branch 'record-ref-in-exchanges' of github.com:gratipay/gratipa…
mattbk Mar 17, 2017
7abbf37
Merge branch 'master' into record-ref-in-exchanges
mattbk Mar 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions gratipay/billing/exchanges.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ def create_card_hold(db, participant, amount):

hold = None
error = ""
ref = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to set this here? For when the transaction fails and we don't have a ref? Shouldn't we just stick with null in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed to set this (but it could be Null) because it kept giving the error that the variable was being referenced before being assigned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some consideration...yes it should be Null if the try..except fails

try:
result = braintree.Transaction.sale({
'amount': str(cents/100.0),
Expand All @@ -82,6 +83,7 @@ def create_card_hold(db, participant, amount):
'options': { 'submit_for_settlement': False },
'custom_fields': {'participant_id': participant.id}
})
ref = result.transaction.id
Copy link
Contributor

Choose a reason for hiding this comment

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

A general question: can ref here and below be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it can be according to the current state of the db table. For recording and accounting purposes we don't want it not to be null but because of the current issue with the missing back data we need to allow nulls until we can back fill all the refs (if that is at all possible).

Copy link
Contributor

@nobodxbodon nobodxbodon Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

To make sure, after this PR, all the newly inserted records afterwards will have ref field as non-null?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do they give us a URL and not just a transaction id? Eventually we're going to want to cross-link from Gratipay (admin) UI over to Braintree (etc.). Transaction id is a start but if we can know a URL at this point then let's just store that, ya?

Copy link
Contributor

Choose a reason for hiding this comment

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

all the newly inserted records afterwards will have ref field as non-null?

I believe they will/should be null if the transaction fails. We still want to record our attempt at a transaction even if the API call fails.

Copy link
Contributor Author

@kaguillera kaguillera Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@whit537 I believe that it should be null if it fails in the try...except block but it may still have a reference in Braintree if fails on there side after the card_hold has been created.

Eventually we're going to want to cross-link from Gratipay (admin) UI over to Braintree (etc.).

Going to look into this now.

Copy link
Contributor

Choose a reason for hiding this comment

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

it may still have a reference in Braintree if fails on there side after the card_hold has been created.

Hmmm ... we should probably differentiate "failure" state as reported by Braintree from an "error" state due to, e.g., network failure during the API call.

Copy link
Contributor Author

@kaguillera kaguillera Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

we can always do that in the note field of the exchange record. (sorry for stating the obvious)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just build a URL to the transaction? E.g., Stripe builds URLs like https://dashboard.stripe.com/payments/ch_19tIgqD5FJNWkW8j1uWePP8O with the charge ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a ridiculous long time researching it...yes we can build a URL to the transaction using transaction.id
I believe it would look something like
https://dashboard.braintreegateway.com/merchants/gratipay/transactions/1vt8z5a3

where 1vt8z5a3 is the transaction.id


if result.is_success and result.transaction.status == 'authorized':
error = ""
Expand All @@ -98,7 +100,7 @@ def create_card_hold(db, participant, amount):
log(msg + "succeeded.")
else:
log(msg + "failed: %s" % error)
record_exchange(db, route, amount, fee, participant, 'failed', error)
record_exchange(db, route, amount, fee, participant, 'failed', ref, error)

return hold, error

Expand All @@ -118,14 +120,15 @@ def capture_card_hold(db, participant, amount, hold):

cents, amount_str, charge_amount, fee = _prep_hit(amount)
amount = charge_amount - fee # account for possible rounding
e_id = record_exchange(db, route, amount, fee, participant, 'pre')
ref = hold.id
e_id = record_exchange(db, route, amount, fee, participant, 'pre', ref)

# TODO: Find a way to link transactions and corresponding exchanges
# meta = dict(participant_id=participant.id, exchange_id=e_id)

error = ''
try:
result = braintree.Transaction.submit_for_settlement(hold.id, str(cents/100.00))
result = braintree.Transaction.submit_for_settlement(ref, str(cents/100.00))
assert result.is_success
if result.transaction.status != 'submitted_for_settlement':
error = result.transaction.status
Expand Down Expand Up @@ -228,7 +231,7 @@ def get_ready_payout_routes_by_network(db, network):
return out


def record_exchange(db, route, amount, fee, participant, status, error=None):
def record_exchange(db, route, amount, fee, participant, status, ref, error=None):
"""Given a Bunch of Stuff, return an int (exchange_id).

Records in the exchanges table have these characteristics:
Expand All @@ -240,6 +243,8 @@ def record_exchange(db, route, amount, fee, participant, status, error=None):

fee The payment processor's fee. It's always positive.

ref transaction id in the external system.

"""

assert route.participant.id == participant.id
Expand All @@ -248,10 +253,10 @@ def record_exchange(db, route, amount, fee, participant, status, error=None):

exchange_id = cursor.one("""
INSERT INTO exchanges
(amount, fee, participant, status, route, note)
VALUES (%s, %s, %s, %s, %s, %s)
(amount, fee, participant, status, route, note, ref)
VALUES (%s, %s, %s, %s, %s, %s, %s)
RETURNING id
""", (amount, fee, participant.username, status, route.id, error))
""", (amount, fee, participant.username, status, route.id, error, ref))

if status == 'failed':
propagate_exchange(cursor, participant, route, error, 0)
Expand Down
4 changes: 2 additions & 2 deletions gratipay/testing/harness.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ def fetch_payday(self):


def make_exchange(self, route, amount, fee, participant, status='succeeded', error='',
address='dummy-address'):
ref='dummy-trans-id', address='dummy-address'):
"""Factory for exchanges.
"""
if not isinstance(route, ExchangeRoute):
Expand All @@ -246,7 +246,7 @@ def make_exchange(self, route, amount, fee, participant, status='succeeded', err
if not route:
route = ExchangeRoute.insert(participant, network, address)
assert route
e_id = record_exchange(self.db, route, amount, fee, participant, 'pre')
e_id = record_exchange(self.db, route, amount, fee, participant, 'pre', ref)
record_exchange_result(self.db, e_id, status, error, participant)
return e_id

Expand Down
5 changes: 4 additions & 1 deletion gratipay/utils/fake_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,14 +220,17 @@ def fake_payment(db, participant, team, timestamp, amount, payday, direction):


def fake_exchange(db, participant, amount, fee, timestamp):
route=get_exchange_route(db, participant.id)
ref = '%s-%s-%s' % (participant.username, route, timestamp.microsecond)
return insert_fake_data( db
, "exchanges"
, timestamp=timestamp
, participant=participant.username
, amount=amount
, fee=fee
, status='succeeded'
, route=get_exchange_route(db, participant.id)
, route=route
, ref=ref
)


Expand Down
18 changes: 12 additions & 6 deletions tests/py/test_billing_exchanges.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,16 +279,18 @@ def test_re_records_exchange(self):
, fee=D("0.41")
, participant=alice
, status='pre'
, ref='alice-braintree-cc-trans-id'
)
actual = self.db.one("""
SELECT amount, fee, participant, status, route
SELECT amount, fee, participant, status, route, ref
FROM exchanges
""", back_as=dict)
expected = { "amount": D('0.59')
, "fee": D('0.41')
, "participant": "alice"
, "status": 'pre'
, "route": ExchangeRoute.from_network(alice, 'braintree-cc').id
, "ref" : 'alice-braintree-cc-trans-id'
}
assert actual == expected

Expand All @@ -302,6 +304,7 @@ def test_re_requires_valid_route(self):
, fee=D("0.41")
, participant=alice
, status='pre'
, ref='bob-braintree-cc-trans-id'
)

def test_re_stores_error_in_note(self):
Expand All @@ -312,6 +315,7 @@ def test_re_stores_error_in_note(self):
, fee=D("0.41")
, participant=alice
, status='pre'
, ref='alice-braintree-cc-trans-id'
, error='Card payment failed'
)
exchange = self.db.one("SELECT * FROM exchanges")
Expand All @@ -325,6 +329,7 @@ def test_re_doesnt_update_balance_for_positive_amounts(self):
, fee=D("0.41")
, participant=alice
, status='pre'
, ref='alice-braintree-cc-trans-id'
)
assert P('alice').balance == D('0.00')

Expand All @@ -336,27 +341,28 @@ def test_re_updates_balance_for_negative_amounts(self):
, fee=D('0.75')
, participant=alice
, status='pre'
, ref='alice-paypal-trans-id'
)
assert P('alice').balance == D('13.41')

def test_re_fails_if_negative_balance(self):
alice = self.make_participant('alice', last_paypal_result='')
ba = ExchangeRoute.from_network(alice, 'paypal')
with pytest.raises(NegativeBalance):
record_exchange(self.db, ba, D("-10.00"), D("0.41"), alice, 'pre')
record_exchange(self.db, ba, D("-10.00"), D("0.41"), alice, 'pre', 'paypal-transid')

def test_re_result_restores_balance_on_error(self):
alice = self.make_participant('alice', balance=30, last_paypal_result='')
ba = ExchangeRoute.from_network(alice, 'paypal')
e_id = record_exchange(self.db, ba, D('-27.06'), D('0.81'), alice, 'pre')
e_id = record_exchange(self.db, ba, D('-27.06'), D('0.81'), alice, 'pre', 'paypal-transid')
assert alice.balance == D('02.13')
record_exchange_result(self.db, e_id, 'failed', 'SOME ERROR', alice)
assert P('alice').balance == D('30.00')

def test_re_result_restores_balance_on_error_with_invalidated_route(self):
alice = self.make_participant('alice', balance=37, last_paypal_result='')
pp = ExchangeRoute.from_network(alice, 'paypal')
e_id = record_exchange(self.db, pp, D('-32.45'), D('0.86'), alice, 'pre')
e_id = record_exchange(self.db, pp, D('-32.45'), D('0.86'), alice, 'pre', 'paypal-transid')
assert alice.balance == D('3.69')
pp.update_error('invalidated')
record_exchange_result(self.db, e_id, 'failed', 'oops', alice)
Expand All @@ -367,15 +373,15 @@ def test_re_result_restores_balance_on_error_with_invalidated_route(self):
def test_re_result_doesnt_restore_balance_on_success(self):
alice = self.make_participant('alice', balance=50, last_paypal_result='')
ba = ExchangeRoute.from_network(alice, 'paypal')
e_id = record_exchange(self.db, ba, D('-43.98'), D('1.60'), alice, 'pre')
e_id = record_exchange(self.db, ba, D('-43.98'), D('1.60'), alice, 'pre', 'paypal-transid')
assert alice.balance == D('4.42')
record_exchange_result(self.db, e_id, 'succeeded', None, alice)
assert P('alice').balance == D('4.42')

def test_re_result_updates_balance_for_positive_amounts(self):
alice = self.make_participant('alice', balance=4, last_bill_result='')
cc = ExchangeRoute.from_network(alice, 'braintree-cc')
e_id = record_exchange(self.db, cc, D('31.59'), D('0.01'), alice, 'pre')
e_id = record_exchange(self.db, cc, D('31.59'), D('0.01'), alice, 'pre', 'paypal-transid')
assert alice.balance == D('4.00')
record_exchange_result(self.db, e_id, 'succeeded', None, alice)
assert P('alice').balance == D('35.59')