From 2c55f4d0cef91007fbba4da4aa4b1cca9a781d12 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 13 Sep 2021 16:31:58 +0100 Subject: [PATCH] hard-code html error message for errorBanner turns out that we're only using errorBanner with a static message, and it's also full of rich html content. This means that it's probably better to put it in the html templates with other content, rather than hidden away in js files if we can help it. Since there are two places, had to dupe the error message but i think that's fine as i don't anticipate this error message being used in significantly more places. making it a string is a bit gross and means we don't get nice syntax highlighting on it, but as it needs to be passed in to a jinja macro that's the way it has to go unfortunately. --- app/assets/javascripts/authenticateSecurityKey.js | 2 +- app/assets/javascripts/errorBanner.js | 3 +-- app/assets/javascripts/registerSecurityKey.js | 2 +- app/templates/views/two-factor-webauthn.html | 8 +++++++- app/templates/views/user-profile/security-keys.html | 8 +++++++- tests/javascripts/authenticateSecurityKey.test.js | 13 +++++-------- tests/javascripts/errorBanner.test.js | 4 ---- tests/javascripts/registerSecurityKey.test.js | 13 +++++-------- 8 files changed, 27 insertions(+), 26 deletions(-) diff --git a/app/assets/javascripts/authenticateSecurityKey.js b/app/assets/javascripts/authenticateSecurityKey.js index 5ce70578a..3cd9178a2 100644 --- a/app/assets/javascripts/authenticateSecurityKey.js +++ b/app/assets/javascripts/authenticateSecurityKey.js @@ -71,7 +71,7 @@ console.error(error); // some browsers will show an error dialogue for some // errors; to be safe we always display an error message on the page. - window.GOVUK.ErrorBanner.showBanner('Something went wrong'); + window.GOVUK.ErrorBanner.showBanner(); }); }); }; diff --git a/app/assets/javascripts/errorBanner.js b/app/assets/javascripts/errorBanner.js index 2b3c259cc..35cdf0c83 100644 --- a/app/assets/javascripts/errorBanner.js +++ b/app/assets/javascripts/errorBanner.js @@ -10,8 +10,7 @@ */ window.GOVUK.ErrorBanner = { hideBanner: () => $('.banner-dangerous').addClass('govuk-!-display-none'), - showBanner: (errorMessage) => $('.banner-dangerous') - .html(`Error: ${errorMessage}`) + showBanner: () => $('.banner-dangerous') .removeClass('govuk-!-display-none') .trigger('focus'), }; diff --git a/app/assets/javascripts/registerSecurityKey.js b/app/assets/javascripts/registerSecurityKey.js index 39005c29c..b9c76865e 100644 --- a/app/assets/javascripts/registerSecurityKey.js +++ b/app/assets/javascripts/registerSecurityKey.js @@ -49,7 +49,7 @@ // some browsers will show an error dialogue for some // errors; to be safe we always display an error message on the page. const message = error.message || error; - window.GOVUK.ErrorBanner.showBanner('Something went wrong'); + window.GOVUK.ErrorBanner.showBanner(); }); }); }; diff --git a/app/templates/views/two-factor-webauthn.html b/app/templates/views/two-factor-webauthn.html index 9fd6a339a..fe311c3e8 100644 --- a/app/templates/views/two-factor-webauthn.html +++ b/app/templates/views/two-factor-webauthn.html @@ -23,7 +23,13 @@ {{ govukErrorMessage({ "classes": "banner-dangerous govuk-!-display-none", - "text": "There was a javascript error.", + "html": ( + 'There’s a problem with your security key' + + '

Check you have the right key and try again. ' + + 'If this does not work, ' + + 'contact us." + + '

' + ), "attributes": { "aria-live": "polite", "tabindex": '-1' diff --git a/app/templates/views/user-profile/security-keys.html b/app/templates/views/user-profile/security-keys.html index b20c7881b..3026156a4 100644 --- a/app/templates/views/user-profile/security-keys.html +++ b/app/templates/views/user-profile/security-keys.html @@ -49,7 +49,13 @@ {{ govukErrorMessage({ "classes": "banner-dangerous govuk-!-display-none", - "text": "There was a javascript error.", + "html": ( + 'There’s a problem with your security key' + + '

Check you have the right key and try again. ' + + 'If this does not work, ' + + 'contact us." + + '

' + ), "attributes": { "aria-live": "polite", "tabindex": '-1' diff --git a/tests/javascripts/authenticateSecurityKey.test.js b/tests/javascripts/authenticateSecurityKey.test.js index 8b83af2aa..ea0275851 100644 --- a/tests/javascripts/authenticateSecurityKey.test.js +++ b/tests/javascripts/authenticateSecurityKey.test.js @@ -91,8 +91,8 @@ describe('Authenticate with security key', () => { }) // this will make the test fail if the error banner is displayed - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - done(msg) + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { + done('didnt expect the banner to be shown') }) button.click() @@ -152,8 +152,7 @@ describe('Authenticate with security key', () => { } }) - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - expect(msg).toEqual('Something went wrong') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) @@ -174,8 +173,7 @@ describe('Authenticate with security key', () => { return Promise.reject(new DOMException('error')) }) - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - expect(msg).toEqual('Something went wrong') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) @@ -222,8 +220,7 @@ describe('Authenticate with security key', () => { }) - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - expect(msg).toEqual('Something went wrong') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) diff --git a/tests/javascripts/errorBanner.test.js b/tests/javascripts/errorBanner.test.js index e80b49811..c32195543 100644 --- a/tests/javascripts/errorBanner.test.js +++ b/tests/javascripts/errorBanner.test.js @@ -30,10 +30,6 @@ describe("Error Banner", () => { window.GOVUK.ErrorBanner.showBanner('Some Err'); }); - test("Will set a specific error message on the element", () => { - expect(document.querySelector('.banner-dangerous').textContent).toEqual('Error: Some Err') - }); - test("Will show the element", () => { expect(document.querySelector('.banner-dangerous').classList).not.toContain('govuk-!-display-none') }); diff --git a/tests/javascripts/registerSecurityKey.test.js b/tests/javascripts/registerSecurityKey.test.js index 03bbbd075..4bd0ac925 100644 --- a/tests/javascripts/registerSecurityKey.test.js +++ b/tests/javascripts/registerSecurityKey.test.js @@ -80,8 +80,8 @@ describe('Register security key', () => { }) // this will make the test fail if the error banner is displayed - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - done(msg) + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { + done('didnt expect the banner to be shown') }) button.click() @@ -99,8 +99,7 @@ describe('Register security key', () => { } }) - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - expect(msg).toEqual('Something went wrong') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) @@ -141,8 +140,7 @@ describe('Register security key', () => { } }) - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - expect(msg).toEqual('Something went wrong') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() }) @@ -163,8 +161,7 @@ describe('Register security key', () => { }) }) - jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation((msg) => { - expect(msg).toEqual('Something went wrong') + jest.spyOn(window.GOVUK.ErrorBanner, 'showBanner').mockImplementation(() => { done() })