From 7bc0b62a0954424435bdcf09fcdc010c50388a65 Mon Sep 17 00:00:00 2001 From: D Chukhin <6025153+dchukhin@users.noreply.github.com> Date: Wed, 11 Mar 2026 11:40:23 -0400 Subject: [PATCH 1/4] add more unit tests for the block --- tests/test_blocks.py | 294 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 289 insertions(+), 5 deletions(-) diff --git a/tests/test_blocks.py b/tests/test_blocks.py index fd36fbd..d500855 100644 --- a/tests/test_blocks.py +++ b/tests/test_blocks.py @@ -1,10 +1,294 @@ -from django.test import SimpleTestCase +from unittest.mock import Mock -from wagtail_link_block.blocks import LinkBlock +from django.test import SimpleTestCase, TestCase +from wagtail.blocks import StreamBlockValidationError +from wagtail.documents.models import Document +from wagtail.models import Page +from wagtail_link_block.blocks import LinkBlock, URLValue -class LinkBlockTestCase(SimpleTestCase): - def test_block(self): + +class URLValueGetURLTestCase(SimpleTestCase): + """Tests for URLValue.get_url() across all link types.""" + + def _make_value(self, data): + """Helper to create a URLValue with the given data.""" block = LinkBlock() + return URLValue(block, data) + + def test_page_with_object(self): + page = Mock() + page.url = "/about-us/" + value = self._make_value({"link_to": "page", "page": page}) + self.assertEqual(value.get_url(), "/about-us/") + + def test_page_with_none(self): + value = self._make_value({"link_to": "page", "page": None}) + self.assertIsNone(value.get_url()) + + def test_file_with_object(self): + doc = Mock() + doc.url = "/documents/report.pdf" + value = self._make_value({"link_to": "file", "file": doc}) + self.assertEqual(value.get_url(), "/documents/report.pdf") + + def test_file_with_none(self): + value = self._make_value({"link_to": "file", "file": None}) + self.assertIsNone(value.get_url()) + + def test_custom_url(self): + value = self._make_value({"link_to": "custom_url", "custom_url": "https://example.com"}) + self.assertEqual(value.get_url(), "https://example.com") + + def test_custom_url_path(self): + value = self._make_value({"link_to": "custom_url", "custom_url": "/some/path"}) + self.assertEqual(value.get_url(), "/some/path") + + def test_custom_url_empty(self): + value = self._make_value({"link_to": "custom_url", "custom_url": ""}) + self.assertEqual(value.get_url(), "") + + def test_anchor(self): + value = self._make_value({"link_to": "anchor", "anchor": "section-1"}) + self.assertEqual(value.get_url(), "#section-1") + + def test_email(self): + value = self._make_value({"link_to": "email", "email": "hello@example.com"}) + self.assertEqual(value.get_url(), "mailto:hello@example.com") + + def test_phone(self): + value = self._make_value({"link_to": "phone", "phone": "+44123456789"}) + self.assertEqual(value.get_url(), "tel:+44123456789") + + def test_no_link_to(self): + value = self._make_value({"link_to": None}) + self.assertIsNone(value.get_url()) + + def test_empty_link_to(self): + value = self._make_value({"link_to": ""}) + self.assertIsNone(value.get_url()) + + def test_missing_link_to_key(self): + value = self._make_value({}) + self.assertIsNone(value.get_url()) + + +class URLValueGetLinkToTestCase(SimpleTestCase): + """Tests for URLValue.get_link_to().""" - self.assertIsNotNone(block.render({})) + def _make_value(self, data): + block = LinkBlock() + return URLValue(block, data) + + def test_returns_link_to_value(self): + value = self._make_value({"link_to": "page"}) + self.assertEqual(value.get_link_to(), "page") + + def test_returns_none_when_missing(self): + value = self._make_value({}) + self.assertIsNone(value.get_link_to()) + + +class LinkBlockCleanTestCase(TestCase): + """Tests for LinkBlock.clean() validation and field clearing.""" + + def _make_block_value(self, link_to="", **kwargs): + """Helper to build the raw value dict that clean() expects.""" + defaults = { + "link_to": link_to, + "page": None, + "file": None, + "custom_url": "", + "anchor": "", + "email": "", + "phone": "", + "new_window": False, + } + defaults.update(kwargs) + return defaults + + def test_clean_custom_url_valid(self): + block = LinkBlock() + value = self._make_block_value(link_to="custom_url", custom_url="https://example.com") + result = block.clean(value) + self.assertEqual(result["custom_url"], "https://example.com") + self.assertEqual(result["link_to"], "custom_url") + # Other link fields should be cleared + self.assertIsNone(result["page"]) + self.assertIsNone(result["file"]) + self.assertEqual(result["anchor"], "") + self.assertEqual(result["email"], "") + self.assertEqual(result["phone"], "") + + def test_clean_custom_url_missing_raises(self): + block = LinkBlock() + + value_empty_string = self._make_block_value(link_to="custom_url", custom_url="") + with self.assertRaises(StreamBlockValidationError) as ctx: + block.clean(value_empty_string) + self.assertIn("custom_url", ctx.exception.block_errors) + + value_none = self._make_block_value(link_to="custom_url", custom_url=None) + with self.assertRaises(StreamBlockValidationError) as ctx: + block.clean(value_none) + self.assertIn("custom_url", ctx.exception.block_errors) + + def test_clean_email_valid(self): + block = LinkBlock() + value = self._make_block_value(link_to="email", email="hello@example.com") + result = block.clean(value) + self.assertEqual(result["email"], "hello@example.com") + # Other link fields should be cleared + self.assertIsNone(result["page"]) + self.assertIsNone(result["file"]) + self.assertEqual(result["custom_url"], "") + self.assertEqual(result["anchor"], "") + self.assertEqual(result["phone"], "") + + def test_clean_email_missing_raises(self): + block = LinkBlock() + value = self._make_block_value(link_to="email", email="") + with self.assertRaises(StreamBlockValidationError) as ctx: + block.clean(value) + self.assertIn("email", ctx.exception.block_errors) + + def test_clean_anchor_valid(self): + block = LinkBlock() + value = self._make_block_value(link_to="anchor", anchor="my-section") + result = block.clean(value) + self.assertEqual(result["anchor"], "my-section") + self.assertIsNone(result["page"]) + self.assertIsNone(result["file"]) + self.assertEqual(result["custom_url"], "") + self.assertEqual(result["email"], "") + self.assertEqual(result["phone"], "") + + def test_clean_anchor_missing_raises(self): + block = LinkBlock() + value = self._make_block_value(link_to="anchor", anchor="") + with self.assertRaises(StreamBlockValidationError) as ctx: + block.clean(value) + self.assertIn("anchor", ctx.exception.block_errors) + + def test_clean_phone_valid(self): + block = LinkBlock() + value = self._make_block_value(link_to="phone", phone="+44123456789") + result = block.clean(value) + self.assertEqual(result["phone"], "+44123456789") + self.assertIsNone(result["page"]) + self.assertIsNone(result["file"]) + self.assertEqual(result["custom_url"], "") + self.assertEqual(result["anchor"], "") + self.assertEqual(result["email"], "") + + def test_clean_phone_missing_raises(self): + block = LinkBlock() + value = self._make_block_value(link_to="phone", phone="") + with self.assertRaises(StreamBlockValidationError) as ctx: + block.clean(value) + self.assertIn("phone", ctx.exception.block_errors) + + def test_clean_page_valid(self): + block = LinkBlock() + page = Page.objects.first() + value = self._make_block_value(link_to="page", page=page.pk) + result = block.clean(value) + self.assertEqual(result["page"], page) + self.assertIsNone(result["file"]) + self.assertEqual(result["custom_url"], "") + self.assertEqual(result["anchor"], "") + self.assertEqual(result["email"], "") + self.assertEqual(result["phone"], "") + + def test_clean_page_missing_raises(self): + block = LinkBlock() + value = self._make_block_value(link_to="page", page=None) + with self.assertRaises(StreamBlockValidationError) as ctx: + block.clean(value) + self.assertIn("page", ctx.exception.block_errors) + + def test_clean_file_valid(self): + block = LinkBlock() + doc = Document.objects.create(title="Test Document") + value = self._make_block_value(link_to="file", file=doc.pk) + result = block.clean(value) + self.assertEqual(result["file"], doc) + self.assertIsNone(result["page"]) + self.assertEqual(result["custom_url"], "") + self.assertEqual(result["anchor"], "") + self.assertEqual(result["email"], "") + self.assertEqual(result["phone"], "") + + def test_clean_file_missing_raises(self): + block = LinkBlock() + value = self._make_block_value(link_to="file", file=None) + with self.assertRaises(StreamBlockValidationError) as ctx: + block.clean(value) + self.assertIn("file", ctx.exception.block_errors) + + def test_clean_empty_link_to_passes(self): + block = LinkBlock() + value = self._make_block_value(link_to="") + result = block.clean(value) + # All link fields should be reset to defaults + self.assertIsNone(result["page"]) + self.assertIsNone(result["file"]) + self.assertEqual(result["custom_url"], "") + self.assertEqual(result["anchor"], "") + self.assertEqual(result["email"], "") + self.assertEqual(result["phone"], "") + + def test_clean_clears_unselected_fields(self): + """When selecting one link type, other fields with stale data get cleared.""" + block = LinkBlock() + value = self._make_block_value( + link_to="anchor", + anchor="top", + custom_url="https://stale.com", + email="stale@example.com", + phone="+000", + ) + result = block.clean(value) + self.assertEqual(result["anchor"], "top") + self.assertEqual(result["custom_url"], "") + self.assertEqual(result["email"], "") + self.assertEqual(result["phone"], "") + self.assertIsNone(result["page"]) + self.assertIsNone(result["file"]) + + def test_clean_error_message_format(self): + block = LinkBlock() + value = self._make_block_value(link_to="custom_url", custom_url="") + with self.assertRaises(StreamBlockValidationError) as ctx: + block.clean(value) + error_list = ctx.exception.block_errors["custom_url"] + self.assertIn("You need to add a custom url link", [str(e) for e in error_list]) + + +class LinkBlockInitTestCase(SimpleTestCase): + """Tests for LinkBlock.__init__() required propagation.""" + + def test_default_not_required(self): + block = LinkBlock() + self.assertFalse(block.child_blocks["link_to"].field.required) + + def test_required_true_propagates(self): + block = LinkBlock(required=True) + self.assertTrue(block.child_blocks["link_to"].field.required) + + def test_required_does_not_leak_between_instances(self): + block_required = LinkBlock(required=True) + block_optional = LinkBlock(required=False) + self.assertTrue(block_required.child_blocks["link_to"].field.required) + self.assertFalse(block_optional.child_blocks["link_to"].field.required) + + +class LinkBlockSetNameTestCase(SimpleTestCase): + """Tests for LinkBlock.set_name() label override.""" + + def test_set_name_sets_name_without_label(self): + block = LinkBlock() + block.set_name("my_link") + self.assertEqual(block.name, "my_link") + # Label should remain None (not auto-set from name) + self.assertIsNone(block.meta.label) From ea5b36102948418fc78d885ebe4d97cef5da95f7 Mon Sep 17 00:00:00 2001 From: D Chukhin <6025153+dchukhin@users.noreply.github.com> Date: Wed, 11 Mar 2026 12:56:58 -0400 Subject: [PATCH 2/4] make block methods more extendible by using helper methods --- tests/test_blocks.py | 39 +++++++++++++ wagtail_link_block/blocks.py | 103 +++++++++++++++++++++++++---------- 2 files changed, 112 insertions(+), 30 deletions(-) diff --git a/tests/test_blocks.py b/tests/test_blocks.py index d500855..9e86224 100644 --- a/tests/test_blocks.py +++ b/tests/test_blocks.py @@ -1,3 +1,4 @@ +from unittest import mock from unittest.mock import Mock from django.test import SimpleTestCase, TestCase @@ -72,6 +73,19 @@ def test_missing_link_to_key(self): value = self._make_value({}) self.assertIsNone(value.get_url()) + def test_get_url_dispatches_to_helper(self): + """get_url() calls the per-type helper method.""" + value = self._make_value({"link_to": "page", "page": Mock(url="/test/")}) + with mock.patch.object(URLValue, "get_page_url", return_value="/mocked/") as m: + result = value.get_url() + m.assert_called_once() + self.assertEqual(result, "/mocked/") + + def test_get_url_unknown_type_returns_none(self): + """get_url() returns None for an unrecognized link_to value.""" + value = self._make_value({"link_to": "unknown_type"}) + self.assertIsNone(value.get_url()) + class URLValueGetLinkToTestCase(SimpleTestCase): """Tests for URLValue.get_link_to().""" @@ -264,6 +278,31 @@ def test_clean_error_message_format(self): error_list = ctx.exception.block_errors["custom_url"] self.assertIn("You need to add a custom url link", [str(e) for e in error_list]) + def test_get_url_field_default_values_contains_all_types(self): + block = LinkBlock() + defaults = block.get_url_field_default_values() + self.assertEqual( + set(defaults.keys()), + {"page", "file", "custom_url", "anchor", "email", "phone"}, + ) + + def test_clean_link_type_returns_empty_dict_for_valid_value(self): + block = LinkBlock() + result = block.clean_link_type("custom_url", {"custom_url": "https://example.com"}) + self.assertEqual(result, {}) + + def test_clean_link_type_returns_error_for_empty_value(self): + block = LinkBlock() + result = block.clean_link_type("custom_url", {"custom_url": ""}) + self.assertIn("custom_url", result) + + def test_clean_link_type_dispatches_to_per_type_method(self): + """clean_link_type() calls clean_() when defined.""" + block = LinkBlock() + with mock.patch.object(LinkBlock, "clean_page", create=True, return_value={}) as m: + block.clean_link_type("page", {"page": Mock()}) + m.assert_called_once() + class LinkBlockInitTestCase(SimpleTestCase): """Tests for LinkBlock.__init__() required propagation.""" diff --git a/wagtail_link_block/blocks.py b/wagtail_link_block/blocks.py index 8cbf817..e3b1ec8 100644 --- a/wagtail_link_block/blocks.py +++ b/wagtail_link_block/blocks.py @@ -31,21 +31,34 @@ class URLValue(StructValue): def get_url(self): link_to = self.get("link_to") - - if link_to in ("page", "file"): - # If file or page check obj is not None - if self.get(link_to): - return self.get(link_to).url - elif link_to == "custom_url": - return self.get(link_to) - elif link_to == "anchor": - return "#" + self.get(link_to) - elif link_to == "email": - return f"mailto:{self.get(link_to)}" - elif link_to == "phone": - return f"tel:{self.get(link_to)}" + method = getattr(self, f"get_{link_to}_url", None) + if method: + return method() return None + def get_page_url(self): + page = self.get("page") + return page.url if page else None + + def get_file_url(self): + file = self.get("file") + return file.url if file else None + + def get_custom_url_url(self): + return self.get("custom_url") + + def get_anchor_url(self): + anchor = self.get("anchor") + return "#" + anchor if anchor else None + + def get_email_url(self): + email = self.get("email") + return f"mailto:{email}" if email else None + + def get_phone_url(self): + phone = self.get("phone") + return f"tel:{phone}" if phone else None + def get_link_to(self): """ Return link type for accessing in templates @@ -116,11 +129,16 @@ def set_name(self, name): """ self.name = name - def clean(self, value): - clean_values = super().clean(value) - errors = {} + def get_url_field_default_values(self): + """Return a dict mapping each URL-type field name to its empty/default value. - url_default_values = { + Subclasses should override this to add new link types, e.g.: + def get_url_field_default_values(self): + defaults = super().get_url_field_default_values() + defaults["relative_url"] = "" + return defaults + """ + return { "page": None, "file": None, "custom_url": "", @@ -128,21 +146,46 @@ def clean(self, value): "email": "", "phone": "", } + + def clean_link_type(self, url_type, clean_values): + """Validate the selected link type's value. Called by clean(). + + Dispatches to ``clean_(clean_values)`` if such a method + exists on the class. Otherwise performs the default check that the + selected type's value is not empty. + + Subclasses can add per-type validation by defining a method, e.g.:: + + def clean_relative_url(self, clean_values): + # custom validation ... + return {} # or {field_name: ErrorList([...])} + """ + method = getattr(self, f"clean_{url_type}", None) + if method: + return method(clean_values) + # Default: check that the selected type has a non-empty value + if clean_values.get(url_type) in [None, ""]: + return { + url_type: ErrorList( + ["You need to add a {} link".format(url_type.replace("_", " "))] + ) + } + return {} + + def clean(self, value): + clean_values = super().clean(value) + errors = {} + url_default_values = self.get_url_field_default_values() url_type = clean_values.get("link_to") - # Check that a value has been uploaded for the chosen link type - if url_type != "" and clean_values.get(url_type) in [None, ""]: - errors[url_type] = ErrorList( - ["You need to add a {} link".format(url_type.replace("_", " "))] - ) - else: - try: - # Remove values added for link types not selected - url_default_values.pop(url_type, None) - for field in url_default_values: - clean_values[field] = url_default_values[field] - except KeyError: - errors[url_type] = ErrorList(["Enter a valid link type"]) + if url_type != "": + errors.update(self.clean_link_type(url_type, clean_values)) + + if not errors: + # Remove values added for link types not selected + url_default_values.pop(url_type, None) + for field in url_default_values: + clean_values[field] = url_default_values[field] if errors: raise StreamBlockValidationError(block_errors=errors, non_block_errors=ErrorList([])) From 18a1740857b6b11880351dcb282144bdcf55e522 Mon Sep 17 00:00:00 2001 From: D Chukhin <6025153+dchukhin@users.noreply.github.com> Date: Wed, 11 Mar 2026 12:57:50 -0400 Subject: [PATCH 3/4] make template code more extendible by using more generic logic, and not hard-coding field names --- .../static/link_block/link_block.js | 59 +++++++------------ .../wagtailadmin/block_forms/link_block.html | 6 +- 2 files changed, 24 insertions(+), 41 deletions(-) diff --git a/wagtail_link_block/static/link_block/link_block.js b/wagtail_link_block/static/link_block/link_block.js index d4a4f9c..3bef99f 100644 --- a/wagtail_link_block/static/link_block/link_block.js +++ b/wagtail_link_block/static/link_block/link_block.js @@ -2,47 +2,30 @@ (function() { 'use strict'; - // For Links, only show the selected field types. So if 'Page Link' is selected, - // only show the Page Chooser. If URL field, only show the URL field. Etc. + // Fields that should show the "new window" toggle when selected. + const NEW_WINDOW_TYPES = ['page', 'custom_url', 'anchor']; function setRelatedFieldsVisibility(link_type_selector) { const value = link_type_selector.value; - const parent = link_type_selector.closest('.link_block'), - page_link = parent.querySelector('.page_link_field'), - file_link = parent.querySelector('.file_link_field'), - custom_url_link = parent.querySelector('.custom_url_link_field'), - anchor_link = parent.querySelector('.anchor_link_field'), - new_window_toggle = parent.querySelector('.new_window_link_field'), - email_address = parent.querySelector('.email_link_field'), - phone_link = parent.querySelector('.phone_link_field'); - - // first hide all - page_link.classList.add('link-block__hidden'); - file_link.classList.add('link-block__hidden'); - custom_url_link.classList.add('link-block__hidden'); - anchor_link.classList.add('link-block__hidden'); - new_window_toggle.classList.add('link-block__hidden'); - email_address.classList.add('link-block__hidden'); - phone_link.classList.add('link-block__hidden'); - - // display the one - if (value === 'page') { - page_link.classList.remove('link-block__hidden'); - new_window_toggle.classList.remove('link-block__hidden'); - } else if (value === 'file') { - file_link.classList.remove('link-block__hidden'); - } else if (value === 'custom_url') { - custom_url_link.classList.remove('link-block__hidden'); - new_window_toggle.classList.remove('link-block__hidden'); - } else if (value === 'anchor') { - anchor_link.classList.remove('link-block__hidden'); - new_window_toggle.classList.remove('link-block__hidden'); - } else if (value === 'email') { - email_address.classList.remove('link-block__hidden'); - } else if (value === 'phone') { - phone_link.classList.remove('link-block__hidden'); - } else { - // I don't know what to display here. + const parent = link_type_selector.closest('.link_block'); + + // Hide all _link_field elements + parent.querySelectorAll('[class*="_link_field"]').forEach(function(el) { + el.classList.add('link-block__hidden'); + }); + + if (value) { + // Show the field matching the selected type + const field = parent.querySelector('.' + value + '_link_field'); + if (field) { + field.classList.remove('link-block__hidden'); + } + + // Show new_window toggle for applicable types + const newWindowField = parent.querySelector('.new_window_link_field'); + if (newWindowField && NEW_WINDOW_TYPES.indexOf(value) !== -1) { + newWindowField.classList.remove('link-block__hidden'); + } } } diff --git a/wagtail_link_block/templates/wagtailadmin/block_forms/link_block.html b/wagtail_link_block/templates/wagtailadmin/block_forms/link_block.html index 4d6bd01..e4d179b 100644 --- a/wagtail_link_block/templates/wagtailadmin/block_forms/link_block.html +++ b/wagtail_link_block/templates/wagtailadmin/block_forms/link_block.html @@ -1,5 +1,5 @@ {% load i18n %} -{# extended to add `link_block_field` and default `hidden` classes to link detail fields. #} +{# Adds `_link_field` suffix class and default `hidden` class to all fields except link_to. #}
{% if help_text %} @@ -13,13 +13,13 @@ {% for child in children.values %}