From da8321863d72e14be7c338df36d4b8db18e1b387 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 18 Aug 2020 16:36:40 +0100 Subject: [PATCH] =?UTF-8?q?Remove=20choice=20of=20=E2=80=98End=20time?= =?UTF-8?q?=E2=80=99=20from=20broadcast=20journey?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we added the end time picker: - we have discovered that broadcasts can’t be longer than 24h - we have observed that most users confuse picking the end time for scheduling the message, or don’t understand exactly what it means for the broadcast to ‘end’ - we’ve developed the concept of ‘training mode’, which you should be going through before sending a real broadcast We also think that, for most scenarios, you won’t necessarily know when a broadcast should end at the time of starting it because the cause of the danger is not within your control. So giving you control of the end time before the broadcast has even been approved is a confusing distraction. Having to pick a time at all also makes the whole process feel more planned and less immediate. Whereas in reality all the phones in the area will be getting the message in seconds. It’s only people coming into the area later to whom the ‘ongoing’ aspect of the broadcast applies. The best place to explain what’s happening with the phones is at the approval stage and once you’ve sent your first (training mode) broadcast. It’s easier to explain what’s happened if it’s in direct response to something you’ve just done. Later on we should add some kind of email reminder after 12 hours to make sure you still want the broadcast live, again after 18 hours, etc. We could let you schedule an end time once the broadcast is live, but don’t think there’s a strong need. Knowing enough that you want to cancel is one thing, but knowing enough to want to cancel but wanting to wait a bit… nah. --- app/main/forms.py | 18 ----- app/main/views/broadcast.py | 8 +- app/models/broadcast_message.py | 10 +-- .../views/broadcast/preview-message.html | 11 --- tests/app/main/views/test_broadcast.py | 77 +------------------ 5 files changed, 8 insertions(+), 116 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 41d870327..8fa55bb56 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1254,24 +1254,6 @@ 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( - 'End time', - ) - - 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 025939f4c..d27f14c25 100644 --- a/app/main/views/broadcast.py +++ b/app/main/views/broadcast.py @@ -13,7 +13,6 @@ from app.main import main from app.main.forms import ( BroadcastAreaForm, BroadcastAreaFormWithSelectAll, - ChooseBroadcastDurationForm, SearchByNameForm, ) from app.models.broadcast_message import BroadcastMessage, BroadcastMessages @@ -211,10 +210,8 @@ def preview_broadcast_message(service_id, broadcast_message_id): broadcast_message_id, service_id=current_service.id, ) - form = ChooseBroadcastDurationForm() - - if form.validate_on_submit(): - broadcast_message.request_approval(until=form.finishes_at.data) + if request.method == 'POST': + broadcast_message.request_approval() return redirect(url_for( '.view_broadcast_message', service_id=current_service.id, @@ -224,7 +221,6 @@ def preview_broadcast_message(service_id, broadcast_message_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 0422fc077..769e36ebb 100644 --- a/app/models/broadcast_message.py +++ b/app/models/broadcast_message.py @@ -1,4 +1,4 @@ -from datetime import datetime +from datetime import datetime, timedelta from notifications_utils.template import BroadcastPreviewTemplate from orderedset import OrderedSet @@ -153,15 +153,15 @@ class BroadcastMessage(JSONModel): data=kwargs, ) - def request_approval(self, until): - self._update( - finishes_at=until, - ) + def request_approval(self): self._set_status_to('pending-approval') def approve_broadcast(self): self._update( starts_at=datetime.utcnow().isoformat(), + finishes_at=( + datetime.utcnow() + timedelta(hours=23, minutes=59) + ).isoformat(), ) self._set_status_to('broadcasting') diff --git a/app/templates/views/broadcast/preview-message.html b/app/templates/views/broadcast/preview-message.html index 70a196078..fd8f51f65 100644 --- a/app/templates/views/broadcast/preview-message.html +++ b/app/templates/views/broadcast/preview-message.html @@ -29,17 +29,6 @@ {{ broadcast_message.template|string }} {% call form_wrapper() %} -

- Start time -

-

- Your broadcast will start when it’s approved by another member of your team. -

- {{ radio_select( - form.finishes_at, - show_now_as_default=False, - bold_legend=True - ) }} {{ page_footer('Submit for approval') }} {% endcall %} diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index c8bbeb2e1..a6a610456 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -532,29 +532,12 @@ 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'] @@ -582,47 +565,21 @@ def test_preview_broadcast_message_page( 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 - -@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, mock_get_draft_broadcast_message, mock_get_broadcast_template, - 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, - }, _expected_redirect=url_for( 'main.view_broadcast_message', service_id=SERVICE_ONE_ID, @@ -630,13 +587,6 @@ def test_start_broadcasting( _external=True, ), ), - 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, @@ -644,32 +594,6 @@ def test_start_broadcasting( ) -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'] - page = client_request.post( - '.preview_broadcast_message', - service_id=SERVICE_ONE_ID, - broadcast_message_id=fake_uuid, - _data={}, - _expected_status=200, - ) - 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' - ) - - @pytest.mark.parametrize('extra_fields, expected_paragraphs', ( ({ 'status': 'broadcasting', @@ -945,6 +869,7 @@ def test_approve_broadcast( broadcast_message_id=fake_uuid, data={ 'starts_at': '2020-02-22T22:22:22', + 'finishes_at': '2020-02-23T22:21:22', }, ) mock_update_broadcast_message_status.assert_called_once_with(