Usage records and other fixes by pandomic · Pull Request #197 · adrienverge/localstripe

pandomic · 2021-09-07T12:22:42Z

Summary

Fix "list" endpoint for subscription items (3e99357)
Add usage record endpoints (ab721fa)

H--o-l · 2021-09-07T12:42:16Z

About the commit fix(javascript): Be less pedantic about already modified DOM can you describe why you need it? What is your use case? What issue does it solve for you?
Also the commit change two lines that I'm not sure are related. With an explanation inside the commit message, it would be easier for me to understand what you are doing.

About chore(gitignore): Add .venv and .idea to gitignore and chore(Dockerfile): Add Dockerfile , localstripe doesn't need that, it's specific to your environment, and we don't want to maintain it. Can you remove them?

About the last two commits, they look great!
We will try to review them in-deep soon.

pandomic · 2021-09-07T17:14:17Z

Hi @H--o-l, I left only subscription items and usage records-related changes. Looking forward to getting them merged.

Going back to your question, js-fix was there because of the mounting issue we had with React but leaving it out of scope for now.

I hope in the future we will see a containerized version as well ;)

H--o-l

Hey @pandomic !

Again thanks for your work on this PR!

Here is the first series of requests, I mostly check the first commit.

H--o-l · 2021-09-08T07:52:36Z

+        li = List(url, limit=limit, starting_after=starting_after)
+        li._list = [value for key, value in store.items()
+                    if key.startswith(cls.object + ':')
+                    and value.subscription == subscription]


Plop!

Looking at implementation of _api_list_all in a few other object, I think here we should use something more like this:

li = super(SubscriptionItem, cls)._api_list_all( url, limit=limit, starting_after=starting_after) li._list = [obj for obj in li._list if value.subscription == subscription]

What it does it's that it re-uses the code of the parent class instead of accessing the store directly.
Do you think it would work?
(I didn't test it, maybe it needs improvement).

H--o-l · 2021-09-08T07:55:38Z

+        except AssertionError:
+            raise UserError(400, 'Bad request')
+
+        # TODO: implement ending_before


There is a few TODO in the existing code, but they always suggest a better way for something already existing.
If we were to TODO all missing arguments of the API, the file would be 50% TODOs.
Instead, I propose to remove the missing feature from the function SubscriptionItem._api_list_all() prototype.
Like that, a motivated contributor could realize that it's missing and open a PR.

H--o-l · 2021-09-08T08:33:04Z


+si=$(curl -sSfg -u $SK: $HOST/v1/subscription_items?subscription=$sub \
+     | grep -oE 'si_\w+')
+[ -n "$si" ]


In this first commit, si is not a subscription item, it's two subscription items.
So maybe there could be a first res that stores them, a check of how many items there is in res and then, in the second commit a si=$(echo "$res" | head -n 1).
It would be more readable for future readers of this code.
What do you think?

👍 split into proposed steps

H--o-l · 2021-09-08T08:33:46Z

 SK=sk_test_12345

+curl -X DELETE $HOST/_config/data
+


Because it's not related to a localstripe feature, but it's more a CI change, this change needs to be inside another PR.
Like that we would be able to discuss it, as long as needed, without blocking this PR.

👍 removed this line

H--o-l · 2021-09-08T08:35:10Z

+        try:
+            assert _type(subscription) is str
+            assert subscription.startswith('sub_')
+            Subscription._api_retrieve(subscription)


Great to add this line!
But can it be moved outside the try/catch, I think there is no reason for this line to be in it.
Also can you add a small comment like # to return 404 if not existant on this line?

* override _api_list_all for subscription items * add `subscription` argument * filter subscription items by subscription id * add a test to verify endpoint works

* add api to record usage * add api to retrieve usage record summaries * add tests for usage records and summaries

pandomic · 2021-09-10T17:02:45Z

Hi @H--o-l, thanks for the feedback, I added proposed changes and force-pushed them to the current branch

H--o-l

Hey @pandomic!

Here is a few new comments, I let you see, and don't hesitate if you need me.

H--o-l · 2021-09-13T07:23:38Z

        if kwargs:
            raise UserError(400, 'Unexpected ' + ', '.join(kwargs.keys()))

+        # TODO: implement ending_before


Hey!
Please remove this TODO.

H--o-l · 2021-09-13T07:24:17Z


+    @classmethod
+    def _api_list_all(cls, url, subscription=None, limit=None,
+                      starting_after=None, ending_before=None, **kwargs):


Please remove ending_before=None here, so users know it's not implemented.

H--o-l · 2021-09-13T07:25:14Z

+            raise UserError(400, 'Unexpected ' + ', '.join(kwargs.keys()))
+
+        # to return 404 if not existant
+        Subscription._api_retrieve(subscription)


Please move these two lines after the try/except like in the other interfaces.

H--o-l · 2021-09-13T07:31:23Z

+        )
+
+        li._list = [obj for obj in li._list
+                    if obj.subscription == subscription]


