Add support for v1/issuer_fraud_records endpoint #456
Conversation
|
Thanks! Fay, as discussed in chat, stripe-node is not quite on stripe-mock yet. Can you see if you can get your failing test fixed? You can see CI for details, but should also be able to reproduce it locally. |
lib/resources/IssuerFraudRecords.js
Outdated
| retrieveFromCharge: stripeMethod({ | ||
| method: 'GET', | ||
| path: function(urlData) { | ||
| return '?charge=' + urlData.chargeId; |
There was a problem hiding this comment.
@brandur @matt-stripe any advice here? This still isn't right, because then the url is /v1/issuer_fraud_records/?charge=.... whereas I want it to be /v1/issuer_fraud_records?charge=....
I haven't found another example of this ^ but please do direct me if there is one
There was a problem hiding this comment.
I also would actually like the test to be data: {charge: 'ch_123456789'} versus doing the urlparams. At least, I think that's what we'd prefer to test here
|
I got the tests to pass but I'm not sure if there's a better / more correct way of doing the |
|
I think I keep tagging your brandur handle instead of brandur-stripe. Sorry about that! |
There was a problem hiding this comment.
Left a couple comments, but I think we're almost there.
ptal @fay-stripe
lib/resources/IssuerFraudRecords.js
Outdated
| return 'issuer_fraud_records?charge=' + urlData.chargeId; | ||
| }, | ||
| urlParams: ['chargeId'], | ||
| }), |
There was a problem hiding this comment.
Like in Ruby, list allows you to pass parameters to it. Here's an example from the subscriptions test suite:
stripe.subscriptions.list({
limit: 3,
customer: 'test_cus',
plan: 'gold',
});
lib/resources/IssuerFraudRecords.js
Outdated
| var stripeMethod = StripeResource.method; | ||
|
|
||
| module.exports = StripeResource.extend({ | ||
| path: '', |
There was a problem hiding this comment.
Why empty the path up here and then pass one in each method? I think you should be able to implement this almost identically to country spec:
module.exports = StripeResource.extend({
path: 'country_specs',
includeBasic: [
'list', 'retrieve',
],
});There was a problem hiding this comment.
👍 I didn't know list could take a param as I missed the other examples of it. When I implemented a new method, the base path automatically tacks on / when anything is appended to it, so I separated it out in order to make sure the / didn't get inserted.
But I'll switch it over to list and that should make everything cleaner. Thanks!
| data: {}, | ||
| headers: {}, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I think we can safely drop this test case, but if we don't, it should go in the list block below instead of this retrieve block.
|
This build is taking a long time to start. I think I made the right changes though ptal @brandur-stripe |
|
|
||
| describe('IssuerFraudRecord Resource', function() { | ||
| describe('retrieve', function() { | ||
| it('Sends the correct request for issfr ID', function() { |
There was a problem hiding this comment.
Can you truncate this down to just "Sends the correct request" for consistency?
| headers: {}, | ||
| }); | ||
| }); | ||
| it('Sends the correct request for charge ID', function() { |
There was a problem hiding this comment.
Let's just remove this test case please. It's not actually exercising any new code that we've written.
There was a problem hiding this comment.
Yes, that looks much better! A couple more minor comments, but looks good.
ptal @fay-stripe
|
Made those changes! |
|
LGTM. Thanks Fay! |
|
Released as 5.9.0. |
need to wait for update to stripe mock