From 1df3c11ae9f6b7163684d06a8e7e7faa1b0b66f9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 12 Dec 2016 11:25:43 +0000 Subject: [PATCH] Split support into two pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The kind of communications we’re getting at the moment can broadly be broken down into: - problems - questions and feedback We will need to triage problems differently, because they could potentially be urgent/severe/emergency/P1/whatever language we use. Questions or feedback will never be P1. Two reasons for making the user categorise their tickets themselves: - Outside of hours we can’t get someone out of bed in order to decide if a ticket is a problem or just feedback - We can tailor the subsequent pages to whether it’s a problem or feedback (eg showing a link to the status page if the user is having a problem) This commit let’s users make the choice with a pair of radio buttons. It also cleans up a bunch of the tests and parameterizes them so we’re testing the flow for both ticket types. --- app/main/forms.py | 13 +- app/main/validators.py | 2 +- app/main/views/feedback.py | 27 +++- app/templates/views/support/index.html | 15 +- .../support/{feedback.html => problem.html} | 12 +- app/templates/views/support/question.html | 31 ++++ app/templates/views/terms-of-use.html | 6 +- tests/app/main/views/test_feedback.py | 142 ++++++++++-------- 8 files changed, 158 insertions(+), 90 deletions(-) rename app/templates/views/support/{feedback.html => problem.html} (80%) create mode 100644 app/templates/views/support/question.html diff --git a/app/main/forms.py b/app/main/forms.py index 441bc4d47..8c33f4f28 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -392,10 +392,21 @@ class CreateKeyForm(Form): raise ValidationError('A key with this name already exists') +class SupportType(Form): + support_type = RadioField( + 'How can we help you?', + choices=[ + ('problem', 'Report a problem'), + ('question', 'Ask a question or give feedback'), + ], + validators=[DataRequired()] + ) + + class Feedback(Form): name = StringField('Name') email_address = StringField('Email address') - feedback = TextAreaField(u'', validators=[DataRequired(message="Can’t be empty")]) + feedback = TextAreaField('Your message', validators=[DataRequired(message="Can’t be empty")]) class RequestToGoLiveForm(Form): diff --git a/app/main/validators.py b/app/main/validators.py index c44b9774d..d07d7cab7 100644 --- a/app/main/validators.py +++ b/app/main/validators.py @@ -35,7 +35,7 @@ class ValidGovEmail(object): message = ( 'Enter a central government email address.' ' If you think you should have access' - ' contact us').format(url_for('main.feedback')) + ' contact us').format(url_for('main.support')) if not is_gov_user(field.data.lower()): raise ValidationError(message) diff --git a/app/main/views/feedback.py b/app/main/views/feedback.py index e481676ad..5f87a0d2a 100644 --- a/app/main/views/feedback.py +++ b/app/main/views/feedback.py @@ -1,16 +1,24 @@ import requests from flask import render_template, url_for, redirect, flash, current_app, abort from app.main import main -from app.main.forms import Feedback +from app.main.forms import SupportType, Feedback @main.route('/support', methods=['GET', 'POST']) def support(): - return render_template('views/support/index.html') + form = SupportType() + if form.validate_on_submit(): + return redirect(url_for( + '.feedback', + ticket_type=form.support_type.data, + )) + return render_template('views/support/index.html', form=form) -@main.route('/support/feedback', methods=['GET', 'POST']) -def feedback(): +@main.route('/support/contact/', methods=['GET', 'POST']) +def feedback(ticket_type): + if ticket_type not in ['problem', 'question']: + abort(404) form = Feedback() if form.validate_on_submit(): user_supplied_email = form.email_address.data != '' @@ -25,7 +33,8 @@ def feedback(): 'department_id': current_app.config.get('DESKPRO_DEPT_ID'), 'agent_team_id': current_app.config.get('DESKPRO_ASSIGNED_AGENT_TEAM_ID'), 'subject': 'Notify feedback', - 'message': feedback_msg + 'message': feedback_msg, + 'label': ticket_type, } headers = { "X-DeskPRO-API-Key": current_app.config.get('DESKPRO_API_KEY'), @@ -43,6 +52,10 @@ def feedback(): ) abort(500, "Feedback submission failed") flash("Thanks, we’ve received your feedback", 'default_with_tick') - return redirect(url_for('.support')) + return redirect(url_for('.support', ticket_type=ticket_type)) - return render_template('views/support/feedback.html', form=form) + return render_template( + 'views/support/{}.html'.format(ticket_type), + form=form, + ticket_type=ticket_type + ) diff --git a/app/templates/views/support/index.html b/app/templates/views/support/index.html index cfcf3958f..5081b9f39 100644 --- a/app/templates/views/support/index.html +++ b/app/templates/views/support/index.html @@ -1,9 +1,9 @@ {% extends "withoutnav_template.html" %} -{% from "components/textbox.html" import textbox %} +{% from "components/radios.html" import radios %} {% from "components/page-footer.html" import page_footer %} {% block page_title %} - Feedback – GOV.UK Notify + Support – GOV.UK Notify {% endblock %} {% block maincolumn_content %} @@ -15,12 +15,10 @@
-

