fix(editor): remove SEO panel duplication and auto-default draft metadata #54
@@ -8,9 +8,12 @@ from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator
|
|||||||
from django.db import models
|
from django.db import models
|
||||||
from django.db.models import CASCADE, PROTECT, SET_NULL, Prefetch
|
from django.db.models import CASCADE, PROTECT, SET_NULL, Prefetch
|
||||||
from django.shortcuts import get_object_or_404
|
from django.shortcuts import get_object_or_404
|
||||||
|
from django.utils.html import strip_tags
|
||||||
|
from django.utils.text import slugify
|
||||||
from modelcluster.contrib.taggit import ClusterTaggableManager
|
from modelcluster.contrib.taggit import ClusterTaggableManager
|
||||||
from modelcluster.fields import ParentalKey
|
from modelcluster.fields import ParentalKey
|
||||||
from taggit.models import Tag, TaggedItemBase
|
from taggit.models import Tag, TaggedItemBase
|
||||||
|
from wagtail.admin.forms.pages import WagtailAdminPageForm
|
||||||
from wagtail.admin.panels import FieldPanel, ObjectList, PageChooserPanel, TabbedInterface
|
from wagtail.admin.panels import FieldPanel, ObjectList, PageChooserPanel, TabbedInterface
|
||||||
from wagtail.contrib.routable_page.models import RoutablePageMixin, route
|
from wagtail.contrib.routable_page.models import RoutablePageMixin, route
|
||||||
from wagtail.fields import RichTextField, StreamField
|
from wagtail.fields import RichTextField, StreamField
|
||||||
@@ -18,9 +21,29 @@ from wagtail.models import Page
|
|||||||
from wagtail.search import index
|
from wagtail.search import index
|
||||||
from wagtailseo.models import SeoMixin
|
from wagtailseo.models import SeoMixin
|
||||||
|
|
||||||
|
from apps.authors.models import Author
|
||||||
from apps.blog.blocks import ARTICLE_BODY_BLOCKS
|
from apps.blog.blocks import ARTICLE_BODY_BLOCKS
|
||||||
|
|
||||||
|
|
||||||
|
def _generate_summary_from_stream(body: Any, *, max_chars: int = 220) -> str:
|
||||||
|
parts: list[str] = []
|
||||||
|
if body is None:
|
||||||
|
return ""
|
||||||
|
for block in body:
|
||||||
|
if getattr(block, "block_type", None) == "code":
|
||||||
|
continue
|
||||||
|
value = getattr(block, "value", block)
|
||||||
|
text = value.source if hasattr(value, "source") else str(value)
|
||||||
|
clean_text = strip_tags(text)
|
||||||
|
if clean_text:
|
||||||
|
parts.append(clean_text)
|
||||||
|
summary = re.sub(r"\s+", " ", " ".join(parts)).strip()
|
||||||
|
if len(summary) <= max_chars:
|
||||||
|
return summary
|
||||||
|
truncated = summary[:max_chars].rsplit(" ", 1)[0].strip()
|
||||||
|
return truncated or summary[:max_chars].strip()
|
||||||
|
|
||||||
|
|
||||||
class HomePage(Page):
|
class HomePage(Page):
|
||||||
featured_article = models.ForeignKey(
|
featured_article = models.ForeignKey(
|
||||||
"blog.ArticlePage", null=True, blank=True, on_delete=SET_NULL, related_name="+"
|
"blog.ArticlePage", null=True, blank=True, on_delete=SET_NULL, related_name="+"
|
||||||
@@ -181,6 +204,93 @@ class TagMetadata(models.Model):
|
|||||||
return mapping.get(self.colour, self.get_fallback_css())
|
return mapping.get(self.colour, self.get_fallback_css())
|
||||||
|
|
||||||
|
|
||||||
|
class ArticlePageAdminForm(WagtailAdminPageForm):
|
||||||
|
SUMMARY_MAX_CHARS = 220
|
||||||
|
|
||||||
|
def __init__(self, *args, **kwargs):
|
||||||
|
super().__init__(*args, **kwargs)
|
||||||
|
for name in ("slug", "author", "category", "summary"):
|
||||||
|
if name in self.fields:
|
||||||
|
self.fields[name].required = False
|
||||||
|
|
||||||
|
default_author = self._get_default_author(create=False)
|
||||||
|
if default_author and not self.initial.get("author"):
|
||||||
|
self.initial["author"] = default_author.pk
|
||||||
|
|
||||||
|
default_category = self._get_default_category(create=False)
|
||||||
|
if default_category and not self.initial.get("category"):
|
||||||
|
self.initial["category"] = default_category.pk
|
||||||
|
|
||||||
|
def clean(self):
|
||||||
|
cleaned_data = super().clean()
|
||||||
|
title = (cleaned_data.get("title") or "").strip()
|
||||||
|
|
||||||
|
if not cleaned_data.get("slug") and title:
|
||||||
|
cleaned_data["slug"] = self._build_unique_page_slug(title)
|
||||||
|
if not cleaned_data.get("author"):
|
||||||
|
cleaned_data["author"] = self._get_default_author(create=True)
|
||||||
|
if not cleaned_data.get("category"):
|
||||||
|
cleaned_data["category"] = self._get_default_category(create=True)
|
||||||
|
if not cleaned_data.get("summary"):
|
||||||
|
cleaned_data["summary"] = _generate_summary_from_stream(
|
||||||
|
cleaned_data.get("body"),
|
||||||
|
max_chars=self.SUMMARY_MAX_CHARS,
|
||||||
|
) or title
|
||||||
|
|
||||||
|
if not cleaned_data.get("slug"):
|
||||||
|
self.add_error("slug", "Slug is required.")
|
||||||
|
if not cleaned_data.get("author"):
|
||||||
|
self.add_error("author", "Author is required.")
|
||||||
|
if not cleaned_data.get("category"):
|
||||||
|
self.add_error("category", "Category is required.")
|
||||||
|
if not cleaned_data.get("summary"):
|
||||||
|
self.add_error("summary", "Summary is required.")
|
||||||
|
return cleaned_data
|
||||||
|
|
||||||
|
def _get_default_author(self, *, create: bool) -> Author | None:
|
||||||
|
user = self.for_user
|
||||||
|
if not user or not user.is_authenticated:
|
||||||
|
return None
|
||||||
|
existing = Author.objects.filter(user=user).first()
|
||||||
|
if existing or not create:
|
||||||
|
return existing
|
||||||
|
|
||||||
|
base_name = (user.get_full_name() or user.get_username() or f"user-{user.pk}").strip()
|
||||||
|
base_slug = slugify(base_name) or f"user-{user.pk}"
|
||||||
|
slug = base_slug
|
||||||
|
suffix = 2
|
||||||
|
while Author.objects.filter(slug=slug).exists():
|
||||||
|
slug = f"{base_slug}-{suffix}"
|
||||||
|
suffix += 1
|
||||||
|
return Author.objects.create(user=user, name=base_name, slug=slug)
|
||||||
|
|
||||||
|
def _get_default_category(self, *, create: bool):
|
||||||
|
existing = Category.objects.filter(slug="general").first()
|
||||||
|
if existing or not create:
|
||||||
|
return existing
|
||||||
|
category, _ = Category.objects.get_or_create(
|
||||||
|
slug="general",
|
||||||
|
defaults={"name": "General", "description": "General articles", "colour": "neutral"},
|
||||||
|
)
|
||||||
|
return category
|
||||||
|
|
||||||
|
def _build_unique_page_slug(self, title: str) -> str:
|
||||||
|
base_slug = slugify(title) or "article"
|
||||||
|
parent_page = self.parent_page
|
||||||
|
if parent_page is None and self.instance.pk:
|
||||||
|
parent_page = self.instance.get_parent()
|
||||||
|
if parent_page is None:
|
||||||
|
return base_slug
|
||||||
|
|
||||||
|
sibling_pages = parent_page.get_children().exclude(pk=self.instance.pk)
|
||||||
|
slug = base_slug
|
||||||
|
suffix = 2
|
||||||
|
while sibling_pages.filter(slug=slug).exists():
|
||||||
|
slug = f"{base_slug}-{suffix}"
|
||||||
|
suffix += 1
|
||||||
|
return slug
|
||||||
|
|
||||||
|
|
||||||
class ArticlePage(SeoMixin, Page):
|
class ArticlePage(SeoMixin, Page):
|
||||||
category = models.ForeignKey("blog.Category", on_delete=PROTECT, related_name="+")
|
category = models.ForeignKey("blog.Category", on_delete=PROTECT, related_name="+")
|
||||||
author = models.ForeignKey("authors.Author", on_delete=PROTECT)
|
author = models.ForeignKey("authors.Author", on_delete=PROTECT)
|
||||||
@@ -200,6 +310,7 @@ class ArticlePage(SeoMixin, Page):
|
|||||||
|
|
||||||
parent_page_types = ["blog.ArticleIndexPage"]
|
parent_page_types = ["blog.ArticleIndexPage"]
|
||||||
subpage_types: list[str] = []
|
subpage_types: list[str] = []
|
||||||
|
base_form_class = ArticlePageAdminForm
|
||||||
|
|
||||||
content_panels = [
|
content_panels = [
|
||||||
FieldPanel("title"),
|
FieldPanel("title"),
|
||||||
@@ -226,10 +337,7 @@ class ArticlePage(SeoMixin, Page):
|
|||||||
ObjectList(content_panels, heading="Content"),
|
ObjectList(content_panels, heading="Content"),
|
||||||
ObjectList(metadata_panels, heading="Metadata"),
|
ObjectList(metadata_panels, heading="Metadata"),
|
||||||
ObjectList(publishing_panels, heading="Publishing"),
|
ObjectList(publishing_panels, heading="Publishing"),
|
||||||
ObjectList(
|
ObjectList(SeoMixin.seo_panels, heading="SEO"),
|
||||||
Page.promote_panels + SeoMixin.seo_panels,
|
|
||||||
heading="SEO",
|
|
||||||
),
|
|
||||||
]
|
]
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -262,6 +370,8 @@ class ArticlePage(SeoMixin, Page):
|
|||||||
slug="general",
|
slug="general",
|
||||||
defaults={"name": "General", "description": "General articles", "colour": "neutral"},
|
defaults={"name": "General", "description": "General articles", "colour": "neutral"},
|
||||||
)
|
)
|
||||||
|
if not (self.summary or "").strip():
|
||||||
|
self.summary = _generate_summary_from_stream(self.body) or self.title
|
||||||
if not self.published_date and self.first_published_at:
|
if not self.published_date and self.first_published_at:
|
||||||
self.published_date = self.first_published_at
|
self.published_date = self.first_published_at
|
||||||
self.read_time_mins = self._compute_read_time()
|
self.read_time_mins = self._compute_read_time()
|
||||||
|
|||||||
@@ -1,10 +1,11 @@
|
|||||||
from datetime import timedelta
|
from datetime import timedelta
|
||||||
|
from types import SimpleNamespace
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
from django.test import override_settings
|
from django.test import override_settings
|
||||||
from django.utils import timezone
|
from django.utils import timezone
|
||||||
|
|
||||||
from apps.blog.models import ArticleIndexPage, ArticlePage
|
from apps.blog.models import ArticleIndexPage, ArticlePage, ArticlePageAdminForm, Category
|
||||||
from apps.blog.tests.factories import AuthorFactory
|
from apps.blog.tests.factories import AuthorFactory
|
||||||
|
|
||||||
|
|
||||||
@@ -273,3 +274,107 @@ def test_article_search_fields_include_summary():
|
|||||||
f.field_name for f in ArticlePage.search_fields if hasattr(f, "field_name")
|
f.field_name for f in ArticlePage.search_fields if hasattr(f, "field_name")
|
||||||
]
|
]
|
||||||
assert "summary" in field_names
|
assert "summary" in field_names
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.django_db
|
||||||
|
def test_article_admin_form_relaxes_initial_required_fields(article_index, django_user_model):
|
||||||
|
"""Slug/author/category/summary should not block initial draft validation."""
|
||||||
|
user = django_user_model.objects.create_user(
|
||||||
|
username="writer",
|
||||||
|
email="writer@example.com",
|
||||||
|
password="writer-pass",
|
||||||
|
)
|
||||||
|
form_class = ArticlePage.get_edit_handler().get_form_class()
|
||||||
|
form = form_class(parent_page=article_index, for_user=user)
|
||||||
|
|
||||||
|
assert form.fields["slug"].required is False
|
||||||
|
assert form.fields["author"].required is False
|
||||||
|
assert form.fields["category"].required is False
|
||||||
|
assert form.fields["summary"].required is False
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.django_db
|
||||||
|
def test_article_admin_form_clean_applies_defaults(article_index, django_user_model, monkeypatch):
|
||||||
|
"""Form clean should auto-fill slug/author/category/summary when omitted."""
|
||||||
|
user = django_user_model.objects.create_user(
|
||||||
|
username="writer",
|
||||||
|
email="writer@example.com",
|
||||||
|
password="writer-pass",
|
||||||
|
first_name="Writer",
|
||||||
|
last_name="User",
|
||||||
|
)
|
||||||
|
form_class = ArticlePage.get_edit_handler().get_form_class()
|
||||||
|
form = form_class(parent_page=article_index, for_user=user)
|
||||||
|
|
||||||
|
body = [
|
||||||
|
SimpleNamespace(block_type="code", value=SimpleNamespace(raw_code="print('ignore')")),
|
||||||
|
SimpleNamespace(block_type="rich_text", value=SimpleNamespace(source="<p>Hello world body text.</p>")),
|
||||||
|
]
|
||||||
|
|
||||||
|
def fake_super_clean(_self):
|
||||||
|
_self.cleaned_data = {
|
||||||
|
"title": "Auto Defaults Title",
|
||||||
|
"slug": "",
|
||||||
|
"author": None,
|
||||||
|
"category": None,
|
||||||
|
"summary": "",
|
||||||
|
"body": body,
|
||||||
|
}
|
||||||
|
return _self.cleaned_data
|
||||||
|
|
||||||
|
mro = form.__class__.__mro__
|
||||||
|
super_form_class = mro[mro.index(ArticlePageAdminForm) + 1]
|
||||||
|
monkeypatch.setattr(super_form_class, "clean", fake_super_clean)
|
||||||
|
cleaned = form.clean()
|
||||||
|
|
||||||
|
assert cleaned["slug"] == "auto-defaults-title"
|
||||||
|
assert cleaned["author"] is not None
|
||||||
|
assert cleaned["author"].user_id == user.id
|
||||||
|
assert cleaned["category"] is not None
|
||||||
|
assert cleaned["category"].slug == "general"
|
||||||
|
assert cleaned["summary"] == "Hello world body text."
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.django_db
|
||||||
|
def test_article_seo_tab_fields_not_duplicated():
|
||||||
|
"""SEO tab should include each promote/SEO field only once."""
|
||||||
|
handler = ArticlePage.get_edit_handler()
|
||||||
|
seo_tab = next(panel for panel in handler.children if panel.heading == "SEO")
|
||||||
|
|
||||||
|
def flatten_field_names(panel):
|
||||||
|
names = []
|
||||||
|
for child in panel.children:
|
||||||
|
if hasattr(child, "field_name"):
|
||||||
|
names.append(child.field_name)
|
||||||
|
else:
|
||||||
|
names.extend(flatten_field_names(child))
|
||||||
|
return names
|
||||||
|
|
||||||
|
field_names = flatten_field_names(seo_tab)
|
||||||
|
assert field_names.count("slug") == 1
|
||||||
|
assert field_names.count("seo_title") == 1
|
||||||
|
assert field_names.count("search_description") == 1
|
||||||
|
assert field_names.count("show_in_menus") == 1
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.django_db
|
||||||
|
def test_article_save_autogenerates_summary_when_missing(article_index):
|
||||||
|
"""Model save fallback should generate summary from prose blocks."""
|
||||||
|
category = Category.objects.create(name="Guides", slug="guides")
|
||||||
|
author = AuthorFactory()
|
||||||
|
article = ArticlePage(
|
||||||
|
title="Summary Auto",
|
||||||
|
slug="summary-auto",
|
||||||
|
author=author,
|
||||||
|
category=category,
|
||||||
|
summary="",
|
||||||
|
body=[
|
||||||
|
("code", {"language": "python", "filename": "", "raw_code": "print('skip')"}),
|
||||||
|
("rich_text", "<p>This should become the summary text.</p>"),
|
||||||
|
],
|
||||||
|
)
|
||||||
|
|
||||||
|
article_index.add_child(instance=article)
|
||||||
|
article.save()
|
||||||
|
|
||||||
|
assert article.summary == "This should become the summary text."
|
||||||
|
|||||||
@@ -25,6 +25,8 @@ def test_check_content_integrity_fails_for_blank_summary(home_page):
|
|||||||
)
|
)
|
||||||
index.add_child(instance=article)
|
index.add_child(instance=article)
|
||||||
article.save_revision().publish()
|
article.save_revision().publish()
|
||||||
|
# Simulate legacy/bad data by bypassing model save() auto-summary fallback.
|
||||||
|
ArticlePage.objects.filter(pk=article.pk).update(summary=" ")
|
||||||
|
|
||||||
with pytest.raises(CommandError, match="empty summary"):
|
with pytest.raises(CommandError, match="empty summary"):
|
||||||
call_command("check_content_integrity")
|
call_command("check_content_integrity")
|
||||||
|
|||||||
Reference in New Issue
Block a user