adrienverge / localstripe

A fake but stateful Stripe server that you can run locally, for testing purposes.
GNU General Public License v3.0
192 stars 59 forks source link

add several list filtering improvements #186

Closed thrau closed 3 years ago

thrau commented 3 years ago

This PR adds several improvements to listing products, plans, customers, and subscriptions (stripe features that we use at @localstack)

I tried to conform to the semantic commit messages i found in the history, and tried to keep the code style as close to the original code as possible. Happy to make changes.

thrau commented 3 years ago

thanks for the comments, will make the changes towards the weekend

thrau commented 3 years ago

so i amended the history with the changes as suggested. and sorry in advance for the scope creep: i also added a couple of flake8 fixes and two lines to the gitignore (pycharm directories and the default .venv folder). also added the implementation of the customers/:id/sources?object= filter

thrau commented 3 years ago

i updated the PR with your change requests @H--o-l, hope to get this over the line now

thrau commented 3 years ago

with respect to the if kwargs check:

stripe does return an error on unknown arguments. however the error message is completely different from what localstripe returns:

 % curl "https://api.stripe.com/v1/plans?foo=bar" -u sk_test_4eC39HqLyjWDarjtT1zdp7dc:
{
  "error": {
    "code": "parameter_unknown",
    "doc_url": "https://stripe.com/docs/error-codes/parameter-unknown",
    "message": "Received unknown parameter: foo",
    "param": "foo",
    "type": "invalid_request_error"
  }
}

i mean sure, i'll add the assert to the methods i added (it's your project of course, so your decision). however

  1. some existing code is still inconsistent (e.g., PaymentMethod._api_list_all doesn't do the check either.
  2. i think this impedes the use of localstripe. some list APIs don't have all the existing optional arguments in their signatures, so using any of the query functionality that exists in stripe but isn't implemented in localstripe will break user code, which i think is worse than not getting errors back for requests that have invalid parameters. mostly you will use stripe clients to talk to the stripe server, which will check that stuff anyway.
  3. i'll have to resort using our fork, or spend significant time adding all the optional parameters to make it work

which is all fine i suppose, just something to think about.

anyway. i made all the requested changes.