Update stripe-mock to v0.30.0 #414

mcrumm · 2018-08-31T22:33:48Z

Relates to #413

Changes:

Update to stripe-mock v0.30.0 in tests (2f8396d)
Pass query param validation -- ensure GET requests send all params in the query string (2b996b3)
Fix Stripe.Account.create/2 sending account in the POST body (it's in the path) (c50b31b)
Fix return values for .delete/2 calls (411c124)
Remove forwards-incompatible params from test suite
Skip a few tests that are either forwards-incompatible (at_period_end on subscription delete), or erroneously failing parameter validation (external_accounts with ?object=) (545ece2)

Sends request params in the query string, to account for stripe/stripe-mock@09826da introducing parameter validation (and removed request body validation) for `GET` requests, since the body of a GET request has no semantic meaning. This broke endpoints like `/invoices/upcoming?customer=cus_1234`, because we were sending the `customer` param in the request body.

coveralls · 2018-08-31T22:38:47Z

Coverage decreased (-3.1%) to 81.111% when pulling 162b077 on mcrumm:with-stripe-mock-0_30_0 into 4d4a6dd on code-corps:master.

snewcomer

@mcrumm You did a really nice job on this PR. Lmk about a few questions below and then we can work to get this merged asap!

snewcomer · 2018-09-04T04:09:32Z

test/stripe/core_resources/customer_test.exs

@@ -17,11 +17,10 @@ defmodule Stripe.CustomerTest do
    assert_stripe_requested(:post, "/v1/customers/cus_123")
  end

-  test "is deleteable" do
+  test "is deleteble" do


r/deleteble/deletable

Nice catch! I dropped the wrong letter 😄

snewcomer · 2018-09-04T04:31:25Z

test/stripe/subscriptions/subscription_test.exs

@@ -42,6 +44,7 @@ defmodule Stripe.SubscriptionTest do
      assert_stripe_requested(:delete, "/v1/subscriptions/#{subscription.id}")
    end

+    @tag :skip


I think the delete/2 && delete/3 can be simplified to delete/2 that simply takes the Stripe.id and optional Stripe.options. Basically at_period_end is not a potential parameter to pass anymore

https://stripe.com/docs/api#cancel_subscription

Yep, that's absolutely true as of 2018-08-23. I was trying not to muddy the concept of the stripe-mock update with the Stripe API version update.

I'd be happy to make that change here, or I can leave it for the API update PR. Let me know what makes the most sense to you.

@mcrumm Oh I see what you did. So follow up PR is probably a good idea 👍

snewcomer · 2018-09-04T04:42:37Z

.travis.yml

@@ -10,7 +10,7 @@ otp_release:
 sudo: false
 env:
  global:
-    - STRIPE_MOCK_VERSION=0.16.1
+    - STRIPE_MOCK_VERSION=0.30.0


snewcomer · 2018-09-04T04:48:27Z

lib/stripe/connect/account.ex

@@ -317,7 +317,6 @@ defmodule Stripe.Account do
          {:ok, t} | {:error, Stripe.Error.t()}
  def reject(id, reason, opts \\ []) do
    params = %{
-      account: id,


snewcomer · 2018-09-04T04:51:44Z

lib/stripe/api.ex

-  @doc """
-  """
-  @spec request_file_upload(body, method, String.t(), headers, list) ::
-          {:ok, map} | {:error, Stripe.Error.t()}


Should we leave this?

This was a duplication on the second function signature. The first signature for request_file_upload already defines the @spec, and I think I just got tired of looking past the dialyzer warning 😄

snewcomer · 2018-09-04T04:56:04Z

lib/stripe/api.ex

+      |> Stripe.URI.encode_query()
+      |> prepend_url("#{base_url}#{endpoint}")
+
+    perform_request(req_url, :get, "", headers, opts)


So the main difference between the two requests is :get will pass "" for the body and the other won't? I'm wondering if both of these request/5 methods could be simplified into one...

Otherwise, this change looks great!!

Correct, and any additional parameters for a :get request go into the query string. For instance, /v1/invoices/upcoming?customer=cus_1234 previously sent customer=cus_1234 in the GET body, which stopped validating on the latest stripe-mock.

My methodology here was basically "make the least number of changes required", but I agree that it seems harder to follow than it should be.

Gotcha. So we basically have expand request parameters and other parameters. We can send them in the url or body. Currently we send the expandable items in the url and the rest in the body. With a GET request, we send both in the url.

I'm thinking we should brainstorm something to reduce the duplication and have 1 request/5 method but that can be a future refactor as you stated.

What do you think?

Yep, that was my thought as well. Refactoring the URL building functions should make it simple enough to reduce request/5 to a single function body.

snewcomer · 2018-09-05T05:10:31Z

test/stripe/subscriptions/subscription_item_test.exs

@@ -30,9 +30,8 @@ defmodule Stripe.SubscriptionItemTest do
  describe "delete/2" do
    test "deletes a subscription" do
      {:ok, subscription_item} = Stripe.SubscriptionItem.retrieve("sub_123")
-      assert {:ok, %{deleted: deleted, id: _id}} = Stripe.SubscriptionItem.delete("sub_123")
+      assert {:ok, %Stripe.SubscriptionItem{}} = Stripe.SubscriptionItem.delete("sub_123")


I should clarify that this changed because stripe-mock now returns the "object" in the response (not sure of exact version), thus we convert to a struct with id populated and rest of keys nil, whereas before, we couldn't determine the struct, so we just processed the bare map.

Meaning stripe-mock is returning %{deleted: deleted, id: _id, object: object} but we convert it to a proper struct.

snewcomer · 2018-09-05T05:25:39Z

lib/stripe/api.ex

@@ -135,6 +135,20 @@ defmodule Stripe.API do
  """
  @spec request(body, method, String.t(), headers, list) ::
          {:ok, map} | {:error, Stripe.Error.t()}
+  def request(body, :get, endpoint, headers, opts) do


Lastly, do you think this is properly tested? e.g. a GET request with a body and expand params?

I do. On its own, the :get request change fixed the upcoming invoice test (you can verify the failure by testing the current master against a v0.30.x version of stripe-mock), and I've tested this branch against the live (well, test) Stripe API as well.

Our Stripe logs verify parameters coming in now as Request query parameters instead of Request POST body for GET requests.

snewcomer

@mcrumm Really really great work! Super easy to follow this PR.

Looks like we have 2 follow ups before we cut a proper release (2.3):

One delete/2 method
One request/5 method.

Otherwise, lmk if you are done working on this and we can hit the merge button!

mcrumm · 2018-09-05T23:42:44Z

Thanks for the feedback! Yep, I think this is good to merge, and then we can tackle the actual API update.

mcrumm added 5 commits August 31, 2018 11:52

Update stripe-mock in travis to v0.30.0

2f8396d

Remove account param from POST body

c50b31b

Fix return values for delete calls in tests

411c124

Fix/skip tests with deprecated/invalid fields

545ece2

snewcomer reviewed Sep 4, 2018

View reviewed changes

Fix test customer is deletable

162b077

snewcomer reviewed Sep 5, 2018

View reviewed changes

snewcomer assigned mcrumm Sep 5, 2018

snewcomer added enhancement 2.0 labels Sep 5, 2018

snewcomer reviewed Sep 5, 2018

View reviewed changes

snewcomer approved these changes Sep 5, 2018

View reviewed changes

snewcomer merged commit 212c8ee into beam-community:master Sep 6, 2018

mcrumm deleted the with-stripe-mock-0_30_0 branch September 7, 2018 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update stripe-mock to v0.30.0 #414

Update stripe-mock to v0.30.0 #414

mcrumm commented Aug 31, 2018

coveralls commented Aug 31, 2018 •

edited

Loading

snewcomer left a comment

snewcomer Sep 4, 2018

mcrumm Sep 4, 2018

snewcomer Sep 4, 2018

mcrumm Sep 4, 2018

snewcomer Sep 4, 2018

snewcomer Sep 4, 2018

snewcomer Sep 4, 2018

snewcomer Sep 4, 2018

mcrumm Sep 4, 2018

snewcomer Sep 4, 2018

snewcomer Sep 4, 2018

mcrumm Sep 4, 2018

snewcomer Sep 5, 2018 •

edited

Loading

mcrumm Sep 5, 2018

snewcomer Sep 5, 2018

snewcomer Sep 5, 2018

snewcomer Sep 5, 2018

mcrumm Sep 5, 2018

snewcomer left a comment

mcrumm commented Sep 5, 2018

Update stripe-mock to v0.30.0 #414

Update stripe-mock to v0.30.0 #414

Conversation

mcrumm commented Aug 31, 2018

coveralls commented Aug 31, 2018 • edited Loading

snewcomer left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer Sep 5, 2018 • 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

snewcomer left a comment

Choose a reason for hiding this comment

mcrumm commented Sep 5, 2018

coveralls commented Aug 31, 2018 •

edited

Loading

snewcomer Sep 5, 2018 •

edited

Loading