From 1fb20fe3047cf53eff258705d12319015ba96747 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 14 Mar 2019 14:06:24 +0000 Subject: [PATCH 1/3] [IMP] base: try to batch update of children address & commercial fields With profiling enabled, importing 10k partners, with all of them having the same parent (though not with all of them having the same is_company setting) lowers sync / post-process time from ~550s to ~330s. Put it in an override to _load_records_create so it's only active for imports (which is the original report & test case), use a context key to avoid going the post-processing work in create. --- odoo/addons/base/models/res_partner.py | 77 +++++++++++++++++++++----- 1 file changed, 62 insertions(+), 15 deletions(-) diff --git a/odoo/addons/base/models/res_partner.py b/odoo/addons/base/models/res_partner.py index 93d7b175a78..7512252988d 100644 --- a/odoo/addons/base/models/res_partner.py +++ b/odoo/addons/base/models/res_partner.py @@ -2,6 +2,7 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. import base64 +import collections import datetime import hashlib import pytz @@ -469,21 +470,25 @@ class Partner(models.Model): self.update_address(onchange_vals) # 2. To DOWNSTREAM: sync children - if self.child_ids: - # 2a. Commercial Fields: sync if commercial entity - if self.commercial_partner_id == self: - commercial_fields = self._commercial_fields() - if any(field in values for field in commercial_fields): - self._commercial_sync_to_children() - for child in self.child_ids.filtered(lambda c: not c.is_company): - if child.commercial_partner_id != self.commercial_partner_id : - self._commercial_sync_to_children() - break - # 2b. Address fields: sync if address changed - address_fields = self._address_fields() - if any(field in values for field in address_fields): - contacts = self.child_ids.filtered(lambda c: c.type == 'contact') - contacts.update_address(values) + self._children_sync(values) + + def _children_sync(self, values): + if not self.child_ids: + return + # 2a. Commercial Fields: sync if commercial entity + if self.commercial_partner_id == self: + commercial_fields = self._commercial_fields() + if any(field in values for field in commercial_fields): + self._commercial_sync_to_children() + for child in self.child_ids.filtered(lambda c: not c.is_company): + if child.commercial_partner_id != self.commercial_partner_id: + self._commercial_sync_to_children() + break + # 2b. Address fields: sync if address changed + address_fields = self._address_fields() + if any(field in values for field in address_fields): + contacts = self.child_ids.filtered(lambda c: c.type == 'contact') + contacts.update_address(values) @api.multi def _handle_first_contact_creation(self): @@ -556,11 +561,53 @@ class Partner(models.Model): vals['image'] = self._get_default_image(vals.get('type'), vals.get('is_company'), vals.get('parent_id')) tools.image_resize_images(vals, sizes={'image': (1024, None)}) partners = super(Partner, self).create(vals_list) + + if self.env.context.get('_partners_skip_fields_sync'): + return partners + for partner, vals in pycompat.izip(partners, vals_list): partner._fields_sync(vals) partner._handle_first_contact_creation() return partners + def _load_records_create(self, vals_list): + partners = super(Partner, self.with_context(_partners_skip_fields_sync=True))._load_records_create(vals_list) + + # batch up first part of _fields_sync + # group partners by commercial_partner_id (if not self) and parent_id (if type == contact) + groups = collections.defaultdict(list) + for partner, vals in pycompat.izip(partners, vals_list): + cp_id = None + if vals.get('parent_id') and partner.commercial_partner_id != partner: + cp_id = partner.commercial_partner_id.id + + add_id = None + if partner.parent_id and partner.type == 'contact': + add_id = partner.parent_id.id + groups[(cp_id, add_id)].append(partner.id) + + for (cp_id, add_id), children in groups.items(): + # values from parents (commercial, regular) written to their common children + to_write = {} + # commercial fields from commercial partner + if cp_id: + to_write = self.browse(cp_id)._update_fields_values(self._commercial_fields()) + # address fields from parent + if add_id: + parent = self.browse(add_id) + for f in self._address_fields(): + v = parent[f] + if v: + to_write[f] = v.id if isinstance(v, models.BaseModel) else v + if to_write: + self.browse(children).write(to_write) + + # do the second half of _fields_sync the "normal" way + for partner, vals in pycompat.izip(partners, vals_list): + partner._children_sync(vals) + partner._handle_first_contact_creation() + return partners + @api.multi def create_company(self): self.ensure_one() From b4ae543098edadd203b3e55e8b388c33a678c9ce Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 14 Mar 2019 16:14:14 +0000 Subject: [PATCH 2/3] [IMP] base: batching of _compute_commercial_partner Should correctly handle / fallback to regular code when processing non-stored partners. Perf difference according to cprofile on my machine: original python: ncalls tottime percall cumtime percall filename:lineno(function) 1 0.134 0.134 205.380 205.380 res_partner.py:277(_compute_commercial_partner) SQLized 1 0.118 0.118 67.239 67.239 res_partner.py:280(_compute_commercial_partner) most of the time leftover seems to be in modified_draft --- odoo/addons/base/models/res_partner.py | 26 ++++++++++++++++++- odoo/addons/base/tests/test_base.py | 35 ++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/odoo/addons/base/models/res_partner.py b/odoo/addons/base/models/res_partner.py index 7512252988d..625446c95be 100644 --- a/odoo/addons/base/models/res_partner.py +++ b/odoo/addons/base/models/res_partner.py @@ -275,8 +275,32 @@ class Partner(models.Model): @api.depends('is_company', 'parent_id.commercial_partner_id') def _compute_commercial_partner(self): + self.env.cr.execute(""" + WITH RECURSIVE cpid(id, parent_id, commercial_partner_id, final) AS ( + SELECT + id, parent_id, id, + (coalesce(is_company, false) OR parent_id IS NULL) as final + FROM res_partner + WHERE id = ANY(%s) + UNION + SELECT + cpid.id, p.parent_id, p.id, + (coalesce(is_company, false) OR p.parent_id IS NULL) as final + FROM res_partner p + JOIN cpid ON (cpid.parent_id = p.id) + WHERE NOT cpid.final + ) + SELECT cpid.id, cpid.commercial_partner_id + FROM cpid + WHERE final AND id = ANY(%s); + """, [self.ids, self.ids]) + + d = dict(self.env.cr.fetchall()) for partner in self: - if partner.is_company or not partner.parent_id: + fetched = d.get(partner.id) + if fetched is not None: + partner.commercial_partner_id = fetched + elif partner.is_company or not partner.parent_id: partner.commercial_partner_id = partner else: partner.commercial_partner_id = partner.parent_id.commercial_partner_id diff --git a/odoo/addons/base/tests/test_base.py b/odoo/addons/base/tests/test_base.py index 5e1a24958bf..a823c65dc31 100644 --- a/odoo/addons/base/tests/test_base.py +++ b/odoo/addons/base/tests/test_base.py @@ -252,6 +252,41 @@ class TestBase(TransactionCase): self.assertEqual(leaf111.address_get([]), {'contact': branch11.id}, 'Invalid address resolution, branch11 should now be contact') + def test_commercial_partner_nullcompany(self): + """ The commercial partner is the first/nearest ancestor-or-self which + is a company or doesn't have a parent + """ + P = self.env['res.partner'] + p0 = P.create({'name': '0', 'email': '0'}) + self.assertEqual(p0.commercial_partner_id, p0, "partner without a parent is their own commercial partner") + + p1 = P.create({'name': '1', 'email': '1', 'parent_id': p0.id}) + self.assertEqual(p1.commercial_partner_id, p0, "partner's parent is their commercial partner") + p12 = P.create({'name': '12', 'email': '12', 'parent_id': p1.id}) + self.assertEqual(p12.commercial_partner_id, p0, "partner's GP is their commercial partner") + + p2 = P.create({'name': '2', 'email': '2', 'parent_id': p0.id, 'is_company': True}) + self.assertEqual(p2.commercial_partner_id, p2, "partner flagged as company is their own commercial partner") + p21 = P.create({'name': '21', 'email': '21', 'parent_id': p2.id}) + self.assertEqual(p21.commercial_partner_id, p2, "commercial partner is closest ancestor with themselves as commercial partner") + + p3 = P.create({'name': '3', 'email': '3', 'is_company': True}) + self.assertEqual(p3.commercial_partner_id, p3, "being both parent-less and company should be the same as either") + + notcompanies = p0 | p1 | p12 | p21 + self.env.cr.execute('update res_partner set is_company=null where id = any(%s)', [notcompanies.ids]) + for parent in notcompanies: + p = P.create({ + 'name': parent.name + '_sub', + 'email': parent.email + '_sub', + 'parent_id': parent.id, + }) + self.assertEqual( + p.commercial_partner_id, + parent.commercial_partner_id, + "check that is_company=null is properly handled when looking for ancestor" + ) + def test_50_res_partner_commercial_sync(self): res_partner = self.env['res.partner'] p0 = res_partner.create({'name': 'Sigurd Sunknife', From 6c2b54ff9c536500407223f86bbbd59216689f00 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 14 Mar 2019 09:34:53 +0000 Subject: [PATCH 3/3] [IMP] base_address_partner: optimise transfer betwen parent and child Mostly a concern during bulk import of partners with parents. First remove syncing extended fields, that seems completely unnecessary since the street gets sync'd and will get split through the normal process (updating the sub-fields), by also syncing the split fields we're redundantly calling _set_steet and rewriting the address we just sync'd. Second if write()ing both the country and street, the override would then go and re-write the street based on what had *just* been split into sub-street fields, which could waste a lot of time during the import post-process as address fields get moved back and forth between parents and children leading to *lots* of writing both country and address together, we're talking: ncalls tottime percall cumtime percall filename:lineno(function) 10002/2 0.111 0.000 344.390 172.195 res_partner.py:181(write) [...] 2 0.455 0.228 165.264 82.632 base_address_extended.py:41(_set_street) My initial instinct was to just add the country_id to _set_street and remove the write override but it would break 88ff6beb016702cbda710aa31bbf10d51ab68fab: if the country alone is updated on a partner, we do want to re-format the "unified" address based on individual fields and the new country's format. Note: it might be that the logic would be more sensible by inversing the entire thing, such that the split fields are the proper source (regular stored fields) and street is converted to a computed field instead, but that'd be a larger model change. Seems like it'd make more sense though, at least given what the modules attempts to do (OTOH the module probably fails https://www.mjt.me.uk/posts/falsehoods-programmers-believe-about-addresses/ in just about all the ways). --- .../base_address_extended/models/base_address_extended.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/addons/base_address_extended/models/base_address_extended.py b/addons/base_address_extended/models/base_address_extended.py index 766fac55a8b..0fd99cc8e63 100644 --- a/addons/base_address_extended/models/base_address_extended.py +++ b/addons/base_address_extended/models/base_address_extended.py @@ -33,11 +33,6 @@ class Partner(models.Model): street_number2 = fields.Char('Door', compute='_split_street', help="Door Number", inverse='_set_street', store=True) - @api.model - def _address_fields(self): - """Returns the list of address fields that are synced from the parent.""" - return super(Partner, self)._address_fields() + ['street_name', 'street_number', 'street_number2'] - def get_street_fields(self): """Returns the fields that can be used in a street format. Overwrite this function if you want to add your own fields.""" @@ -144,7 +139,7 @@ class Partner(models.Model): def write(self, vals): res = super(Partner, self).write(vals) - if 'country_id' in vals: + if 'country_id' in vals and 'street' not in vals: self._set_street() return res