Skip to content

Commit

Permalink
[FIX] website: fix authentication of archived visitor
Browse files Browse the repository at this point in the history
If a visitor logs in (and has a visitor_id in his cookie), the partner will be
linked to the visitor. If, after 1 week, the visitor tries to connect again
with a different session (or another visitor_id in cookies), the authenticate
will crash because

  * _cron_archive_visitors applies on visitor inactive since at least a week
  * there can be only one visitor per partner (sql constraint)
  * the visitor linked to the partner is not retrieved (because archived) and
  we try to link the partner to a new visitor.

Further than that, if the visitor is archived and the linked partner wants to
login again with a new visitor_id, we should

  * reactivate the previous visitor,
  * copy history from newest to previous one,
  * delete the newest one

Note that last two points were already done before this commit.

Task ID: 2120464
PR #40199
Fixes #40077
Fixes #40301
  • Loading branch information
dbeguin authored and tde-banana-odoo committed Dec 2, 2019
1 parent 827bfb1 commit 33db89b
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 18 deletions.
6 changes: 5 additions & 1 deletion addons/website/models/res_users.py
Expand Up @@ -72,10 +72,14 @@ def authenticate(cls, db, login, password, user_agent_env):
visitor_sudo = env['website.visitor']._get_visitor_from_request() visitor_sudo = env['website.visitor']._get_visitor_from_request()
if visitor_sudo: if visitor_sudo:
partner = env.user.partner_id partner = env.user.partner_id
partner_visitor = env['website.visitor'].sudo().search([('partner_id', '=', 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: 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.website_track_ids.write({'visitor_id': partner_visitor.id})
visitor_sudo.unlink() 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})
else: else:
vals = { vals = {
'partner_id': partner.id, 'partner_id': partner.id,
Expand Down
75 changes: 58 additions & 17 deletions addons/website/tests/test_website_visitor.py
@@ -1,13 +1,15 @@
# coding: utf-8 # coding: utf-8
from odoo.tests import HttpCase from odoo import tests
from datetime import datetime, timedelta




class WebsiteVisitorTests(HttpCase): class WebsiteVisitorTests(tests.HttpCase):
def test_create_visitor_on_tracked_page(self): def setUp(self):
super(WebsiteVisitorTests, self).setUp()
Page = self.env['website.page'] Page = self.env['website.page']
View = self.env['ir.ui.view'] View = self.env['ir.ui.view']
Visitor = self.env['website.visitor'] self.Visitor = self.env['website.visitor']
Track = self.env['website.track'] self.Track = self.env['website.track']
untracked_view = View.create({ untracked_view = View.create({
'name': 'Base', 'name': 'Base',
'type': 'qweb', 'type': 'qweb',
Expand All @@ -30,7 +32,7 @@ def test_create_visitor_on_tracked_page(self):
'key': 'test.base_view', 'key': 'test.base_view',
'track': True, 'track': True,
}) })
[untracked_view, tracked_view] = Page.create([ [self.untracked_view, self.tracked_view] = Page.create([
{ {
'view_id': untracked_view.id, 'view_id': untracked_view.id,
'url': '/untracked_view', 'url': '/untracked_view',
Expand All @@ -43,14 +45,53 @@ def test_create_visitor_on_tracked_page(self):
}, },
]) ])


self.assertEqual(len(Visitor.search([])), 0, "No visitor at the moment") def test_create_visitor_on_tracked_page(self):
self.assertEqual(len(Track.search([])), 0, "No track at the moment") self.assertEqual(len(self.Visitor.search([])), 0, "No visitor at the moment")
self.url_open(untracked_view.url) self.assertEqual(len(self.Track.search([])), 0, "No track at the moment")
self.assertEqual(len(Visitor.search([])), 0, "No visitor created after visiting an untracked view") self.url_open(self.untracked_view.url)
self.assertEqual(len(Track.search([])), 0, "No track created after visiting an untracked view") self.assertEqual(len(self.Visitor.search([])), 0, "No visitor created after visiting an untracked view")
self.url_open(tracked_view.url) self.assertEqual(len(self.Track.search([])), 0, "No track created after visiting an untracked view")
self.assertEqual(len(Visitor.search([])), 1, "A visitor should be created after visiting a tracked view") self.url_open(self.tracked_view.url)
self.assertEqual(len(Track.search([])), 1, "A track should be created after visiting a tracked view") self.assertEqual(len(self.Visitor.search([])), 1, "A visitor should be created after visiting a tracked view")
self.url_open(tracked_view.url) self.assertEqual(len(self.Track.search([])), 1, "A track should be created after visiting a tracked view")
self.assertEqual(len(Visitor.search([])), 1, "No visitor should be created after visiting another tracked view") self.url_open(self.tracked_view.url)
self.assertEqual(len(Track.search([])), 1, "No track should be created after visiting another tracked view before 30 min") self.assertEqual(len(self.Visitor.search([])), 1, "No visitor should be created after visiting another tracked view")
self.assertEqual(len(self.Track.search([])), 1, "No track should be created after visiting another tracked view before 30 min")

def test_long_period_inactivity(self):
# link visitor to partner
old_visitor = self.Visitor.create({
'lang_id': self.env.ref('base.lang_en').id,
'country_id': self.env.ref('base.be').id,
'website_id': 1,
})
partner_demo = self.env.ref('base.partner_demo')
old_visitor.partner_id = partner_demo.id
partner_demo.visitor_ids = [(6, 0, [old_visitor.id])] # TODO DBE : Remove this line in Master (13.1) after visitor_ids field declaration master fix
self.assertEqual(partner_demo.visitor_ids.id, old_visitor.id, "The partner visitor should be set correctly.")

# 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")

# 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")

# 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)

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)])
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.")

0 comments on commit 33db89b

Please sign in to comment.