From 5558af25272b3863f51ef6ed125ab83a19baca31 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 6 Jan 2022 11:06:55 +0000 Subject: [PATCH 1/3] Refactor for reuse --- app/main/validators.py | 23 +++++++++++++++---- tests/app/main/views/test_letter_branding.py | 24 +++++++++++++------- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/app/main/validators.py b/app/main/validators.py index ec46113ac..cda57b3a5 100644 --- a/app/main/validators.py +++ b/app/main/validators.py @@ -1,4 +1,5 @@ import re +from abc import ABC, abstractmethod from notifications_utils.field import Field from notifications_utils.formatters import formatted_list @@ -77,18 +78,30 @@ class NoCommasInPlaceHolders: raise ValidationError(self.message) -class NoEmbeddedImagesInSVG: +class NoElementInSVG(ABC): - def __init__(self, message='This SVG has an embedded raster image in it and will not render well'): - self.message = message + @property + @abstractmethod + def element(self): + pass + + @property + @abstractmethod + def message(self): + pass def __call__(self, form, field): - is_image_embedded = ' + + ''', + 'This SVG has an embedded raster image in it and will not render well', + ), +)) +def test_create_letter_branding_fails_validation_when_uploading_SVG_with_bad_element( mocker, platform_admin_client, - fake_uuid + fake_uuid, + svg_contents, + expected_error, ): filename = 'test.svg' @@ -359,18 +371,14 @@ def test_create_letter_branding_fails_validation_when_uploading_SVG_with_embedde response = platform_admin_client.post( url_for('.create_letter_branding'), - data={'file': (BytesIO(""" - - - """.encode('utf-8')), filename)}, + data={'file': (BytesIO(svg_contents.encode('utf-8')), filename)}, follow_redirects=True, ) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert normalize_spaces(page.find('h1').text) == "Add letter branding" - message = 'This SVG has an embedded raster image in it and will not render well' - assert normalize_spaces(page.find("span", {"class": "error-message"}).text) == message + assert normalize_spaces(page.select_one(".error-message").text) == expected_error assert page.findAll('div', {'id': 'logo-img'}) == [] From 291906e9fd0aa6dcc59fedb8dff9e6b029f6de89 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 6 Jan 2022 11:21:12 +0000 Subject: [PATCH 2/3] =?UTF-8?q?Don=E2=80=99t=20allow=20``=20elements?= =?UTF-8?q?=20in=20letter=20logos?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To render text in an SVG consistently the system rendering the SVG must have the fonts specified by the SVG installed. If the fonts are not installed then the renderer will fall back to a system font and the text will look different. This is especially bad news for branding where the right font is an integral part of any brand. To fix this, the text should instead be converted to `` elements. This process is sometimes called ‘outlining’. A few of our logos had this problem, and I’ve fixed most of them by hand. Adding this validation will stop the problem, coming up again. --- app/main/forms.py | 4 +++- app/main/validators.py | 5 +++++ tests/app/main/views/test_letter_branding.py | 8 ++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/app/main/forms.py b/app/main/forms.py index 4b1deece3..fd0e85f50 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -53,6 +53,7 @@ from app.main.validators import ( NoCommasInPlaceHolders, NoEmbeddedImagesInSVG, NoPlaceholders, + NoTextInSVG, OnlySMSCharacters, ValidEmail, ValidGovEmail, @@ -1884,7 +1885,8 @@ class SVGFileUpload(StripWhitespaceForm): validators=[ FileAllowed(['svg'], 'SVG Images only!'), DataRequired(message="You need to upload a file to submit"), - NoEmbeddedImagesInSVG() + NoEmbeddedImagesInSVG(), + NoTextInSVG(), ] ) diff --git a/app/main/validators.py b/app/main/validators.py index cda57b3a5..949c1e941 100644 --- a/app/main/validators.py +++ b/app/main/validators.py @@ -102,6 +102,11 @@ class NoEmbeddedImagesInSVG(NoElementInSVG): message = 'This SVG has an embedded raster image in it and will not render well' +class NoTextInSVG(NoElementInSVG): + element = 'text' + message = 'This SVG has text which has not been converted to paths and may not render well' + + class OnlySMSCharacters: def __init__(self, *args, template_type, **kwargs): diff --git a/tests/app/main/views/test_letter_branding.py b/tests/app/main/views/test_letter_branding.py index 4a5bdcbe6..3c150222e 100644 --- a/tests/app/main/views/test_letter_branding.py +++ b/tests/app/main/views/test_letter_branding.py @@ -357,6 +357,14 @@ def test_create_letter_branding_when_uploading_valid_file( ''', 'This SVG has an embedded raster image in it and will not render well', ), + ( + ''' + + Will render differently depending on fonts installed + + ''', + 'This SVG has text which has not been converted to paths and may not render well', + ), )) def test_create_letter_branding_fails_validation_when_uploading_SVG_with_bad_element( mocker, From 75f8c160712ba90633a14c346425f8272561eca4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 6 Jan 2022 11:26:43 +0000 Subject: [PATCH 3/3] Convert test to use client_request fixture This is to avoid merge conflicts with https://github.com/alphagov/notifications-admin/pull/4118/files --- tests/app/main/views/test_letter_branding.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/app/main/views/test_letter_branding.py b/tests/app/main/views/test_letter_branding.py index 3c150222e..afbde14c9 100644 --- a/tests/app/main/views/test_letter_branding.py +++ b/tests/app/main/views/test_letter_branding.py @@ -368,7 +368,8 @@ def test_create_letter_branding_when_uploading_valid_file( )) def test_create_letter_branding_fails_validation_when_uploading_SVG_with_bad_element( mocker, - platform_admin_client, + client_request, + platform_admin_user, fake_uuid, svg_contents, expected_error, @@ -377,14 +378,13 @@ def test_create_letter_branding_fails_validation_when_uploading_SVG_with_bad_ele mock_s3_upload = mocker.patch('app.s3_client.s3_logo_client.utils_s3upload') - response = platform_admin_client.post( - url_for('.create_letter_branding'), - data={'file': (BytesIO(svg_contents.encode('utf-8')), filename)}, - follow_redirects=True, + client_request.login(platform_admin_user) + page = client_request.post( + '.create_letter_branding', + _data={'file': (BytesIO(svg_contents.encode('utf-8')), filename)}, + _follow_redirects=True, ) - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert normalize_spaces(page.find('h1').text) == "Add letter branding" assert normalize_spaces(page.select_one(".error-message").text) == expected_error