From 169eaec793220c018211c9519db38cb29982b639 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Dufays Date: Tue, 6 Jun 2023 15:11:47 +0000 Subject: [PATCH] [IMP] base,test_new_api,website:prevent merge of partners with more than 1 user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We prevent to merge partner linked to more than one user (by raising an exception) to avoid having multiple user pointing to the same partner as a consequence of a merge. We want to avoid this situation because it generates weird behaviors. As res.user inherits from res.partner, having multiple user pointing to the same partner makes the fields of that partner shared with all those users. This was decided following the tentative to improve partner merge in website_slides (odoo/odoo#114840), where we also noticed strange behavior like completing a lesson with one user were adding karma to all the users linked to a same partner. We have also adapted WebsiteVisitorTests tests in website that were failing because they were merging partners with more than one user: simply by merging partners with only one user. Task-3167160 closes odoo/odoo#125322 Signed-off-by: Stéphane Debauche (std) --- addons/website/tests/test_website_visitor.py | 91 ++++++++++--------- odoo/addons/base/wizard/base_partner_merge.py | 4 + .../test_new_api/tests/test_one2many.py | 22 ++++- 3 files changed, 75 insertions(+), 42 deletions(-) diff --git a/addons/website/tests/test_website_visitor.py b/addons/website/tests/test_website_visitor.py index b3768a3c0ec..df81a10d314 100644 --- a/addons/website/tests/test_website_visitor.py +++ b/addons/website/tests/test_website_visitor.py @@ -101,6 +101,8 @@ class WebsiteVisitorTests(MockVisitor, HttpCaseWithUserDemo): 'partner_id': self.partner_portal.id, 'groups_id': [(6, 0, [self.env.ref('base.group_portal').id])], }) + # Partner with no user associated, to test partner merge that forbids merging partners with more than 1 user + self.partner_admin_duplicate = self.env['res.partner'].create({'name': 'Mitchell'}) def _get_last_visitor(self): return self.env['website.visitor'].search([], limit=1, order="id DESC") @@ -393,38 +395,40 @@ class WebsiteVisitorTests(MockVisitor, HttpCaseWithUserDemo): def test_merge_partner_with_visitor_both(self): """ See :meth:`test_merge_partner_with_visitor_single` """ - # Setup a visitor for demo and none for admin + # Setup a visitor for admin_duplicate and none for admin Visitor = self.env['website.visitor'] - (self.partner_demo + self.partner_admin).visitor_ids.unlink() - [visitor_demo, visitor_admin] = Visitor.create([{ - 'partner_id': self.partner_demo.id, - 'access_token': self.partner_demo.id, + (self.partner_admin_duplicate + self.partner_admin).visitor_ids.unlink() + [visitor_admin_duplicate, visitor_admin] = Visitor.create([{ + 'partner_id': self.partner_admin_duplicate.id, + 'access_token': self.partner_admin_duplicate.id, }, { 'partner_id': self.partner_admin.id, 'access_token': self.partner_admin.id, }]) - # | id | access_token | partner_id | - # | -- | ------------ | ---------- | - # | 1 | demo_id | demo_id | - # | | 1062141 | 1062141 | - # | 2 | admin_id | admin_id | - # | | 5013266 | 5013266 | - self.assertTrue(visitor_demo.partner_id.id == int(visitor_demo.access_token) == self.partner_demo.id) + # | id | access_token | partner_id | + # | -- | ---------------------- | --------------------- | + # | 1 | admin_duplicate_id | admin_duplicate_id | + # | | 1062141 | 1062141 | + # | 2 | admin_id | admin_id | + # | | 5013266 | 5013266 | + self.assertTrue(visitor_admin_duplicate.partner_id.id == + int(visitor_admin_duplicate.access_token) == + self.partner_admin_duplicate.id) self.assertTrue(visitor_admin.partner_id.id == int(visitor_admin.access_token) == self.partner_admin.id) self.env['website.track'].create([{ - 'visitor_id': visitor_demo.id, - 'url': '/demo' + 'visitor_id': visitor_admin_duplicate.id, + 'url': '/admin/about-duplicate' }, { 'visitor_id': visitor_admin.id, 'url': '/admin' }]) - self.assertEqual(visitor_demo.website_track_ids.url, '/demo') + self.assertEqual(visitor_admin_duplicate.website_track_ids.url, '/admin/about-duplicate') self.assertEqual(visitor_admin.website_track_ids.url, '/admin') - # Merge demo partner in admin partner + # Merge admin_duplicate partner (no user associated) in admin partner self.env['base.partner.merge.automatic.wizard']._merge( - (self.partner_admin + self.partner_demo).ids, + (self.partner_admin + self.partner_admin_duplicate).ids, self.partner_admin ) # Should be @@ -433,11 +437,12 @@ class WebsiteVisitorTests(MockVisitor, HttpCaseWithUserDemo): # | 2 | admin_id | admin_id | # | | 5013266 | 5013266 | self.assertTrue(visitor_admin.exists()) - self.assertFalse(visitor_demo.exists()) - self.assertFalse(Visitor.search_count([('partner_id', '=', self.partner_demo.id)]), - "The demo visitor should've been merged (and deleted) with the admin one.") + self.assertFalse(visitor_admin_duplicate.exists()) + self.assertFalse(Visitor.search_count([('partner_id', '=', self.partner_admin_duplicate.id)]), + "The admin_duplicate visitor should've been merged (and deleted) with the admin one.") # Track check - self.assertEqual(visitor_admin.website_track_ids.sorted('url').mapped('url'), ['/admin', '/demo']) + self.assertEqual(visitor_admin.website_track_ids.sorted('url').mapped('url'), + ['/admin', '/admin/about-duplicate']) def test_merge_partner_with_visitor_single(self): """ The partner merge feature of Odoo is auto discovering relations to @@ -464,35 +469,39 @@ class WebsiteVisitorTests(MockVisitor, HttpCaseWithUserDemo): Case 1 is tested here. Cade 2 is tested in :meth:`test_merge_partner_with_visitor_both`. """ - # Setup a visitor for demo and none for admin + # Setup a visitor for admin_duplicate and none for admin Visitor = self.env['website.visitor'] - (self.partner_demo + self.partner_admin).visitor_ids.unlink() - visitor_demo = Visitor.create({ - 'partner_id': self.partner_demo.id, - 'access_token': self.partner_demo.id, + (self.partner_admin_duplicate + self.partner_admin).visitor_ids.unlink() + visitor_admin_duplicate = Visitor.create({ + 'partner_id': self.partner_admin_duplicate.id, + 'access_token': self.partner_admin_duplicate.id, }) - # | id | access_token | partner_id | - # | -- | ------------ | ---------- | - # | 1 | demo_id | demo_id | - # | | 1062141 | 1062141 | - self.assertTrue(visitor_demo.partner_id.id == int(visitor_demo.access_token) == self.partner_demo.id) + # | id | access_token | partner_id | + # | -- | ---------------------- | --------------------- | + # | 1 | admin_duplicate_id | admin_duplicate_id | + # | | 1062141 | 1062141 | + self.assertTrue(visitor_admin_duplicate.partner_id.id == + int(visitor_admin_duplicate.access_token) == + self.partner_admin_duplicate.id) - # Merge demo partner in admin partner + # Merge admin_duplicate partner (no user associated) in admin partner self.env['base.partner.merge.automatic.wizard']._merge( - (self.partner_admin + self.partner_demo).ids, + (self.partner_admin + self.partner_admin_duplicate).ids, self.partner_admin ) # This should not happen.. - # | id | access_token | partner_id | - # | -- | ------------ | ---------- | - # | 1 | demo_id | admin_id | <-- Mismatch - # | | 1062141 | 5013266 | + # | id | access_token | partner_id | + # | -- | ---------------------- | ---------- | + # | 1 | admin_duplicate_id | admin_id | <-- Mismatch + # | | 1062141 | 5013266 | # .. it should be: # | id | access_token | partner_id | # | -- | ------------ | ---------- | # | 1 | admin_id | admin_id | <-- No mismatch, became admin_id # | | 5013266 | 5013266 | - self.assertTrue(visitor_demo.partner_id.id == int(visitor_demo.access_token) == self.partner_admin.id, - "The demo visitor should now be linked to the admin partner.") - self.assertFalse(Visitor.search_count([('partner_id', '=', self.partner_demo.id)]), - "The demo visitor should've been merged (and deleted) with the admin one.") + self.assertTrue(visitor_admin_duplicate.partner_id.id == + int(visitor_admin_duplicate.access_token) == + self.partner_admin.id, + "The admin_duplicate visitor should now be linked to the admin partner.") + self.assertFalse(Visitor.search_count([('partner_id', '=', self.partner_admin_duplicate.id)]), + "The admin_duplicate visitor should've been merged (and deleted) with the admin one.") diff --git a/odoo/addons/base/wizard/base_partner_merge.py b/odoo/addons/base/wizard/base_partner_merge.py index 85df8df4506..f93a470b08d 100644 --- a/odoo/addons/base/wizard/base_partner_merge.py +++ b/odoo/addons/base/wizard/base_partner_merge.py @@ -310,6 +310,10 @@ class MergePartnerAutomatic(models.TransientModel): if partner_ids & child_ids: raise UserError(_("You cannot merge a contact with one of his parent.")) + # check if the list of partners to merge are linked to more than one user + if len(partner_ids.with_context(active_test=False).user_ids) > 1: + raise UserError(_("You cannot merge contacts linked to more than one user even if only one is active.")) + if extra_checks and len(set(partner.email for partner in partner_ids)) > 1: raise UserError(_("All contacts must have the same email. Only the Administrator can merge contacts with different emails.")) diff --git a/odoo/addons/test_new_api/tests/test_one2many.py b/odoo/addons/test_new_api/tests/test_one2many.py index e9116da4891..3b287afdb42 100644 --- a/odoo/addons/test_new_api/tests/test_one2many.py +++ b/odoo/addons/test_new_api/tests/test_one2many.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- from odoo.tests.common import TransactionCase -from odoo.exceptions import MissingError +from odoo.exceptions import MissingError, UserError from odoo import Command from odoo.tools import mute_logger @@ -189,6 +189,26 @@ class One2manyCase(TransactionCase): self.assertTrue(p2.exists()) self.assertFalse(p3.exists()) + def test_partner_merge_wizard_more_than_one_user_error(self): + """ Test that partners cannot be merged if linked to more than one user even if only one is active. """ + p1, p2, dst_partner = self.env['res.partner'].create([{'name': f'test{idx + 1}'} for idx in range(3)]) + u1, u2 = self.env['res.users'].create([{'name': 'test1', 'login': 'test1', 'partner_id': p1.id}, + {'name': 'test2', 'login': 'test2', 'partner_id': p2.id}]) + MergeWizard_with_context = self.env['base.partner.merge.automatic.wizard'].with_context( + active_ids=(u1.partner_id + u2.partner_id + dst_partner).ids, active_model='res.partner') + + with self.assertRaises(UserError): + MergeWizard_with_context.create({}).action_merge() + + u2.action_archive() + with self.assertRaises(UserError): + MergeWizard_with_context.create({}).action_merge() + + u2.unlink() + MergeWizard_with_context.create({}).action_merge() + self.assertTrue(dst_partner.exists()) + self.assertEqual(u1.partner_id.id, dst_partner.id) + def test_cache_invalidation(self): """ Cache invalidation for one2many with integer inverse. """ record0 = self.env['test_new_api.attachment.host'].create({})