[16.0][IMP] shopinvader signin jwt anonymous cart #1523
Conversation
qgroulard
force-pushed
the
16.0-shopinvader_signin_anonymous_cart-qgr
branch
3 times, most recently
from
b54f554
to
68e4be1
Compare
| "shopinvader_api_signin_jwt.signin_router.helper" | ||
| ]._create_partner_from_payload(payload) | ||
| response.status_code = status.HTTP_201_CREATED | ||
| anonymous_partner = env["res.partner"]._get_anonymous_partner__cookie( |
There was a problem hiding this comment.
@qgroulard who is supposed to call _create_anonymous_partner__cookie?
Shouldn't it happen the 1st time you call a cart endpoint w/o authenticated customer?
| if not cart: | ||
| cart = self._create_empty_cart(partner_id) | ||
| cart._update_cart_lines_from_cart(self) | ||
| self.unlink() |
There was a problem hiding this comment.
Why deleting the cart?
And what about the UUID?
If you delete it you should keep they UUID for ext reference.
There was a problem hiding this comment.
You are right, it doesn't make sense to delete the cart there.
I need to delete the anonymous cart to be able to delete the anonymous partner. I do it in the /signin route now.
qgroulard
force-pushed
the
16.0-shopinvader_signin_anonymous_cart-qgr
branch
from
68e4be1
to
cf58a78
Compare
|
|
||
| def _transfer_cart(self, partner_id): | ||
| """ | ||
| Transfer the current cart to a given partner |
There was a problem hiding this comment.
Maybe expand this docstring. Something like this?
| Transfer the current cart to a given partner | |
| Copy the lines of this cart to the current cart of a given partner. Merge lines of identical products. | |
| If the partner does not have a current cart yet, create one. | |
| This cart is not deleted. It is the responsibility of the caller to remove it if desired. |
There was a problem hiding this comment.
This cart is not deleted. It is the responsibility of the caller to remove it if desired.
this part is very helpful 😉
There was a problem hiding this comment.
maybe a delete option could be useful?
There was a problem hiding this comment.
Or another method _transfer_and_unlink_cart
There was a problem hiding this comment.
But in the end it's simply a matter of adding an unlink line at the call site, so a different method or a delete flag may not ease the life of the caller that much.
There was a problem hiding this comment.
Thanks for the input.
I like to keep it simple, I think this is not necessary to add a delete option or a _transfer_and_unlink_cart() method. As @sbidoul said, this is just a matter of adding an unlink after the call.
I have added the suggested docstring.
There was a problem hiding this comment.
I'm fine w/ the proposed solution, however... I don't really see the point for keeping this cart.
Especially in the ctx of the anon partner you MUST delete the cart.
As this meth is called "transfer cart" where do you see a use for keeping the old cart?
There was a problem hiding this comment.
I don't know, you tell me 😂 I thought you had a use case for that, as you started this discussion with this comment:
Why deleting the cart?
And what about the UUID?
If you delete it you should keep they UUID for ext reference.
For me personally, the only use I have of this method is in the context of anonymous partners. But I like the idea to keep this method atomic and let people do what they want with the cart.
qgroulard
force-pushed
the
16.0-shopinvader_signin_anonymous_cart-qgr
branch
from
cf58a78
to
ba4e512
Compare
There was a problem hiding this comment.
@qgroulard Can you add some doc on how to enable this functionality when defining your fastapi app (the required addon, how you mount the router and define the authentication policy, ...)
|
Documentation will be added into another PR |
|
/ocabot merge minor |
|
This PR looks fantastic, let's merge it! |
|
Congratulations, your PR was merged at 97e8563. Thanks a lot for contributing to shopinvader. ❤️ |
Handle anonymous cart at signin:
Discussion threads regarding
/signinand anonymous carts can be found here: #1428 and #1510