-
-
Notifications
You must be signed in to change notification settings - Fork 360
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
[14.0][FIX] delivery_schenker: Use display_name in name1 field of address #805
base: 14.0
Are you sure you want to change the base?
[14.0][FIX] delivery_schenker: Use display_name in name1 field of address #805
Conversation
Failing test should repair itself |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
@OCA/logistics-maintainers Could this be re-opened? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you better explain your use case ?
Shouldn't you fail if the partner has no name ?
I don't like the idea of taking the display_name
as this could contain internal information to ease the selection of a right partner (like code, country...) that should not be sent to schenker
@RuudFreshfilter Do you remember for which partner this happened? I think it was for Delivery contacts that have no name by itself but just rely on the name of their parent |
@@ -266,7 +266,7 @@ def _prepare_schenker_address( | |||
""" | |||
vals = { | |||
"type": address_type, | |||
"name1": partner.name, | |||
"name1": partner.display_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this can happen when the address belongs to another partner (likely the company) and itself has no name. Then maybe better user partner.name or partner.parent_id.name
. This will prevent the possibility of sending internal info, and when there is no parent, or the parent has no name, using display_name will not help anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RuudFreshfilter How about using partner.name or partner.commercial_partner_id.name
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds like a good solution. I applied that.
This was the issue. it was for delivery and invoice addresses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
@@ -266,7 +266,7 @@ def _prepare_schenker_address( | |||
""" | |||
vals = { | |||
"type": address_type, | |||
"name1": partner.name, | |||
"name1": partner.name or partner.commercial_partner_id.name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"name1": partner.name or partner.commercial_partner_id.name, | |
"name1": partner.name or partner.parent_name |
Thanks for your contribution.
But the commercial partner name should not be used, because this belongs maybe not to the actual address.
You should better use partner.parent_name. Like it is done in the contact widget of odoo. https://github.com/odoo/odoo/blob/cc0060e889603eb2e47fa44a8a22a70d7d784185/odoo/addons/base/views/ir_qweb_widget_templates.xml#L8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @jbaudoux
Not always is the
partner.name
filled. If this is the case then the Schenker API returns an error.