Typo for retrieving invoice items + Adds handler for Usage endpoint #433

eecaro · 2018-12-06T18:55:08Z

https://stripe.com/docs/api/usage_records
https://raw.githubusercontent.com/stripe/openapi/master/openapi/spec3.yaml

close #431

snewcomer

Awesome work! Look forward to getting this in. Just a few comments for the moment. 🙇

snewcomer · 2018-12-06T19:08:09Z

lib/stripe/subscriptions/usage.ex

+          livemode: boolean,
+          quantity: non_neg_integer,
+          subscription_item: Stripe.id() | Stripe.SubscriptionItem.t() | nil,
+          timestamp: Stripe.timestamp() | nil


Are timestamp and subscription item nullable?

I look in here usually: https://raw.githubusercontent.com/stripe/openapi/master/openapi/spec3.yaml

and search usage_record:

snewcomer · 2018-12-06T19:10:05Z

lib/stripe/subscriptions/usage.ex

+          optional(:starting_after) => t | Stripe.id()
+        } 
+  def list(params, opts \\ []) do
+    url = 


minor formatting here and above

url = params |> Map.pop(:subscription_item) |> build_url

snewcomer · 2018-12-06T19:12:12Z

lib/stripe/subscriptions/usage.ex

+    |> Map.pop(:subscription_item)
+    |> build_url
+
+    new_request(opts)


Looks like for list endpoint uses usage_record_summaries.

/v1/subscription_items/{SUBSCRIPTION_ITEM_ID}/usage_record_summaries

snewcomer · 2018-12-06T19:13:38Z

lib/stripe/subscriptions/line_item.ex

@@ -69,7 +69,7 @@ defmodule Stripe.LineItem do
             } | %{}
  def retrieve(id, params \\ %{}, opts \\ []) do
    new_request(opts)
-    |> put_endpoint("invoices" <> "/#{get_id!(id)}" <> "lines")
+    |> put_endpoint("invoices" <> "/#{get_id!(id)}/" <> "lines")


nice catch! 👍

snewcomer · 2018-12-06T19:14:03Z

lib/stripe/subscriptions/usage.ex

+    url = 
+    params 
+    |> Map.pop(:subscription_item)
+    |> build_url


r/build_url/build_url() here and below

snewcomer · 2018-12-06T19:16:12Z

lib/stripe/subscriptions/usage.ex

+  @type t :: %__MODULE__{
+          id: Stripe.id(),
+          object: String.t(),
+          livemode: boolean,


So it looks like there are two structs - Usage && UsageSummary, the latter having a field invoice: String.t() | nil. I think we could just use one struct and add invoice. What do you think?

yeah, I was looking at this too, but following how things were setup, I was thinking some might want it treated differently. So was trying to stay close to how Stripe has it setup

Right that makes sense. However, I think since the list method is wrapped up in this module, we probably need to include this in the struct or else that k-v will be thrown out of the response. The other option is to make a new struct/module for the UsageSummary, which might be overkill.

https://stripe.com/docs/api/usage_records/subscription_item_summary_list

good point, adding this in

snewcomer · 2018-12-06T19:22:27Z

lib/stripe/subscriptions/usage.ex

+    new_request(opts)
+    |> put_endpoint(url)
+    |> put_method(:get)
+    |> put_params(params)


|> cast_to_id([:ending_before, :starting_after])

Put this after put_params?

yep after! This will just ensure if a user passes an object struct.

snewcomer · 2018-12-06T19:23:14Z

lib/stripe/subscriptions/usage.ex

+    new_request(opts)
+    |> put_endpoint(url)
+    |> put_method(:post)
+    |> put_params(params)


|> cast_to_id([:subscription_item])

(dup of comment above) We need |> put_param(:subscription_item, id) since that is a required param to pass to stripe.

From their docs, it looks like :subscription_item is required, but as a required path param. When passing it along with the other request parameters, Stripe returns an error that the additional properties are not allowed: "subscription item"

{:error, %Stripe.Error{ code: :invalid_request_error, extra: %{ http_status: 400, raw_error: %{ "message" => "Request validation error: validator 0xc00049ba70 failed: additional properties are not allowed: subscription_item", "type" => "invalid_request_error" } }, message: "Request validation error: validator 0xc00049ba70 failed: additional properties are not allowed: subscription_item", request_id: {"Request-Id", "req_123"}, source: :stripe, user_message: nil }}

Oh you are right! THx for testing

snewcomer · 2018-12-06T19:31:08Z

lib/stripe/subscriptions/usage.ex

+  def list(params, opts \\ []) do
+    url = 
+    params 
+    |> Map.pop(:subscription_item)


Map.pop returns a tuple {value(), map()}

However, maybe we can get around that. Since subscription_item.id is a required param, what do you think about removing subscription_item from params and making it the first argument of the list function?

@spec list(Stripe.id(), params, Stripe.options) def list(id, params \\ %{}, opts \\ []) do

I like this, and feels cleaner, thanks for the suggestion

Also going to update the create for this as well

coveralls · 2018-12-08T08:27:35Z

Coverage increased (+0.6%) to 83.493% when pulling 5f32352 on MetisMachine:webhooks into 8dd6441 on code-corps:master.

Cleaned up create and list functions for a little more simplicity

* missing path separator * Corrections per recommendations Cleaned up create and list functions for a little more simplicity * add header function and function matching criteria * expect a Stripe.List object of line items * build usage record summaries url * change to test file convention * additional checks for line items test

eecaro · 2018-12-12T22:22:53Z

@snewcomer hi for some reason the checks did not kick off for the latest commits, but they were checked in the duplicate PR #431 and passed. Would you mind seeing if the checks could be run for this PR or revert back to considering #431? thanks!

snewcomer · 2018-12-14T18:06:15Z

lib/stripe/subscriptions/usage.ex

+    new_request(opts)
+    |> put_endpoint(build_url(item))
+    |> put_method(:post)
+    |> put_params(params)


Here we need |> put_param(:subscription_item, id) since that is a required param to pass to stripe.

Also, I think we can get rid of the first block here and just require an id to be passed instead of an object. DO you have a case where only the object can be passed?

I think that is a good call out. If you have the object, you can grab the id out of it before calling the function. I was just thinking you could streamline the process by allowing the object passed in.

It is a good idea, but to keep parity with most other methods, we probably want to just pass the id. THanks for sticking with this PR!

snewcomer

This is great! Some minor cleanup items and this is g2g!

snewcomer · 2018-12-27T16:30:48Z

lib/stripe/subscriptions/usage.ex

+              :timestamp => Stripe.timestamp() | non_neg_integer,
+              optional(:action) => String.t()
+             } 
+  def create(id, params, opts) do


