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

Add support for v1/issuer_fraud_records endpoint #425

Merged
merged 4 commits into from
May 10, 2018
Merged

Conversation

fay-stripe
Copy link
Contributor

Need to update stripe mock first

@fay-stripe fay-stripe self-assigned this May 7, 2018
self.assertIsInstance(resource, stripe.IssuerFraudRecord)

def test_is_retrievable_by_charge(self):
resource = stripe.IssuerFraudRecord.retrieve(TEST_CHARGE_ID)
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably doesn't take the exact same arguments as retrieving by issuer fraud record ID right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that's not right. Thanks for catching!



class IssuerFraudRecord(ListableAPIResource):
OBJECT_NAME = 'issuer_fraud_record'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you might need something like this here:

    @classmethod
    def class_name(cls):
        return 'issuer_fraud_record'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@brandur-stripe
Copy link
Contributor

Looks good! Can you try updating stripe-mock and addressing the couple points above?

ptal @fay-stripe

@fay-stripe
Copy link
Contributor Author

@brandur-stripe I don't think I can update stripe-mock yet because #426 isn't in yet, right?

@brandur-stripe
Copy link
Contributor

Yeah, you'll have to leave this for now.

@brandur-stripe
Copy link
Contributor

@fay-stripe Can you rebase and take another look at this one? I just shipped a new version of stripe-mock that takes care of the previous incompatibility problems. Thanks!

@fay-stripe
Copy link
Contributor Author

ptal @brandur-stripe

'get',
'/v1/issuer_fraud_records/%s' % TEST_RESOURCE_ID
)
self.assertIsInstance(resource, stripe.IssuerFraudRecord)
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 removed the test case around CHARGE_ID since that should come as part of list's functionality, just like in the other libraries



TEST_RESOURCE_ID = 'issfr_123'
TEST_CHARGE_ID = 'ch_123'
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 get rid of TEST_CHARGE_ID? I don't think it's used for anything anymore.

ptal @fay-stripe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup saw it right after I assigned you :)

@fay-stripe
Copy link
Contributor Author

ptal @brandur-stripe

@brandur-stripe
Copy link
Contributor

Excellent. Thanks Fay!

@brandur-stripe brandur-stripe merged commit 7d5bc40 into master May 10, 2018
@brandur-stripe brandur-stripe deleted the fay/issfr branch May 10, 2018 22:15
@brandur-stripe
Copy link
Contributor

Released as 1.81.0.

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

Successfully merging this pull request may close these issues.

None yet

2 participants