diff --git a/addons/website/models/res_users.py b/addons/website/models/res_users.py index 060d5e7cc14..d64f94db6b4 100644 --- a/addons/website/models/res_users.py +++ b/addons/website/models/res_users.py @@ -68,26 +68,33 @@ class ResUsers(models.Model): @classmethod def authenticate(cls, db, login, password, user_agent_env): - """ Override to link the logged in user's res.partner to website.visitor """ + """ Override to link the logged in user's res.partner to website.visitor. + If both a request-based visitor and a user-based visitor exist we try + to update them (have same partner_id), and move sub records to the main + visitor (user one). Purpose is to try to keep a main visitor with as + much sub-records (tracked pages, leads, ...) as possible. """ uid = super(ResUsers, cls).authenticate(db, login, password, user_agent_env) if uid: with cls.pool.cursor() as cr: env = api.Environment(cr, uid, {}) visitor_sudo = env['website.visitor']._get_visitor_from_request() if visitor_sudo: - partner = env.user.partner_id - partner_visitor = env['website.visitor'].with_context(active_test=False).sudo().search([('partner_id', '=', partner.id)]) - if partner_visitor and partner_visitor.id != visitor_sudo.id: - # Link history to older Visitor and delete the newest - visitor_sudo.website_track_ids.write({'visitor_id': partner_visitor.id}) - visitor_sudo.unlink() - # If archived (most likely by the cron for inactivity reasons), reactivate the partner's visitor - if not partner_visitor.active: - partner_visitor.write({'active': True}) + user_partner = env.user.partner_id + other_user_visitor_sudo = env['website.visitor'].with_context(active_test=False).sudo().search( + [('partner_id', '=', user_partner.id), ('id', '!=', visitor_sudo.id)], + order='last_connection_datetime DESC', + ) # current 13.3 state: 1 result max as unique visitor / partner + 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_main.name = user_partner.name + visitor_main.active = True + visitor_main._update_visitor_last_visit() else: - vals = { - 'partner_id': partner.id, - 'name': partner.name - } - visitor_sudo.write(vals) + if visitor_sudo.partner_id != user_partner: + visitor_sudo._link_to_partner( + user_partner, + update_values={'partner_id': user_partner.id}) + visitor_sudo._update_visitor_last_visit() return uid diff --git a/addons/website/models/website_visitor.py b/addons/website/models/website_visitor.py index 7ff7702df94..ca54d20d883 100644 --- a/addons/website/models/website_visitor.py +++ b/addons/website/models/website_visitor.py @@ -4,7 +4,7 @@ from datetime import datetime, timedelta import uuid -from odoo import fields, models, api, registry, _ +from odoo import fields, models, api, _ from odoo.addons.base.models.res_partner import _tz_get from odoo.exceptions import UserError from odoo.tools.misc import _format_time_ago @@ -240,9 +240,43 @@ class WebsiteVisitor(models.Model): vals['name'] = self.env.user.partner_id.name return self.sudo().create(vals) + def _link_to_partner(self, partner, update_values=None): + """ Link visitors to a partner. This method is meant to be overridden in + order to propagate, if necessary, partner information to sub records. + + :param partner: partner used to link sub records; + :param update_values: optional values to update visitors to link; + """ + vals = {'name': partner.name} + if update_values: + vals.update(update_values) + self.write(vals) + + def _link_to_visitor(self, target, keep_unique=True): + """ 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 + 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: + self._link_to_partner(target.partner_id) + # Link sub records of self to target visitor + self.website_track_ids.write({'visitor_id': target.id}) + + if keep_unique: + self.unlink() + + return target + def _cron_archive_visitors(self): - one_week_ago = datetime.now() - timedelta(days=7) - visitors_to_archive = self.env['website.visitor'].sudo().search([('last_connection_datetime', '<', one_week_ago)]) + delay_days = int(self.env['ir.config_parameter'].sudo().get_param('website.visitor.live.days', 30)) + 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}) def _update_visitor_last_visit(self): diff --git a/addons/website/tests/test_website_visitor.py b/addons/website/tests/test_website_visitor.py index 0179bfacef4..db0af8e5039 100644 --- a/addons/website/tests/test_website_visitor.py +++ b/addons/website/tests/test_website_visitor.py @@ -1,32 +1,54 @@ # coding: utf-8 +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from contextlib import contextmanager from datetime import datetime, timedelta +from unittest.mock import patch -from odoo import tests +from odoo.addons.base.tests.common import HttpCaseWithUserDemo from odoo.addons.website.tools import MockRequest +from odoo.addons.website.models.website_visitor import WebsiteVisitor +from odoo.tests import common, tagged + + +class MockVisitor(common.BaseCase): + + @contextmanager + def mock_visitor_from_request(self, force_visitor=False): + + def _get_visitor_from_request(model, *args, **kwargs): + return force_visitor + + with patch.object(WebsiteVisitor, '_get_visitor_from_request', + autospec=True, wraps=WebsiteVisitor, + side_effect=_get_visitor_from_request) as _get_visitor_from_request_mock: + yield + + +@tagged('-at_install', 'post_install', 'website_visitor') +class WebsiteVisitorTests(MockVisitor, HttpCaseWithUserDemo): -@tests.tagged('-at_install', 'post_install') -class WebsiteVisitorTests(tests.HttpCase): def setUp(self): - super().setUp() - self.website = self.env['website'].browse(1) + super(WebsiteVisitorTests, self).setUp() + + self.website = self.env['website'].search([ + ('company_id', '=', self.env.user.company_id.id) + ], limit=1) self.cookies = {} - self.Visitor = self.env['website.visitor'] - self.Track = self.env['website.track'] - Page = self.env['website.page'] - View = self.env['ir.ui.view'] - untracked_view = View.create({ - 'name': 'Base', + + untracked_view = self.env['ir.ui.view'].create({ + 'name': 'UntackedView', 'type': 'qweb', 'arch': ''' - I am a generic page + I am a generic page² ''', 'key': 'test.base_view', 'track': False, }) - tracked_view = View.create({ - 'name': 'Base', + tracked_view = self.env['ir.ui.view'].create({ + 'name': 'TrackedView', 'type': 'qweb', 'arch': ''' @@ -36,8 +58,8 @@ class WebsiteVisitorTests(tests.HttpCase): 'key': 'test.base_view', 'track': True, }) - tracked_view_2 = View.create({ - 'name': 'Base', + tracked_view_2 = self.env['ir.ui.view'].create({ + 'name': 'TrackedView2', 'type': 'qweb', 'arch': ''' @@ -47,7 +69,7 @@ class WebsiteVisitorTests(tests.HttpCase): 'key': 'test.base_view', 'track': True, }) - [self.untracked_view, self.tracked_view, self.tracked_view_2] = Page.create([ + [self.untracked_page, self.tracked_page, self.tracked_page_2] = self.env['website.page'].create([ { 'view_id': untracked_view.id, 'url': '/untracked_view', @@ -65,104 +87,170 @@ class WebsiteVisitorTests(tests.HttpCase): }, ]) - def test_create_visitor_on_tracked_page(self): - self.assertEqual(len(self.Visitor.search([])), 0, "No visitor at the moment") - self.assertEqual(len(self.Track.search([])), 0, "No track at the moment") - self.url_open(self.untracked_view.url) - self.url_open(self.tracked_view.url) - self.url_open(self.tracked_view.url) - self.assertEqual(len(self.Visitor.search([])), 1, "1 visitor should be created") - self.assertEqual(len(self.Track.search([])), 1, "There should be 1 tracked page") + self.user_portal = self.env['res.users'].search([('login', '=', 'portal')]) + self.partner_portal = self.user_portal.partner_id + if not self.user_portal: + self.env['ir.config_parameter'].sudo().set_param('auth_password_policy.minlength', 4) + self.partner_portal = self.env['res.partner'].create({ + 'name': 'Joel Willis', + 'email': 'joel.willis63@example.com', + }) + self.user_portal = self.env['res.users'].create({ + 'login': 'portal', + 'password': 'portal', + 'partner_id': self.partner_portal.id, + 'groups_id': [(6, 0, [self.env.ref('base.group_portal').id])], + }) - # admin connects - visitor_admin = self.Visitor.search([]) - self.cookies = {'visitor_uuid': visitor_admin.access_token} + def _get_last_visitor(self): + return self.env['website.visitor'].search([], limit=1, order="id DESC") + + def assertPageTracked(self, visitor, page): + """ Check a page is in visitor tracking data """ + self.assertIn(page, visitor.website_track_ids.page_id) + self.assertIn(page, visitor.page_ids) + + def assertVisitorTracking(self, visitor, pages): + """ Check the whole tracking history of a visitor """ + for page in pages: + self.assertPageTracked(visitor, page) + self.assertEqual( + len(visitor.website_track_ids), + len(pages) + ) + + def test_visitor_creation_on_tracked_page(self): + """ Test various flows involving visitor creation and update. """ + existing_visitors = self.env['website.visitor'].search([]) + existing_tracks = self.env['website.track'].search([]) + self.url_open(self.untracked_page.url) + self.url_open(self.tracked_page.url) + self.url_open(self.tracked_page.url) + + new_visitor = self.env['website.visitor'].search([('id', 'not in', existing_visitors.ids)]) + new_track = self.env['website.track'].search([('id', 'not in', existing_tracks.ids)]) + self.assertEqual(len(new_visitor), 1, "1 visitor should be created") + self.assertEqual(len(new_track), 1, "There should be 1 tracked page") + self.assertEqual(new_visitor.visit_count, 1) + self.assertEqual(new_visitor.website_track_ids, new_track) + self.assertVisitorTracking(new_visitor, self.tracked_page) + + # ------------------------------------------------------------ + # Admin connects + # ------------------------------------------------------------ + + self.cookies = {'visitor_uuid': new_visitor.access_token} with MockRequest(self.env, website=self.website, cookies=self.cookies): - self.authenticate('admin', 'admin') + self.authenticate(self.user_admin.login, 'admin') + + visitor_admin = new_visitor # visit a page - self.url_open(self.tracked_view_2.url) + self.url_open(self.tracked_page_2.url) - visitor_admin.refresh() - # page is tracked - self.assertEqual(len(visitor_admin.website_track_ids), 2, "There should be 2 tracked pages for the admin") - # visitor is linked - self.assertEqual(visitor_admin.partner_id, self.env['res.users'].browse(self.session.uid).partner_id, "self.Visitor should be linked with connected partner") + # check tracking and visitor / user sync + self.assertVisitorTracking(visitor_admin, self.tracked_page | self.tracked_page_2) + self.assertEqual(visitor_admin.partner_id, self.partner_admin) + self.assertEqual(visitor_admin.name, self.partner_admin.name) + + # ------------------------------------------------------------ + # Portal connects + # ------------------------------------------------------------ - # portal user connects with MockRequest(self.env, website=self.website, cookies=self.cookies): - self.authenticate('portal', 'portal') - self.assertEqual(len(self.Visitor.search([])), 1, "No extra visitor should be created") + self.authenticate(self.user_portal.login, 'portal') + + self.assertFalse( + self.env['website.visitor'].search([('id', 'not in', (existing_visitors | visitor_admin).ids)]), + "No extra visitor should be created") + # visit a page - self.url_open(self.tracked_view.url) - self.url_open(self.untracked_view.url) - self.url_open(self.tracked_view_2.url) - self.url_open(self.tracked_view_2.url) # 2 time to be sure it does not record twice + self.url_open(self.tracked_page.url) + self.url_open(self.untracked_page.url) + self.url_open(self.tracked_page_2.url) + self.url_open(self.tracked_page_2.url) # 2 time to be sure it does not record twice # new visitor is created - self.assertEqual(len(self.Visitor.search([])), 2, "One extra visitor should be created") - visitor_portal = self.Visitor.search([])[0] - self.cookies['visitor_uuid'] = visitor_portal.access_token - # visitor is linked - self.assertEqual(visitor_portal.partner_id, self.env['res.users'].browse(self.session.uid).partner_id, "self.Visitor should be linked with connected partner") - # tracks are created - self.assertEqual(len(visitor_portal.website_track_ids), 2, "There should be 2 tracked pages for the portal user") + new_visitors = self.env['website.visitor'].search([('id', 'not in', existing_visitors.ids)]) + self.assertEqual(len(new_visitors), 2, "One extra visitor should be created") + visitor_portal = new_visitors[0] + self.assertEqual(visitor_portal.partner_id, self.partner_portal) + self.assertEqual(visitor_portal.name, self.partner_portal.name) + self.assertVisitorTracking(visitor_portal, self.tracked_page | self.tracked_page_2) + + # ------------------------------------------------------------ + # Back to anonymous + # ------------------------------------------------------------ # portal user disconnects self.logout() # visit some pages - self.url_open(self.tracked_view.url) - self.url_open(self.untracked_view.url) - self.url_open(self.tracked_view_2.url) - self.url_open(self.tracked_view_2.url) # 2 time to be sure it does not record twice + self.url_open(self.tracked_page.url) + self.url_open(self.untracked_page.url) + self.url_open(self.tracked_page_2.url) + self.url_open(self.tracked_page_2.url) # 2 time to be sure it does not record twice # new visitor is created - self.assertEqual(len(self.Visitor.search([])), 3, "One extra visitor should be created") - visitor = self.Visitor.search([])[0] - self.cookies['visitor_uuid'] = visitor.access_token - # tracks are created - self.assertEqual(len(visitor.website_track_ids), 2, "There should be 2 tracked page for the visitor") - # visitor is not linked - self.assertFalse(visitor.partner_id, "self.Visitor should not be linked to any partner") + new_visitors = self.env['website.visitor'].search([('id', 'not in', existing_visitors.ids)]) + self.assertEqual(len(new_visitors), 3, "One extra visitor should be created") + visitor_anonymous = new_visitors[0] + self.cookies['visitor_uuid'] = visitor_anonymous.access_token + self.assertFalse(visitor_anonymous.name) + self.assertFalse(visitor_anonymous.partner_id) + self.assertVisitorTracking(visitor_anonymous, self.tracked_page | self.tracked_page_2) + visitor_anonymous_tracks = visitor_anonymous.website_track_ids + + # ------------------------------------------------------------ + # Admin connects again + # ------------------------------------------------------------ - # admin connects with MockRequest(self.env, website=self.website, cookies=self.cookies): - self.authenticate('admin', 'admin') + self.authenticate(self.user_admin.login, 'admin') # one visitor is deleted - self.assertEqual(len(self.Visitor.search([])), 2, "One visitor should be deleted") - admin_partner_id = self.env['res.users'].browse(self.session.uid).partner_id - visitor_admin = self.Visitor.search([('partner_id', '=', admin_partner_id.id)]) + visitor_anonymous = self.env['website.visitor'].with_context(active_test=False).search([('id', '=', visitor_anonymous.id)]) + self.assertFalse(visitor_anonymous) + new_visitors = self.env['website.visitor'].search([('id', 'not in', existing_visitors.ids)]) + self.assertEqual(new_visitors, visitor_admin | visitor_portal) + visitor_admin = self.env['website.visitor'].search([('partner_id', '=', self.partner_admin.id)]) # tracks are linked + self.assertTrue(visitor_anonymous_tracks < visitor_admin.website_track_ids) self.assertEqual(len(visitor_admin.website_track_ids), 4, "There should be 4 tracked page for the admin") - # admin user disconnects + # ------------------------------------------------------------ + # Back to anonymous + # ------------------------------------------------------------ + + # admin disconnects self.logout() # visit some pages - self.url_open(self.tracked_view.url) - self.url_open(self.untracked_view.url) - self.url_open(self.tracked_view_2.url) - self.url_open(self.tracked_view_2.url) # 2 time to be sure it does not record twice + self.url_open(self.tracked_page.url) + self.url_open(self.untracked_page.url) + self.url_open(self.tracked_page_2.url) + self.url_open(self.tracked_page_2.url) # 2 time to be sure it does not record twice # new visitor created - self.assertEqual(len(self.Visitor.search([])), 3, "One extra visitor should be created") - visitor = self.Visitor.search([])[0] - self.cookies['visitor_uuid'] = visitor.access_token - # tracks are created - self.assertEqual(len(visitor.website_track_ids), 2, "There should be 2 tracked page for the visitor") - # visitor is not linked - self.assertFalse(visitor.partner_id, "self.Visitor should not be linked to any partner") + new_visitors = self.env['website.visitor'].search([('id', 'not in', existing_visitors.ids)]) + self.assertEqual(len(new_visitors), 3, "One extra visitor should be created") + visitor_anonymous_2 = new_visitors[0] + self.cookies['visitor_uuid'] = visitor_anonymous_2.access_token + self.assertFalse(visitor_anonymous_2.name) + self.assertFalse(visitor_anonymous_2.partner_id) + self.assertVisitorTracking(visitor_anonymous_2, self.tracked_page | self.tracked_page_2) + visitor_anonymous_2_tracks = visitor_anonymous_2.website_track_ids - # portal user connects + # ------------------------------------------------------------ + # Portal connects again + # ------------------------------------------------------------ with MockRequest(self.env, website=self.website, cookies=self.cookies): - self.authenticate('portal', 'portal') + self.authenticate(self.user_portal.login, 'portal') # one visitor is deleted - self.assertEqual(len(self.Visitor.search([])), 2, "One visitor should be deleted") - portal_partner_id = self.env['res.users'].browse(self.session.uid).partner_id - visitor_portal = self.Visitor.search([('partner_id', '=', portal_partner_id.id)]) + new_visitors = self.env['website.visitor'].search([('id', 'not in', existing_visitors.ids)]) + self.assertEqual(new_visitors, visitor_admin | visitor_portal) # tracks are linked + self.assertTrue(visitor_anonymous_2_tracks < visitor_portal.website_track_ids) self.assertEqual(len(visitor_portal.website_track_ids), 4, "There should be 4 tracked page for the portal user") # simulate the portal user comes back 30min later @@ -170,51 +258,51 @@ class WebsiteVisitorTests(tests.HttpCase): track.write({'visit_datetime': track.visit_datetime - timedelta(minutes=30)}) # visit a page - self.url_open(self.tracked_view.url) - visitor_portal.refresh() + self.url_open(self.tracked_page.url) + visitor_portal.invalidate_cache(fnames=['website_track_ids']) # tracks are created self.assertEqual(len(visitor_portal.website_track_ids), 5, "There should be 5 tracked page for the portal user") # simulate the portal user comes back 8hours later visitor_portal.write({'last_connection_datetime': visitor_portal.last_connection_datetime - timedelta(hours=8)}) - self.url_open(self.tracked_view.url) - visitor_portal.refresh() + self.url_open(self.tracked_page.url) + visitor_portal.invalidate_cache(fnames=['visit_count']) # check number of visits self.assertEqual(visitor_portal.visit_count, 2, "There should be 2 visits for the portal user") - def test_long_period_inactivity(self): - # link visitor to partner - old_visitor = self.Visitor.create({ + 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({ '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, }) - partner_demo = self.env.ref('base.partner_demo') - old_visitor.partner_id = partner_demo.id - self.assertEqual(partner_demo.visitor_ids.id, old_visitor.id, "The partner visitor should be set correctly.") + self.assertTrue(old_visitor.active) + self.assertEqual(partner_demo.visitor_ids, old_visitor, "Visitor and its partner should be synchronized") # archive old visitor old_visitor.last_connection_datetime = datetime.now() - timedelta(days=8) - self.Visitor._cron_archive_visitors() - self.assertEqual(old_visitor.active, False, "The visitor should be archived after one week of inactivity") + self.env['website.visitor']._cron_archive_visitors() + self.assertEqual(old_visitor.active, False, "Visitor should be archived after inactivity") # reconnect with new visitor. - self.url_open(self.tracked_view.url) - new_visitor = self.Visitor.search([('id', '!=', old_visitor.id)], limit=1, order="id desc") # get the last created visitor - new_visitor_id = new_visitor.id - self.assertEqual(new_visitor_id > old_visitor.id, True, "A new visitor should have been created.") - self.assertEqual(len(new_visitor), 1, "A visitor should be created after visiting a tracked view") - self.assertEqual(len(self.Track.search([('visitor_id', '=', new_visitor.id)])), 1, - "A track for the new visitor should be created after visiting a tracked view") + 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) - # override the get_visitor_from_request to mock that is new_visitor that authenticates - def get_visitor_from_request(self_mock, force_create=False): - return new_visitor - self.patch(type(self.env['website.visitor']), '_get_visitor_from_request', get_visitor_from_request) + with self.mock_visitor_from_request(force_visitor=new_visitor): + self.authenticate('demo', 'demo') + (new_visitor | old_visitor).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.") - self.authenticate('demo', 'demo') - self.assertEqual(partner_demo.visitor_ids.id, old_visitor.id, "The partner visitor should be back to the 'old' visitor.") - - new_visitor = self.Visitor.search([('id', '=', new_visitor_id)]) + 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.") diff --git a/odoo/addons/base/tests/common.py b/odoo/addons/base/tests/common.py index 4d7e7e00c87..fcaa4df8be1 100644 --- a/odoo/addons/base/tests/common.py +++ b/odoo/addons/base/tests/common.py @@ -32,7 +32,9 @@ class HttpCaseWithUserDemo(HttpCase): def setUp(self): super(HttpCaseWithUserDemo, self).setUp() - self.env.ref('base.partner_admin').write({'name': 'Mitchell Admin'}) + self.user_admin = self.env.ref('base.user_admin') + self.user_admin.write({'name': 'Mitchell Admin'}) + self.partner_admin = self.user_admin.partner_id self.user_demo = self.env['res.users'].search([('login', '=', 'demo')]) self.partner_demo = self.user_demo.partner_id