[FIX] website_sale_aplicoop: three defects in the group order slug
- create(): slugs were only checked against the database, so records created in the same batch (duplicating several orders from the list view, importing rows sharing a name) collided on group_order_slug_uniq. _generate_unique_slug now also skips the slugs already handed out in the batch. - _redirect_to_slug_url(): `post` is passed as a dict instead of splatted, so a query parameter named `suffix` no longer binds to the keyword argument of the same name (HTTP 500 on /eskaera/<id>/checkout?suffix=x, and a corrupted path on the shop route). - post-migrate: the backfill runs with tracking_disable, `slug` is tracked and the upgrade posted a chatter message on every pre-existing order. Tests for the two reachable cases: batch create and query parameters kept across the legacy redirect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
23edee6154
commit
157b344d4b
4 changed files with 65 additions and 20 deletions
|
|
@ -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/<id> URL to its /eskaera/<slug> equivalent."""
|
||||
def _redirect_to_slug_url(self, order_id, post, suffix=""):
|
||||
"""Send a legacy /eskaera/<id> URL to its /eskaera/<slug> 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/<int:order_id>"], 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/<string:group_order_slug>"],
|
||||
|
|
@ -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/<string:group_order_slug>/checkout"],
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue