From 83156bd16e4be4a9450b156540dc756cb540cb86 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 16 Jul 2020 10:49:21 +0100 Subject: [PATCH 1/6] Let users choose when to end a broadcast Different emergencies will need broadcasts to last for a variable amount of time. We give users some control over this by letting them stop a broadcast early. But we should also let them set a maximum broadcast time, for: - when the duration of the danger is known - when the broadcast has been live long enough to alert everyone who needs to know about it This code re-uses the pattern for scheduling jobs, which has some constraints that are probably OK for now: - end time is limited to an hour - longest duration is 3 whole days (eg if you start broadcasting Friday you have the choice of Saturday, Sunday and all of Monday, up to midnight) --- app/assets/javascripts/radioSelect.js | 52 +++++--- app/main/forms.py | 20 ++- app/main/views/broadcast.py | 16 ++- app/models/broadcast_message.py | 7 +- app/templates/components/radios.html | 5 +- .../views/broadcast/preview-message.html | 9 +- tests/app/main/test_choose_time_form.py | 14 +-- tests/app/main/views/test_broadcast.py | 117 +++++++++++++++--- tests/javascripts/radioSelect.test.js | 2 +- 9 files changed, 187 insertions(+), 55 deletions(-) diff --git a/app/assets/javascripts/radioSelect.js b/app/assets/javascripts/radioSelect.js index 9426c0ab4..6edfd5392 100644 --- a/app/assets/javascripts/radioSelect.js +++ b/app/assets/javascripts/radioSelect.js @@ -7,12 +7,14 @@ let states = { 'initial': Hogan.compile(` -
-
- - + {{#showNowAsDefault}} +
+
+ + +
-
+ {{/showNowAsDefault}}
{{#categories}} @@ -20,12 +22,14 @@
`), 'choose': Hogan.compile(` -
-
- - + {{#showNowAsDefault}} +
+
+ + +
-
+ {{/showNowAsDefault}}
{{#choices}}
@@ -37,12 +41,14 @@
`), 'chosen': Hogan.compile(` -
-
- - + {{#showNowAsDefault}} +
+
+ + +
-
+ {{/showNowAsDefault}}
{{#choices}}
@@ -80,10 +86,15 @@ let categories = $component.data('categories').split(','); let name = $component.find('input').eq(0).attr('name'); let mousedownOption = null; + let showNowAsDefault = ( + $component.data('show-now-as-default').toLowerCase() === 'true' ? + {'name': name} : false + ); const reset = () => { render('initial', { 'categories': categories, - 'name': name + 'name': name, + 'showNowAsDefault': showNowAsDefault }); }; const selectOption = (value) => { @@ -91,7 +102,8 @@ 'choices': choices.filter( element => element.value == value ), - 'name': name + 'name': name, + 'showNowAsDefault': showNowAsDefault }); focusSelected(component); }; @@ -153,7 +165,8 @@ 'choices': choices.filter( element => element.value == $selection.eq(0).attr('value') ), - 'name': name + 'name': name, + 'showNowAsDefault': showNowAsDefault }); } else { @@ -174,7 +187,8 @@ render('initial', { 'categories': categories, - 'name': name + 'name': name, + 'showNowAsDefault': showNowAsDefault }); $component.css({'height': 'auto'}); diff --git a/app/main/forms.py b/app/main/forms.py index 957e7c11a..985f50bc6 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -100,7 +100,7 @@ def get_next_hours_until(until): now = datetime.utcnow() hours = int((until - now).total_seconds() / (60 * 60)) return [ - (now + timedelta(hours=i)).replace(minute=0, second=0).replace(tzinfo=pytz.utc) + (now + timedelta(hours=i)).replace(minute=0, second=0, microsecond=0).replace(tzinfo=pytz.utc) for i in range(1, hours + 1) ] @@ -979,6 +979,24 @@ class ChooseTimeForm(StripWhitespaceForm): ) +class ChooseBroadcastDurationForm(StripWhitespaceForm): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.finishes_at.choices = [ + get_time_value_and_label(hour) for hour in get_next_hours_until( + get_furthest_possible_scheduled_time() + ) + ] + self.finishes_at.categories = get_next_days_until( + get_furthest_possible_scheduled_time() + ) + + finishes_at = RadioField( + 'When should this broadcast end?', + ) + + class CreateKeyForm(StripWhitespaceForm): def __init__(self, existing_keys, *args, **kwargs): self.existing_key_names = [ diff --git a/app/main/views/broadcast.py b/app/main/views/broadcast.py index 7dda048e8..be2f07ecc 100644 --- a/app/main/views/broadcast.py +++ b/app/main/views/broadcast.py @@ -1,8 +1,12 @@ -from flask import abort, jsonify, redirect, render_template, request, url_for +from flask import abort, jsonify, redirect, render_template, url_for from app import current_service from app.main import main -from app.main.forms import BroadcastAreaForm, SearchByNameForm +from app.main.forms import ( + BroadcastAreaForm, + ChooseBroadcastDurationForm, + SearchByNameForm, +) from app.models.broadcast_message import BroadcastMessage, BroadcastMessages from app.utils import service_has_permission, user_has_permissions @@ -141,15 +145,19 @@ def preview_broadcast_message(service_id, broadcast_message_id): broadcast_message_id, service_id=current_service.id, ) - if request.method == 'POST': - broadcast_message.request_approval() + form = ChooseBroadcastDurationForm() + + if form.validate_on_submit(): + broadcast_message.request_approval(until=form.finishes_at.data) return redirect(url_for( '.broadcast_dashboard', service_id=current_service.id, )) + return render_template( 'views/broadcast/preview-message.html', broadcast_message=broadcast_message, + form=form, ) diff --git a/app/models/broadcast_message.py b/app/models/broadcast_message.py index 8e7deb224..6703401c1 100644 --- a/app/models/broadcast_message.py +++ b/app/models/broadcast_message.py @@ -1,4 +1,4 @@ -from datetime import datetime, timedelta +from datetime import datetime from notifications_utils.broadcast_areas import broadcast_area_libraries from notifications_utils.template import BroadcastPreviewTemplate @@ -33,7 +33,6 @@ class BroadcastMessage(JSONModel): 'approved_by_id', 'cancelled_by_id', } - DEFAULT_TTL = timedelta(hours=72) libraries = broadcast_area_libraries @@ -132,9 +131,9 @@ class BroadcastMessage(JSONModel): data=kwargs, ) - def request_approval(self): + def request_approval(self, until): self._update( - finishes_at=(datetime.utcnow() + self.DEFAULT_TTL).isoformat(), + finishes_at=until, ) self._set_status_to('pending-approval') diff --git a/app/templates/components/radios.html b/app/templates/components/radios.html index e41e57af4..eb55217b0 100644 --- a/app/templates/components/radios.html +++ b/app/templates/components/radios.html @@ -23,7 +23,8 @@ {% macro radio_select( field, hint=None, - wrapping_class='form-group' + wrapping_class='form-group', + show_now_as_default=True ) %}
@@ -35,7 +36,7 @@ {% endif %} -
+
{% for option in field %}
diff --git a/app/templates/views/broadcast/preview-message.html b/app/templates/views/broadcast/preview-message.html index bed3cae07..8fa7a771c 100644 --- a/app/templates/views/broadcast/preview-message.html +++ b/app/templates/views/broadcast/preview-message.html @@ -1,7 +1,8 @@ {% from "components/button/macro.njk" import govukButton %} {% from "components/form.html" import form_wrapper %} {% from "components/page-header.html" import page_header %} -{% from "components/page-footer.html" import sticky_page_footer %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/radios.html" import radio_select %} {% extends "withnav_template.html" %} @@ -28,7 +29,11 @@ {{ broadcast_message.template|string }} {% call form_wrapper() %} - {{ sticky_page_footer('Submit for approval') }} + {{ radio_select( + form.finishes_at, + show_now_as_default=False + ) }} + {{ page_footer('Submit for approval') }} {% endcall %} {% endblock %} diff --git a/tests/app/main/test_choose_time_form.py b/tests/app/main/test_choose_time_form.py index a0f2618c4..41b32f90a 100644 --- a/tests/app/main/test_choose_time_form.py +++ b/tests/app/main/test_choose_time_form.py @@ -11,19 +11,19 @@ def test_form_contains_next_24h(app_): # Friday assert choices[0] == ('', 'Now') - assert choices[1] == ('2016-01-01T12:00:00.061258', 'Today at midday') - assert choices[13] == ('2016-01-02T00:00:00.061258', 'Today at midnight') + assert choices[1] == ('2016-01-01T12:00:00', 'Today at midday') + assert choices[13] == ('2016-01-02T00:00:00', 'Today at midnight') # Saturday - assert choices[14] == ('2016-01-02T01:00:00.061258', 'Tomorrow at 1am') - assert choices[37] == ('2016-01-03T00:00:00.061258', 'Tomorrow at midnight') + assert choices[14] == ('2016-01-02T01:00:00', 'Tomorrow at 1am') + assert choices[37] == ('2016-01-03T00:00:00', 'Tomorrow at midnight') # Sunday - assert choices[38] == ('2016-01-03T01:00:00.061258', 'Sunday at 1am') + assert choices[38] == ('2016-01-03T01:00:00', 'Sunday at 1am') # Monday - assert choices[84] == ('2016-01-04T23:00:00.061258', 'Monday at 11pm') - assert choices[85] == ('2016-01-05T00:00:00.061258', 'Monday at midnight') + assert choices[84] == ('2016-01-04T23:00:00', 'Monday at 11pm') + assert choices[85] == ('2016-01-05T00:00:00', 'Monday at midnight') with pytest.raises(IndexError): assert choices[ diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index edad43dd8..e9b2a761d 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -278,22 +278,79 @@ def test_remove_broadcast_area_page( ) +@pytest.mark.parametrize('end_time', ( + + # Before now + pytest.param('2020-02-02T02:00:00', marks=pytest.mark.xfail), + + # End of the current hour + pytest.param('2020-02-02T03:00:00'), + + # Midnight 3 days ahead + pytest.param('2020-02-06T00:00:00'), + + # 1am 4 days ahead + pytest.param('2020-02-06T01:00:00', marks=pytest.mark.xfail), + +)) +@freeze_time('2020-02-02 02:02:02') def test_preview_broadcast_message_page( client_request, service_one, mock_get_draft_broadcast_message, mock_get_broadcast_template, fake_uuid, + end_time, ): service_one['permissions'] += ['broadcast'] - client_request.get( + + page = client_request.get( '.preview_broadcast_message', service_id=SERVICE_ONE_ID, broadcast_message_id=fake_uuid, - ), + ) + + assert [ + normalize_spaces(area.text) + for area in page.select('.area-list-item.area-list-item--unremoveable') + ] == [ + 'England', + 'Scotland', + ] + + assert normalize_spaces( + page.select_one('.broadcast-message-wrapper').text + ) == ( + 'This is a test' + ) + + form = page.select_one('form') + assert form['method'] == 'post' + assert 'action' not in form + + radio_choices = [ + choice['value'] for choice in form.select('input[type=radio][name=finishes_at]') + ] + assert len(radio_choices) == 94 + assert end_time in radio_choices -@freeze_time('2020-02-02 02:02:02.222222') +@pytest.mark.parametrize('end_time', ( + + # Before now + pytest.param('2020-02-02T02:00:00', marks=pytest.mark.xfail), + + # End of the current hour + pytest.param('2020-02-02T03:00:00'), + + # Midnight 3 days ahead + pytest.param('2020-02-06T00:00:00'), + + # 1am 4 days ahead + pytest.param('2020-02-06T01:00:00', marks=pytest.mark.xfail), + +)) +@freeze_time('2020-02-02 02:02:02') def test_start_broadcasting( client_request, service_one, @@ -302,24 +359,54 @@ def test_start_broadcasting( mock_update_broadcast_message, mock_update_broadcast_message_status, fake_uuid, + end_time, +): + service_one['permissions'] += ['broadcast'] + client_request.post( + '.preview_broadcast_message', + service_id=SERVICE_ONE_ID, + broadcast_message_id=fake_uuid, + _data={ + 'finishes_at': end_time, + } + ), + mock_update_broadcast_message.assert_called_once_with( + service_id=SERVICE_ONE_ID, + broadcast_message_id=fake_uuid, + data={ + 'finishes_at': end_time, + }, + ) + mock_update_broadcast_message_status.assert_called_once_with( + 'pending-approval', + service_id=SERVICE_ONE_ID, + broadcast_message_id=fake_uuid, + ) + + +def test_start_broadcasting_shows_validation_error( + client_request, + service_one, + mock_get_draft_broadcast_message, + mock_get_broadcast_template, + mock_update_broadcast_message, + mock_update_broadcast_message_status, + fake_uuid, ): service_one['permissions'] += ['broadcast'] - client_request.post( + page = client_request.post( '.preview_broadcast_message', service_id=SERVICE_ONE_ID, broadcast_message_id=fake_uuid, - ), - mock_update_broadcast_message.assert_called_once_with( - service_id=SERVICE_ONE_ID, - broadcast_message_id=fake_uuid, - data={ - 'finishes_at': '2020-02-05T02:02:02.222222', - }, + _data={}, + _expected_status=200, ) - mock_update_broadcast_message_status.assert_called_once_with( - 'pending-approval', - service_id=SERVICE_ONE_ID, - broadcast_message_id=fake_uuid, + assert mock_update_broadcast_message.called is False + assert mock_update_broadcast_message_status.called is False + assert normalize_spaces( + page.select_one('form fieldset legend .error-message').text + ) == ( + 'Select an option' ) diff --git a/tests/javascripts/radioSelect.test.js b/tests/javascripts/radioSelect.test.js index 31b8dba85..c41481208 100644 --- a/tests/javascripts/radioSelect.test.js +++ b/tests/javascripts/radioSelect.test.js @@ -99,7 +99,7 @@ describe('RadioSelect', () => { When should Notify send these messages? -
+
From 636cb6825b09ae7af2665c9d4f1e2a927981cf2b Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 16 Jul 2020 16:03:13 +0100 Subject: [PATCH 2/6] Fix use of querySelector in radioSelect tests --- tests/javascripts/radioSelect.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/javascripts/radioSelect.test.js b/tests/javascripts/radioSelect.test.js index c41481208..d8227e865 100644 --- a/tests/javascripts/radioSelect.test.js +++ b/tests/javascripts/radioSelect.test.js @@ -114,7 +114,7 @@ describe('RadioSelect', () => {
`; - originalOptionsForAllCategories = Array.from(document.querySelector('.radio-select__column:nth-child(2) .multiple-choice')) + originalOptionsForAllCategories = Array.from(document.querySelectorAll('.radio-select__column:nth-child(2) .multiple-choice')) .map(option => getDataFromOption(option)); }); @@ -173,7 +173,7 @@ describe('RadioSelect', () => { test("show the options for it, with the right label and value", () => { // check options this reveals against those originally in the page for this category - const options = document.querySelector('.radio-select__column:nth-child(2) .multiple-choice'); + const options = document.querySelectorAll('.radio-select__column:nth-child(2) .multiple-choice'); const optionsThatMatchOriginals = Array.from(options).filter((option, idx) => { const optionData = getDataFromOption(option); From 909f2d4678bdbf502ee1a30afd5794a2e838d556 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 16 Jul 2020 16:06:48 +0100 Subject: [PATCH 3/6] Fix numbering on hours in radioSelect test --- tests/javascripts/radioSelect.test.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/javascripts/radioSelect.test.js b/tests/javascripts/radioSelect.test.js index d8227e865..9360486c1 100644 --- a/tests/javascripts/radioSelect.test.js +++ b/tests/javascripts/radioSelect.test.js @@ -68,11 +68,12 @@ describe('RadioSelect', () => { hours.forEach((hour, idx) => { const hourLabel = getHourLabel(hour); + const num = idx + 1; result += `
- -
`; From e45bbcf99e2505321fe4f9f8ee5f161b7e1f50f3 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 16 Jul 2020 16:28:59 +0100 Subject: [PATCH 4/6] Fix comparison of original categories with new These tests make sure the sub-categories shown when a category is selected are right but the comparison looked for a direct match between the labels for both types. This looks for the category label in the sub-category label instead, ie. 'Today at' in 'Today at 1pm'. ...instead of 'Today at' === 'Today at 1pm', which will always fail. --- tests/javascripts/radioSelect.test.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/javascripts/radioSelect.test.js b/tests/javascripts/radioSelect.test.js index 9360486c1..abd307672 100644 --- a/tests/javascripts/radioSelect.test.js +++ b/tests/javascripts/radioSelect.test.js @@ -159,10 +159,13 @@ describe('RadioSelect', () => { describe(`clicking the button for ${category} should`, () => { + const categoryRegExp = new RegExp('^' + category); + let originalOptionsForcategory; + beforeEach(() => { // get all the options in the original page for this category - originalOptionsForCategory = originalOptionsForAllCategories.filter(option => option.label === category); + originalOptionsForCategory = originalOptionsForAllCategories.filter(option => categoryRegExp.test(option.label)); // start module window.GOVUK.modules.start(); From b5360c14cc748030146a1f8ae66cd8a932fa616f Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 16 Jul 2020 19:27:33 +0100 Subject: [PATCH 5/6] Lowercase value of 'show-now-as-default' To match the boolean value in JavaScript. --- app/assets/javascripts/radioSelect.js | 5 +++-- app/templates/components/radios.html | 2 +- tests/javascripts/radioSelect.test.js | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/radioSelect.js b/app/assets/javascripts/radioSelect.js index 6edfd5392..fbd4175b9 100644 --- a/app/assets/javascripts/radioSelect.js +++ b/app/assets/javascripts/radioSelect.js @@ -87,7 +87,7 @@ let name = $component.find('input').eq(0).attr('name'); let mousedownOption = null; let showNowAsDefault = ( - $component.data('show-now-as-default').toLowerCase() === 'true' ? + $component.data('show-now-as-default').toString() === 'true' ? {'name': name} : false ); const reset = () => { @@ -131,7 +131,8 @@ 'choices': choices.filter( element => element.label.toLowerCase().indexOf(day) > -1 ), - 'name': name + 'name': name, + 'showNowAsDefault': showNowAsDefault }); focusSelected(component); diff --git a/app/templates/components/radios.html b/app/templates/components/radios.html index eb55217b0..477f0135c 100644 --- a/app/templates/components/radios.html +++ b/app/templates/components/radios.html @@ -36,7 +36,7 @@ {% endif %} -
+
{% for option in field %}
diff --git a/tests/javascripts/radioSelect.test.js b/tests/javascripts/radioSelect.test.js index abd307672..dd7811b97 100644 --- a/tests/javascripts/radioSelect.test.js +++ b/tests/javascripts/radioSelect.test.js @@ -100,7 +100,7 @@ describe('RadioSelect', () => { When should Notify send these messages? -
+
From 741ab9288f6605140d6213a48c7a83d105284029 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 16 Jul 2020 20:03:10 +0100 Subject: [PATCH 6/6] Add tests for 'data-show-now-as-default' on/off --- tests/javascripts/radioSelect.test.js | 56 ++++++++++++++++++++------- 1 file changed, 43 insertions(+), 13 deletions(-) diff --git a/tests/javascripts/radioSelect.test.js b/tests/javascripts/radioSelect.test.js index dd7811b97..4ab998df1 100644 --- a/tests/javascripts/radioSelect.test.js +++ b/tests/javascripts/radioSelect.test.js @@ -123,30 +123,60 @@ describe('RadioSelect', () => { document.body.innerHTML = ''; }); - describe("when the page has loaded it should have a button for each category", () => { + describe("when the page has loaded", () => { - let categoryButtons; + describe("if the 'data-show-now-as-default' attribute", () => { - beforeEach(() => { + test("is set to true the module should have a 'Now' option", () => { - // start module - window.GOVUK.modules.start(); + // default is for it to be set to true - categoryButtons = document.querySelectorAll('.radio-select__column:nth-child(2) .radio-select__button--category'); + // start module + window.GOVUK.modules.start(); + + expect(document.querySelectorAll('.radio-select__column').length).toEqual(2); + + }); + + test("is set to false the module should not have a 'Now' option", () => { + + document.querySelector('.radio-select').setAttribute('data-show-now-as-default', 'false'); + + // start module + window.GOVUK.modules.start(); + + expect(document.querySelectorAll('.radio-select__column').length).toEqual(1); + + }); }); - test("the number of buttons should match the categories", () => { + describe("it should have a button for each category", () => { - expect(categoryButtons.length).toBe(CATEGORIES.length); + let categoryButtons; - }); + beforeEach(() => { - test("each button's text should match their category", () => { + // start module + window.GOVUK.modules.start(); + + categoryButtons = document.querySelectorAll('.radio-select__column:nth-child(2) .radio-select__button--category'); + + }); + + test("the number of buttons should match the categories", () => { + + expect(categoryButtons.length).toBe(CATEGORIES.length); + + }); + + test("each button's text should match their category", () => { + + // check the buttons have the right text + CATEGORIES.forEach((category, idx) => { + expect(categoryButtons[idx].getAttribute('value')).toEqual(category); + }); - // check the buttons have the right text - CATEGORIES.forEach((category, idx) => { - expect(categoryButtons[idx].getAttribute('value')).toEqual(category); }); });