A quick diff proposition here:

don't add a new line before ) so we save one line

don't add a new line after ) so we save one line, and it's more similar to the other API

--- a/localstripe/resources.py +++ b/localstripe/resources.py @@ -3160,9 +3160,7 @@ class SubscriptionItem(StripeObject): raise UserError(400, 'Bad request') li = super(SubscriptionItem, cls)._api_list_all( - url, limit=limit, starting_after=starting_after - ) - + url, limit=limit, starting_after=starting_after) li._list = [obj for obj in li._list if obj.subscription == subscription]

H--o-l · 2021-09-13T07:38:49Z

+                 timestamp=None):
+        self.quantity = quantity
+        self.subscription_item = subscription_item_id
+        self.timestamp = timestamp


Can you add check of kwargs and assert of parameters like in the other ressources __init__ function?
Assert should follow the Stripe API documentation on what is mandatory and what's not.

H--o-l · 2021-09-13T07:39:19Z

+        self.subscription_item = subscription_item_id
+        self.timestamp = timestamp
+
+        super().__init__()


Can you add a small # All exceptions must be raised before this point. above this line like in the other ressources __init__?

H--o-l · 2021-09-13T07:53:38Z

+
+        usage_records = [value for key, value in store.items()
+                         if key.startswith(cls.object + ':')
+                         and value.subscription_item == subscription_item_id]


When I see this operation, and the routes below (/v1/subscription_items/{subscription_item_id}/usage_records, /v1/subscription_items/{subscription_item_id}/usage_record_summaries) I think it would be simpler and more logical to have usage record stored direcly inside the corresponding subscription item.

Also I'm OK with a new object class UsageRecord() but I think I'm NOK with a class UsageRecord(StripeObject), because usage record are never used with the standard APIs that all other StripeObject implement, ie api_create, api_retrieve, api_update, api_delete, api_list_all and associated HTTP route and method (GET object/id, POST object/id etc).

So I think that routes /usage_records and /usage_record_summaries should be extensions of the object subscription item.

What do you think?

H--o-l · 2021-09-13T07:54:07Z

+  curl -sSfg -u $SK: $HOST/v1/subscription_items/$si/usage_record_summaries \
+  | grep -oP '"total_usage": \K([0-9]+)')
+[ "$total_usage" -eq 115 ]
+


pandomic mentioned this pull request Sep 7, 2021

feat(usage): add usage records anf fix sub item error #195

Closed

pandomic force-pushed the usage-records-and-other-fixes branch from 3b62a25 to 2e3ff02 Compare September 7, 2021 17:10

H--o-l reviewed Sep 8, 2021

View reviewed changes

pandomic added 2 commits September 10, 2021 19:50

fix(subscription-items): fix list endpoint

3e99357

* override _api_list_all for subscription items * add `subscription` argument * filter subscription items by subscription id * add a test to verify endpoint works

feat(usage-records): add usage records

ab721fa

* add api to record usage * add api to retrieve usage record summaries * add tests for usage records and summaries

pandomic force-pushed the usage-records-and-other-fixes branch from 2e3ff02 to ab721fa Compare September 10, 2021 16:58

pandomic requested a review from H--o-l September 10, 2021 17:03

H--o-l reviewed Sep 13, 2021

View reviewed changes

Conversation

pandomic commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

H--o-l commented Sep 7, 2021

Uh oh!

pandomic commented Sep 7, 2021

Uh oh!

H--o-l left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

H--o-l Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

H--o-l Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pandomic commented Sep 10, 2021

Uh oh!

H--o-l left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pandomic commented Sep 7, 2021 •

edited

Loading

H--o-l Sep 8, 2021 •

edited

Loading

H--o-l Sep 8, 2021 •

edited

Loading