From b31c9fbc0d055971632af0f2945ed9cb165a0717 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 2 Mar 2016 17:36:20 +0000 Subject: [PATCH 1/2] Make job page poll for updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a first go at having the job page update without refreshing. The approach I’ve taken is to do all the rendering of HTML on the server side, rather than use a Javascipt templating engine like mustache. This ensures that we don’t have to maintain two sets of templates. So the approach is to split the job page into partials. These partials can then: - be included in the job page to render the whole page - be rendered indivudually and then returned as a blob of HTML inside a JSON response Then I’ve added a Javascript module which looks for areas of the page that should be reloaded. For each area of the page it will poll a URL and re-render that section of the page when it gets new HTML. It implements some throttling so that API calls will never happen more frequently than 0.67 times/second. --- app/assets/javascripts/updateContent.js | 44 +++++++++ app/main/views/jobs.py | 51 ++++++++-- app/templates/partials/jobs/count.html | 24 +++++ .../partials/jobs/notifications.html | 22 +++++ app/templates/partials/jobs/status.html | 9 ++ app/templates/views/job.html | 97 ------------------- app/templates/views/jobs/job.html | 69 +++++++++++++ app/templates/views/{ => jobs}/jobs.html | 0 gulpfile.babel.js | 1 + tests/app/main/views/test_jobs.py | 57 ++++++++--- 10 files changed, 259 insertions(+), 115 deletions(-) create mode 100644 app/assets/javascripts/updateContent.js create mode 100644 app/templates/partials/jobs/count.html create mode 100644 app/templates/partials/jobs/notifications.html create mode 100644 app/templates/partials/jobs/status.html delete mode 100644 app/templates/views/job.html create mode 100644 app/templates/views/jobs/job.html rename app/templates/views/{ => jobs}/jobs.html (100%) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js new file mode 100644 index 000000000..64cfc7325 --- /dev/null +++ b/app/assets/javascripts/updateContent.js @@ -0,0 +1,44 @@ +(function(GOVUK, Modules) { + "use strict"; + + const interval = 1500; // milliseconds + + GOVUK.timeCache = {}; + GOVUK.resultCache = {}; + + let getter = function(resource, render) { + + if ( + GOVUK.resultCache[resource] && + (Date.now() < GOVUK.timeCache[resource]) + ) { + render(GOVUK.resultCache[resource]); + } else { + GOVUK.timeCache[resource] = Date.now() + interval; + $.get( + resource, + response => render(GOVUK.resultCache[resource] = response) + ); + } + + }; + + let poller = (resource, key, component) => () => getter( + resource, response => component.html(response[key]) + ); + + Modules.UpdateContent = function() { + + this.start = function(component) { + + const $component = $(component); + + setInterval( + poller($component.data('resource'), $component.data('key'), $component), + interval / 5 + ); + + }; + }; + +})(window.GOVUK, window.GOVUK.Modules); diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index bb0013a63..093c91163 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -4,7 +4,8 @@ import time from flask import ( render_template, - abort + abort, + jsonify ) from flask_login import login_required from notifications_python_client.errors import HTTPError @@ -15,8 +16,6 @@ from app.main import main from app.main.dao import templates_dao from app.main.dao import services_dao -now = time.strftime('%H:%M') - @main.route("/services//jobs") @login_required @@ -24,7 +23,7 @@ def view_jobs(service_id): try: jobs = job_api_client.get_job(service_id)['data'] return render_template( - 'views/jobs.html', + 'views/jobs/jobs.html', jobs=jobs, service_id=service_id ) @@ -44,16 +43,16 @@ def view_job(service_id, job_id): notifications = notification_api_client.get_notifications_for_service(service_id, job_id) finished = job['status'] == 'finished' return render_template( - 'views/job.html', + 'views/jobs/job.html', notifications=notifications['notifications'], counts={ 'queued': 0 if finished else job['notification_count'], 'sent': job['notification_count'] if finished else 0, - 'failed': 0 + 'failed': 0, + 'cost': u'£0.00' }, uploaded_at=job['created_at'], finished_at=job['updated_at'] if finished else None, - cost=u'£0.00', uploaded_file_name=job['original_file_name'], template=Template( templates_dao.get_service_template_or_404(service_id, job['template'])['data'], @@ -70,9 +69,47 @@ def view_job(service_id, job_id): raise e +@main.route("/services//jobs/.json") +@login_required +def view_job_updates(service_id, job_id): + service = services_dao.get_service_by_id_or_404(service_id) + try: + job = job_api_client.get_job(service_id, job_id)['data'] + notifications = notification_api_client.get_notifications_for_service(service_id, job_id) + finished = job['status'] == 'finished' + return jsonify(**{ + 'counts': render_template( + 'partials/jobs/count.html', + counts={ + 'queued': 0 if finished else job['notification_count'], + 'sent': job['notification_count'] if finished else 0, + 'failed': 0, + 'cost': u'£0.00' + } + ), + 'notifications': render_template( + 'partials/jobs/notifications.html', + notifications=notifications['notifications'] + ), + 'status': render_template( + 'partials/jobs/status.html', + uploaded_at=job['created_at'], + finished_at=job['updated_at'] if finished else None + ), + }) + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e + + @main.route("/services//jobs//notification/") @login_required def view_notification(service_id, job_id, notification_id): + + now = time.strftime('%H:%M') + return render_template( 'views/notification.html', message=[ diff --git a/app/templates/partials/jobs/count.html b/app/templates/partials/jobs/count.html new file mode 100644 index 000000000..f88e0aed0 --- /dev/null +++ b/app/templates/partials/jobs/count.html @@ -0,0 +1,24 @@ +{% from "components/big-number.html" import big_number %} +
    +
  • + {{ big_number( + counts.queued, 'queued' + )}} +
    • +
  • + {{ big_number( + counts.sent, 'sent' + )}} +
    • +
  • + {{ big_number( + counts.failed, + 'failed' + )}} +
    • +
  • + {{ big_number( + counts.cost, 'total cost' + )}} +
    • +
diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html new file mode 100644 index 000000000..bc4ecfaaf --- /dev/null +++ b/app/templates/partials/jobs/notifications.html @@ -0,0 +1,22 @@ +{% from "components/table.html" import list_table, field, right_aligned_field_heading %} + +{% call(item) list_table( + notifications, + caption=uploaded_file_name, + caption_visible=False, + empty_message="No messages to show yet", + field_headings=[ + 'Recipient', + right_aligned_field_heading('Status') + ] +) %} + {% call field() %} + {{ item.to }} + {% endcall %} + {% call field( + align='right', + status='error' if item.status == 'Failed' else 'default' + ) %} + {{ item.status|title }} at {{ item.sent_at|format_time }} + {% endcall %} +{% endcall %} diff --git a/app/templates/partials/jobs/status.html b/app/templates/partials/jobs/status.html new file mode 100644 index 000000000..775d0c2d1 --- /dev/null +++ b/app/templates/partials/jobs/status.html @@ -0,0 +1,9 @@ +{% if finished_at %} +

+ Finished {{ finished_at|format_datetime }} +

+{% else %} +

+ Started {{ uploaded_at|format_datetime }} +

+{% endif %} diff --git a/app/templates/views/job.html b/app/templates/views/job.html deleted file mode 100644 index 63346c7ea..000000000 --- a/app/templates/views/job.html +++ /dev/null @@ -1,97 +0,0 @@ -{% extends "withnav_template.html" %} -{% from "components/table.html" import list_table, field, right_aligned_field_heading %} -{% from "components/big-number.html" import big_number %} -{% from "components/banner.html" import banner %} -{% from "components/sms-message.html" import sms_message %} -{% from "components/email-message.html" import email_message %} - -{% block page_title %} - {{ uploaded_file_name }} – GOV.UK Notify -{% endblock %} - -{% block maincolumn_content %} - -

- {{ uploaded_file_name }} -

- - {% if 'sms' == template.template_type %} -
-
- {{ sms_message( - template.formatted_as_markup, - )}} -
-
- {% elif 'email' == template.template_type %} - {{ email_message( - template.subject, - template, - from_address='{}@notifications.service.gov.uk'.format(service.email_from), - from_name=from_name - )}} - {% endif %} - - {% if finished_at %} -

- Finished {{ finished_at|format_datetime }} -

- {% else %} -

- Started {{ uploaded_at|format_datetime }} -

- {% endif %} - -
    -
  • - {{ big_number( - counts.queued, 'queued' - )}} -
    • -
  • - {{ big_number( - counts.sent, 'sent' - )}} -
    • -
  • - {{ big_number( - counts.failed, - 'failed' - )}} -
    • -
  • - {{ big_number( - cost, 'total cost' - )}} -
    • -
- - - {% if notifications %} - {% call(item) list_table( - notifications, - caption=uploaded_file_name, - caption_visible=False, - empty_message="Messages go here", - field_headings=[ - 'Recipient', - right_aligned_field_heading('Status') - ] - ) %} - {% call field() %} - {{ item.to }} - {% endcall %} - {% call field( - align='right', - status='error' if item.status == 'Failed' else 'default' - ) %} - {{ item.status|title }} at {{ item.sent_at|format_time }} - {% endcall %} - {% endcall %} - {% else %} -

- Refresh -

- {% endif %} - -{% endblock %} diff --git a/app/templates/views/jobs/job.html b/app/templates/views/jobs/job.html new file mode 100644 index 000000000..3af945de8 --- /dev/null +++ b/app/templates/views/jobs/job.html @@ -0,0 +1,69 @@ +{% extends "withnav_template.html" %} +{% from "components/banner.html" import banner %} +{% from "components/sms-message.html" import sms_message %} +{% from "components/email-message.html" import email_message %} + +{% block page_title %} + {{ uploaded_file_name }} – GOV.UK Notify +{% endblock %} + +{% block maincolumn_content %} + +

+ {{ uploaded_file_name }} +

+ + {% if 'sms' == template.template_type %} +
+
+ {{ sms_message( + template.formatted_as_markup, + )}} +
+
+ {% elif 'email' == template.template_type %} + {{ email_message( + template.subject, + template, + from_address='{}@notifications.service.gov.uk'.format(service.email_from), + from_name=from_name + )}} + {% endif %} + + {% if not finished_at %} +
+ {% endif %} + {% include 'partials/jobs/status.html' %} + {% if not finished_at %} +
+ {% endif %} + + {% if not finished_at %} +
+ {% endif %} + {% include 'partials/jobs/count.html' %} + {% if not finished_at %} +
+ {% endif %} + + {% if not finished_at %} +
+ {% endif %} + {% include 'partials/jobs/notifications.html' %} + {% if not finished_at %} +
+ {% endif %} + +{% endblock %} diff --git a/app/templates/views/jobs.html b/app/templates/views/jobs/jobs.html similarity index 100% rename from app/templates/views/jobs.html rename to app/templates/views/jobs/jobs.html diff --git a/gulpfile.babel.js b/gulpfile.babel.js index 030c5eeb7..b5470e712 100644 --- a/gulpfile.babel.js +++ b/gulpfile.babel.js @@ -54,6 +54,7 @@ gulp.task('javascripts', () => gulp paths.src + 'javascripts/autofocus.js', paths.src + 'javascripts/highlightTags.js', paths.src + 'javascripts/fileUpload.js', + paths.src + 'javascripts/updateContent.js', paths.src + 'javascripts/main.js' ]) .pipe(plugins.babel({ diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 8a8ff0aff..2b8d4e794 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -1,5 +1,6 @@ from flask import url_for from bs4 import BeautifulSoup +import json def test_should_return_list_of_all_jobs(app_, @@ -21,17 +22,19 @@ def test_should_return_list_of_all_jobs(app_, assert len(jobs) == 5 -def test_should_show_page_for_one_job(app_, - service_one, - api_user_active, - mock_login, - mock_get_user, - mock_get_user_by_email, - mock_get_service, - mock_get_service_template, - job_data, - mock_get_job, - mock_get_notifications): +def test_should_show_page_for_one_job( + app_, + service_one, + api_user_active, + mock_login, + mock_get_user, + mock_get_user_by_email, + mock_get_service, + mock_get_service_template, + job_data, + mock_get_job, + mock_get_notifications +): service_id = job_data['service'] job_id = job_data['id'] file_name = job_data['original_file_name'] @@ -45,3 +48,35 @@ def test_should_show_page_for_one_job(app_, content = response.get_data(as_text=True) assert "Test Service: Your vehicle tax is about to expire" in content assert file_name in content + + +def test_should_show_updates_for_one_job_as_json( + app_, + service_one, + api_user_active, + mock_login, + mock_get_user, + mock_get_user_by_email, + mock_get_service, + mock_get_service_template, + job_data, + mock_get_job, + mock_get_notifications +): + service_id = job_data['service'] + job_id = job_data['id'] + file_name = job_data['original_file_name'] + + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + response = client.get(url_for('main.view_job_updates', service_id=service_id, job_id=job_id)) + + assert response.status_code == 200 + content = json.loads(response.get_data(as_text=True)) + assert 'sent' in content['counts'] + assert 'queued' in content['counts'] + assert 'failed' in content['counts'] + assert 'Recipient' in content['notifications'] + assert 'Status' in content['notifications'] + assert 'Started' in content['status'] From 990e626631c52b70d37378f71eab5c1c11031b14 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 2 Mar 2016 21:19:02 +0000 Subject: [PATCH 2/2] Remove tick banner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I’d like to see if we can get away without this now that the page updates. In research the ‘We’ve started’ part of the message confused people, especially when they’d only sent one message. --- app/main/views/send.py | 1 - tests/app/main/views/test_send.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 8f7aa93e9..da7bc6ba6 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -230,7 +230,6 @@ def check_messages(service_id, upload_id): else: raise e - flash('We’ve started sending your messages', 'default_with_tick') return redirect( url_for('main.view_job', service_id=service_id, job_id=upload_id) ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 7e8b9e087..5404291d8 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -265,5 +265,5 @@ def test_create_job_should_call_api( response = client.post(url, data=job_data, follow_redirects=True) assert response.status_code == 200 - assert 'We’ve started sending your messages' in response.get_data(as_text=True) + assert original_file_name in response.get_data(as_text=True) mock_create_job.assert_called_with(job_id, service_id, template_id, original_file_name, notification_count)