TaxID: Expand API to allow retrieving or deleting a single TaxID

[feliixx]

API: Add 'object' to the response on Deletion

The goal is to be closer to Stripe API. Also, create a new DeletedObject class
that will be easier to use.

---

TaxID: Expand API to allow retrieving or deleting a single TaxID

Creating and listing TaxID have been supported for a long time, but retrieving
or deleting a single TaxID wasn't implemented, so let's change that.

Stripe doc:

• https://stripe.com/docs/api/customer_tax_ids/retrieve
• https://stripe.com/docs/api/customer_tax_ids/delete

[adrienverge]

Nice addition

[adrienverge]

For consistency with other routes,, I propose to keep the 2 GET before the POST.

[feliixx]

Ok, done.

[adrienverge]

Could it be simpler, and safer, to delete the item from obj.tax_ids._list that matches the ID? Instead of retrieving the whole object?

[feliixx]

I'm not sure that it would be simpler, as we would need to:

• iterate the list
• throw a 404 if the item is not in the list
• remove the item if tax_id match the id of the item
  Here, the first two steps are properly handled by _api_retrieve_tax_id, allowing us to avoid duplicating the logic.

I don't think that it would be safer either, as tax ID can't be modified (they can only be created or deleted), so it's not possible for a tax ID to change between _api_retrieve_tax_id and obj.tax_ids._list.remove (eg if there was a concurrent request). But maybe you had something else in mind?

Last argument: we're already doing something similar in the code:

localstripe/localstripe/resources.py

Lines 903 to 906 in cb071d2

	source_obj = cls._api_retrieve_source(id, source_id)
	type(source_obj)._api_delete(source_id)
	obj.sources._list.remove(source_obj)

[adrienverge]

OK, I must admit all these are good arguments 👍

Let's just keep in mind that .remove() can be tricky in some cases:

class O:
    def __init__(self, a):
        self.a = a

    def __repr__(self):
        return f' {self.a} '

>>> l = [O('a'), O('b'), O('c')]
>>> l.remove(l[1])
>>> print(l)
[ a ,  c ]

>>> l = [O('a'), O('b'), O('c')]
>>> l.remove(O('b'))
>>> print(l)
ValueError: list.remove(x): x not in list

[adrienverge]

Usually in these error message we use the English name of objects: "does not have a subscription", "no such invoice"... So I would use "does not have a tax ID" (even if it repeats the word ID), what do you think?

Stripe docs:

[feliixx]

ok, replaced does not have a TaxId... with does not have a tax ID...

[adrienverge]

(sorry I missed that on first review)

If I remember correctly, the spirit of these api_update() / api_delete() / api_extra() etc. is to only return JSON for types that we expect. Here, it looks like you modify api_extra() for your use-case (return direct JSON in a new delete case), but not the other functions. Also, if a new function returns something else than a dict or None, it cannot be serializable by json_response().

Instead of this implementation, I would rather see _api_delete_tax_id() return a new DeletedObject type (maybe it should inherit from StripeObject) to keep things consistent. What do you think?

[adrienverge]

Other (simpler) option: do like in api_delete() and allow returning either a dict or an object with _export() (but only those two):

localstripe/localstripe/server.py

Line 242 in cb071d2

return json_response(ret if isinstance(ret, dict) else ret._export())

[feliixx]

Declaring a new DeletedObject type was my initial idea until I saw

localstripe/localstripe/resources.py

Lines 156 to 161 in cb071d2

	def _api_delete(cls, id):
	key = cls.object + ':' + id
	if key not in store.keys():
	raise UserError(404, 'Not Found')
	del store[key]
	return {'deleted': True, 'id': id}

😄 (it's the only place where we're doing this though).

So I did it this way to keep the code similar, but I agree that this ret if not hasattr(ret, '_export') is ugly. I'll update both call site to use a DeletedObject if you agree!

[feliixx]

Other (simpler) option: do like in api_delete() and allow returning either a dict or an object with _export() (but only those two):

Our comments crossed, +1 for this solution that's indeed simpler

[adrienverge]

👍

Both options (isinstance(ret, dict) and DeletedObject) are good to me, with a slight preference for DeletedObject which seems more extendable.

[feliixx]

In the end went with DeletedObject that feels cleaner. Change extracted in another commit for extra clarity, as it also add the object attribute to the response (to be closer to Stripe API)

[adrienverge]

Much more satisfying 🙂

[adrienverge]

This is great, thanks for contributing!

[adrienverge]

An optimization could make it simpler to call the class:

Suggested change

	def __init__(self, id, object):
	self.id = id
	self.object = object
	def __init__(self, deleted_object):
	self.id = deleted_object.id
	self.object = deleted_object.object

(take it or leave it)

[feliixx]

we don't have the deleted_object in _api_delete, we only have its id and its class, so doing this would require to fetch the object first like this:

deleted_obj = store.get(cls.object + ':' + id)
...
del store[key]
return DeletedObject(deleted_obj)

which seems a bit heavy just to save an argument, so I prefer keeping it as is!

[adrienverge]

You're right 👍

[adrienverge]

Much more satisfying 🙂

[bravier]

Looking good and clean 👌

[H--o-l]

Good for me too!

[feliixx]

Thanks for the reviews!