diff --git a/docs/trailhead/tools.md b/docs/trailhead/tools.md index 2d12738b..493c971e 100644 --- a/docs/trailhead/tools.md +++ b/docs/trailhead/tools.md @@ -473,12 +473,27 @@ instances in September 2026 (a rename). needs. This also makes the command idempotent. - Rewrites descendants whose `url_path` is already inconsistent with their parent, which is what Wagtail leaves behind when rows are orphaned; `Page.save()`'s own cascade only - rewrites paths sharing the old prefix. + rewrites paths sharing the old prefix. These are worked out **after** the renames, from + live rows, because a rename moves the very prefix they are built from. +- Rewrites `slug` and `url_path` in **every revision** of every page in the repaired trees. + `Page.save()` touches live rows only, and the Wagtail edit form populates from the latest + revision — so without this, one *Save draft* in the admin puts the old slug back and the + instance breaks again with nobody having touched a slug field. On a swap it is worse than + a revert: after Longmont's September 2026 rename, `longmont-2021`'s Spanish root still had + a revision saying `longmont-1`, by then the live slug of `longmont`'s Spanish root, and + publishing it would have put two live siblings on one `url_path`. +- Scans the translated trees for both of the above, not just the primary-language one. - Calls `invalidate_cache()` on each instance, since the GraphQL response cache holds the old paths. -Wagtail's redirect handler creates redirects from the old paths as a side effect, so -existing bookmarks keep working. +Because of the revision pass, the command has work to do even on an instance whose slugs +already look right — that is exactly the state a rename done by hand leaves behind. Run it +with no `--apply` as an audit. + +Old paths are **not** redirected. The public site resolves pages only through the GraphQL +`page(path:)` resolver, which never consults `wagtail.contrib.redirects`, and instance root +pages are not Wagtail `Site` roots, so the auto-redirect signal handler has no site to +create redirects for. Links to a pre-rename path stay broken. ## sync_instance_to_db diff --git a/src/nodes/management/commands/repair_instance_page_slugs.py b/src/nodes/management/commands/repair_instance_page_slugs.py index 96abc990..32894e56 100644 --- a/src/nodes/management/commands/repair_instance_page_slugs.py +++ b/src/nodes/management/commands/repair_instance_page_slugs.py @@ -34,7 +34,8 @@ ``(-[0-9]+)?`` in the regex is for, so the Spanish site breaks and heals by the same rule. Translations are resolved by ``translation_key`` and locale (``InstanceConfig.get_translated_root_page``), never by slug, so renaming them moves no -references. +references. Every tree the instance serves is repaired the same way: slug, descendant +paths, and revisions. ## Two passes, because the slugs collide @@ -48,7 +49,26 @@ ``Page.save()`` cascades ``url_path`` to descendants when the slug changes, so the children follow. Descendants whose ``url_path`` is *already* inconsistent with their parent -- which Wagtail leaves behind when rows are orphaned -- are reported and rewritten too, since the -cascade only rewrites paths that share the old prefix. +cascade only rewrites paths that share the old prefix. Those rewrites are worked out +*after* the renames, from live rows, because the renames move the very prefix they are +built from. + +## Revisions have to move with the live rows + +``Page.save()`` rewrites live rows only. A revision keeps the ``slug`` and ``url_path`` it +was saved with, and the Wagtail edit form populates from the latest revision -- so after a +rename, one *Save draft* in the admin writes the old slug straight back, and the instance +breaks again with nobody having touched a slug field. + +On a swap it is worse than a revert. When Longmont was renamed by hand in September 2026, +``longmont-2021``'s Spanish root kept a revision saying ``longmont-1``, a slug that by then +belonged to ``longmont``'s Spanish root; publishing it would have put two live siblings on +one ``url_path``, where ``resolve_page`` returns whichever the queryset yields first. So +every revision of every page in the repaired trees is rewritten to the live values. + +That rewrite is also why this command has something to do on an instance whose slugs are +already correct: a rename applied without carrying the revisions along leaves exactly that +state behind. """ from __future__ import annotations @@ -79,6 +99,32 @@ def needed(self) -> bool: return self.page.slug != self.desired +def _parent_prefix(page: Page) -> str: + """``url_path`` of the page's parent, which every child path is built on top of.""" + parent = page.get_parent() + return parent.url_path if parent is not None else '/' + + +def _own_url_path(page: Page) -> str: + """Return the ``url_path`` this page should have for the slug it currently carries.""" + return f'{_parent_prefix(page)}{page.slug}/' + + +def _root_pages(ic: InstanceConfig) -> list[Page]: + """ + Return the instance's root page and every translation of it. + + Resolved by ``translation_key``, the same way ``InstanceConfig.get_translated_root_page`` + finds them, so this covers every tree the instance actually serves -- not just the + primary-language one that ``root_page`` points at. + """ + root = ic.root_page + if root is None: + return [] + translations = Page.objects.filter(translation_key=root.translation_key).exclude(pk=root.pk).order_by('locale_id', 'pk') + return [root, *translations] + + def _desired_slugs(ic: InstanceConfig) -> list[SlugChange]: """ Desired slug for the instance's root page and each of its translations. @@ -88,32 +134,135 @@ def _desired_slugs(ic: InstanceConfig) -> list[SlugChange]: carries no meaning -- ``resolve_url_path`` accepts any digits -- it only has to be unique among the siblings. """ - root = ic.root_page - if root is None: + pages = _root_pages(ic) + if not pages: return [] + root, *translations = pages changes = [SlugChange(root, ic.identifier, ic.identifier)] - translations = Page.objects.filter(translation_key=root.translation_key).exclude(pk=root.pk).order_by('locale_id', 'pk') for n, page in enumerate(translations, start=1): changes.append(SlugChange(page, f'{ic.identifier}-{n}', ic.identifier)) return changes -def _inconsistent_descendants(root: Page) -> list[tuple[Page, str]]: - """Descendants whose ``url_path`` does not continue their parent's, with the corrected value.""" - out: list[tuple[Page, str]] = [] - by_pk = {root.pk: root.url_path} +def _expected_paths(root: Page, root_url_path: str) -> dict[int, str]: + """ + ``url_path`` for the root and every descendant, given the root's final path. + + Walked top-down (``path`` order puts parents before children) so each level builds on + the corrected level above it rather than on what is currently in the database. That is + what lets the planning stage quote targets in post-rename terms. + """ + expected = {root.pk: root_url_path} for page in root.get_descendants().order_by('path'): parent = page.get_parent() - parent_path = by_pk.get(parent.pk) if parent is not None else None - if parent_path is None: + base = expected.get(parent.pk) if parent is not None else None + if base is None: + continue + expected[page.pk] = f'{base}{page.slug}/' + return expected + + +def _disconnected_pages(root: Page, root_url_path: str) -> list[tuple[Page, str]]: + """ + Pages whose ``url_path`` does not continue their parent's, with the value to write. + + These are the rows the ``Page.save()`` cascade cannot repair: it rewrites only paths + sharing the old prefix, so anything Wagtail left orphaned keeps its stale path straight + through a rename. + + Detection compares against the parent's *current* path -- a page merely about to be + moved by a rename is not disconnected, the cascade has that covered -- while the target + comes from the post-rename layout, so what is reported is what will be written. + """ + target = _expected_paths(root, root_url_path) + out: list[tuple[Page, str]] = [] + for page in [root, *root.get_descendants().order_by('path')]: + if page.url_path == _own_url_path(page): continue - expected = f'{parent_path}{page.slug}/' - by_pk[page.pk] = expected - if page.url_path != expected: - out.append((page, expected)) + want = target.get(page.pk) + if want is not None and want != page.url_path: + out.append((page, want)) return out +def _revision_reverts(content: dict[str, Any], want_slug: str, want_path: str | None) -> bool: + """Whether publishing this revision would put back a slug or path the repair removes.""" + if 'slug' in content and content['slug'] != want_slug: + return True + return want_path is not None and 'url_path' in content and content['url_path'] != want_path + + +def _stale_revisions(root: Page, root_url_path: str, root_slug: str) -> list[tuple[Page, int]]: + """ + Pages holding revisions that would undo the repair, with how many each has. + + Compared against the values the page will have *after* the repair, so this catches both + a rename that has not happened yet and one that was applied without the revisions being + carried along. + """ + target = _expected_paths(root, root_url_path) + out: list[tuple[Page, int]] = [] + for page in [root, *root.get_descendants().order_by('path')]: + want_slug = root_slug if page.pk == root.pk else page.slug + want_path = target.get(page.pk) + n = sum(1 for rev in page.revisions.all() if _revision_reverts(rev.content, want_slug, want_path)) + if n: + out.append((page, n)) + return out + + +def _force_subtree_paths(root_pk: int) -> list[tuple[int, str, str]]: + """ + Rewrite ``url_path`` down a tree so every level continues its parent. + + Read from live rows and run after the slug passes, so it needs nothing carried over + from planning -- which is what keeps it correct when a rename has just moved the prefix + underneath the rows being repaired. + + ``queryset.update`` deliberately bypasses ``Page.save()``: the slug cascade has already + run, and re-entering it here would re-derive the paths this is correcting. + """ + root = Page.objects.get(pk=root_pk) + fixed: list[tuple[int, str, str]] = [] + + want_root = _own_url_path(root) + if root.url_path != want_root: + fixed.append((root.pk, root.url_path, want_root)) + Page.objects.filter(pk=root.pk).update(url_path=want_root) + + expected = {root.pk: want_root} + for page in root.get_descendants().order_by('path'): + parent = page.get_parent() + base = expected.get(parent.pk) if parent is not None else None + if base is None: + continue + want = f'{base}{page.slug}/' + expected[page.pk] = want + if page.url_path != want: + fixed.append((page.pk, page.url_path, want)) + Page.objects.filter(pk=page.pk).update(url_path=want) + return fixed + + +def _sync_revisions(page: Page) -> int: + """Rewrite ``slug``/``url_path`` in every revision of the page to match the live row.""" + n = 0 + for revision in page.revisions.all(): + content = revision.content + changed = False + if 'slug' in content and content['slug'] != page.slug: + content['slug'] = page.slug + changed = True + if 'url_path' in content and content['url_path'] != page.url_path: + content['url_path'] = page.url_path + changed = True + if changed: + revision.content = content + revision.save(update_fields=['content']) + n += 1 + return n + + def _resolve_instances(identifiers: list[str], use_all: bool) -> list[InstanceConfig]: if use_all: if identifiers: @@ -140,46 +289,60 @@ def add_arguments(self, parser: ArgumentParser) -> None: def handle(self, *args: Any, **options: Any) -> None: instances = _resolve_instances(options['instances'], options['all']) - changes, path_fixes = self._collect(instances) + changes, path_fixes, stale_revisions = self._collect(instances) - if not changes and not path_fixes: + if not changes and not path_fixes and not stale_revisions: self.stdout.write(self.style.SUCCESS('Nothing to do: every root page slug matches its identifier.')) return - self._report(changes, path_fixes) + self._report(changes, path_fixes, stale_revisions) if not options['apply']: self.stdout.write(self.style.WARNING('\nPlan only. Re-run with --apply to write.')) return - self._apply(instances, changes, path_fixes) - self.stdout.write(self.style.SUCCESS(f'\nApplied {len(changes)} slug changes, {len(path_fixes)} path fixes.')) + n_paths, n_revisions = self._apply(instances, changes) + self.stdout.write( + self.style.SUCCESS(f'\nApplied {len(changes)} slug changes, {n_paths} path fixes, {n_revisions} revision rewrites.') + ) - def _collect(self, instances: list[InstanceConfig]) -> tuple[list[SlugChange], list[tuple[Page, str]]]: + def _collect( + self, instances: list[InstanceConfig] + ) -> tuple[list[SlugChange], list[tuple[Page, str]], list[tuple[Page, int]]]: changes: list[SlugChange] = [] path_fixes: list[tuple[Page, str]] = [] + stale_revisions: list[tuple[Page, int]] = [] for ic in instances: if ic.root_page is None: self.stdout.write(f'{ic.identifier}: no root page, skipped') continue - changes.extend(change for change in _desired_slugs(ic) if change.needed) - path_fixes.extend(_inconsistent_descendants(ic.root_page)) - return changes, path_fixes - - def _report(self, changes: list[SlugChange], path_fixes: list[tuple[Page, str]]) -> None: + for change in _desired_slugs(ic): + if change.needed: + changes.append(change) + # Every tree the instance serves, whether or not its own slug has to move: + # a rename applied earlier without the revisions leaves work here too. + root_url_path = f'{_parent_prefix(change.page)}{change.desired}/' + path_fixes.extend(_disconnected_pages(change.page, root_url_path)) + stale_revisions.extend(_stale_revisions(change.page, root_url_path, change.desired)) + return changes, path_fixes, stale_revisions + + def _report( + self, + changes: list[SlugChange], + path_fixes: list[tuple[Page, str]], + stale_revisions: list[tuple[Page, int]], + ) -> None: for change in changes: self.stdout.write( f'{change.instance_identifier}: page {change.page.pk} ' f'{change.page.slug!r} -> {change.desired!r} (url_path {change.page.url_path!r})' ) for page, expected in path_fixes: - self.stdout.write(f' page {page.pk} url_path {page.url_path!r} -> {expected!r} (was inconsistent)') + self.stdout.write(f' page {page.pk} url_path {page.url_path!r} -> {expected!r} (was disconnected)') + for page, count in stale_revisions: + self.stdout.write(f' page {page.pk} {count} revision(s) would put back the old slug/url_path') - def _apply( - self, - instances: list[InstanceConfig], - changes: list[SlugChange], - path_fixes: list[tuple[Page, str]], - ) -> None: + def _apply(self, instances: list[InstanceConfig], changes: list[SlugChange]) -> tuple[int, int]: + n_paths = n_revisions = 0 with transaction.atomic(): # Park every mover on a slug nothing can collide with, so a swap between two # instances does not hit the sibling uniqueness constraint mid-way. @@ -189,10 +352,13 @@ def _apply( for change in changes: change.page.slug = change.desired change.page.save() - for page, expected in path_fixes: - fresh = Page.objects.get(pk=page.pk) - if fresh.url_path != expected: - Page.objects.filter(pk=page.pk).update(url_path=expected) + # Recomputed here rather than reused from the plan: the renames above have just + # moved the prefix these paths are built from. for ic in instances: + for root in _root_pages(ic): + n_paths += len(_force_subtree_paths(root.pk)) + for page in Page.objects.get(pk=root.pk).get_descendants(inclusive=True): + n_revisions += _sync_revisions(page) ic.invalidate_cache() + return n_paths, n_revisions diff --git a/src/nodes/tests/test_repair_instance_page_slugs.py b/src/nodes/tests/test_repair_instance_page_slugs.py index f17345f2..19a6adb1 100644 --- a/src/nodes/tests/test_repair_instance_page_slugs.py +++ b/src/nodes/tests/test_repair_instance_page_slugs.py @@ -30,6 +30,13 @@ def _root_page(ic: InstanceConfig, slug: str) -> Page: return page +def _child(parent: Page, slug: str) -> Page: + """Add a plain child page, the way the default page creation does.""" + from pages.models import InstanceRootPage + + return parent.add_child(instance=InstanceRootPage(title=slug.title(), slug=slug)) + + def _exposed_then_looked_up(ic: InstanceConfig, page: Page) -> Page | None: """ Replay what the two resolvers do, which is where the 404 came from. @@ -121,3 +128,129 @@ def test_plan_writes_nothing_and_is_idempotent() -> None: def test_unknown_instance_is_refused() -> None: with pytest.raises(CommandError, match='No instance'): call_command('repair_instance_page_slugs', 'no-such-instance', stdout=StringIO()) + + +def test_disconnected_descendant_is_repaired_in_post_rename_terms() -> None: + """ + A disconnected child's target has to be worked out after the rename, not before. + + ``Page.save()``'s cascade rewrites only paths sharing the old prefix, so a child Wagtail + left orphaned survives the rename untouched. Computing its corrected path from the + parent's *pre-rename* ``url_path`` writes the old prefix straight back, and the page + stays unreachable behind a path that now looks plausible. + """ + ic = InstanceConfigFactory.create(identifier='longmont', name='Longmont') + root = _root_page(ic, slug='longmont-dev') + child = root.get_children().get(slug='actions') + Page.objects.filter(pk=child.pk).update(url_path='/orphaned/actions/') + + call_command('repair_instance_page_slugs', 'longmont', '--apply', stdout=StringIO()) + + child.refresh_from_db() + assert child.url_path == '/longmont/actions/' + assert _exposed_then_looked_up(ic, child) is not None + + +def test_disconnected_descendant_in_a_translated_tree_is_repaired() -> None: + """Translated trees are served by the same two resolvers, so they are scanned the same way.""" + ic = InstanceConfigFactory.create(identifier='longmont', name='Longmont') + root = _root_page(ic, slug='longmont-dev') + locale, _ = Locale.objects.get_or_create(language_code='es-US') + translation = root.copy_for_translation(locale) + translation.slug = 'longmont-dev-1' + translation.save() + es_child = _child(translation, 'actions') + Page.objects.filter(pk=es_child.pk).update(url_path='/orphaned-es/actions/') + + call_command('repair_instance_page_slugs', 'longmont', '--apply', stdout=StringIO()) + + es_child.refresh_from_db() + assert es_child.url_path == '/longmont-1/actions/' + + +def test_revisions_are_rewritten_so_a_later_save_cannot_revert_the_rename() -> None: + """ + A stale revision is one *Save draft* away from putting the old slug back. + + The Wagtail edit form populates from the latest revision, so the repair has to reach + revisions and not only the live rows. + """ + ic = InstanceConfigFactory.create(identifier='longmont', name='Longmont') + root = _root_page(ic, slug='longmont-dev') + child = root.get_children().get(slug='actions') + root.save_revision() + child.specific.save_revision() + + call_command('repair_instance_page_slugs', 'longmont', '--apply', stdout=StringIO()) + + root.refresh_from_db() + child.refresh_from_db() + assert root.latest_revision is not None + assert root.latest_revision.content['slug'] == 'longmont' + assert root.latest_revision.content['url_path'] == '/longmont/' + assert child.latest_revision is not None + assert child.latest_revision.content['url_path'] == '/longmont/actions/' + + +def test_stale_revisions_are_repaired_when_the_slugs_are_already_correct() -> None: + """ + The state a rename done by hand leaves behind: live rows right, revisions not. + + The command has to find work here, otherwise it reports 'nothing to do' over an + instance that is one admin save away from breaking again. + """ + ic = InstanceConfigFactory.create(identifier='longmont', name='Longmont') + root = _root_page(ic, slug='longmont-dev') + root.save_revision() + call_command('repair_instance_page_slugs', 'longmont', '--apply', stdout=StringIO()) + + # Re-stale the revision the way an out-of-band rename would have left it. + root.refresh_from_db() + revision = root.latest_revision + assert revision is not None + revision.content['slug'] = 'longmont-dev' + revision.content['url_path'] = '/longmont-dev/' + revision.save(update_fields=['content']) + + out = StringIO() + call_command('repair_instance_page_slugs', 'longmont', stdout=out) + assert 'would put back the old slug' in out.getvalue() + + call_command('repair_instance_page_slugs', 'longmont', '--apply', stdout=StringIO()) + revision.refresh_from_db() + assert revision.content['slug'] == 'longmont' + assert revision.content['url_path'] == '/longmont/' + + +def test_swap_leaves_no_revision_holding_a_siblings_slug() -> None: + """ + Longmont's real failure mode, one step worse than a revert. + + After the September 2026 rename, ``longmont-2021``'s Spanish root still had a revision + saying ``longmont-1`` -- by then the live slug of ``longmont``'s Spanish root. Publishing + it would have put two live siblings on one ``url_path``, and ``resolve_page`` returns + whichever the queryset happens to yield first. + """ + new = InstanceConfigFactory.create(identifier='longmont', name='Longmont') + old = InstanceConfigFactory.create(identifier='longmont-2021', name='Longmont 2021') + new_root = _root_page(new, slug='longmont-dev') + old_root = _root_page(old, slug='longmont') + locale, _ = Locale.objects.get_or_create(language_code='es-US') + for root, slug in ((new_root, 'longmont-dev-1'), (old_root, 'longmont-1')): + translation = root.copy_for_translation(locale) + translation.slug = slug + translation.save() + translation.save_revision() + + call_command('repair_instance_page_slugs', 'longmont', 'longmont-2021', '--apply', stdout=StringIO()) + + wagtail_root = Page.get_first_root_node() + assert wagtail_root is not None + tops = list(wagtail_root.get_children()) + assert len({page.slug for page in tops}) == len(tops), 'two root pages share a slug' + for page in Page.objects.filter(depth__gte=2): + for revision in page.revisions.all(): + assert revision.content['slug'] == page.slug, ( + f'page {page.pk} has a revision that would revert its slug to {revision.content["slug"]!r}' + ) + assert revision.content['url_path'] == page.url_path