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 #645

Merged
merged 6 commits into from
May 9, 2018
Merged

Conversation

fay-stripe
Copy link
Contributor

No description provided.

@fay-stripe fay-stripe self-assigned this May 4, 2018
@fay-stripe
Copy link
Contributor Author

cc @matt-stripe

@brandur-stripe
Copy link
Contributor

@fay A couple notes to maybe save you a bit of time. You're going to need a resource_url override for your class. You can look at subscription items as an example:

    def self.resource_url
      "/v1/subscription_items"
    end

Next, we're going to need to make sure that the new endpoints are in the OpenAPI spec, which probably means changing their stability in the backend, then making sure that we get the new version out and into stripe-mock so that the tests can run against them.

Lastly, it'll probably be beneficial to make sure you're able to get everything up and running locally so that your testing loop isn't CI. There's instructions in the README.

@fay-stripe
Copy link
Contributor Author

I think I've got it!

ptal @brandur-stripe

@fay-stripe
Copy link
Contributor Author

Never mind it's still failing, let me fix that real quick.

@fay-stripe
Copy link
Contributor Author

Ok now it should be right

ptal @brandur-stripe


OBJECT_NAME = "issuer_fraud_record".freeze

def self.from_charge(params, opts = {})
Copy link
Contributor

Choose a reason for hiding this comment

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

@fay-stripe I don't think we need this because the built-in list method will accept optional parameters that allow you to pass a charge. Here's an example of it in action for invoices:

charges = Stripe::Invoice.list(customer: "cus_123")

}
assert issfrs.data.is_a?(Array)
assert issfrs.data[0].is_a?(Stripe::IssuerFraudRecord)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also take this test out — in practice it won't really be exercising a different code path (and we already have tests around the core list functionality).

@brandur-stripe
Copy link
Contributor

@fay-stripe You have a few Rubocop offenses:

lib/stripe/issuer_fraud_record.rb:21:4: C: Layout/TrailingBlankLines: Final newline missing.
end
   
lib/stripe/util.rb:4:3: C: Metrics/ModuleLength: Module has too many lines. [307/306]
  module Util ...
  ^^^^^^^^^^^
test/stripe/issuer_fraud_record_test.rb:14:25: C: Style/TrailingCommaInArguments: Avoid comma after the last parameter of a method call.
        charge: "ch_123",
                        ^

For the "module has too many lines", I think unfortunately the answer is to pop open .rubocop_todo.yml, find Metrics/ModuleLength and increment the number there. The test offense will probably disappear when you remove that test case.

ptal @fay-stripe

@stripe-ci
Copy link

@fay-stripe
Copy link
Contributor Author

ptal @brandur-stripe

@brandur-stripe
Copy link
Contributor

Thanks Fay! LGTM.

@brandur-stripe brandur-stripe merged commit 6ce4519 into master May 9, 2018
@brandur-stripe brandur-stripe deleted the fay/issfr branch May 9, 2018 21:55
@brandur-stripe
Copy link
Contributor

Released as 3.14.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.

3 participants