diff --git a/addons/website/__manifest__.py b/addons/website/__manifest__.py index 63a9070c1ee..eb60fa0f3ff 100644 --- a/addons/website/__manifest__.py +++ b/addons/website/__manifest__.py @@ -113,6 +113,7 @@ ], 'demo': [ 'data/website_demo.xml', + 'data/website_visitor_demo.xml', ], 'application': True, 'post_init_hook': 'post_init_hook', diff --git a/addons/website/data/website_visitor_cron.xml b/addons/website/data/website_visitor_cron.xml index fc5e6aae667..d4a9131e672 100644 --- a/addons/website/data/website_visitor_cron.xml +++ b/addons/website/data/website_visitor_cron.xml @@ -1,10 +1,10 @@ - Website Visitor : Archive old visitors + Website Visitor : clean inactive visitors code - model._cron_archive_visitors() + model._cron_unlink_old_visitors() 1 days -1 diff --git a/addons/website/data/website_visitor_demo.xml b/addons/website/data/website_visitor_demo.xml new file mode 100644 index 00000000000..b03141e5fc1 --- /dev/null +++ b/addons/website/data/website_visitor_demo.xml @@ -0,0 +1,35 @@ + + + + + Edwin Hansen + + + + + + + Soham Palmer + + + + + + + Philipe J. Fry + + + + + + + Philipe J. Fry (old) + + + + + + + + + diff --git a/addons/website/models/res_users.py b/addons/website/models/res_users.py index d64f94db6b4..1173757b10e 100644 --- a/addons/website/models/res_users.py +++ b/addons/website/models/res_users.py @@ -87,7 +87,7 @@ class ResUsers(models.Model): if other_user_visitor_sudo: visitor_main = other_user_visitor_sudo[0] other_visitors = other_user_visitor_sudo[1:] # normally void - (visitor_sudo + other_visitors)._link_to_visitor(visitor_main, keep_unique=True) + (visitor_sudo + other_visitors)._link_to_visitor(visitor_main) visitor_main.name = user_partner.name visitor_main.active = True visitor_main._update_visitor_last_visit() diff --git a/addons/website/models/website_visitor.py b/addons/website/models/website_visitor.py index 400afd00a7f..8b3cae7807c 100644 --- a/addons/website/models/website_visitor.py +++ b/addons/website/models/website_visitor.py @@ -35,6 +35,7 @@ class WebsiteVisitor(models.Model): active = fields.Boolean('Active', default=True) website_id = fields.Many2one('website', "Website", readonly=True) partner_id = fields.Many2one('res.partner', string="Contact", help="Partner of the last logged in user.", index='btree_not_null') + parent_id = fields.Many2one('website.visitor', string="Parent", ondelete='set null', index='btree_not_null', help="Main identity") partner_image = fields.Binary(related='partner_id.image_1920') # localisation and info @@ -161,10 +162,17 @@ class WebsiteVisitor(models.Model): } def _get_visitor_from_request(self, force_create=False): - """ Return the visitor as sudo from the request if there is a visitor_uuid cookie. - It is possible that the partner has changed or has disconnected. - In that case the cookie is still referencing the old visitor and need to be replaced - with the one of the visitor returned !!!. """ + """ Return the visitor as sudo from the request if there is a + visitor_uuid cookie. + + When fetching visitor, now that duplicates are linked to a main visitor + instead of unlinked, you may have more collisions issues with cookie + being set (after a de-connection for example). + + The visitor associated to a partner in case of public user is not taken + into account, it is considered as desynchronized cookie. + In addition, we also discard if the visitor has a main visitor whose + partner is set (aka wrong after logout partner). """ # This function can be called in json with mobile app. # In case of mobile app, no uid is set on the jsonRequest env. @@ -189,10 +197,21 @@ class WebsiteVisitor(models.Model): # Cookie associated to a Partner visitor = Visitor + # also check that visitor parent partner is not different from user's one + # (indicates duplicate due to invalid or wrong cookie) + if visitor and visitor.parent_id.partner_id: + if self.env.user._is_public(): + visitor = self.env['website.visitor'].sudo() + elif not visitor.partner_id: + visitor = self.env['website.visitor'].sudo().with_context(active_test=False).search( + [('partner_id', '=', self.env.user.partner_id.id)] + ) + if visitor and not visitor.timezone: tz = self._get_visitor_timezone() if tz: visitor._update_visitor_timezone(tz) + if not visitor and force_create: visitor = self._create_visitor() @@ -265,15 +284,14 @@ class WebsiteVisitor(models.Model): vals.update(update_values) self.write(vals) - def _link_to_visitor(self, target, keep_unique=True): + def _link_to_visitor(self, target): """ Link visitors to target visitors, because they are linked to the same identity. Purpose is mainly to propagate partner identity to sub records to ease database update and decide what to do with "duplicated". - THis method is meant to be overridden in order to implement some specific + This method is meant to be overridden in order to implement some specific behavior linked to sub records of duplicate management. :param target: main visitor, target of link process; - :param keep_unique: if True, find a way to make target unique; """ # Link sub records of self to target partner if target.partner_id: @@ -281,16 +299,34 @@ class WebsiteVisitor(models.Model): # Link sub records of self to target visitor self.website_track_ids.write({'visitor_id': target.id}) - if keep_unique: - self.unlink() + # Archive current record and set its parent visitor + self.partner_id = False + self.parent_id = target.id + self.active = False return target - def _cron_archive_visitors(self): - delay_days = int(self.env['ir.config_parameter'].sudo().get_param('website.visitor.live.days', 30)) + def _cron_unlink_old_visitors(self): + """ Unlink inactive visitors (see '_inactive_visitors_domain' for details). + + Visitors were previously archived but we came to the conclusion that archived visitors + have very little value and bloat the database for no reason. """ + + inactive_visitors = self.env['website.visitor'].sudo() \ + .with_context(active_test=False) \ + .search(self._inactive_visitors_domain()) + inactive_visitors.unlink() + + def _inactive_visitors_domain(self): + """ This method defines the domain of visitors that can be cleaned. By default visitors + not linked to any partner and not active for 'website.visitor.live.days' days (default being 60) + are considered as inactive. + + This method is meant to be overridden by sub-modules to further refine inactivity conditions. """ + + delay_days = int(self.env['ir.config_parameter'].sudo().get_param('website.visitor.live.days', 60)) deadline = datetime.now() - timedelta(days=delay_days) - visitors_to_archive = self.env['website.visitor'].sudo().search([('last_connection_datetime', '<', deadline)]) - visitors_to_archive.write({'active': False}) + return [('last_connection_datetime', '<', deadline), ('partner_id', '=', False)] def _update_visitor_timezone(self, timezone): """ We need to do this part here to avoid concurrent updates error. """ diff --git a/addons/website/tests/test_website_visitor.py b/addons/website/tests/test_website_visitor.py index 374594f955c..ad4727c1674 100644 --- a/addons/website/tests/test_website_visitor.py +++ b/addons/website/tests/test_website_visitor.py @@ -120,21 +120,16 @@ class WebsiteVisitorTests(MockVisitor, HttpCaseWithUserDemo): ) def assertVisitorDeactivated(self, visitor, main_visitor): - """ Temporary method to check that a visitor has been de-activated / merged + """ Method that checks that a visitor has been de-activated / merged with other visitor, notably in case of login (see User.authenticate() as - well as Visitor._link_to_visitor() ). + well as Visitor._link_to_visitor() ). """ + self.assertTrue(bool(visitor)) + self.assertFalse(visitor.active) + self.assertTrue(main_visitor.active) + self.assertEqual(visitor.parent_id, main_visitor) - As final result depends on installed modules (see overrides) due to stable - improvements linked to EventOnline, this method contains a hack to avoid - doing too much overrides just for that behavior. """ - if 'parent_id' in self.env['website.visitor']: - self.assertTrue(bool(visitor)) - self.assertFalse(visitor.active) - self.assertTrue(main_visitor.active) - self.assertEqual(visitor.parent_id, main_visitor) - else: - self.assertFalse(visitor) - self.assertTrue(bool(main_visitor)) + # tracks are linked + self.assertTrue(visitor.website_track_ids < main_visitor.website_track_ids) def test_visitor_creation_on_tracked_page(self): """ Test various flows involving visitor creation and update. """ @@ -287,40 +282,110 @@ class WebsiteVisitorTests(MockVisitor, HttpCaseWithUserDemo): # check number of visits self.assertEqual(visitor_portal.visit_count, 2, "There should be 2 visits for the portal user") - def test_visitor_archive(self): - """ Test cron archiving inactive visitors and their re-activation when - authenticating an user. """ - self.env['ir.config_parameter'].sudo().set_param('website.visitor.live.days', 7) - - partner_demo = self.partner_demo - old_visitor = self.env['website.visitor'].create({ + def test_clean_inactive_visitors(self): + inactive_visitors = self.env['website.visitor'].create([{ + 'name': 'Crazy Lazy Bernie', 'lang_id': self.env.ref('base.lang_en').id, 'country_id': self.env.ref('base.be').id, 'website_id': 1, - 'partner_id': partner_demo.id, - }) - self.assertTrue(old_visitor.active) - self.assertEqual(partner_demo.visitor_ids, old_visitor, "Visitor and its partner should be synchronized") + 'last_connection_datetime': datetime.now() - timedelta(days=8) + }, { + 'name': 'Sleeping Joe', + 'lang_id': self.env.ref('base.lang_en').id, + 'country_id': self.env.ref('base.be').id, + 'website_id': 1, + 'last_connection_datetime': datetime.now() - timedelta(days=15) + }]) - # archive old visitor - old_visitor.last_connection_datetime = datetime.now() - timedelta(days=8) - self.env['website.visitor']._cron_archive_visitors() - self.assertEqual(old_visitor.active, False, "Visitor should be archived after inactivity") + active_visitors = self.env['website.visitor'].create([{ + 'name': 'Active Donald', + 'lang_id': self.env.ref('base.lang_en').id, + 'country_id': self.env.ref('base.be').id, + 'website_id': 1, + 'last_connection_datetime': datetime.now() - timedelta(days=1) + }, { + 'name': 'Vladimir Customer', + 'lang_id': self.env.ref('base.lang_en').id, + 'country_id': self.env.ref('base.be').id, + 'website_id': 1, + 'partner_id': self.partner_demo.id, + 'last_connection_datetime': datetime.now() - timedelta(days=15) + }]) - # reconnect with new visitor. - self.url_open(self.tracked_page.url) - new_visitor = self._get_last_visitor() - self.assertFalse(new_visitor.partner_id) - self.assertTrue(new_visitor.id > old_visitor.id, "A new visitor should have been created.") - self.assertVisitorTracking(new_visitor, self.tracked_page) + self._test_unlink_old_visitors(inactive_visitors, active_visitors) - with self.mock_visitor_from_request(force_visitor=new_visitor): - self.authenticate('demo', 'demo') - (new_visitor | old_visitor).flush() - partner_demo.flush() - partner_demo.invalidate_cache(fnames=['visitor_ids']) - self.assertEqual(partner_demo.visitor_ids, old_visitor, "The partner visitor should be back to the 'old' visitor.") + def _test_unlink_old_visitors(self, inactive_visitors, active_visitors): + """ This method will test that the visitors are correctly deleted when inactive. - new_visitor = self.env['website.visitor'].search([('id', '=', new_visitor.id)]) - self.assertEqual(len(new_visitor), 0, "The new visitor should be deleted when visitor authenticate once again.") - self.assertEqual(old_visitor.active, True, "The old visitor should be reactivated when visitor authenticates once again.") + - inactive_visitors: all visitors that should be unlinked by the CRON + '_cron_unlink_old_visitors' + - active_visitors: all visitors that should NOT be cleaned because they are either active + or have some important data linked to them (partner, ...) and we want to keep them. + + We use this method as a private tool so that sub-module can also test the cleaning of visitors + based on their own sets of conditions. """ + + WebsiteVisitor = self.env['website.visitor'].with_context(active_test=False) + + self.env['ir.config_parameter'].sudo().set_param('website.visitor.live.days', 7) + + # ensure we keep a single query by correct usage of "not inselect" + # (+1 query to fetch the 'ir.config_parameter') + with self.assertQueryCount(2): + WebsiteVisitor.search(WebsiteVisitor._inactive_visitors_domain()) + + inactive_visitor_ids = inactive_visitors.ids + active_visitor_ids = active_visitors.ids + + WebsiteVisitor._cron_unlink_old_visitors() + if inactive_visitor_ids: + # all inactive visitors should be deleted + self.assertFalse(bool(WebsiteVisitor.search([('id', 'in', inactive_visitor_ids)]))) + if active_visitor_ids: + # all active visitors should be kept + self.assertEqual(active_visitors, WebsiteVisitor.search([('id', 'in', active_visitor_ids)])) + + def test_link_to_visitor(self): + """ Visitors are 'linked' together when the user, previously not connected, authenticates + and the system detects it already had a website.visitor for that partner_id. + This can happen quite often if the user switches browsers / hardwares. + + When 'linking' visitors together, the new visitor is archived and all its relevant data is + merged within the main visitor. See 'website.visitor#_link_to_visitor' for more details. + + This test ensures that all the relevant data are properly merged. + + We build this logic with sub-methods so that sub-modules can easily add their own data and + test that they are correctly merged.""" + + [main_visitor, linked_visitor] = self.env['website.visitor'].create([ + self._prepare_main_visitor_data(), + self._prepare_linked_visitor_data() + ]) + linked_visitor._link_to_visitor(main_visitor) + + self.assertVisitorDeactivated(linked_visitor, main_visitor) + + def _prepare_main_visitor_data(self): + return { + 'name': "Mitchell Main", + 'lang_id': self.env.ref('base.lang_en').id, + 'country_id': self.env.ref('base.be').id, + 'website_id': 1, + 'website_track_ids': [(0, 0, { + 'page_id': self.tracked_page.id, + 'url': self.tracked_page.url + })] + } + + def _prepare_linked_visitor_data(self): + return { + 'name': "Mitchell Main's Phone", + 'lang_id': self.env.ref('base.lang_en').id, + 'country_id': self.env.ref('base.be').id, + 'website_id': 1, + 'website_track_ids': [(0, 0, { + 'page_id': self.tracked_page_2.id, + 'url': self.tracked_page_2.url + })] + } diff --git a/addons/website/views/website_visitor_views.xml b/addons/website/views/website_visitor_views.xml index 6b0b46b0b96..8ec39791db4 100644 --- a/addons/website/views/website_visitor_views.xml +++ b/addons/website/views/website_visitor_views.xml @@ -128,12 +128,16 @@
Visits
- -
Last Page
+ + + +
Last Page
- -
Visited Pages
+ + + +
Visited Pages