[16.0] [IMP] shopinvader anonymous partner promotion

[paradoxxxzero]

This PR removes the cart transfer from shopinvader_api_signin_jwt (and its shopinvader_sale_cart dependency) and instead create a new _promote_anonymous_partner function that is now called by shopinvader_fastapi_auth_* modules when the two partner coexist.

A new shopinvader_sale_cart_anonymous_partner glue module handles the cart transfer on promotion.

This slightly changes the time when the cart is transfered from the login to the next request (which should be the cart GET anyway)

FYI @sebastienbeau

[sebastienbeau]

See my comment.
Otherwise thanks a lot for you good code ! The promotion implementation is super cool !

[paradoxxxzero]

@sbidoul, @lmignon could you review this PR please?

[sbidoul]

That's a big scary change :)

To help with the review, could you describe why this change is necessary? What is the use case that was not satisfied by the current implementation?

This slightly changes the time when the cart is transfered from the login to the next request (which should be the cart GET anyway)

This raises a warning in my head, as a GET should not normally not have side effects.

[sbidoul]

cc/ @qgroulard

[paradoxxxzero]

Of course, the main use case is this akretion#16: providing a way for different auth mechanisms to have the same behavior when an anonymous partner becomes a real partner (here called partner promotion) mainly resulting in a cart transfer.

I strongly think this shouldn't be handled by a module named shopinvader_api_signin_jwt as it is cart specific but not jwt specific.

As for the GET side effect I generally agree with this principle, as this does not get along really well with cache, but here the implementation is not GET specific and is triggered as soon two different partners kind are detected in the current auth dependency and as far as I can see the anonymous partner creation is currently also done here (

odoo-shopinvader/shopinvader_anonymous_partner/models/res_partner.py

Line 51 in 67e7319

def _create_anonymous_partner__token(self):

from

odoo-shopinvader/shopinvader_fastapi_auth_jwt/dependencies.py

Line 74 in 67e7319

anonymous_partner = env["res.partner"]._create_anonymous_partner__cookie(

).

[simahawk]

Please use BaseCommon class to disable mail tracking

[paradoxxxzero]

wdym?

[sbidoul]

@paradoxxxzero I think we should refactor step by step.

I strongly think this shouldn't be handled by a module named shopinvader_api_signin_jwt as it is cart specific but not jwt specific.

I tend to agree with that. But when we designed shopinvader_api_signin_jwt with @qgroulard our reasoning was that if anyone needs that jwt signin route without the cart, a refactoring could be done later.

Your idea of some kind of _promote method sounds good. So maybe we should start with that refactoring. I mean doing a PR that factors out these 4 lines, and nothing else, and see where these leads us.

        anonymous_cart = env["sale.order"].sudo()._find_open_cart(anonymous_partner.id)
        if anonymous_cart:
            anonymous_cart._transfer_cart(partner.id)
            anonymous_cart.unlink()

I also think these promote methods should be totally ignorant of any authentication stuff, so the part that returns a flag about the cookie does not belong there.

[paradoxxxzero]

Alright I removed the changes to the dependencies and called directly _promote_anonymous_partner at signin.

For reference (@sebastienbeau) I also updated the shopinvader_fastapi_auth_partner PR which wasn't trivial since I needed to update the fastapi_auth_partner PR, it will be high time to merge all these.

[sbidoul]

A couple more comment, but I like the direction it takes.

[sbidoul]

I think I would leave this logic in shopinvader_api_signin_jwt because it is tricky and so closely related to the authentication method.

[paradoxxxzero]

We are in the shopinvader_anonymous_partner module here which role is to identify an anonymous partner with a cookie, unless I am missing something it seems the right place to delete the cookie. IMO if we sought a more generic approach, we should have an implementation agnostic interface and no __cookie methods.

[sbidoul]

Maybe, but I don't think we should encourage re-use or override of that part. It is only 4 lines and quite delicate, so I feel it's better to copy it (probably with subtle changes) in other signin methods.

[paradoxxxzero]

That's why the _promote_from_anonymous_partner method exists, that's the one that should be inherited to implement a promotion mechanism.

[sbidoul]

That's why the _promote_from_anonymous_partner method exists, that's the one that should be inherited to implement a promotion mechanism.

Yes that part is ok. It's specifically this _promote_anonymous_partner method that I'm not comfortable with.

[paradoxxxzero]

Yes I know, I just don't understand why :). If you override the _promote_anonymous_partner in the shopinvader_anonymous_partner you can only mean to change the promotion mechanism of anonymous partners and in this case yeah you can break the anonymous partner flow.

I am certainly missing something here, but I expect a shopinvader_anonymous_partner module to handle the flow of anonymous partners regardless of how the identified partner authentication is implemented.

[sbidoul]

just don't understand why

It makes the logic of the signin route much harder to follow, for no real benefit.

[sbidoul]

👍

[sbidoul]

Suggested change

	env["res.partner"]._promote_anonymous_partner(partner, request.cookies, response)
	if anonymous_partner:
	partner._promote_from_anonymous_partner(anonymous_partner)
	env["res.partner"]._delete_anonymous_partner__cookie(request.cookies, response)

I find this much easier to follow and don't see the benefit of factoring it out.