Start recording ref in exchanges table #4361

kaguillera · 2017-03-02T18:34:16Z

This should fix the problem of not recording the ref to the external system. In this PR only the braintree ref id is being recorded. @whit537 suggested that the Paypal MassPay reference be done in another PR.
Please note that this first commit should fail since I have not updated the test_billing_exchanges.py as yet.

Tests should fail

kaguillera · 2017-03-02T21:28:29Z

I do believe that this is ready for review. @mattbk @rohitpaulk @nobodxbodon et.al

nobodxbodon · 2017-03-03T07:08:11Z

gratipay/billing/exchanges.py

@@ -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


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

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).

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

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?

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.

@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.

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.

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

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.

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

chadwhitacre · 2017-03-03T12:46:51Z

gratipay/billing/exchanges.py

@@ -74,6 +74,7 @@ def create_card_hold(db, participant, amount):

    hold = None
    error = ""
+    ref = ""


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?

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

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

kaguillera · 2017-03-03T21:38:10Z

This last commit just changes the ref from an empty string to Null if the create_card_hold fails in the try_except. Any other failures would be on Braintree and thus a ref (transaction.id) should exist and thus would server as a record for us as the properties of the transaction.

It should be noted that all of the transaction history is attached to the one id on creation of the transaction in the Braintree system. Refund transactions is the only action that has a different id from the origin transaction for which it is made. There is a field in the refund transaction that links to the original transaction for which the refund is being made. IRL @whit537 suggested that eventually we may want to record the origin transaction ids for refunds that we do in its own field. Currently it is recorded in the notes field of the exchange record. (or atleast should be)

kaguillera · 2017-03-03T21:45:06Z

ok ready for review/merge

mattbk · 2017-03-03T22:28:27Z

Merge conflict!

kaguillera · 2017-03-17T18:18:51Z

Working on the conflict files no sure why it is saying that.

kaguillera · 2017-03-17T18:43:50Z

I just can't figure out the conflict

…y.com into record-ref-in-exchanges

mattbk · 2017-03-17T18:57:58Z

I'm not sure what I just did. Was following the command line instructions mentioned in the box at the bottom of the PR.

kaguillera · 2017-03-17T19:04:14Z

I was trying not to do that since the commands merge the branch into master (at least that is what I think it does) and I did not want to do that until the conflict was resolved.

...
Guess not

!m @mattbk

Ready for review and merging if acceptable

mattbk · 2017-03-17T19:07:36Z

Yeah, I stopped once I had merged master into this branch, and then pushed from there. I think it was only the first block of commands.

git fetch origin
git checkout -b record-ref-in-exchanges origin/record-ref-in-exchanges
git merge master

and then
git push origin record-ref-in-exchanges

I think you got the right change in 9445dda, but for some reason it wasn't working right.

kaguillera added 3 commits March 2, 2017 13:15

Added ref parameter to the record_exchange def

f10e1c7

Tests should fail

Fixed the test that were failing due to changes.

cdaf7a2

Fixed the make_exchange in harness that uses record_exchange

4bafcf9

kaguillera added the Review label Mar 2, 2017

nobodxbodon reviewed Mar 3, 2017

View reviewed changes

nobodxbodon approved these changes Mar 3, 2017

View reviewed changes

chadwhitacre reviewed Mar 3, 2017

View reviewed changes

Change ref to Null if try...except fails in create_card_hold

54f70d1

Attempt to resolve github file conflict

9445dda

kaguillera and others added 5 commits March 17, 2017 14:49

Moved fake_data.py location as per master current state.

b14e1e3

Merge branch 'master' into record-ref-in-exchanges

6e8ed29

Merge branch 'record-ref-in-exchanges'

c604b2a

Merge branch 'record-ref-in-exchanges' of github.com:gratipay/gratipa…

f3cf8af

…y.com into record-ref-in-exchanges

Merge branch 'master' into record-ref-in-exchanges

7abbf37

mattbk merged commit 129ca1b into master Mar 17, 2017

mattbk deleted the record-ref-in-exchanges branch March 17, 2017 19:50

kaguillera mentioned this pull request Apr 27, 2017

backfill remaining exchanges #3912

Closed

chadwhitacre mentioned this pull request Jun 2, 2017

Clean up exchanges #4442

Closed

15 tasks

kaguillera mentioned this pull request Jun 15, 2017

Start recording ref when recording an exchange in masspay #4518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start recording ref in exchanges table #4361

Start recording ref in exchanges table #4361

kaguillera commented Mar 2, 2017

kaguillera commented Mar 2, 2017

nobodxbodon Mar 3, 2017

kaguillera Mar 3, 2017

nobodxbodon Mar 3, 2017 •

edited

Loading

chadwhitacre Mar 3, 2017

chadwhitacre Mar 3, 2017

kaguillera Mar 3, 2017 •

edited

Loading

chadwhitacre Mar 3, 2017

kaguillera Mar 3, 2017 •

edited

Loading

mattbk Mar 3, 2017

kaguillera Mar 3, 2017

chadwhitacre Mar 3, 2017

kaguillera Mar 3, 2017

kaguillera Mar 3, 2017

kaguillera commented Mar 3, 2017

kaguillera commented Mar 3, 2017

mattbk commented Mar 3, 2017

kaguillera commented Mar 17, 2017

kaguillera commented Mar 17, 2017

mattbk commented Mar 17, 2017 •

edited

Loading

kaguillera commented Mar 17, 2017 •

edited

Loading

mattbk commented Mar 17, 2017 •

edited

Loading

Start recording ref in exchanges table #4361

Start recording ref in exchanges table #4361

Conversation

kaguillera commented Mar 2, 2017

kaguillera commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nobodxbodon Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaguillera Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaguillera Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaguillera commented Mar 3, 2017

kaguillera commented Mar 3, 2017

mattbk commented Mar 3, 2017

kaguillera commented Mar 17, 2017

kaguillera commented Mar 17, 2017

mattbk commented Mar 17, 2017 • edited Loading

kaguillera commented Mar 17, 2017 • edited Loading

mattbk commented Mar 17, 2017 • edited Loading

nobodxbodon Mar 3, 2017 •

edited

Loading

kaguillera Mar 3, 2017 •

edited

Loading

kaguillera Mar 3, 2017 •

edited

Loading

mattbk commented Mar 17, 2017 •

edited

Loading

kaguillera commented Mar 17, 2017 •

edited

Loading

mattbk commented Mar 17, 2017 •

edited

Loading