From 470b8a2912c4c42318a7bc199d094825acc2e92b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 4 Apr 2019 11:09:09 +0100 Subject: [PATCH] Remove domains from branding forms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’re deprecating storing the domain as text on a branding in favour of a database relationship between branding and organisation. We need to do this now in order to remove the validation on these fields (which depends on the data in `domains.yml`) --- app/main/forms.py | 19 +-- app/main/validators.py | 33 +---- app/main/views/email_branding.py | 3 - app/main/views/letter_branding.py | 13 +- app/notify_client/email_branding_client.py | 12 +- app/notify_client/letter_branding_client.py | 7 +- .../views/email-branding/manage-branding.html | 1 - .../views/email-branding/select-branding.html | 7 -- .../manage-letter-branding.html | 1 - .../select-letter-branding.html | 7 -- tests/app/main/views/test_email_branding.py | 119 ++---------------- tests/app/main/views/test_letter_branding.py | 95 ++++---------- .../test_email_branding_client.py | 8 +- .../test_letter_branding_client.py | 8 +- tests/conftest.py | 13 +- 15 files changed, 48 insertions(+), 298 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 5878b129c..184f65d5f 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -36,10 +36,8 @@ from wtforms.widgets import CheckboxInput, ListWidget from app.main.validators import ( Blacklist, - CanonicalGovernmentDomain, CsvFileValidator, DoesNotStartWithDoubleZero, - KnownGovernmentDomain, LettersNumbersAndFullStopsOnly, NoCommasInPlaceHolders, OnlyGSMCharacters, @@ -47,7 +45,7 @@ from app.main.validators import ( ValidGovEmail, ) from app.models.user import permissions, roles -from app.utils import AgreementInfo, guess_name_from_email_address +from app.utils import guess_name_from_email_address def get_time_value_and_label(future_time): @@ -990,23 +988,9 @@ class PreviewBranding(StripWhitespaceForm): branding_style = HiddenFieldWithNoneOption('branding_style') -class GovernmentDomainField(StringField): - validators = [ - KnownGovernmentDomain(), - CanonicalGovernmentDomain(), - ] - - def post_validate(self, form, validation_stopped): - if self.data == '': - self.data = None - if self.data and not self.errors: - self.data = AgreementInfo(self.data).canonical_domain - - class ServiceUpdateEmailBranding(StripWhitespaceForm): name = StringField('Name of brand') text = StringField('Text') - domain = GovernmentDomainField('Domain') colour = StringField( 'Colour', validators=[ @@ -1041,7 +1025,6 @@ class SVGFileUpload(StripWhitespaceForm): class ServiceLetterBrandingDetails(StripWhitespaceForm): name = StringField('Name of brand', validators=[DataRequired()]) - domain = GovernmentDomainField('Domain') class PDFUploadForm(StripWhitespaceForm): diff --git a/app/main/validators.py b/app/main/validators.py index 9b27e09e9..216038726 100644 --- a/app/main/validators.py +++ b/app/main/validators.py @@ -11,7 +11,7 @@ from wtforms.validators import Email from app import formatted_list from app.main._blacklisted_passwords import blacklisted_passwords -from app.utils import AgreementInfo, Spreadsheet, is_gov_user +from app.utils import Spreadsheet, is_gov_user class Blacklist: @@ -111,34 +111,3 @@ class DoesNotStartWithDoubleZero: def __call__(self, form, field): if field.data and field.data.startswith("00"): raise ValidationError(self.message) - - -class KnownGovernmentDomain: - - message = 'Not a known government domain (you might need to update domains.yml)' - - def __call__(self, form, field): - if field.data and AgreementInfo(field.data).owner is None: - raise ValidationError(self.message) - - -class CanonicalGovernmentDomain: - - message = 'Not {} domain (use {} if appropriate)' - - def __call__(self, form, field): - - if not field.data: - return - - domain = AgreementInfo(field.data) - - if not domain.is_canonical: - raise ValidationError( - self.message.format('a canonical', domain.canonical_domain) - ) - - if field.data != domain.canonical_domain: - raise ValidationError( - self.message.format('an organisation-level', domain.canonical_domain) - ) diff --git a/app/main/views/email_branding.py b/app/main/views/email_branding.py index 7fbb9aec8..521b7b1e8 100644 --- a/app/main/views/email_branding.py +++ b/app/main/views/email_branding.py @@ -39,7 +39,6 @@ def update_email_branding(branding_id, logo=None): name=email_branding['name'], text=email_branding['text'], colour=email_branding['colour'], - domain=email_branding['domain'], brand_type=email_branding['brand_type'] ) @@ -67,7 +66,6 @@ def update_email_branding(branding_id, logo=None): name=form.name.data, text=form.text.data, colour=form.colour.data, - domain=form.domain.data, brand_type=form.brand_type.data, ) @@ -115,7 +113,6 @@ def create_email_branding(logo=None): name=form.name.data, text=form.text.data, colour=form.colour.data, - domain=form.domain.data, brand_type=form.brand_type.data, ) diff --git a/app/main/views/letter_branding.py b/app/main/views/letter_branding.py index c2a126878..634dfa777 100644 --- a/app/main/views/letter_branding.py +++ b/app/main/views/letter_branding.py @@ -56,7 +56,6 @@ def update_letter_branding(branding_id, logo=None): file_upload_form = SVGFileUpload() letter_branding_details_form = ServiceLetterBrandingDetails( name=letter_branding['name'], - domain=letter_branding['domain'] ) file_upload_form_submitted = file_upload_form.file.data @@ -87,7 +86,6 @@ def update_letter_branding(branding_id, logo=None): branding_id=branding_id, filename=db_filename, name=letter_branding_details_form.name.data, - domain=letter_branding_details_form.domain.data ) return redirect(url_for('main.letter_branding')) @@ -98,7 +96,6 @@ def update_letter_branding(branding_id, logo=None): branding_id=branding_id, filename=db_filename, name=letter_branding_details_form.name.data, - domain=letter_branding_details_form.domain.data ) upload_letter_logos(logo, db_filename, png_file, session['user_id']) @@ -106,9 +103,7 @@ def update_letter_branding(branding_id, logo=None): return redirect(url_for('main.letter_branding')) except HTTPError as e: - if 'domain' in e.message: - letter_branding_details_form.domain.errors.append(e.message['domain'][0]) - elif 'name' in e.message: + if 'name' in e.message: letter_branding_details_form.name.errors.append(e.message['name'][0]) else: raise e @@ -118,7 +113,6 @@ def update_letter_branding(branding_id, logo=None): branding_id=branding_id, filename=letter_branding['filename'], name=letter_branding['name'], - domain=letter_branding['domain'] ) file_upload_form.file.errors = ['Error saving uploaded file - try uploading again'] @@ -165,7 +159,6 @@ def create_letter_branding(logo=None): letter_branding_client.create_letter_branding( filename=db_filename, name=letter_branding_details_form.name.data, - domain=letter_branding_details_form.domain.data, ) upload_letter_logos(logo, db_filename, png_file, session['user_id']) @@ -173,9 +166,7 @@ def create_letter_branding(logo=None): return redirect(url_for('main.letter_branding')) except HTTPError as e: - if 'domain' in e.message: - letter_branding_details_form.domain.errors.append(e.message['domain'][0]) - elif 'name' in e.message: + if 'name' in e.message: letter_branding_details_form.name.errors.append(e.message['name'][0]) else: raise e diff --git a/app/notify_client/email_branding_client.py b/app/notify_client/email_branding_client.py index 776c0d1e6..469e6f7c5 100644 --- a/app/notify_client/email_branding_client.py +++ b/app/notify_client/email_branding_client.py @@ -14,33 +14,25 @@ class EmailBrandingClient(NotifyAdminAPIClient): brandings.sort(key=lambda branding: branding[sort_key].lower()) return brandings - def get_email_branding_id_for_domain(self, domain): - for branding in self.get_all_email_branding(): - if domain and branding.get('domain') == domain: - return branding['id'] - return None - @cache.delete('email_branding') - def create_email_branding(self, logo, name, text, colour, domain, brand_type): + def create_email_branding(self, logo, name, text, colour, brand_type): data = { "logo": logo, "name": name, "text": text, "colour": colour, - "domain": domain, "brand_type": brand_type } return self.post(url="/email-branding", data=data) @cache.delete('email_branding') @cache.delete('email_branding-{branding_id}') - def update_email_branding(self, branding_id, logo, name, text, colour, domain, brand_type): + def update_email_branding(self, branding_id, logo, name, text, colour, brand_type): data = { "logo": logo, "name": name, "text": text, "colour": colour, - "domain": domain, "brand_type": brand_type } return self.post(url="/email-branding/{}".format(branding_id), data=data) diff --git a/app/notify_client/letter_branding_client.py b/app/notify_client/letter_branding_client.py index 0a577d55e..736d9d025 100644 --- a/app/notify_client/letter_branding_client.py +++ b/app/notify_client/letter_branding_client.py @@ -12,22 +12,19 @@ class LetterBrandingClient(NotifyAdminAPIClient): return self.get(url='/letter-branding') @cache.delete('letter_branding') - def create_letter_branding(self, filename, name, domain): + def create_letter_branding(self, filename, name): data = { "filename": filename, "name": name, - "domain": domain, } return self.post(url="/letter-branding", data=data) @cache.delete('letter_branding') @cache.delete('letter_branding-{branding_id}') - def update_letter_branding(self, branding_id, filename, name, domain): + def update_letter_branding(self, branding_id, filename, name): data = { "filename": filename, "name": name, - "domain": domain, - } return self.post(url="/letter-branding/{}".format(branding_id), data=data) diff --git a/app/templates/views/email-branding/manage-branding.html b/app/templates/views/email-branding/manage-branding.html index dad8e212d..42d695786 100644 --- a/app/templates/views/email-branding/manage-branding.html +++ b/app/templates/views/email-branding/manage-branding.html @@ -28,7 +28,6 @@
{{textbox(form.name)}}
{{textbox(form.text)}}
{{ textbox(form.colour, width='1-4', colour_preview=True) }} -
{{textbox(form.domain)}}
{{ radios(form.brand_type) }} {{ page_footer( 'Save', diff --git a/app/templates/views/email-branding/select-branding.html b/app/templates/views/email-branding/select-branding.html index 2d9f703f7..27c8086ef 100644 --- a/app/templates/views/email-branding/select-branding.html +++ b/app/templates/views/email-branding/select-branding.html @@ -24,13 +24,6 @@ {{ brand.name or 'Unnamed' }} -

- {% if brand.domain %} - Default for {{ brand.domain }} - {% else %} - – - {% endif %} -

{% endfor %} diff --git a/app/templates/views/letter-branding/manage-letter-branding.html b/app/templates/views/letter-branding/manage-letter-branding.html index 1dd6c7b8a..e815d1057 100644 --- a/app/templates/views/letter-branding/manage-letter-branding.html +++ b/app/templates/views/letter-branding/manage-letter-branding.html @@ -25,7 +25,6 @@ {% call form_wrapper() %}
{{textbox(letter_branding_details_form.name)}}
-
{{textbox(letter_branding_details_form.domain)}}
{{ page_footer( 'Save', button_name='operation', diff --git a/app/templates/views/letter-branding/select-letter-branding.html b/app/templates/views/letter-branding/select-letter-branding.html index b915c3b73..3b50744f5 100644 --- a/app/templates/views/letter-branding/select-letter-branding.html +++ b/app/templates/views/letter-branding/select-letter-branding.html @@ -24,13 +24,6 @@ {{ brand.name }}
-

- {% if brand.domain %} - Default for {{ brand.domain }} - {% else %} - – - {% endif %} -

{% endfor %} diff --git a/tests/app/main/views/test_email_branding.py b/tests/app/main/views/test_email_branding.py index be702a3bd..e49f8ecef 100644 --- a/tests/app/main/views/test_email_branding.py +++ b/tests/app/main/views/test_email_branding.py @@ -28,7 +28,6 @@ def test_email_branding_page_shows_full_branding_list( links = page.select('.message-name a') brand_names = [normalize_spaces(link.text) for link in links] hrefs = [link['href'] for link in links] - brand_hints = [normalize_spaces(hint.text) for hint in page.select('.message-type')] assert normalize_spaces( page.select_one('h1').text @@ -36,14 +35,12 @@ def test_email_branding_page_shows_full_branding_list( assert page.select_one('.column-three-quarters a')['href'] == url_for('main.create_email_branding') - assert list(zip( - brand_names, brand_hints - )) == [ - ('org 1', '–'), - ('org 2', '–'), - ('org 3', '–'), - ('org 4', 'Default for nhs.uk'), - ('org 5', 'Default for voa.gov.uk'), + assert brand_names == [ + 'org 1', + 'org 2', + 'org 3', + 'org 4', + 'org 5', ] assert hrefs == [ url_for('.update_email_branding', branding_id=1), @@ -70,7 +67,6 @@ def test_edit_email_branding_shows_the_correct_branding_info( assert page.select_one('#name').attrs.get('value') == 'Organisation name' assert page.select_one('#text').attrs.get('value') == 'Organisation text' assert page.select_one('#colour').attrs.get('value') == '#f00' - assert page.select_one('#domain').attrs.get('value') == 'sample.com' def test_create_email_branding_does_not_show_any_branding_info( @@ -89,27 +85,19 @@ def test_create_email_branding_does_not_show_any_branding_info( assert page.select_one('#name').attrs.get('value') == '' assert page.select_one('#text').attrs.get('value') == '' assert page.select_one('#colour').attrs.get('value') == '' - assert page.select_one('#domain').attrs.get('value') == '' -@pytest.mark.parametrize('posted_domain, persisted_domain', [ - ('voa.gov.uk', 'voa.gov.uk'), - ('', None), -]) def test_create_new_email_branding_without_logo( logged_in_platform_admin_client, mocker, fake_uuid, mock_create_email_branding, - posted_domain, - persisted_domain, ): data = { 'logo': None, 'colour': '#ff0000', 'text': 'new text', 'name': 'new name', - 'domain': posted_domain, 'brand_type': 'org' } @@ -128,96 +116,11 @@ def test_create_new_email_branding_without_logo( name=data['name'], text=data['text'], colour=data['colour'], - domain=persisted_domain, brand_type=data['brand_type'] ) assert mock_persist.call_args_list == [] -def test_cant_create_new_email_branding_with_unknown_domain( - client_request, - mocker, - fake_uuid, - mock_create_email_branding -): - mock_persist = mocker.patch('app.main.views.email_branding.persist_logo') - mocker.patch('app.main.views.email_branding.delete_email_temp_files_created_by') - - client_request.login(platform_admin_user(fake_uuid)) - page = client_request.post( - '.create_email_branding', - content_type='multipart/form-data', - _data={ - 'logo': None, - 'colour': '#ff0000', - 'text': 'new text', - 'name': 'new name', - 'domain': 'example.gov.uk', - 'brand_type': 'org', - }, - _expected_status=200, - ) - - assert mock_create_email_branding.called is False - assert mock_persist.called is False - - assert page.select_one('.error-message').text.strip() == ( - 'Not a known government domain (you might need to update domains.yml)' - ) - assert page.select_one('input[name=domain]')['value'] == ( - 'example.gov.uk' - ) - - -@pytest.mark.parametrize('posted_domain, expected_error', [ - ( - 'voa.gsi.gov.uk', - 'Not a canonical domain (use voa.gov.uk if appropriate)', - ), - ( - 'hmcts.net', - 'Not a canonical domain (use hmcts.gov.uk if appropriate)', - ), - ( - 'southend.essex.gov.uk', - 'Not an organisation-level domain (use essex.gov.uk if appropriate)', - ), - pytest.param( - 'voa.gov.uk', - '', - marks=pytest.mark.xfail(raises=AssertionError) - ), -]) -def test_rejects_non_canonical_domain_when_adding_email_branding( - client_request, - mocker, - fake_uuid, - mock_create_email_branding, - posted_domain, - expected_error, -): - mocker.patch('app.main.views.email_branding.persist_logo') - mocker.patch('app.main.views.email_branding.delete_email_temp_files_created_by') - data = { - 'logo': None, - 'colour': '#ff0000', - 'text': 'new text', - 'name': 'new name', - 'domain': posted_domain, - 'brand_type': 'org', - } - client_request.login(platform_admin_user(fake_uuid)) - page = client_request.post( - '.create_email_branding', - content_type='multipart/form-data', - _data=data, - _expected_status=200, - ) - - assert page.select_one('.error-message').text.strip() == expected_error - assert mock_create_email_branding.called is False - - def test_create_email_branding_requires_a_name_when_submitting_logo_details( client_request, mocker, @@ -232,7 +135,6 @@ def test_create_email_branding_requires_a_name_when_submitting_logo_details( 'colour': '#ff0000', 'text': 'new text', 'name': '', - 'domain': '', 'brand_type': 'org', } client_request.login(platform_admin_user(fake_uuid)) @@ -258,7 +160,6 @@ def test_create_email_branding_does_not_require_a_name_when_uploading_a_file( 'colour': '', 'text': '', 'name': '', - 'domain': '', 'brand_type': 'org', } client_request.login(platform_admin_user(fake_uuid)) @@ -286,7 +187,6 @@ def test_create_new_email_branding_when_branding_saved( 'colour': '#ff0000', 'text': 'new text', 'name': 'new name', - 'domain': 'voa.gov.uk', 'brand_type': 'org_banner' } @@ -307,7 +207,6 @@ def test_create_new_email_branding_when_branding_saved( 'name': data['name'], 'text': data['text'], 'cdn_url': 'https://static-logos.cdn.com', - 'domain': data['domain'], 'brand_type': data['brand_type'] } ) @@ -320,7 +219,6 @@ def test_create_new_email_branding_when_branding_saved( name=data['name'], text=data['text'], colour=data['colour'], - domain=data['domain'], brand_type=data['brand_type'] ) @@ -387,7 +285,6 @@ def test_update_existing_branding( 'colour': '#0000ff', 'text': 'new text', 'name': 'new name', - 'domain': 'voa.gov.uk', 'brand_type': 'both' } @@ -405,7 +302,7 @@ def test_update_existing_branding( content_type='multipart/form-data', data={'colour': data['colour'], 'name': data['name'], 'text': data['text'], 'cdn_url': 'https://static-logos.cdn.com', - 'domain': data['domain'], 'brand_type': data['brand_type'] + 'brand_type': data['brand_type'] } ) @@ -418,7 +315,6 @@ def test_update_existing_branding( name=data['name'], text=data['text'], colour=data['colour'], - domain=data['domain'], brand_type=data['brand_type'] ) @@ -526,7 +422,6 @@ def test_colour_regex_validation( 'colour': colour_hex, 'text': 'new text', 'name': 'new name', - 'domain': 'voa.gov.uk', 'brand_type': 'org' } diff --git a/tests/app/main/views/test_letter_branding.py b/tests/app/main/views/test_letter_branding.py index 2520d16b0..89ef8bc43 100644 --- a/tests/app/main/views/test_letter_branding.py +++ b/tests/app/main/views/test_letter_branding.py @@ -2,7 +2,6 @@ from io import BytesIO from unittest.mock import Mock, call from uuid import UUID -import pytest from botocore.exceptions import ClientError as BotoClientError from bs4 import BeautifulSoup from flask import current_app, url_for @@ -29,7 +28,6 @@ def test_letter_branding_page_shows_full_branding_list( links = page.select('.message-name a') brand_names = [normalize_spaces(link.text) for link in links] hrefs = [link['href'] for link in links] - brand_hints = [normalize_spaces(hint.text) for hint in page.select('.message-type')] assert normalize_spaces( page.select_one('h1').text @@ -37,12 +35,10 @@ def test_letter_branding_page_shows_full_branding_list( assert page.select_one('.column-three-quarters a')['href'] == url_for('main.create_letter_branding') - assert list(zip( - brand_names, brand_hints - )) == [ - ('HM Government', '–'), - ('Land Registry', 'Default for landregistry.gov.uk'), - ('Animal and Plant Health Agency', '–'), + assert brand_names == [ + 'HM Government', + 'Land Registry', + 'Animal and Plant Health Agency', ] assert hrefs == [ @@ -66,7 +62,6 @@ def test_update_letter_branding_shows_the_current_letter_brand( assert page.find('h1').text == 'Update letter branding' assert page.select_one('#logo-img > img')['src'].endswith('/hm-government.svg') assert page.select_one('#name').attrs.get('value') == 'HM Government' - assert page.select_one('#domain').attrs.get('value') == 'cabinet-office.gov.uk' def test_update_letter_branding_with_new_valid_file( @@ -95,7 +90,6 @@ def test_update_letter_branding_with_new_valid_file( assert page.select_one('#logo-img > img')['src'].endswith(expected_temp_filename) assert page.select_one('#name').attrs.get('value') == 'HM Government' - assert page.select_one('#domain').attrs.get('value') == 'cabinet-office.gov.uk' assert mock_s3_upload.called mock_delete_temp_files.assert_not_called() @@ -161,7 +155,6 @@ def test_update_letter_branding_with_original_file_and_new_details( url_for('.update_letter_branding', branding_id=fake_uuid), data={ 'name': 'Updated name', - 'domain': 'bl.uk', 'operation': 'branding-details' }, follow_redirects=True @@ -175,42 +168,12 @@ def test_update_letter_branding_with_original_file_and_new_details( mock_client_update.assert_called_once_with( branding_id=fake_uuid, - domain='bl.uk', filename='hm-government', name='Updated name' ) -def test_update_letter_branding_does_not_require_a_domain( - mocker, - logged_in_platform_admin_client, - mock_get_all_letter_branding, - mock_get_letter_branding_by_id, - fake_uuid -): - mock_client_update = mocker.patch('app.main.views.letter_branding.letter_branding_client.update_letter_branding') - logo = permanent_letter_logo_name('hm-government', 'svg') - - response = logged_in_platform_admin_client.post( - url_for('.update_letter_branding', branding_id=fake_uuid, logo=logo), - data={ - 'name': 'Updated name', - 'domain': '', - 'operation': 'branding-details' - }, - follow_redirects=True - ) - assert response.status_code == 200 - - mock_client_update.assert_called_once_with( - branding_id=fake_uuid, - domain=None, - filename='hm-government', - name='Updated name' - ) - - -def test_update_letter_branding_shows_form_errors_on_name_and_domain_fields( +def test_update_letter_branding_shows_form_errors_on_name_fields( mocker, logged_in_platform_admin_client, mock_get_letter_branding_by_id, @@ -224,7 +187,6 @@ def test_update_letter_branding_shows_form_errors_on_name_and_domain_fields( url_for('.update_letter_branding', branding_id=fake_uuid, logo=logo), data={ 'name': '', - 'domain': 'example.com', 'operation': 'branding-details' }, follow_redirects=True @@ -234,18 +196,15 @@ def test_update_letter_branding_shows_form_errors_on_name_and_domain_fields( error_messages = page.find_all('span', class_='error-message') assert page.find('h1').text == 'Update letter branding' - assert len(error_messages) == 2 + assert len(error_messages) == 1 assert error_messages[0].text.strip() == 'This field is required.' - assert error_messages[1].text.strip() == 'Not a known government domain (you might need to update domains.yml)' -@pytest.mark.parametrize('error_field', ['name', 'domain']) -def test_update_letter_branding_shows_database_errors_on_name_and_domain_fields( +def test_update_letter_branding_shows_database_errors_on_name_field( mocker, logged_in_platform_admin_client, mock_get_letter_branding_by_id, fake_uuid, - error_field ): mocker.patch('app.main.views.letter_branding.get_png_file_from_svg') mocker.patch('app.main.views.letter_branding.letter_branding_client.update_letter_branding', side_effect=HTTPError( @@ -254,20 +213,19 @@ def test_update_letter_branding_shows_database_errors_on_name_and_domain_fields( json={ 'result': 'error', 'message': { - error_field: { - '{} already in use'.format(error_field) + 'name': { + 'name already in use' } } } ), - message={error_field: ['{} already in use'.format(error_field)]} + message={'name': ['name already in use']} )) response = logged_in_platform_admin_client.post( url_for('.update_letter_branding', branding_id='abc'), data={ 'name': 'my brand', - 'domain': None, 'operation': 'branding-details' } ) @@ -276,7 +234,7 @@ def test_update_letter_branding_shows_database_errors_on_name_and_domain_fields( error_message = page.find('span', class_='error-message').text.strip() assert page.find('h1').text == 'Update letter branding' - assert error_message == '{} already in use'.format(error_field) + assert error_message == 'name already in use' def test_update_letter_branding_with_new_file_and_new_details( @@ -300,7 +258,6 @@ def test_update_letter_branding_with_new_file_and_new_details( url_for('.update_letter_branding', branding_id=fake_uuid, logo=temp_logo), data={ 'name': 'Updated name', - 'domain': 'bl.uk', 'operation': 'branding-details' }, follow_redirects=True @@ -312,7 +269,6 @@ def test_update_letter_branding_with_new_file_and_new_details( assert mock_template_preview.called mock_client_update.assert_called_once_with( branding_id=fake_uuid, - domain='bl.uk', filename='{}-new_file'.format(fake_uuid), name='Updated name' ) @@ -343,7 +299,6 @@ def test_update_letter_branding_rolls_back_db_changes_and_shows_error_if_saving_ url_for('.update_letter_branding', branding_id=fake_uuid, logo=temp_logo), data={ 'name': 'Updated name', - 'domain': 'bl.uk', 'operation': 'branding-details' }, follow_redirects=True @@ -355,8 +310,8 @@ def test_update_letter_branding_rolls_back_db_changes_and_shows_error_if_saving_ assert mock_client_update.call_count == 2 assert mock_client_update.call_args_list == [ - call(branding_id=fake_uuid, domain='bl.uk', filename='{}-new_file'.format(fake_uuid), name='Updated name'), - call(branding_id=fake_uuid, domain='cabinet-office.gov.uk', filename='hm-government', name='HM Government') + call(branding_id=fake_uuid, filename='{}-new_file'.format(fake_uuid), name='Updated name'), + call(branding_id=fake_uuid, filename='hm-government', name='HM Government') ] @@ -370,7 +325,6 @@ def test_create_letter_branding_does_not_show_branding_info(logged_in_platform_a assert page.select_one('#logo-img > img') is None assert page.select_one('#name').attrs.get('value') == '' - assert page.select_one('#domain').attrs.get('value') == '' def test_create_letter_branding_when_uploading_valid_file( @@ -472,7 +426,6 @@ def test_create_letter_branding_shows_an_error_when_submitting_details_with_no_l url_for('.create_letter_branding'), data={ 'name': 'Test brand', - 'domain': 'bl.uk', 'operation': 'branding-details' } ) @@ -506,7 +459,6 @@ def test_create_letter_branding_persists_logo_when_all_data_is_valid( url_for('.create_letter_branding', logo=temp_logo), data={ 'name': 'Test brand', - 'domain': 'bl.uk', 'operation': 'branding-details' }, follow_redirects=True @@ -517,7 +469,7 @@ def test_create_letter_branding_persists_logo_when_all_data_is_valid( assert page.find('h1').text == 'Letter branding' mock_letter_client.create_letter_branding.assert_called_once_with( - domain='bl.uk', filename='{}-test'.format(fake_uuid), name='Test brand' + filename='{}-test'.format(fake_uuid), name='Test brand' ) assert mock_template_preview.called mock_persist_logo.assert_called_once_with( @@ -532,7 +484,7 @@ def test_create_letter_branding_persists_logo_when_all_data_is_valid( mock_delete_temp_files.assert_called_once_with(user_id) -def test_create_letter_branding_shows_form_errors_on_name_and_domain_fields( +def test_create_letter_branding_shows_form_errors_on_name_field( logged_in_platform_admin_client, fake_uuid ): @@ -545,7 +497,6 @@ def test_create_letter_branding_shows_form_errors_on_name_and_domain_fields( url_for('.create_letter_branding', logo=temp_logo), data={ 'name': '', - 'domain': 'example.com', 'operation': 'branding-details' } ) @@ -554,17 +505,14 @@ def test_create_letter_branding_shows_form_errors_on_name_and_domain_fields( error_messages = page.find_all('span', class_='error-message') assert page.find('h1').text == 'Add letter branding' - assert len(error_messages) == 2 + assert len(error_messages) == 1 assert error_messages[0].text.strip() == 'This field is required.' - assert error_messages[1].text.strip() == 'Not a known government domain (you might need to update domains.yml)' -@pytest.mark.parametrize('error_field', ['name', 'domain']) -def test_create_letter_branding_shows_database_errors_on_name_and_domain_fields( +def test_create_letter_branding_shows_database_errors_on_name_fields( mocker, logged_in_platform_admin_client, fake_uuid, - error_field ): with logged_in_platform_admin_client.session_transaction() as session: user_id = session["user_id"] @@ -576,13 +524,13 @@ def test_create_letter_branding_shows_database_errors_on_name_and_domain_fields( json={ 'result': 'error', 'message': { - error_field: { - '{} already in use'.format(error_field) + 'name': { + 'name already in use' } } } ), - message={error_field: ['{} already in use'.format(error_field)]} + message={'name': ['name already in use']} )) temp_logo = LETTER_TEMP_LOGO_LOCATION.format(user_id=user_id, unique_id=fake_uuid, filename='test.svg') @@ -591,7 +539,6 @@ def test_create_letter_branding_shows_database_errors_on_name_and_domain_fields( url_for('.create_letter_branding', logo=temp_logo), data={ 'name': 'my brand', - 'domain': None, 'operation': 'branding-details' } ) @@ -600,7 +547,7 @@ def test_create_letter_branding_shows_database_errors_on_name_and_domain_fields( error_message = page.find('span', class_='error-message').text.strip() assert page.find('h1').text == 'Add letter branding' - assert error_message == '{} already in use'.format(error_field) + assert error_message == 'name already in use' def test_get_png_file_from_svg(client, mocker, fake_uuid): diff --git a/tests/app/notify_client/test_email_branding_client.py b/tests/app/notify_client/test_email_branding_client.py index ef1b48587..3ea1aa293 100644 --- a/tests/app/notify_client/test_email_branding_client.py +++ b/tests/app/notify_client/test_email_branding_client.py @@ -53,13 +53,13 @@ def test_get_all_email_branding(mocker): def test_create_email_branding(mocker): org_data = {'logo': 'test.png', 'name': 'test name', 'text': 'test name', 'colour': 'red', - 'domain': 'sample.com', 'brand_type': 'org'} + 'brand_type': 'org'} mock_post = mocker.patch('app.notify_client.email_branding_client.EmailBrandingClient.post') mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') EmailBrandingClient().create_email_branding( logo=org_data['logo'], name=org_data['name'], text=org_data['text'], colour=org_data['colour'], - domain=org_data['domain'], brand_type='org' + brand_type='org' ) mock_post.assert_called_once_with( @@ -72,13 +72,13 @@ def test_create_email_branding(mocker): def test_update_email_branding(mocker, fake_uuid): org_data = {'logo': 'test.png', 'name': 'test name', 'text': 'test name', 'colour': 'red', - 'domain': 'sample.com', 'brand_type': 'org'} + 'brand_type': 'org'} mock_post = mocker.patch('app.notify_client.email_branding_client.EmailBrandingClient.post') mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') EmailBrandingClient().update_email_branding( branding_id=fake_uuid, logo=org_data['logo'], name=org_data['name'], text=org_data['text'], - colour=org_data['colour'], domain=org_data['domain'], brand_type='org') + colour=org_data['colour'], brand_type='org') mock_post.assert_called_once_with( url='/email-branding/{}'.format(fake_uuid), diff --git a/tests/app/notify_client/test_letter_branding_client.py b/tests/app/notify_client/test_letter_branding_client.py index a6e9db49d..3b2d74a32 100644 --- a/tests/app/notify_client/test_letter_branding_client.py +++ b/tests/app/notify_client/test_letter_branding_client.py @@ -39,13 +39,13 @@ def test_get_all_letter_branding(mocker): def test_create_letter_branding(mocker): - new_branding = {'filename': 'uuid-test', 'name': 'my letters', 'domain': 'example.com'} + new_branding = {'filename': 'uuid-test', 'name': 'my letters'} mock_post = mocker.patch('app.notify_client.letter_branding_client.LetterBrandingClient.post') mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') LetterBrandingClient().create_letter_branding( - filename=new_branding['filename'], name=new_branding['name'], domain=new_branding['domain'] + filename=new_branding['filename'], name=new_branding['name'], ) mock_post.assert_called_once_with( url='/letter-branding', @@ -56,12 +56,12 @@ def test_create_letter_branding(mocker): def test_update_letter_branding(mocker, fake_uuid): - branding = {'filename': 'uuid-test', 'name': 'my letters', 'domain': 'example.com'} + branding = {'filename': 'uuid-test', 'name': 'my letters'} mock_post = mocker.patch('app.notify_client.letter_branding_client.LetterBrandingClient.post') mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') LetterBrandingClient().update_letter_branding( - branding_id=fake_uuid, filename=branding['filename'], name=branding['name'], domain=branding['domain']) + branding_id=fake_uuid, filename=branding['filename'], name=branding['name']) mock_post.assert_called_once_with( url='/letter-branding/{}'.format(fake_uuid), diff --git a/tests/conftest.py b/tests/conftest.py index 21274f0ab..4abb3aaa5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2539,8 +2539,8 @@ def mock_get_all_email_branding(mocker): non_standard_values = [ {'idx': 1, 'colour': 'red'}, {'idx': 2, 'colour': 'orange'}, - {'idx': 3, 'text': None, 'domain': 'nhs.uk'}, - {'idx': 4, 'colour': 'blue', 'domain': 'voa.gov.uk'}, + {'idx': 3, 'text': None}, + {'idx': 4, 'colour': 'blue'}, ] shuffle = sort_key is None return create_email_brandings(5, non_standard_values=non_standard_values, shuffle=shuffle) @@ -2559,19 +2559,16 @@ def mock_get_all_letter_branding(mocker): 'id': str(UUID(int=0)), 'name': 'HM Government', 'filename': 'hm-government', - 'domain': None, }, { 'id': str(UUID(int=1)), 'name': 'Land Registry', 'filename': 'land-registry', - 'domain': 'landregistry.gov.uk', }, { 'id': str(UUID(int=2)), 'name': 'Animal and Plant Health Agency', 'filename': 'animal', - 'domain': None, } ] @@ -2587,7 +2584,6 @@ def mock_get_letter_branding_by_id(mocker): 'id': _id, 'name': 'HM Government', 'filename': 'hm-government', - 'domain': 'cabinet-office.gov.uk', } return mocker.patch( 'app.letter_branding_client.get_letter_branding', side_effect=_get_branding_by_id @@ -2611,7 +2607,6 @@ def create_email_branding(id, non_standard_values={}): 'text': 'Organisation text', 'id': id, 'colour': '#f00', - 'domain': 'sample.com', 'brand_type': 'org', } @@ -2674,7 +2669,7 @@ def mock_get_email_branding_without_brand_text(mocker, fake_uuid): @pytest.fixture(scope='function') def mock_create_email_branding(mocker): - def _create_email_branding(logo, name, text, colour, domain, brand_type): + def _create_email_branding(logo, name, text, colour, brand_type): return return mocker.patch( @@ -2684,7 +2679,7 @@ def mock_create_email_branding(mocker): @pytest.fixture(scope='function') def mock_update_email_branding(mocker): - def _update_email_branding(branding_id, logo, name, text, colour, domain, brand_type): + def _update_email_branding(branding_id, logo, name, text, colour, brand_type): return return mocker.patch(