-
-
Notifications
You must be signed in to change notification settings - Fork 867
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
[18.0][ADD] partner_type_base extracted from partner_firstname #1905
base: 18.0
Are you sure you want to change the base?
Conversation
aea9e99
to
90d5c7f
Compare
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
90d5c7f
to
b159dba
Compare
@houssine78 I haven't tested the migration script yet, but I've used this command it during the last upgrade. What do you think about this solution? |
a40ba90
to
ad5d5e9
Compare
The upgrade script worked for me in odoo.sh, but i am not sure if we should add a dependency to the util tool in the manifest? In my container, the library was missing and therefore the upgrade failing. |
partner_firstname/__manifest__.py
Outdated
@@ -18,7 +18,7 @@ | |||
"maintainer": "Camptocamp, Acsone", |
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.
This is not correct. The correct key is maintainers
and value should be a list of github logins.
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.
Change also the version number if you want your script running.
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 thought this is done automatically with the correct merge command? But I agree the version number needs to be 18.0.1.2.0 after the merge. I can change this.
Regarding maintainer or maintainers, it is mixed everywhere and not used very often. Is there any documentation beside this where it is without the "s": https://www.odoo.com/documentation/18.0/developer/reference/backend/module.html
I tend to remove it totally.
|
||
def migrate(cr, version): | ||
_logger.info("Installing dependent module 'partner_type_base' by migration script.") | ||
util.force_install_module(cr, "partner_type_base") |
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.
Why do you need this? Odoo's ORM is in charge of that.
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 thought that too, but @houssine78 informed me that there is a bug and from my testing I can confirm that the added dependency is not installed automatically. odoo/odoo#72703
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.
Hasn't odoo/odoo#72661 fixed that ?
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.
not totally as sbidoul wrote at the end of the ticket I've linked:
Half fixed to be precise.
|
||
|
||
class TestResPartner(TransactionCase): | ||
def setUp(self): |
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.
Use classmethod instead.
"maintainer": "CRogos", | ||
"category": "Extra Tools", | ||
"website": "https://github.com/OCA/partner-contact", | ||
"depends": ["base_setup"], |
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.
Why not base
?
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.
Good question... I've copied it from "partner_firstname" module.
There is a module in odoo/addons/base_setup which depends on "base" and "web". It also has some minor changes on the patner kanban view. So maybe a view order issue?
A general question:
If partner_firstname has a dependency to partner_type_base and partner_type_base depends on base, but partner_firstname also references base in a view. Shall partner_firstname reference base explicit in the manifest or should it not add the base to the depends?
partner_firstname.depends = ["partner_type_base"] or ["base", "partner_type_base"] ???
when
partner_type_base.depends = ["base"]
|
||
@api.model | ||
def _search_can_be_parent(self, operator, value): | ||
return [("is_company", operator, value)] |
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.
Is it real ? An individual can have a child partner as delivery address for instance.
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.
This is totally inconsistent in Odoo.
When you go to a contact, you can add a subcontact/address:
When you open this other address, the contact is set as parent:
But when you search for the contact to select it in this view, this is not possible:
In my Opinion a big weakness of Odoo, because it is very unclear to the user how to use it.
After setting a parent, you can also switch the contact to "company" and the parent_id is kept.
Basically this could be good feature to address hierarchies, but I am not sure if all Odoo modules can work with this.
What we are doing is, that we use a 3 level hirache Company -> "Other Address" -> Contact to bundle all people of a certain Office address. Because commerical_partner_id looks upwards until the first company, this should work. But I am not sure if it is a Odoo recommended structure.
To have it more consistent, I added the can_be_parent attribute to make it more flexible and we overload the computation and add "Other Address" to the types which can_be_parent = True in another module.
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 had also opened an Odoo ticket on this topic 9 Months ago, and this was the answer:
Thank you for your patience. I have been trying to find information on whether this behavior is supported or not, and in fact a lot of feedback has been created in the past suggesting we need this feature (three-level hierarchy with contacts). The reply the development team has given us in all of these cases is the behavior is indeed possible, but not entirely supported. Odoo does not support this behavior because it is a very problematic data structure, with a lot of hidden complexities, especially on the accounting aspect. This reply has been the same for feedbacks created 3 to 1 year ago, and the team does admit that if demand for this becomes very high it will be reconsidered.
Therefore, I have created a feedback ticket to shed light on the fact that you, like other customers, do need this hierarchy for your daily business flows. Depending on whether you just need to display that a contact belongs to another, the workaround proposed is to create a Studio field for it. If however this information will be used in other business flows such as accounting flows, custom code will be necessary to satisfy the need.
ad5d5e9
to
4d8cd37
Compare
@rousseldenis could you update your review and share your thoughts regarding adding the odoo.upgrade tool to the python dependencies or not? |
followup in v18 of #1891
We are currently unhappy that invoice or shipping address has a firstname. Shipping addresses sometimes need an department name or other additional information, which are not person/individual like.
Therefore I would like to make the module more flexible and add an overloadable attribute is_individual, where you can control whether firstname should be visible or not. Modules could overload the _compute_contact_type method to change the address type behavior for each type.
I also changed the visibility of the UI elements to is_individual and is_address_readonly instead of the address type.
The is_address_readonly we use to add another address type "contact_address" which is an individual, but also has an editable address like address type "other". We use "other" mainly for office addresses of a company and need to filter out these contacts for not adding these contacts to e.g. newsletters. Nevertheless we need the possibility to add individual addresses e.g. for people who are mostly working in the homeoffice and don't need their mails to the office.
What do you think of these changes?
PS: This module currently does not adapt the base_address_extended module views.
https://github.com/odoo/odoo/blob/18.0/addons/base_address_extended/views/base_address_extended.xml
And there are also some few localizations which have expressions like "type == 'contact'".