diff --git a/website_sale_aplicoop/controllers/website_sale.py b/website_sale_aplicoop/controllers/website_sale.py index 2111a04..4925b6a 100644 --- a/website_sale_aplicoop/controllers/website_sale.py +++ b/website_sale_aplicoop/controllers/website_sale.py @@ -718,8 +718,12 @@ class AplicoopWebsiteSale(WebsiteSale): return "/eskaera" return f"/eskaera/{group_order.slug}{suffix}" - def _redirect_to_slug_url(self, order_id, suffix="", **post): - """Send a legacy /eskaera/ URL to its /eskaera/ equivalent.""" + def _redirect_to_slug_url(self, order_id, post, suffix=""): + """Send a legacy /eskaera/ URL to its /eskaera/ equivalent. + + `post` is passed as a plain dict, not splatted: a query parameter named + `suffix` would otherwise land on the keyword argument of the same name. + """ group_order = request.env["group.order"].sudo().browse(order_id).exists() url = self._eskaera_url(group_order, suffix=suffix) if post and url != "/eskaera": @@ -729,7 +733,7 @@ class AplicoopWebsiteSale(WebsiteSale): @http.route(["/eskaera/"], type="http", auth="user", website=True) def eskaera_shop_legacy(self, order_id, **post): """Keep the old numeric shop URL working, pointing at the slug one.""" - return self._redirect_to_slug_url(order_id, **post) + return self._redirect_to_slug_url(order_id, post) @http.route( ["/eskaera/"], @@ -1296,7 +1300,7 @@ class AplicoopWebsiteSale(WebsiteSale): ) def eskaera_checkout_legacy(self, order_id, **post): """Keep the old numeric checkout URL working, pointing at the slug one.""" - return self._redirect_to_slug_url(order_id, suffix="/checkout", **post) + return self._redirect_to_slug_url(order_id, post, suffix="/checkout") @http.route( ["/eskaera//checkout"], diff --git a/website_sale_aplicoop/migrations/18.0.1.13.0/post-migrate.py b/website_sale_aplicoop/migrations/18.0.1.13.0/post-migrate.py index e29ebf1..2ae012c 100644 --- a/website_sale_aplicoop/migrations/18.0.1.13.0/post-migrate.py +++ b/website_sale_aplicoop/migrations/18.0.1.13.0/post-migrate.py @@ -17,9 +17,13 @@ def migrate(cr, version): if not version: return - env = api.Environment(cr, SUPERUSER_ID, {}) + # tracking_disable: `slug` is a tracked field, backfilling it would post a + # chatter message and notify the followers of every pre-existing order. + env = api.Environment(cr, SUPERUSER_ID, {"tracking_disable": True}) orders = env["group.order"].search([("slug", "=", False)], order="id") for order in orders: + # One write at a time: the search inside `_generate_unique_slug` flushes + # the previous one, so two orders sharing a name get distinct slugs. order.slug = order._generate_unique_slug(order.name) if orders: diff --git a/website_sale_aplicoop/models/group_order.py b/website_sale_aplicoop/models/group_order.py index c21c31d..8c716e0 100644 --- a/website_sale_aplicoop/models/group_order.py +++ b/website_sale_aplicoop/models/group_order.py @@ -398,14 +398,18 @@ class GroupOrder(models.Model): domain.append(("id", "not in", self.ids)) return bool(self.sudo().search_count(domain, limit=1)) - def _generate_unique_slug(self, name): - """Build a free slug out of `name`, adding a counter when needed.""" + def _generate_unique_slug(self, name, taken=()): + """Build a free slug out of `name`, adding a counter when needed. + + `taken` holds the slugs already handed out in the same batch, which are + not in the database yet and so are invisible to `_is_slug_taken`. + """ base = self._normalize_slug(name) if not base or base.isdigit() or base in RESERVED_SLUGS: base = f"{base}-{DEFAULT_SLUG_BASE}" if base else DEFAULT_SLUG_BASE candidate = base counter = 2 - while self._is_slug_taken(candidate): + while candidate in taken or self._is_slug_taken(candidate): candidate = f"{base}-{counter}" counter += 1 return candidate @@ -413,17 +417,15 @@ class GroupOrder(models.Model): @api.model_create_multi def create(self, vals_list): """Give every consumer group order a slug for its public URL.""" - vals_list = [ - dict( - vals, - slug=( - self._normalize_slug(vals.get("slug")) - or self._generate_unique_slug(vals.get("name")) - ), + taken = set() + new_vals_list = [] + for vals in vals_list: + slug = self._normalize_slug(vals.get("slug")) or self._generate_unique_slug( + vals.get("name"), taken=taken ) - for vals in vals_list - ] - return super().create(vals_list) + taken.add(slug) + new_vals_list.append(dict(vals, slug=slug)) + return super().create(new_vals_list) def write(self, vals): """Normalize the slug, regenerating it from the name when emptied.""" diff --git a/website_sale_aplicoop/tests/test_group_order_slug.py b/website_sale_aplicoop/tests/test_group_order_slug.py index 63e463d..f4be4c3 100644 --- a/website_sale_aplicoop/tests/test_group_order_slug.py +++ b/website_sale_aplicoop/tests/test_group_order_slug.py @@ -13,7 +13,7 @@ from odoo.tests.common import TransactionCase class GroupOrderSlugCommon: """Helpers to build consumer group orders with the mandatory fields.""" - def _create_group_order(self, name, **values): + def _group_order_vals(self, name, **values): start_date = datetime.now().date() vals = { "name": name, @@ -26,7 +26,10 @@ class GroupOrderSlugCommon: "cutoff_day": "0", } vals.update(values) - return self.env["group.order"].create(vals) + return vals + + def _create_group_order(self, name, **values): + return self.env["group.order"].create(self._group_order_vals(name, **values)) class TestGroupOrderSlug(GroupOrderSlugCommon, TransactionCase): @@ -53,6 +56,25 @@ class TestGroupOrderSlug(GroupOrderSlugCommon, TransactionCase): self.assertEqual(first.slug, "weekly-order") self.assertEqual(second.slug, "weekly-order-2") + def test_slug_is_unique_within_a_single_create(self): + """Records created in one batch cannot see each other in the database.""" + orders = self.env["group.order"].create( + [self._group_order_vals("Weekly Order") for _ in range(3)] + ) + self.assertEqual( + orders.mapped("slug"), + ["weekly-order", "weekly-order-2", "weekly-order-3"], + ) + + def test_generated_slug_avoids_an_explicit_one_in_the_same_create(self): + orders = self.env["group.order"].create( + [ + self._group_order_vals("Something Else", slug="weekly-order"), + self._group_order_vals("Weekly Order"), + ] + ) + self.assertEqual(orders.mapped("slug"), ["weekly-order", "weekly-order-2"]) + def test_explicit_slug_is_normalized(self): order = self._create_group_order("Weekly Order", slug="Fructuós Gelabert") self.assertEqual(order.slug, "fructuos-gelabert") @@ -151,6 +173,19 @@ class TestGroupOrderSlugRoutes(GroupOrderSlugCommon, HttpCase): f"got {response.headers.get('Location')}", ) + def test_numeric_url_keeps_query_parameters(self): + """A `suffix` parameter must stay in the query string, not in the path.""" + self.authenticate(self.portal_login, self.portal_login) + response = self.url_open( + f"/eskaera/{self.group_order.id}/checkout?suffix=x&search=apple", + allow_redirects=False, + ) + self.assertIn(response.status_code, (301, 302, 303)) + location = response.headers.get("Location", "") + self.assertIn(f"/eskaera/{self.group_order.slug}/checkout?", location) + self.assertIn("suffix=x", location) + self.assertIn("search=apple", location) + def test_unknown_slug_falls_back_to_the_list(self): self.authenticate(self.portal_login, self.portal_login) response = self.url_open("/eskaera/does-not-exist", allow_redirects=False)