- I have a problem or question -

- -
-
+
+ {{ radios(form.support_type) }} + {{ page_footer('Next') }} +

Support process @@ -82,5 +80,4 @@

- {% endblock %} diff --git a/app/templates/views/support/feedback.html b/app/templates/views/support/problem.html similarity index 80% rename from app/templates/views/support/feedback.html rename to app/templates/views/support/problem.html index 681ab338a..2455f7ae2 100644 --- a/app/templates/views/support/feedback.html +++ b/app/templates/views/support/problem.html @@ -3,19 +3,19 @@ {% from "components/page-footer.html" import page_footer %} {% block page_title %} - Feedback – GOV.UK Notify + Report a problem – GOV.UK Notify {% endblock %} {% block maincolumn_content %}

- Support and feedback + Report a problem

- Check our system status + Check our system status page to see if there are any known issues with GOV.UK Notify.

@@ -26,7 +26,11 @@

Leave your details below if you'd like a response.

{{ textbox(form.name, width='1-1') }} {{ textbox(form.email_address, width='1-1') }} - {{ page_footer('Send') }} + {{ page_footer( + 'Send', + secondary_link=url_for('.support'), + secondary_link_text='Back to support', + ) }}
diff --git a/app/templates/views/support/question.html b/app/templates/views/support/question.html new file mode 100644 index 000000000..d856996e1 --- /dev/null +++ b/app/templates/views/support/question.html @@ -0,0 +1,31 @@ +{% extends "withoutnav_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/page-footer.html" import page_footer %} + +{% block page_title %} + Ask a question or give feedback – GOV.UK Notify +{% endblock %} + +{% block maincolumn_content %} + +

+ Ask a question or give feedback +

+
+
+
+ {{ textbox(form.feedback, width='1-1', hint='', rows=10) }} +

Do you want a reply?

+

Leave your details below if you'd like a response.

+ {{ textbox(form.name, width='1-1') }} + {{ textbox(form.email_address, width='1-1') }} + {{ page_footer( + 'Send', + secondary_link=url_for('.support'), + secondary_link_text='Back to support', + ) }} +
+
+
+ +{% endblock %} diff --git a/app/templates/views/terms-of-use.html b/app/templates/views/terms-of-use.html index ceb18e3d1..f4d576421 100644 --- a/app/templates/views/terms-of-use.html +++ b/app/templates/views/terms-of-use.html @@ -18,7 +18,7 @@ Terms of use – GOV.UK Notify

You must accept the GOV.UK Notify data sharing and financial agreement (Memorandum of Understanding) before we can process data for you.

- Contact the Notify team to get a copy of the agreement or to find out if your organisation has already accepted it. + Contact the Notify team to get a copy of the agreement or to find out if your organisation has already accepted it.

{% endcall %} @@ -83,7 +83,7 @@ Terms of use – GOV.UK Notify

Cabinet Office act as data processor, as parent organisation of GOV.UK Notify. Your organisation remains the data controller.

-

Contact us if you want more information about our approach to data protection and information risk management.

+

Contact us if you want more information about our approach to data protection and information risk management.

We agree to give you one months’ notice if we change these terms @@ -167,7 +167,7 @@ Terms of use – GOV.UK Notify Leaving GOV.UK Notify

-

You can remove your service from GOV.UK Notify at any time. Contact us and we’ll delete your account.

+

You can remove your service from GOV.UK Notify at any time. Contact us and we’ll delete your account.

Any data that you have already processed through GOV.UK Notify will be deleted as part of the existing data deletion processes and data retention periods.

