From fff73e1fb03471ed669da1f304bc518e1935de2f Mon Sep 17 00:00:00 2001 From: Romain Derie Date: Tue, 24 Jan 2023 15:09:42 +0000 Subject: [PATCH] [FIX] website: correctly handle website_visitor with merge partners Since the refactoring of website.visitor with [1] (upsert), the `access_token` is supposed to be holding the same value as the `partner_id` when the visitor is linked to a partner: - Anonymous visitor: no `partner_id`, `access_token` is a hash value - Partner visitor: `partner_id` set, `access_token` should be sync with `partner_id`. `partner_id` is just a stored computed field holding the `access_token` value if it is an integer value. There should never be a case where there is a `partner_id` set and the `access_token` is not equal to the `partner_id`. For instance, having a visitor with `partner_id` = 4 and `access_token` = `e4r3ejkj4` is supposed to be impossible. It would lead to crash, because the visitor is only searched based on his `access_token`, meaning that when searching for the visitor of partner 4, none would be found and a new one would try to be created, raising the `uniq_access_token_id` SQL constraint. While the `partner_id`/`access_token` sync might seems weird, it is done to allow the `upsert` use in SQL to improve perfs of this low level behavior. It's actually not as weak as it seems as there is only a single entry point to update the `partner_id` and `access_token`: the authenticate override of website. Those fields are not supposed to be changed elsewhere. Note that modifying the `access_token` would not be an issue as the `partner_id` is just a stored compute based on the `access_token`. But it's only true when modifying through the ORM as if you do that in raw SQL, it won't go through the `api.depends` which is supposed to recompute the stored computed `partner_id` field. But something was forgotten during the initial dev: the partner merge behavior: it does (on top of other thing) auto discover the m2o field relations that points to a `res.partner` and modify those values in raw SQL to the new value. This is obviously wrong regarding the `website.visitor`'s `partner_id` field, the `access_token` should also be updated, or when possible visitors should be merged too. Note that for DB upgrated from previous version to Odoo 16, this is ensured through the following upgrade script [2]: ```sql UPDATE website_visitor SET access_token = partner_id::text WHERE partner_id IS NOT NULL ``` [1]: https://github.com/odoo/odoo/commit/d348bed1ad9d3d16b295f013f015706be6c07820 [2]: https://github.com/odoo/upgrade/commit/0cedcbf70494dfeddeb5a97c13bf875cb6a86886 task-3148111 closes odoo/odoo#111002 X-original-commit: a87b4142dd4a2c05e3e1885b2c54f5e0d3c7ac47 Signed-off-by: Romain Derie (rde) --- addons/website/models/__init__.py | 1 + addons/website/models/base_partner_merge.py | 32 ++++++ addons/website/tests/test_website_visitor.py | 106 +++++++++++++++++++ 3 files changed, 139 insertions(+) create mode 100644 addons/website/models/base_partner_merge.py diff --git a/addons/website/models/__init__.py b/addons/website/models/__init__.py index 40ee6ae2a3b..c9538412b10 100644 --- a/addons/website/models/__init__.py +++ b/addons/website/models/__init__.py @@ -2,6 +2,7 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. from . import assets +from . import base_partner_merge from . import ir_actions_server from . import ir_asset from . import ir_attachment diff --git a/addons/website/models/base_partner_merge.py b/addons/website/models/base_partner_merge.py new file mode 100644 index 00000000000..635a455b0bf --- /dev/null +++ b/addons/website/models/base_partner_merge.py @@ -0,0 +1,32 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + + +from odoo import api, models + + +class MergePartnerAutomatic(models.TransientModel): + _inherit = 'base.partner.merge.automatic.wizard' + + @api.model + def _update_foreign_keys(self, src_partners, dst_partner): + # Case 1: there is a visitor for both src and dst partners. + # Need to merge visitors before `super` to avoid SQL partner_id unique + # constraint to raise as it will change partner_id of the visitor + # record(s) to the `dst_partner` which already exists. + dst_visitor = dst_partner.visitor_ids and dst_partner.visitor_ids[0] + if dst_visitor: + for visitor in src_partners.visitor_ids: + visitor._merge_visitor(dst_visitor) + + super()._update_foreign_keys(src_partners, dst_partner) + + # Case 2: there is a visitor only for src_partners. + # Need to fix the "de-sync" values between `access_token` and + # `partner_id`. + self.env.cr.execute(""" + UPDATE website_visitor + SET access_token = partner_id + WHERE partner_id::int != access_token::int + AND partner_id = %s; + """, (dst_partner.id,)) diff --git a/addons/website/tests/test_website_visitor.py b/addons/website/tests/test_website_visitor.py index e909069e8ef..4767c8dbca5 100644 --- a/addons/website/tests/test_website_visitor.py +++ b/addons/website/tests/test_website_visitor.py @@ -390,3 +390,109 @@ class WebsiteVisitorTests(MockVisitor, HttpCaseWithUserDemo): 'url': self.tracked_page_2.url })] } + + 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 + 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, + }, { + '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) + 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.id, + 'url': '/admin' + }]) + self.assertEqual(visitor_demo.website_track_ids.url, '/demo') + self.assertEqual(visitor_admin.website_track_ids.url, '/admin') + + # Merge demo partner in admin partner + self.env['base.partner.merge.automatic.wizard']._merge( + (self.partner_admin + self.partner_demo).ids, + self.partner_admin + ) + # Should be + # | id | access_token | partner_id | + # | -- | ------------ | ---------- | + # | 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.") + # Track check + self.assertEqual(visitor_admin.website_track_ids.mapped('url'), ['/admin', '/demo']) + + def test_merge_partner_with_visitor_single(self): + """ The partner merge feature of Odoo is auto discovering relations to + ``res_partner`` to change the field value, in raw SQL. + It will change the ``partner_id`` field of visitor without changing the + ``access_token``, which is supposed to be the same value (``partner_id`` + is just a stored computed field holding the ``access_token`` value if it + is an integer value). + This partner_id/access_token "de-sync" need to be handled, this is done + in ``_update_foreign_keys()`` website override. + This test is ensuring that it works as it should. + + There is 2 possible cases: + + 1. There is a visitor for partner 1, none for partner 2. Partner 1 is + merged into partner 2, making partner_id of visitor from partner 1 + becoming partner 2. + -> The ``access_token`` value should also be updated from 1 to 2. + 2. There is a visitor for both partners and partner 1 is merged into + partner 2. + -> Both visitor should be merged too, so data are aggregated into a + single visitor. + + 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 + 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, + }) + # | 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) + + # Merge demo partner in admin partner + self.env['base.partner.merge.automatic.wizard']._merge( + (self.partner_admin + self.partner_demo).ids, + self.partner_admin + ) + # This should not happen.. + # | id | access_token | partner_id | + # | -- | ------------ | ---------- | + # | 1 | demo_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.")