diff --git a/addons/onboarding/controllers/onboarding.py b/addons/onboarding/controllers/onboarding.py index cffaf43368b..7459b051294 100644 --- a/addons/onboarding/controllers/onboarding.py +++ b/addons/onboarding/controllers/onboarding.py @@ -1,6 +1,8 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. +from psycopg2 import IntegrityError + from odoo import http from odoo.http import request @@ -12,11 +14,17 @@ class OnboardingController(http.Controller): return {} onboarding = request.env['onboarding.onboarding'].search([('route_name', '=', route_name)]) - if onboarding and not onboarding._search_or_create_progress().is_onboarding_closed: - # JS implementation of the onboarding panel expects this data structure - return { - 'html': request.env['ir.qweb']._render( - 'onboarding.onboarding_panel', onboarding._prepare_rendering_values()) - } + if onboarding: + try: + progress = onboarding._search_or_create_progress() + except IntegrityError: # Another worker created the record at the same time + return {'code': 503} # Temporarily unavailable - Invites client to try again + + if not progress.is_onboarding_closed: + # JS implementation of the onboarding panel expects this data structure + return { + 'html': request.env['ir.qweb']._render( + 'onboarding.onboarding_panel', onboarding._prepare_rendering_values()) + } return {} diff --git a/addons/onboarding/models/onboarding_progress.py b/addons/onboarding/models/onboarding_progress.py index c5a5dcc7da8..654ab87ae4c 100644 --- a/addons/onboarding/models/onboarding_progress.py +++ b/addons/onboarding/models/onboarding_progress.py @@ -23,10 +23,13 @@ class OnboardingProgress(models.Model): onboarding_id = fields.Many2one( 'onboarding.onboarding', 'Related onboarding tracked', required=True, ondelete='cascade') progress_step_ids = fields.One2many('onboarding.progress.step', 'progress_id', 'Progress Steps Trackers') - _sql_constraints = [ - ('onboarding_company_uniq', 'unique (onboarding_id,company_id)', - 'There cannot be multiple records of the same onboarding completion for the same company.'), - ] + + def init(self): + # not in _sql_constraint because COALESCE is not supported for PostgreSQL constraint + self.env.cr.execute(""" + CREATE UNIQUE INDEX IF NOT EXISTS onboarding_progress_onboarding_company_uniq + ON onboarding_progress (onboarding_id, COALESCE(company_id, 0)) + """) @api.depends('onboarding_id.step_ids', 'progress_step_ids', 'progress_step_ids.step_state') def _compute_onboarding_state(self): diff --git a/addons/onboarding/tests/__init__.py b/addons/onboarding/tests/__init__.py index cd240e870a0..f01d0da0642 100644 --- a/addons/onboarding/tests/__init__.py +++ b/addons/onboarding/tests/__init__.py @@ -2,3 +2,4 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. from . import test_onboarding +from . import test_onboarding_concurrency diff --git a/addons/onboarding/tests/test_onboarding.py b/addons/onboarding/tests/test_onboarding.py index c28886be5fe..94491e49dd8 100644 --- a/addons/onboarding/tests/test_onboarding.py +++ b/addons/onboarding/tests/test_onboarding.py @@ -1,7 +1,10 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. +from psycopg2 import IntegrityError + from odoo.addons.onboarding.tests.common import TestOnboardingCommon +from odoo.tools import mute_logger class TestOnboarding(TestOnboardingCommon): @@ -131,36 +134,34 @@ class TestOnboarding(TestOnboardingCommon): self.assert_onboarding_is_not_done(self.onboarding_1) - def test_no_crash_on_multiple_progress_records(self): - existing_progress = self.env['onboarding.progress'].search([ - ('onboarding_id', '=', self.onboarding_1.id), ('company_id', '=', False) - ]) - self.assertEqual(len(existing_progress), 1) + @mute_logger('odoo.sql_db') + def test_progress_no_company_uniqueness(self): + """Check that there cannot be two progress records created for + the same onboarding when it is configured to be completed only + once for the whole db and not per-company (is_per_company=False). + NB: Postgresql UNIQUE constraint failures raise IntegrityErrors. + """ + self.assertFalse(self.onboarding_1.current_progress_id.company_id) + with self.assertRaises(IntegrityError): + self.env['onboarding.progress'].create({ + 'onboarding_id': self.onboarding_1.id, + 'company_id': False + }) - extra_progress = self.env['onboarding.progress'].create({ - 'onboarding_id': self.onboarding_1.id, - 'company_id': False - }) + @mute_logger('odoo.sql_db') + def test_progress_per_company_uniqueness(self): + """Check that there cannot be two progress records created for + the same company and the same onboarding when the onboarding is + configured to be completed per-company. + See also ``test_progress_no_company_uniqueness`` + """ + # Updating onboarding to per-company + self.onboarding_1.is_per_company = True + # Required after progress reset (simulate role of controller) + self.onboarding_1._search_or_create_progress() - self.env['onboarding.progress.step'].create([{ - 'step_id': self.onboarding_1_step_1.id, - 'progress_id': progress.id - } for progress in (existing_progress, extra_progress) - ]) - - nb_progress = self.env['onboarding.progress'].search([ - ('onboarding_id', '=', self.onboarding_1.id), ('company_id', '=', False)], count=True) - nb_progress_steps = self.env['onboarding.progress.step'].search([ - ('step_id', '=', self.onboarding_1_step_1.id)], count=True) - - # Even though multiple onboarding progress (& steps) records exist - self.assertEqual(nb_progress, 2) - self.assertEqual(nb_progress_steps, 2) - - # no error is raised, and so we can interact - _ = self.onboarding_1_step_1.current_progress_step_id - self.onboarding_1_step_1.action_set_just_done() - - # Same with onboarding progress - _ = self.onboarding_1.current_progress_id - self.onboarding_1.action_close() + with self.assertRaises(IntegrityError): + self.env['onboarding.progress'].create({ + 'onboarding_id': self.onboarding_1.id, + 'company_id': self.env.company.id + }) diff --git a/addons/onboarding/tests/test_onboarding_concurrency.py b/addons/onboarding/tests/test_onboarding_concurrency.py new file mode 100644 index 00000000000..75f23cf7301 --- /dev/null +++ b/addons/onboarding/tests/test_onboarding_concurrency.py @@ -0,0 +1,86 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +import threading +from concurrent.futures import ThreadPoolExecutor + +from psycopg2 import IntegrityError + +import odoo +from odoo.tests.common import get_db_name, tagged, BaseCase +from odoo.tools import mute_logger + + +@tagged('-standard', '-at_install', 'post_install', 'database_breaking') +class TestOnboardingConcurrency(BaseCase): + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.registry = odoo.registry(get_db_name()) + cls.addClassCleanup(cls.cleanUpClass) + + with cls.registry.cursor() as cr: + env = odoo.api.Environment(cr, odoo.SUPERUSER_ID, {}) + cls.onboarding_id = env['onboarding.onboarding'].create([ + { + 'name': 'Test Onboarding Concurrent', + 'is_per_company': False, + 'route_name': 'onboarding_concurrent' + } + ]).id + + @classmethod + def cleanUpClass(cls): + with cls.registry.cursor() as cr: + env = odoo.api.Environment(cr, odoo.SUPERUSER_ID, {}) + env['onboarding.onboarding'].browse(cls.onboarding_id).unlink() + env['onboarding.progress'].search([ + ('onboarding_id', '=', cls.onboarding_id) + ]).unlink() + + @mute_logger('odoo.sql_db') + def test_concurrent_create_progress(self): + barrier = threading.Barrier(2) + + def run(): + raised_unique_violation = False + + with self.registry.cursor() as cr: + env = odoo.api.Environment(cr, odoo.SUPERUSER_ID, {}) + onboarding = env['onboarding.onboarding'].search([ + ('id', '=', self.onboarding_id) + ]) + # There is no progress record + self.assertFalse(env['onboarding.progress'].search([ + ('onboarding_id', '=', self.onboarding_id) + ])) + barrier.wait(timeout=2) + try: + onboarding._create_progress() + except IntegrityError as e: + if e.pgcode == "23505": # UniqueViolation + raised_unique_violation = True + + return raised_unique_violation + + with ThreadPoolExecutor(max_workers=2) as executor: + future_1 = executor.submit(run) + future_2 = executor.submit(run) + raised_1 = future_1.result(timeout=3) + raised_2 = future_2.result(timeout=3) + + with self.registry.cursor() as cr: + env = odoo.api.Environment(cr, odoo.SUPERUSER_ID, {}) + self.assertEqual( + len(env['onboarding.progress'].search([('onboarding_id', '=', self.onboarding_id)])), + 1, + "Exactly one thread should have been able to create a record." + ) + + self.assertEqual( + raised_1 + raised_2, + 1, + "Exactly one thread should have raised a UniqueViolation error even though " + "there was no progress record at the start of its transaction." + ) diff --git a/addons/web/static/src/views/onboarding_banner.js b/addons/web/static/src/views/onboarding_banner.js index 43515ab90c2..de9151f5cf5 100644 --- a/addons/web/static/src/views/onboarding_banner.js +++ b/addons/web/static/src/views/onboarding_banner.js @@ -39,7 +39,11 @@ export class OnboardingBanner extends Component { } async loadBanner(bannerRoute) { - const response = await this.rpc(bannerRoute, { context: this.user.context }); + let response = await this.rpc(bannerRoute, { context: this.user.context }); + if (response.code === 503) { + // Sent by Onboarding Controller when rare concurrent `create` transactions occur + response = await this.rpc(bannerRoute, { context: this.user.context }); + } if (!response.html) { return; }