We should probably default opts to opts \\ [] in case nobody passes the 3rd arg.

I think the case where the opts is not passed in is handled by the header on line 36.

oh yep you are right!

snewcomer · 2018-12-27T16:43:51Z

lib/stripe/subscriptions/usage.ex

+  """
+  @spec create(Stripe.id(), params, Stripe.options()) :: {:ok, t} | {:error, Stripe.Error.t()}
+        when params: %{
+              :quantity => float,


r/float/integer

https://stripe.com/docs/api/usage_records/object

snewcomer · 2018-12-27T16:54:44Z

lib/stripe/subscriptions/usage.ex

+    new_request(opts)
+    |> put_endpoint(url)
+    |> put_method(:post)
+    |> put_params(params)


Oh you are right! THx for testing

snewcomer · 2018-12-27T16:56:35Z

lib/stripe/subscriptions/usage.ex

+
+  defp build_list_url(item) do
+    "#{@plural_endpoint}/#{item}/usage_record_summaries"  
+  end


Feel free to inline this if you wanted.

i.e. probably don't need this method.

snewcomer · 2019-01-14T04:31:05Z

@eecaro @wess Hey! Great work on this feature. Any thoughts on getting the last few things patched up? Probably do a release this week or so. Again, thanks a ton for the hard work on this 🙏

eecaro · 2019-01-15T18:45:11Z

@snewcomer sure thing! I will address these last few changes soon.

snewcomer · 2019-02-06T05:35:19Z

@eecaro @wess sry to bother again, but lmk if you want help with anything (just a few remaining comments)! This is a really nice PR for us to get in.

eecaro · 2019-02-06T13:54:57Z

@snewcomer I think all of the comments have been addressed. Please let me know if we missed one, thanks!

snewcomer

Phenomenal work! Thanks a ton for sticking this out!

wess and others added 5 commits December 4, 2018 15:05

Adds handler for Usage endpoint

d5b1ba1

Coverage fixes

b47837c

Function signature fix in Usage for url building

b781d9e

Removes unused var

81fb8ad

missing path separator

f4c69e5

eecaro changed the title ~~typo for retrieving invoice items~~ Typo for retrieving invoice items + Adds handler for Usage endpoint Dec 6, 2018

snewcomer reviewed Dec 6, 2018

View reviewed changes

snewcomer assigned eecaro Dec 6, 2018

snewcomer added enhancement 2.0 labels Dec 6, 2018

snewcomer mentioned this pull request Dec 7, 2018

Adds handler for Usage endpoint #431

Closed

wess and others added 7 commits December 10, 2018 16:50

Corrections per recommendations

d678f69

Cleaned up create and list functions for a little more simplicity

add header function and function matching criteria

3521e00

expect a Stripe.List object of line items

46ba4d1

build usage record summaries url

7d9e356

change to test file convention

87b1be7

additional checks for line items test

53e8d80

wess and others added 3 commits December 13, 2018 12:12

Adds optional invoice item to Usage struct

d1ebe7f

cast to ids in case of object struct

efba879

Merge branch 'master' into webhooks

a31a789

snewcomer reviewed Dec 14, 2018

View reviewed changes

remove function that accepts subscription item object

35c544f

snewcomer reviewed Dec 27, 2018

View reviewed changes

build url inline

5f32352

snewcomer approved these changes Feb 6, 2019

View reviewed changes

snewcomer merged commit 0c94360 into beam-community:master Feb 6, 2019

Typo for retrieving invoice items + Adds handler for Usage endpoint #433

Typo for retrieving invoice items + Adds handler for Usage endpoint #433

Conversation

eecaro commented Dec 6, 2018 • edited by snewcomer

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

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

coveralls commented Dec 8, 2018 • edited

eecaro commented Dec 12, 2018 • edited

Choose a reason for hiding this comment

snewcomer Dec 14, 2018 • edited

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

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 Dec 27, 2018 • edited

Choose a reason for hiding this comment

snewcomer commented Jan 14, 2019

eecaro commented Jan 15, 2019

snewcomer commented Feb 6, 2019

eecaro commented Feb 6, 2019

snewcomer left a comment

Choose a reason for hiding this comment

eecaro commented Dec 6, 2018 •

edited by snewcomer

coveralls commented Dec 8, 2018 •

edited

eecaro commented Dec 12, 2018 •

edited

snewcomer Dec 14, 2018 •

edited

snewcomer Dec 27, 2018 •

edited