diff --git a/tests/app/main/views/test_feedback.py b/tests/app/main/views/test_feedback.py index bc78e2ed4..e2ca451ef 100644 --- a/tests/app/main/views/test_feedback.py +++ b/tests/app/main/views/test_feedback.py @@ -1,3 +1,5 @@ +from bs4 import BeautifulSoup +from functools import partial import pytest from flask import url_for from werkzeug.exceptions import InternalServerError @@ -24,76 +26,86 @@ def test_get_support_index_page(client): assert resp.status_code == 200 -def test_get_feedback_page(app_): - with app_.test_request_context(): - with app_.test_client() as client: - resp = client.get(url_for('main.feedback')) - assert resp.status_code == 200 +@pytest.mark.parametrize('support_type, expected_h1', [ + ('problem', 'Report a problem'), + ('question', 'Ask a question or give feedback'), +]) +def test_choose_support_type(client, support_type, expected_h1): + response = client.post( + url_for('main.support'), + data={'support_type': support_type}, follow_redirects=True + ) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == expected_h1 -def test_post_feedback_with_name_but_no_email(app_, mocker): +@pytest.mark.parametrize('ticket_type, expected_status_code', [ + ('problem', 200), + ('question', 200), + ('gripe', 404) +]) +def test_get_feedback_page(client, ticket_type, expected_status_code): + response = client.get(url_for('main.feedback', ticket_type=ticket_type)) + assert response.status_code == expected_status_code + + +@pytest.mark.parametrize('data, expected_message, expected_person_name, expected_email', [ + ( + {'feedback': "blah", 'name': 'Fred'}, + 'Environment: http://localhost/\nFred (no email address supplied)\nblah', + 'Fred', + 'donotreply@notifications.service.gov.uk', + ), + ( + {'feedback': "blah"}, + 'Environment: http://localhost/\n (no email address supplied)\nblah', + None, + 'donotreply@notifications.service.gov.uk', + ), + ( + {'feedback': "blah", 'name': "Steve Irwin", 'email_address': 'rip@gmail.com'}, + 'Environment: http://localhost/\n\nblah', + 'Steve Irwin', + 'rip@gmail.com', + ), +]) +@pytest.mark.parametrize('ticket_type', ['problem', 'question']) +def test_post_feedback_with_name_but_no_email( + client, + mocker, + ticket_type, + data, + expected_message, + expected_person_name, + expected_email, +): mock_post = mocker.patch( 'app.main.views.feedback.requests.post', - return_value=Mock(status_code=201)) - with app_.test_request_context(): - with app_.test_client() as client: - resp = client.post(url_for('main.feedback'), data={'feedback': "blah", 'name': 'Fred'}) - assert resp.status_code == 302 - mock_post.assert_called_with( - ANY, - data={ - 'department_id': ANY, - 'agent_team_id': ANY, - 'subject': 'Notify feedback', - 'message': 'Environment: http://localhost/\nFred (no email address supplied)\nblah', - 'person_email': app_.config['DESKPRO_PERSON_EMAIL'], - 'person_name': 'Fred'}, - headers=ANY) + return_value=Mock(status_code=201) + ) + resp = client.post( + url_for('main.feedback', ticket_type=ticket_type), + data=data, + ) + assert resp.status_code == 302 + mock_post.assert_called_with( + ANY, + data={ + 'department_id': ANY, + 'agent_team_id': ANY, + 'subject': 'Notify feedback', + 'message': expected_message.format(ticket_type), + 'person_email': expected_email, + 'person_name': expected_person_name, + 'label': ticket_type, + }, + headers=ANY + ) -def test_post_feedback_with_no_name_or_email(app_, mocker): - mock_post = mocker.patch( - 'app.main.views.feedback.requests.post', - return_value=Mock(status_code=201)) - with app_.test_request_context(): - with app_.test_client() as client: - resp = client.post(url_for('main.feedback'), data={'feedback': "blah"}) - assert resp.status_code == 302 - mock_post.assert_called_with( - ANY, - data={ - 'department_id': ANY, - 'agent_team_id': ANY, - 'subject': 'Notify feedback', - 'message': 'Environment: http://localhost/\n (no email address supplied)\nblah', - 'person_email': app_.config['DESKPRO_PERSON_EMAIL'], - 'person_name': None}, - headers=ANY) - - -def test_post_feedback_with_name_email(app_, mocker): - mock_post = mocker.patch( - 'app.main.views.feedback.requests.post', - return_value=Mock(status_code=201)) - with app_.test_request_context(): - with app_.test_client() as client: - resp = client.post( - url_for('main.feedback'), - data={'feedback': "blah", 'name': "Steve Irwin", 'email_address': 'rip@gmail.com'}) - assert resp.status_code == 302 - mock_post.assert_called_with( - ANY, - data={ - 'subject': 'Notify feedback', - 'department_id': ANY, - 'agent_team_id': ANY, - 'message': 'Environment: http://localhost/\n\nblah', - 'person_name': 'Steve Irwin', - 'person_email': 'rip@gmail.com'}, - headers=ANY) - - -def test_log_error_on_post(app_, mocker): +@pytest.mark.parametrize('ticket_type', ['problem', 'question']) +def test_log_error_on_post(app_, mocker, ticket_type): mock_post = mocker.patch( 'app.main.views.feedback.requests.post', return_value=Mock( @@ -106,7 +118,7 @@ def test_log_error_on_post(app_, mocker): with app_.test_client() as client: with pytest.raises(InternalServerError): resp = client.post( - url_for('main.feedback'), + url_for('main.feedback', ticket_type=ticket_type), data={'feedback': "blah", 'name': "Steve Irwin", 'email_address': 'rip@gmail.com'}) assert mock_post.called mock_logger.assert_called_with(