-
Notifications
You must be signed in to change notification settings - Fork 352
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
Typo for retrieving invoice items + Adds handler for Usage endpoint #433
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! Look forward to getting this in. Just a few comments for the moment. 🙇
lib/stripe/subscriptions/usage.ex
Outdated
livemode: boolean, | ||
quantity: non_neg_integer, | ||
subscription_item: Stripe.id() | Stripe.SubscriptionItem.t() | nil, | ||
timestamp: Stripe.timestamp() | nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
lib/stripe/subscriptions/usage.ex
Outdated
optional(:starting_after) => t | Stripe.id() | ||
} | ||
def list(params, opts \\ []) do | ||
url = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor formatting here and above
url =
params
|> Map.pop(:subscription_item)
|> build_url
|> Map.pop(:subscription_item) | ||
|> build_url | ||
|
||
new_request(opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like for list endpoint uses usage_record_summaries
.
/v1/subscription_items/{SUBSCRIPTION_ITEM_ID}/usage_record_summaries
@@ -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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch! 👍
lib/stripe/subscriptions/usage.ex
Outdated
url = | ||
params | ||
|> Map.pop(:subscription_item) | ||
|> build_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r/build_url
/build_url()
here and below
@type t :: %__MODULE__{ | ||
id: Stripe.id(), | ||
object: String.t(), | ||
livemode: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, adding this in
new_request(opts) | ||
|> put_endpoint(url) | ||
|> put_method(:get) | ||
|> put_params(params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|> cast_to_id([:ending_before, :starting_after])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this after put_params?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep after! This will just ensure if a user passes an object struct.
new_request(opts) | ||
|> put_endpoint(url) | ||
|> put_method(:post) | ||
|> put_params(params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|> cast_to_id([:subscription_item])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(dup of comment above) We need |> put_param(:subscription_item, id) since that is a required param to pass to stripe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you are right! THx for testing
lib/stripe/subscriptions/usage.ex
Outdated
def list(params, opts \\ []) do | ||
url = | ||
params | ||
|> Map.pop(:subscription_item) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, and feels cleaner, thanks for the suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also going to update the create for this as well
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
@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! |
lib/stripe/subscriptions/usage.ex
Outdated
new_request(opts) | ||
|> put_endpoint(build_url(item)) | ||
|> put_method(:post) | ||
|> put_params(params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we need |> put_param(:subscription_item, id)
since that is a required param to pass to stripe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Some minor cleanup items and this is g2g!
:timestamp => Stripe.timestamp() | non_neg_integer, | ||
optional(:action) => String.t() | ||
} | ||
def create(id, params, opts) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably default opts
to opts \\ []
in case nobody passes the 3rd arg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the case where the opts
is not passed in is handled by the header on line 36.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yep you are right!
lib/stripe/subscriptions/usage.ex
Outdated
""" | ||
@spec create(Stripe.id(), params, Stripe.options()) :: {:ok, t} | {:error, Stripe.Error.t()} | ||
when params: %{ | ||
:quantity => float, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r/float/integer
new_request(opts) | ||
|> put_endpoint(url) | ||
|> put_method(:post) | ||
|> put_params(params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you are right! THx for testing
lib/stripe/subscriptions/usage.ex
Outdated
|
||
defp build_list_url(item) do | ||
"#{@plural_endpoint}/#{item}/usage_record_summaries" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to inline this if you wanted.
i.e. probably don't need this method.
@snewcomer sure thing! I will address these last few changes soon. |
@snewcomer I think all of the comments have been addressed. Please let me know if we missed one, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phenomenal work! Thanks a ton for sticking this out!
https://stripe.com/docs/api/usage_records
https://raw.githubusercontent.com/stripe/openapi/master/openapi/spec3.yaml
close #431