Carry revisions and post-rename paths in repair_instance_page_slugs

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
 

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