From bf870cecd3b0258f12ecbbfcd69bb0619b3f6a8b Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Fri, 28 Sep 2018 12:32:36 +0100 Subject: [PATCH] Gracefully handle document links with missing ID attributes in rich text Fixes #4791 Previously, our rich text conversion functions handled the case where a document link specified an ID which is not found in the database. However, they failed with a KeyError when the id attribute was missing completely; links of this second type would occur whenever a link of the first type was re-saved from the Draftail editor. The fix is two-fold: 1) Catch the "missing ID attribute" case - in this case, the resulting link will be missing both the href and id attributes 2) Update the handling of the "ID present but document not found" case so that the id attribute survives the round-trip to the editor and back. The final link as rendered on the front-end will still be an attribute-less element, but the id will be retained in the database (and in the versions rendered within rich text editors) which may be useful for troubleshooting. --- CHANGELOG.txt | 2 +- docs/releases/2.3.rst | 2 +- wagtail/admin/tests/test_contentstate.py | 22 ++++++++++++++++++++++ wagtail/documents/rich_text.py | 17 +++++++++++++---- wagtail/documents/tests/test_rich_text.py | 19 ++++++++++++++++--- 5 files changed, 53 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 5d074d38e..95b7dcb06 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -22,7 +22,7 @@ Changelog * Fix: Disabled autocomplete dropdowns on date/time chooser fields (Janneke Janssen) * Fix: Split up `wagtail.admin.forms` to make it less prone to circular imports (Matt Westcott) * Fix: Disable linking to root page in rich text, making the page non-functional (Matt Westcott) - * Fix: Pages should be editable and save-able even if there are broken links in rich text (Matt Westcott) + * Fix: Pages should be editable and save-able even if there are broken page or document links in rich text (Matt Westcott) 2.2.2 (29.08.2018) diff --git a/docs/releases/2.3.rst b/docs/releases/2.3.rst index 8f931848e..341a69e69 100644 --- a/docs/releases/2.3.rst +++ b/docs/releases/2.3.rst @@ -45,7 +45,7 @@ Bug fixes * Disabled autocomplete dropdowns on date/time chooser fields (Janneke Janssen) * Split up ``wagtail.admin.forms`` to make it less prone to circular imports (Matt Westcott) * Disable linking to root page in rich text, making the page non-functional (Matt Westcott) - * Pages should be editable and save-able even if there are broken links in rich text (Matt Westcott) + * Pages should be editable and save-able even if there are broken page or document links in rich text (Matt Westcott) Upgrade considerations ====================== diff --git a/wagtail/admin/tests/test_contentstate.py b/wagtail/admin/tests/test_contentstate.py index dbe9f24e0..800a63452 100644 --- a/wagtail/admin/tests/test_contentstate.py +++ b/wagtail/admin/tests/test_contentstate.py @@ -390,6 +390,28 @@ class TestHtmlToContentState(TestCase):

a document link

''' )) + self.assertContentStateEqual(result, { + 'entityMap': { + '0': { + 'mutability': 'MUTABLE', 'type': 'DOCUMENT', + 'data': {'id': 9999} + } + }, + 'blocks': [ + { + 'inlineStyleRanges': [], 'text': 'a document link', 'depth': 0, 'type': 'unstyled', 'key': '00000', + 'entityRanges': [{'offset': 2, 'length': 8, 'key': 0}] + }, + ] + }) + + def test_document_link_with_missing_id(self): + converter = ContentstateConverter(features=['document-link']) + result = json.loads(converter.from_database_format( + ''' +

a document link

+ ''' + )) self.assertContentStateEqual(result, { 'entityMap': { '0': { diff --git a/wagtail/documents/rich_text.py b/wagtail/documents/rich_text.py index 06d9c7ef9..c876b4bd9 100644 --- a/wagtail/documents/rich_text.py +++ b/wagtail/documents/rich_text.py @@ -13,7 +13,7 @@ def document_linktype_handler(attrs): try: doc = Document.objects.get(id=attrs['id']) return '' % escape(doc.url) - except Document.DoesNotExist: + except (Document.DoesNotExist, KeyError): return "" @@ -31,7 +31,11 @@ class DocumentLinkHandler: doc = Document.objects.get(id=attrs['id']) return '' % (doc.id, escape(doc.url)) except Document.DoesNotExist: - return "" + # Preserve the ID attribute for troubleshooting purposes, even though it + # points to a missing document + return '' % attrs['id'] + except KeyError: + return '' EditorHTMLDocumentLinkConversionRule = [ @@ -62,10 +66,15 @@ class DocumentLinkElementHandler(LinkElementHandler): def get_attribute_data(self, attrs): Document = get_document_model() try: - doc = Document.objects.get(id=attrs['id']) - except Document.DoesNotExist: + id = int(attrs['id']) + except (KeyError, ValueError): return {} + try: + doc = Document.objects.get(id=id) + except Document.DoesNotExist: + return {'id': id} + return { 'id': doc.id, 'url': doc.url, diff --git a/wagtail/documents/tests/test_rich_text.py b/wagtail/documents/tests/test_rich_text.py index 26f6fceb9..c7e5cc94f 100644 --- a/wagtail/documents/tests/test_rich_text.py +++ b/wagtail/documents/tests/test_rich_text.py @@ -14,16 +14,29 @@ class TestDocumentRichTextLinkHandler(TestCase): self.assertEqual(result, {'id': 'test-id'}) + def test_expand_db_attributes(self): + result = document_linktype_handler({'id': 1}) + self.assertEqual(result, + '') + def test_expand_db_attributes_document_does_not_exist(self): result = document_linktype_handler({'id': 0}) self.assertEqual(result, '') + def test_expand_db_attributes_with_missing_id(self): + result = document_linktype_handler({}) + self.assertEqual(result, '') + def test_expand_db_attributes_for_editor(self): result = DocumentLinkHandler.expand_db_attributes({'id': 1}) self.assertEqual(result, '') - def test_expand_db_attributes_not_for_editor(self): - result = document_linktype_handler({'id': 1}) + def test_expand_db_attributes_for_editor_preserves_id_of_nonexistent_document(self): + result = DocumentLinkHandler.expand_db_attributes({'id': 0}) self.assertEqual(result, - '') + '') + + def test_expand_db_attributes_for_editor_with_missing_id(self): + result = DocumentLinkHandler.expand_db_attributes({}) + self.assertEqual(result, '')