Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Cedric-Pigeon opened this issue Apr 9, 2020 · 17 comments
Closed

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

Cedric-Pigeon opened this issue Apr 9, 2020 · 17 comments
Labels

Comments

@Cedric-Pigeon
Copy link

Hi all,

we faced this case on production yesterday:

  • A customer made an order and confirmed it
  • A few hours later, he did another order but in place of creating a new delivery address he updated the existing one.
    So both orders were delivered at the same address.

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

@acsonefho
Copy link
Contributor

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
Copy link
Contributor

hparfr commented Apr 9, 2020

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. akretion/partner-contact@6a6a979

@rousseldenis
Copy link
Contributor

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
Copy link
Contributor

bealdav commented Apr 9, 2020

@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
Copy link
Contributor

@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
Copy link
Contributor

bealdav commented Apr 9, 2020

Subject have been discussed here OCA/partner-contact#404

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

@Cedric-Pigeon
Copy link
Author

Cedric-Pigeon commented Apr 9, 2020

@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
Copy link
Contributor

simahawk commented Apr 9, 2020

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
Copy link
Contributor

simahawk commented Apr 9, 2020

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

@sebastienbeau
Copy link
Contributor

sebastienbeau commented Apr 9, 2020

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

Cedric-Pigeon commented Apr 16, 2020

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
Copy link
Contributor

bealdav commented Apr 16, 2020

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 OCA/partner-contact#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
Copy link
Author

Cedric-Pigeon commented Apr 16, 2020

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

@Cedric-Pigeon
Copy link
Author

Theses PR's are also required to make everything works correctly:
#620
#621

@github-actions
Copy link

github-actions bot commented Jul 9, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants