diff --git a/app/__init__.py b/app/__init__.py index 1bae233cd..19d889744 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -807,6 +807,7 @@ def add_template_filters(application): format_notification_status_as_url, format_number_in_pounds_as_currency, formatters.formatted_list, + formatters.normalise_lines, nl2br, format_phone_number_human_readable, format_thousands, diff --git a/app/assets/javascripts/apiKey.js b/app/assets/javascripts/apiKey.js index 91c32d399..42e8c6cd4 100644 --- a/app/assets/javascripts/apiKey.js +++ b/app/assets/javascripts/apiKey.js @@ -6,13 +6,24 @@ Modules.ApiKey = function() { const states = { - 'keyVisible': (key, thing) => ` - ${key} - + 'keyVisible': (options) => ` + + ${options.keyLabel ? '' + options.thing + ': ' : ''}${options.key} + + + ${options.onload ? '' : options.thing + ' returned to page, press button to copy to clipboard'} + + `, - 'keyCopied': thing => ` - Copied to clipboard - + 'keyCopied': (options) => ` + + ${options.thing} Copied to clipboard, press button to show in page + + ` }; @@ -30,24 +41,38 @@ this.start = function(component) { const $component = $(component), - key = $component.data('key'), - thing = $component.data('thing'); + stateOptions = { + key: $component.data('key'), + thing: $component.data('thing') + }, + name = $component.data('name'); + + // if the name is distinct from the thing: + // - it will be used in the rendering + // - the key won't be identified by a heading so needs its own label + if (name !== stateOptions.thing) { + stateOptions.name = name; + stateOptions.keyLabel = true; + } $component .addClass('api-key') .css('min-height', $component.height()) - .html(states.keyVisible(key, thing)) - .attr('aria-live', 'polite') + .html(states.keyVisible($.extend({ 'onload': true }, stateOptions))) .on( 'click', '.api-key__button--copy', () => this.copyKey( $('.api-key__key', component)[0], () => - $component.html(states.keyCopied(thing)) + $component + .html(states.keyCopied(stateOptions)) + .find('.govuk-button').focus() ) ) .on( 'click', '.api-key__button--show', () => - $component.html(states.keyVisible(key, thing)) + $component + .html(states.keyVisible(stateOptions)) + .find('.govuk-button').focus() ); if ('stickAtBottomWhenScrolling' in GOVUK) { diff --git a/app/assets/stylesheets/components/api-key.scss b/app/assets/stylesheets/components/api-key.scss index 4a723e5fd..b481c9558 100644 --- a/app/assets/stylesheets/components/api-key.scss +++ b/app/assets/stylesheets/components/api-key.scss @@ -9,6 +9,7 @@ margin-bottom: 5px; } + &__notice, &__key { font-family: monospace; display: block; diff --git a/app/templates/components/api-key.html b/app/templates/components/api-key.html index b2cd6e4aa..57df093a9 100644 --- a/app/templates/components/api-key.html +++ b/app/templates/components/api-key.html @@ -1,10 +1,13 @@ -{% macro api_key(key, name=None, thing="API key") %} - {% if name %} +{% macro api_key(key, name, thing="API key") %} + {% if name == thing %}

{{ name }}

{% endif %} -
- {{ key }} +
+ + {% if name != thing %}{{ thing }}: {% endif %}{{ key }} + +
{% endmacro %} diff --git a/app/templates/views/api/keys/show.html b/app/templates/views/api/keys/show.html index f9012afff..44765a387 100644 --- a/app/templates/views/api/keys/show.html +++ b/app/templates/views/api/keys/show.html @@ -24,7 +24,8 @@ {{ api_key( '{}-{}-{}'.format(key_name, service_id, secret), - 'API key' + name='API key', + thing='API key' ) }} {{ page_footer( diff --git a/app/templates/views/service-settings/email_reply_to.html b/app/templates/views/service-settings/email_reply_to.html index 374386dd2..60c88d6bb 100644 --- a/app/templates/views/service-settings/email_reply_to.html +++ b/app/templates/views/service-settings/email_reply_to.html @@ -34,7 +34,7 @@ Change {% endif %} {% if current_service.count_email_reply_to_addresses > 1 %} - {{ api_key(item.id, thing="ID") }} + {{ api_key(item.id, name=item.email_address, thing="ID") }} {% endif %}
{% endfor %} diff --git a/app/templates/views/service-settings/letter-contact-details.html b/app/templates/views/service-settings/letter-contact-details.html index 08f32436b..0cf921156 100644 --- a/app/templates/views/service-settings/letter-contact-details.html +++ b/app/templates/views/service-settings/letter-contact-details.html @@ -43,7 +43,8 @@ Change {% endif %} {% if letter_contact_details|length > 1 %} - {{ api_key(item.id, thing="ID") }} + {% set first_line_of_contact_block = item.contact_block|normalise_lines|first %} + {{ api_key(item.id, name=first_line_of_contact_block, thing="ID") }} {% endif %} {% endfor %} diff --git a/app/templates/views/service-settings/sms-senders.html b/app/templates/views/service-settings/sms-senders.html index 097a254de..aa7d9806f 100644 --- a/app/templates/views/service-settings/sms-senders.html +++ b/app/templates/views/service-settings/sms-senders.html @@ -36,7 +36,7 @@ Change {% endif %} {% if current_service.count_sms_senders > 1 %} - {{ api_key(item.id, thing="ID") }} + {{ api_key(item.id, name=item.sms_sender, thing="ID") }} {% endif %} {% endfor %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index bed592afa..0643abec0 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -2106,9 +2106,9 @@ def test_api_ids_dont_show_on_option_pages_with_a_single_sender( 'app.service_api_client.get_reply_to_email_addresses', create_multiple_email_reply_to_addresses(), [ - 'test@example.com (default) Change 1234', - 'test2@example.com Change 5678', - 'test3@example.com Change 9457', + 'test@example.com (default) Change ID: 1234', + 'test2@example.com Change ID: 5678', + 'test3@example.com Change ID: 9457', ], ), ( 'main.service_letter_contact_details', @@ -2116,18 +2116,18 @@ def test_api_ids_dont_show_on_option_pages_with_a_single_sender( create_multiple_letter_contact_blocks(), [ 'Blank Make default', - '1 Example Street (default) Change 1234', - '2 Example Street Change 5678', - 'foobaz Change 9457', + '1 Example Street (default) Change ID: 1234', + '2 Example Street Change ID: 5678', + 'foobaz Change ID: 9457', ], ), ( 'main.service_sms_senders', 'app.service_api_client.get_sms_senders', create_multiple_sms_senders(), [ - 'Example (default and receives replies) Change 1234', - 'Example 2 Change 5678', - 'Example 3 Change 9457', + 'Example (default and receives replies) Change ID: 1234', + 'Example 2 Change ID: 5678', + 'Example 3 Change ID: 9457', ], ), ] diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 13f859a78..2d9416639 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -785,7 +785,7 @@ def test_should_show_template_id_on_template_page( template_id=fake_uuid, _test_page_title=False, ) - assert page.select('.api-key__key')[0].text == fake_uuid + assert fake_uuid in page.select('.api-key__key')[0].text def test_should_hide_template_id_for_broadcast_templates( diff --git a/tests/javascripts/apiKey.test.js b/tests/javascripts/apiKey.test.js index 0dbf12093..c801bcd7e 100644 --- a/tests/javascripts/apiKey.test.js +++ b/tests/javascripts/apiKey.test.js @@ -11,14 +11,44 @@ afterAll(() => { describe('API key', () => { - let apiKey; + let apiKey = '00000000-0000-0000-0000-000000000000'; let thing; let component; + let selectionMock; + let rangeMock; + + const setUpDOM = function (options) { + + // set up DOM + document.body.innerHTML =` +

+ ${options.thing} +

+
+ + ${(options.name === options.thing) ? '' + options.thing + ': ' : ''}${apiKey} + + +
`; + + }; + + beforeAll(() => { + + // mock objects used to manipulate the page selection + selectionMock = new helpers.SelectionMock(jest); + rangeMock = new helpers.RangeMock(jest); + + }); beforeEach(() => { - apiKey = '00000000-0000-0000-0000-000000000000'; - thing = 'API key'; + // plug gaps in JSDOM's API for manipulation of selections + window.getSelection = jest.fn(() => selectionMock); + document.createRange = jest.fn(() => rangeMock); + + // plug JSDOM not having execCommand + document.execCommand = jest.fn(() => {}); // mock sticky JS window.GOVUK.stickAtBottomWhenScrolling = { @@ -34,21 +64,14 @@ describe('API key', () => { require('../../app/assets/javascripts/apiKey.js'); - // set up DOM - document.body.innerHTML =` -

- ${thing} -

-
- ${apiKey} -
`; + setUpDOM({ 'thing': 'API key', 'name': 'API key' }); component = document.querySelector('[data-module=api-key]'); // start the module window.GOVUK.modules.start(); - expect(component.querySelector('input[type=button]')).toBeNull(); + expect(component.querySelector('button')).toBeNull(); }); @@ -68,79 +91,132 @@ describe('API key', () => { }); - beforeEach(() => { - - // set up DOM - document.body.innerHTML =` -

- ${thing} -

-
- ${apiKey} -
`; - - component = document.querySelector('[data-module=api-key]'); - - // set default style for component height (queried by jQuery before checking DOM APIs) - const stylesheet = document.createElement('style'); - stylesheet.innerHTML = '[data-module=api-key] { height: auto; }'; // set to browser default - document.getElementsByTagName('head')[0].appendChild(stylesheet); - - componentHeightOnLoad = 50; - - // mock the DOM APIs called for the position & dimension of the component - screenMock = new helpers.ScreenMock(jest); - screenMock.setWindow({ - width: 1990, - height: 940, - scrollTop: 0 - }); - screenMock.mockPositionAndDimension('component', component, { - offsetHeight: componentHeightOnLoad, - offsetWidth: 641, - offsetTop: 0 - }); - - // start the module - window.GOVUK.modules.start(); - - }); - afterEach(() => { screenMock.reset(); }); describe("On page load", () => { - test("It should add a button for copying the key to the clipboard", () => { + describe("For all variations of the initial HTML", () => { - expect(component.querySelector('input[type=button]')).not.toBeNull(); + beforeEach(() => { + + setUpDOM({ 'thing': 'API key', 'name': 'API key' }); + + component = document.querySelector('[data-module=api-key]'); + + // set default style for component height (queried by jQuery before checking DOM APIs) + const stylesheet = document.createElement('style'); + stylesheet.innerHTML = '[data-module=api-key] { height: auto; }'; // set to browser default + document.getElementsByTagName('head')[0].appendChild(stylesheet); + + componentHeightOnLoad = 50; + + // mock the DOM APIs called for the position & dimension of the component + screenMock = new helpers.ScreenMock(jest); + screenMock.setWindow({ + width: 1990, + height: 940, + scrollTop: 0 + }); + screenMock.mockPositionAndDimension('component', component, { + offsetHeight: componentHeightOnLoad, + offsetWidth: 641, + offsetTop: 0 + }); + + // start the module + window.GOVUK.modules.start(); + + }); + + test("It should add a button for copying the key to the clipboard", () => { + + expect(component.querySelector('button')).not.toBeNull(); + + }); + + test("It should add the 'api-key' class", () => { + + expect(component.classList.contains('api-key')).toBe(true); + + }); + + test("It should tell any sticky JS present the page has changed", () => { + + // recalculate forces the sticky JS to recalculate any stored DOM position/dimensions + expect(window.GOVUK.stickAtBottomWhenScrolling.recalculate).toHaveBeenCalled(); + + }); + + test("It should set the component's minimum height based on its height when the page loads", () => { + + // to prevent the position of the button moving when the state changes + expect(window.getComputedStyle(component)['min-height']).toEqual(`${componentHeightOnLoad}px`); + + }); }); - test("It should add the 'api-key' class", () => { + describe("If it's one of many in the page", () => { - expect(component.classList.contains('api-key')).toBe(true); + beforeEach(() => { + + // If 'thing' (what the id is) and 'name' (its specific idenifier on the page) are + // different, it will be one of others called the same 'thing'. + setUpDOM({ 'thing': 'ID', 'name': 'Default' }); + + component = document.querySelector('[data-module=api-key]'); + + // start the module + window.GOVUK.modules.start(); + + }); + + // Because it is not the only 'thing' on the page, the id will not have a heading + // and so needs some prefix text to label it + test("The id should have a hidden prefix to label what it is", () => { + + const keyPrefix = component.querySelector('.api-key__key .govuk-visually-hidden'); + expect(keyPrefix).not.toBeNull(); + expect(keyPrefix.textContent).toEqual('ID: '); + + }); + + test("the button should have a hidden suffix naming the id it is for", () => { + + const buttonSuffix = component.querySelector('button .govuk-visually-hidden'); + expect(buttonSuffix).not.toBeNull(); + expect(buttonSuffix.textContent).toEqual(' for Default'); + + }); }); - test("It should change aria-live to 'polite'", () => { + describe("If it's the only one on the page", () => { - expect(component.getAttribute('aria-live')).toEqual('polite'); + beforeEach(() => { - }); + // The heading is added if 'thing' (what the id is) has the same value as 'name' + // (its specific identifier on the page) because this means it can assume it is + // the only one of its type there + setUpDOM({ 'thing': 'API key', 'name': 'API key' }); - test("It should tell any sticky JS present the page has changed", () => { + component = document.querySelector('[data-module=api-key]'); - // recalculate forces the sticky JS to recalculate any stored DOM position/dimensions - expect(window.GOVUK.stickAtBottomWhenScrolling.recalculate).toHaveBeenCalled(); + // start the module + window.GOVUK.modules.start(); - }); + }); - test("It should set the component's minimum height based on its height when the page loads", () => { + test("Its button and id shouldn't have extra hidden text to identify them", () => { - // to prevent the position of the button moving when the state changes - expect(window.getComputedStyle(component)['min-height']).toEqual(`${componentHeightOnLoad}px`); + const keyPrefix = component.querySelector('.api-key__key .govuk-visually-hidden'); + const buttonSuffix = component.querySelector('button .govuk-visually-hidden'); + expect(keyPrefix).toBeNull(); + expect(buttonSuffix).toBeNull(); + + }) }); @@ -148,78 +224,152 @@ describe('API key', () => { describe("If you click the 'Copy API key to clipboard' button", () => { - let selectionMock; - let rangeMock; - let keyEl; - let copyButton; + describe("For all variations of the initial HTML", () => { - beforeEach(() => { - - keyEl = component.querySelector('span'); - copyButton = component.querySelector('input[type=button]'); - - // mock objects used to manipulate the page selection - selectionMock = new helpers.SelectionMock(jest); - rangeMock = new helpers.RangeMock(jest); - - // plug gaps in JSDOM's API for manipulation of selections - window.getSelection = jest.fn(() => selectionMock); - document.createRange = jest.fn(() => rangeMock); - - // plug JSDOM not having execCommand - document.execCommand = jest.fn(() => {}); - - helpers.triggerEvent(copyButton, 'click'); - - }); - - test("It should change the text to confirm the copy action", () => { - - expect(component.querySelector('span').textContent.trim()).toEqual('Copied to clipboard'); - - }); - - test("It should swap the button for one to show the API key", () => { - - expect(component.querySelector('input[type=button]').getAttribute('value')).toEqual('Show API key'); - - }); - - test("It should copy the key to the clipboard", () => { - - // it should make a selection (a range) from the contents of the element containing the API key - expect(rangeMock.selectNodeContents.mock.calls[0]).toEqual([keyEl]); - - // that selection (a range) should be added to that for the page (a selection) - expect(selectionMock.addRange.mock.calls[0]).toEqual([rangeMock]); - - expect(document.execCommand).toHaveBeenCalled(); - expect(document.execCommand.mock.calls[0]).toEqual(['copy']); - - // reset any methods in the global space - window.queryCommandSupported = undefined; - window.getSelection = undefined; - document.createRange = undefined; - - }); - - describe("If you then click the 'Show API key'", () => { + let keyEl; beforeEach(() => { - helpers.triggerEvent(component.querySelector('input[type=button]'), 'click'); + setUpDOM({ 'thing': 'API key', 'name': 'API key' }); + + // start the module + window.GOVUK.modules.start(); + + component = document.querySelector('[data-module=api-key]'); + keyEl = component.querySelector('.api-key__key'); + + helpers.triggerEvent(component.querySelector('button'), 'click'); }); - test("It should change the text to show the API key", () => { + test("The live-region should be shown and its text should confirm the copy action", () => { - expect(component.querySelector('span').textContent.trim()).toEqual(apiKey); + const liveRegion = component.querySelector('.api-key__notice'); + + expect(liveRegion.classList.contains('govuk-visually-hidden')).toBe(false); + expect(liveRegion.textContent.trim()).toEqual( + expect.stringContaining('Copied to clipboard') + ); }); - test("It should swap the button for one to copy the key to the clipboard", () => { + // The button also says this but its text after being changed is not announced due to being + // lower priority than the live-region + test("The live-region should contain some hidden text giving context to the statement shown", () => { - expect(component.querySelector('input[type=button]').getAttribute('value')).toEqual('Copy API key to clipboard'); + const liveRegionHiddenText = component.querySelectorAll('.api-key__notice .govuk-visually-hidden'); + + expect(liveRegionHiddenText.length).toEqual(2); + expect(liveRegionHiddenText[0].textContent).toEqual('API key '); + expect(liveRegionHiddenText[1].textContent).toEqual(', press button to show in page'); + + }); + + test("It should swap the button for one to show the API key", () => { + + expect(component.querySelector('button').textContent.trim()).toEqual( + expect.stringContaining('Show API key') + ); + + }); + + test("It should remove the id from the page", () => { + + expect(component.querySelector('.api-key__key')).toBeNull(); + + }); + + test("It should copy the key to the clipboard", () => { + + // it should make a selection (a range) from the contents of the element containing the API key + expect(rangeMock.selectNodeContents.mock.calls[0]).toEqual([keyEl]); + + // that selection (a range) should be added to that for the page (a selection) + expect(selectionMock.addRange.mock.calls[0]).toEqual([rangeMock]); + + expect(document.execCommand).toHaveBeenCalled(); + expect(document.execCommand.mock.calls[0]).toEqual(['copy']); + + // reset any methods in the global space + window.queryCommandSupported = undefined; + window.getSelection = undefined; + document.createRange = undefined; + + }); + + describe("If you then click the 'Show API key'", () => { + + beforeEach(() => { + + helpers.triggerEvent(component.querySelector('button'), 'click'); + + }); + + test("It should change the text to show the API key", () => { + + expect(component.querySelector('.api-key__key')).not.toBeNull(); + + }); + + test("It should swap the button for one to copy the key to the clipboard", () => { + + expect(component.querySelector('button').textContent.trim()).toEqual( + expect.stringContaining('Copy API key to clipboard') + ); + + }) + + }); + + }); + + describe("If it's one of many in the page", () => { + + test("the button should have a hidden suffix naming the id it is for", () => { + + // If 'thing' (what the id is) and 'name' (its specific idenifier on the page) are + // different, it will be one of others called the same 'thing'. + setUpDOM({ 'thing': 'ID', 'name': 'Default' }); + + // start the module + window.GOVUK.modules.start(); + + component = document.querySelector('[data-module=api-key]'); + + helpers.triggerEvent(component.querySelector('button'), 'click'); + + const buttonSuffix = component.querySelector('button .govuk-visually-hidden'); + expect(buttonSuffix).not.toBeNull(); + expect(buttonSuffix.textContent).toEqual(' for Default'); + + }); + + }); + + describe("If it's the only one on the page", () => { + + beforeEach(() => { + + // The heading is added if 'thing' (what the id is) has the same value as 'name' + // (its specific identifier on the page) because this means it can assume it is + // the only one of its type there + setUpDOM({ 'thing': 'API key', 'name': 'API key' }); + + // start the module + window.GOVUK.modules.start(); + + component = document.querySelector('[data-module=api-key]'); + + helpers.triggerEvent(component.querySelector('button'), 'click'); + + }); + + test("Its button and id shouldn't have extra hidden text to identify them", () => { + + const keyPrefix = component.querySelector('.api-key__key .govuk-visually-hidden'); + const buttonSuffix = component.querySelector('button .govuk-visually-hidden'); + expect(keyPrefix).toBeNull(); + expect(buttonSuffix).toBeNull(); })