shopinvader / odoo-shopinvader

Odoo Modules. Sorry Magento, Shopinvader is coming
GNU Affero General Public License v3.0
119 stars 102 forks source link

Upgrade of a delivery address linked to an open order #609

Closed Cedric-Pigeon closed 1 year ago

Cedric-Pigeon commented 4 years ago

Hi all,

we faced this case on production yesterday:

How should we prevent that? Should we block update of an address linked to an order?

acsonefho commented 4 years ago

I see 2 different solutions (both with pros and cons): 1) When the address is related to a not already delivered SO (or used into delivery picking not already done), block the update of address fields (that can force the user to create a new one). `` Block only for an update done by the front-side. Maybe the backend user can do it.

2) When a res.partner is edited (address fields), instead of updating it, do a create of a new contact (who contains only address fields). A little bit like an history. + You always know the real delivery address of a SO; - Each update of an address' field will do a create and we can have a lot of records;

IMO the first is better than the second.

hparfr commented 4 years ago

How should we prevent that? Should we block update of an address linked to an order? Yes, it should be delivered to the two places.

@bguillot did some experimentations on this topic if I remember well. https://github.com/akretion/partner-contact/commit/6a6a9791a1d87c9eb47c09fad79a8043b3b7b481

rousseldenis commented 4 years ago

I see 2 different solutions (both with pros and cons):

  1. When the address is related to a not already delivered SO (or used into delivery picking not already done), block the update of address fields (that can force the user to create a new one). `` Block only for an update done by the front-side. Maybe the backend user can do it.
  2. When a res.partner is edited (address fields), instead of updating it, do a create of a new contact (who contains only address fields). A little bit like an history. + You always know the real delivery address of a SO; - Each update of an address' field will do a create and we can have a lot of records;

IMO the first is better than the second.

For me, the second is better idea. We just need to set the existing one as active=False.

At school, we learnt that for order addresses, we need to keep all details(street, zip and so on) and do not point to a record. Even in real case it's not possible, the second behaviour with the active False is the one that is most similar to that.

bealdav commented 4 years ago

@hparfr it's a merged module https://github.com/OCA/partner-contact/tree/10.0/partner_address_version and I think it fix the case mentionned by @Cedric-Pigeon

rousseldenis commented 4 years ago

@hparfr it's a merged module https://github.com/OCA/partner-contact/tree/10.0/partner_address_version and I think it fix the case mentionned by @Cedric-Pigeon

Isn't a too big bang solution compare to the one proposed ?

bealdav commented 4 years ago

Subject have been discussed here https://github.com/OCA/partner-contact/issues/404

I couldn't find this issue anymore sorry, may be because its number is 404 ;-)

Cedric-Pigeon commented 4 years ago

@rousseldenis you are right I am not sure we need to go so far to solve such case I would avoid to include a new dependency within shopinvader.

simahawk commented 4 years ago

I think this behavior should be optional via backend settings. By default, I'd go for opt 1 -> is very easy: on v13 I've added permission/access information on profile and addresses, hence we can already block editing. See access_info.

For opt 2 I'd add a new module shopinvader_partner_version and use partner_address_version because it does exactly what you need IMO.

simahawk commented 4 years ago

FTR @thibaultrey Locomotive PR is still pending here https://github.com/shopinvader/shopinvader-template/pull/21 -> it can be unlocked when this feature lands in all versions.

sebastienbeau commented 4 years ago

You do not need to create a shopinvader_partner_version. Just installing the module sale_partner_version and you are done : https://github.com/OCA/sale-workflow/tree/10.0/sale_partner_version

We have build this module for solving this issue and it's the cleanest way to do it (no more issue on changing invoice address...), we have customer in production with it and shopinvader and there not issue.

I do not think we should handle this into shopinvader the issue is a pure odoo issue (record are linked to addresses and addresses can be changed from shopinvader or from odoo backoffice).

If you really want to do this in shopinvader I prefer to have it in a separed module because I think it's an hack, and it's not a good user experience on frontend.

Note I do not think that we should depend on sale_partner_version, but we can recommend to use it as it fix this Odoo issue

Cedric-Pigeon commented 4 years ago

@sebastienbeau Actually I just had a deeper look on your suggested add-ons. Unfortunately, it is not compliant with a shopinvader feature:https://github.com/shopinvader/odoo-shopinvader/blob/10.0/shopinvader/models/res_partner.py#L30

As the versioning feature creates a duplicate of the contact, it triggers the constrains on email unicity.

Should we add an active where_clause in the sql query on the constrains? Any other idea?

sebastienbeau commented 4 years ago

Hum, we do not have this option activated, this is why we didn't had the issue. But after looking at the SQL constraint maybe the issue is here. Indeed the sql request do not take in account the active field, we should exclude inactive partner

Cedric-Pigeon commented 4 years ago

I just discovered that sale_partner_version duplicates all addresses at each sale order confirmation. So even if the customer never updates his addresses we will always have them twice. Even if the duplicate is inactive, I am wondering if there isn't any better solution. I think that the price to pay is high regarding the frequency of such use cases.

bealdav commented 4 years ago

I confirmed that tells cedric and it's an unrequested features or a bug IMHO. It wasn't the initial conception probably lost in this demoniac number https://github.com/OCA/partner-contact/issues/404. Probably missing test, then

You can test yourself duplicating this sale http://3418459-10-0-7c9805.runbot2-2.odoo-community.org/web#id=16&view_type=form&model=sale.order&menu_id=210&action=279

cc @bguillot could you confirm ?

Cedric-Pigeon commented 4 years ago

work in progress here to trigger the versioning on address update if used: https://github.com/OCA/partner-contact/pull/888 https://github.com/OCA/sale-workflow/pull/1119 https://github.com/OCA/sale-workflow/pull/1121

Cedric-Pigeon commented 4 years ago

Theses PR's are also required to make everything works correctly: https://github.com/shopinvader/odoo-shopinvader/pull/620 https://github.com/shopinvader/odoo-shopinvader/pull/621

github-actions[bot] commented 1 year ago

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.