-
Notifications
You must be signed in to change notification settings - Fork 37
Re-implement content pages ToC with nesting #418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # Generated by Django 5.2.9 on 2026-01-15 16:55 | ||
|
|
||
| from django.db import migrations | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('core', '0011_alter_footercontent_locale_alter_footeritem_locale'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RemoveField( | ||
| model_name='contentpage', | ||
| name='table_of_contents', | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,33 @@ | ||
| from django.test import TestCase | ||
|
|
||
| from apps.core.factories import ContentPageFactory | ||
| from apps.core.models.content import create_table_of_contents | ||
|
|
||
|
|
||
| class TestContentPage(TestCase): | ||
| def setUp(self): | ||
| self.content_page = ContentPageFactory() | ||
|
|
||
| def test_can_create_page(self): | ||
| self.assertIsNotNone(self.content_page) | ||
| self.assertIsNotNone(self.content_page.id) | ||
|
|
||
| def test_table_of_contents_property_exists(self): | ||
| toc = self.content_page.table_of_contents | ||
| self.assertIsInstance(toc, str) | ||
|
|
||
|
|
||
| class TableOfContentsTest(TestCase): | ||
| def test_create_table_of_contents_empty(self): | ||
| toc = create_table_of_contents("") | ||
| self.assertEqual(toc, "") | ||
|
|
||
| def test_create_table_of_contents_no_id(self): | ||
| # If there's no id, generate it by slugifying the text. | ||
| self.assertEqual(self.content_page.table_of_contents, "") | ||
| self.content_page.body = '[{"type": "text", "value": "<h2>Foo bar</h2>"}]' | ||
| self.content_page.save_revision() | ||
| body_html = "<h2>Foo bar</h2>" | ||
| toc = create_table_of_contents(body_html) | ||
| self.assertEqual( | ||
| self.content_page.table_of_contents, | ||
| toc, | ||
| '<ul><li><a href="#foo-bar">Foo bar</a></li></ul>', | ||
| ) | ||
|
|
||
|
|
@@ -22,12 +36,45 @@ def test_create_table_of_contents_existing_id(self): | |
| # translating with wagtail-localize, so the id isn't updated. | ||
| # If there's an existing id, make sure to use that instead so the link | ||
| # still works. | ||
| self.assertEqual(self.content_page.table_of_contents, "") | ||
| self.content_page.body = ( | ||
| '[{"type": "text", "value": "<h2 id=\\"something\\">ekkie</h2>"}]' | ||
| ) | ||
| self.content_page.save_revision() | ||
| body_html = '<h2 id="something">ekkie</h2>' | ||
| toc = create_table_of_contents(body_html) | ||
| self.assertEqual( | ||
| self.content_page.table_of_contents, | ||
| toc, | ||
| '<ul><li><a href="#something">ekkie</a></li></ul>', | ||
| ) | ||
|
|
||
| def test_create_table_of_contents_simple(self): | ||
| body_html = """ | ||
| <h2>Introduction</h2> | ||
| <p>Some text</p> | ||
| <h3>Details</h3> | ||
| <p>More text</p> | ||
| <h2>Conclusion</h2> | ||
| """ | ||
| toc = create_table_of_contents(body_html) | ||
|
|
||
| self.assertIn('<li><a href="#introduction">Introduction</a>', toc) | ||
| self.assertIn('<ul><li><a href="#details">Details</a></li></ul>', toc) | ||
| self.assertIn('<li><a href="#conclusion">Conclusion</a>', toc) | ||
|
||
|
|
||
| def test_create_table_of_contents_nested(self): | ||
| body_html = """ | ||
| <h2>First</h2> | ||
| <h3>Nested 1</h3> | ||
| <h3>Nested 2</h3> | ||
| <h2>Second</h2> | ||
| """ | ||
| toc = create_table_of_contents(body_html) | ||
|
|
||
| expected_part = '<li><a href="#first">First</a><ul><li><a href="#nested-1">Nested 1</a></li><li><a href="#nested-2">Nested 2</a></li></ul></li>' | ||
| self.assertIn(expected_part, toc) | ||
|
|
||
| def test_create_table_of_contents_orphaned_h3(self): | ||
| body_html = """ | ||
| <h3>Orphaned</h3> | ||
| <h2>Main</h2> | ||
| """ | ||
| toc = create_table_of_contents(body_html) | ||
|
|
||
| self.assertIn('<li><a href="#orphaned">Orphaned</a></li>', toc) | ||
| self.assertIn('<li><a href="#main">Main</a>', toc) | ||
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting table_of_contents from a stored field to a property means the ToC will be computed on every access. This involves rendering the StreamField body to HTML and parsing it with BeautifulSoup, which could have performance implications if the property is accessed multiple times per request or if pages have large bodies. Consider adding caching (e.g., using @cached_property) to avoid recomputing the ToC multiple times during a single request.