From 697803a10c1c4edc722a91d31677d662e209c46d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 24 Mar 2020 15:45:13 +0000 Subject: [PATCH] =?UTF-8?q?Add=20a=20=E2=80=98general=E2=80=99=20ticket=20?= =?UTF-8?q?type?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is for tickets coming from non-logged-in users. It’s effectively the same as reporting a problem, but doesn’t have the banner about the status page (because we can’t tell if they’re actually reporting a problem now we’re not asking). It also gives a more generic page title. --- app/main/views/feedback.py | 34 +++++-- app/models/feedback.py | 1 + app/templates/views/support/form.html | 4 +- app/templates/views/support/triage.html | 4 +- app/url_converters.py | 8 +- tests/app/main/views/test_feedback.py | 120 ++++++++++++++++++++---- 6 files changed, 136 insertions(+), 35 deletions(-) diff --git a/app/main/views/feedback.py b/app/main/views/feedback.py index f28fefaee..4b09e40f3 100644 --- a/app/main/views/feedback.py +++ b/app/main/views/feedback.py @@ -13,7 +13,11 @@ from app.main.forms import ( SupportType, Triage, ) -from app.models.feedback import PROBLEM_TICKET_TYPE +from app.models.feedback import ( + GENERAL_TICKET_TYPE, + PROBLEM_TICKET_TYPE, + QUESTION_TICKET_TYPE, +) def get_prefilled_message(): @@ -50,7 +54,7 @@ def support(): else: return redirect(url_for( '.feedback', - ticket_type=PROBLEM_TICKET_TYPE, + ticket_type=GENERAL_TICKET_TYPE, )) return render_template('views/support/index.html', form=form) @@ -62,17 +66,22 @@ def support_public(): @main.route('/support/triage', methods=['GET', 'POST']) -def triage(): +@main.route('/support/triage/', methods=['GET', 'POST']) +def triage(ticket_type=PROBLEM_TICKET_TYPE): form = Triage() if form.validate_on_submit(): return redirect(url_for( '.feedback', - ticket_type=PROBLEM_TICKET_TYPE, + ticket_type=ticket_type, severe=form.severe.data )) return render_template( 'views/support/triage.html', - form=form + form=form, + page_title={ + PROBLEM_TICKET_TYPE: 'Report a problem', + GENERAL_TICKET_TYPE: 'Contact GOV.UK Notify support', + }.get(ticket_type) ) @@ -89,14 +98,14 @@ def feedback(ticket_type): severe = None out_of_hours_emergency = all(( - ticket_type == PROBLEM_TICKET_TYPE, + ticket_type != QUESTION_TICKET_TYPE, not in_business_hours(), severe, )) if needs_triage(ticket_type, severe): session['feedback_message'] = form.feedback.data - return redirect(url_for('.triage')) + return redirect(url_for('.triage', ticket_type=ticket_type)) if needs_escalation(ticket_type, severe): return redirect(url_for('.bat_phone')) @@ -144,7 +153,12 @@ def feedback(ticket_type): return render_template( 'views/support/form.html', form=form, - is_problem=(ticket_type == PROBLEM_TICKET_TYPE), + show_status_page_banner=(ticket_type == PROBLEM_TICKET_TYPE), + page_title={ + GENERAL_TICKET_TYPE: 'Contact GOV.UK Notify support', + PROBLEM_TICKET_TYPE: 'Report a problem', + QUESTION_TICKET_TYPE: 'Ask a question or give feedback', + }.get(ticket_type), ) @@ -254,7 +268,7 @@ def has_live_services(user_id): def needs_triage(ticket_type, severe): return all(( - ticket_type == PROBLEM_TICKET_TYPE, + ticket_type != QUESTION_TICKET_TYPE, severe is None, ( not current_user.is_authenticated or has_live_services(current_user.id) @@ -265,7 +279,7 @@ def needs_triage(ticket_type, severe): def needs_escalation(ticket_type, severe): return all(( - ticket_type == PROBLEM_TICKET_TYPE, + ticket_type != QUESTION_TICKET_TYPE, severe, not current_user.is_authenticated, not in_business_hours(), diff --git a/app/models/feedback.py b/app/models/feedback.py index fffbf0a29..345be5e6a 100644 --- a/app/models/feedback.py +++ b/app/models/feedback.py @@ -1,2 +1,3 @@ QUESTION_TICKET_TYPE = 'ask-question-give-feedback' PROBLEM_TICKET_TYPE = 'report-problem' +GENERAL_TICKET_TYPE = 'general' diff --git a/app/templates/views/support/form.html b/app/templates/views/support/form.html index 342d5cd5d..62a67e403 100644 --- a/app/templates/views/support/form.html +++ b/app/templates/views/support/form.html @@ -5,8 +5,6 @@ {% from "components/page-header.html" import page_header %} {% from "components/form.html" import form_wrapper %} -{% set page_title = 'Report a problem' if is_problem else 'Ask a question or give feedback' %} - {% block per_page_title %} {{ page_title }} {% endblock %} @@ -19,7 +17,7 @@ ) }}
- {% if is_problem %} + {% if show_status_page_banner %}

Check our system status diff --git a/app/templates/views/support/triage.html b/app/templates/views/support/triage.html index 8124ce8e4..9b32dd13c 100644 --- a/app/templates/views/support/triage.html +++ b/app/templates/views/support/triage.html @@ -5,7 +5,7 @@ {% from "components/form.html" import form_wrapper %} {% block per_page_title %} - Feedback + {{ page_title }} {% endblock %} {% block maincolumn_content %} @@ -13,7 +13,7 @@

{{ page_header( - 'Report a problem', + page_title, back_link=url_for('.support') ) }} {% call form_wrapper() %} diff --git a/app/url_converters.py b/app/url_converters.py index aa2b5dabe..847543b96 100644 --- a/app/url_converters.py +++ b/app/url_converters.py @@ -1,6 +1,10 @@ from werkzeug.routing import BaseConverter -from app.models.feedback import PROBLEM_TICKET_TYPE, QUESTION_TICKET_TYPE +from app.models.feedback import ( + GENERAL_TICKET_TYPE, + PROBLEM_TICKET_TYPE, + QUESTION_TICKET_TYPE, +) class TemplateTypeConverter(BaseConverter): @@ -10,7 +14,7 @@ class TemplateTypeConverter(BaseConverter): class TicketTypeConverter(BaseConverter): - regex = f'(?:{PROBLEM_TICKET_TYPE}|{QUESTION_TICKET_TYPE})' + regex = f'(?:{PROBLEM_TICKET_TYPE}|{QUESTION_TICKET_TYPE}|{GENERAL_TICKET_TYPE})' class LetterFileExtensionConverter(BaseConverter): diff --git a/tests/app/main/views/test_feedback.py b/tests/app/main/views/test_feedback.py index cfe649ab7..138bfa536 100644 --- a/tests/app/main/views/test_feedback.py +++ b/tests/app/main/views/test_feedback.py @@ -7,7 +7,11 @@ from flask import url_for from freezegun import freeze_time from app.main.views.feedback import has_live_services, in_business_hours -from app.models.feedback import PROBLEM_TICKET_TYPE, QUESTION_TICKET_TYPE +from app.models.feedback import ( + GENERAL_TICKET_TYPE, + PROBLEM_TICKET_TYPE, + QUESTION_TICKET_TYPE, +) from tests.conftest import normalize_spaces @@ -82,6 +86,7 @@ def test_choose_support_type( ) +@freeze_time('2016-12-12 12:00:00.000000') def test_get_support_as_someone_in_the_public_sector( client_request, ): @@ -92,7 +97,7 @@ def test_get_support_as_someone_in_the_public_sector( _follow_redirects=True, ) assert normalize_spaces(page.select('h1')) == ( - 'Report a problem' + 'Contact GOV.UK Notify support' ) assert page.select_one('form textarea[name=feedback]') assert page.select_one('form input[name=name]') @@ -344,8 +349,8 @@ def test_urgency( ids, params = zip(*[ ('non-logged in users always have to triage', ( - PROBLEM_TICKET_TYPE, False, False, True, - 302, partial(url_for, 'main.triage') + GENERAL_TICKET_TYPE, False, False, True, + 302, partial(url_for, 'main.triage', ticket_type=GENERAL_TICKET_TYPE) )), ('trial services are never high priority', ( PROBLEM_TICKET_TYPE, False, True, False, @@ -361,7 +366,7 @@ ids, params = zip(*[ )), ('should triage out of hours', ( PROBLEM_TICKET_TYPE, False, True, True, - 302, partial(url_for, 'main.triage') + 302, partial(url_for, 'main.triage', ticket_type=PROBLEM_TICKET_TYPE) )) ]) @@ -395,6 +400,21 @@ def test_redirects_to_triage( assert response.location == expected_redirect(_external=True) +@pytest.mark.parametrize('ticket_type, expected_h1', ( + (PROBLEM_TICKET_TYPE, 'Report a problem'), + (GENERAL_TICKET_TYPE, 'Contact GOV.UK Notify support'), +)) +def test_options_on_triage_page( + client_request, + ticket_type, + expected_h1, +): + page = client_request.get('main.triage', ticket_type=ticket_type) + assert normalize_spaces(page.select_one('h1').text) == expected_h1 + assert page.select('form input[type=radio]')[0]['value'] == 'yes' + assert page.select('form input[type=radio]')[1]['value'] == 'no' + + def test_doesnt_lose_message_if_post_across_closing( client_request, mocker, @@ -408,7 +428,7 @@ def test_doesnt_lose_message_if_post_across_closing( ticket_type=PROBLEM_TICKET_TYPE, _data={'feedback': 'foo'}, _expected_status=302, - _expected_redirect=url_for('.triage', _external=True), + _expected_redirect=url_for('.triage', ticket_type=PROBLEM_TICKET_TYPE, _external=True), ) with client_request.session_transaction() as session: assert session['feedback_message'] == 'foo' @@ -458,18 +478,31 @@ def test_in_business_hours(when, is_in_business_hours): assert in_business_hours() == is_in_business_hours +@pytest.mark.parametrize('ticket_type', ( + GENERAL_TICKET_TYPE, + PROBLEM_TICKET_TYPE, +)) @pytest.mark.parametrize('choice, expected_redirect_param', [ ('yes', 'yes'), ('no', 'no'), ]) -def test_triage_redirects_to_correct_url(client, choice, expected_redirect_param): - response = client.post(url_for('main.triage'), data={'severe': choice}) - assert response.status_code == 302 - assert response.location == url_for( - 'main.feedback', - ticket_type=PROBLEM_TICKET_TYPE, - severe=expected_redirect_param, - _external=True, +def test_triage_redirects_to_correct_url( + client_request, + ticket_type, + choice, + expected_redirect_param, +): + client_request.post( + 'main.triage', + ticket_type=ticket_type, + _data={'severe': choice}, + _expected_status=302, + _expected_redirect=url_for( + 'main.feedback', + ticket_type=ticket_type, + severe=expected_redirect_param, + _external=True, + ), ) @@ -499,15 +532,15 @@ def test_triage_redirects_to_correct_url(client, choice, expected_redirect_param # Treat empty query param as mangled URL – ask question again ( False, '', - 302, partial(url_for, 'main.triage'), - 302, partial(url_for, 'main.triage'), + 302, partial(url_for, 'main.triage', ticket_type=PROBLEM_TICKET_TYPE), + 302, partial(url_for, 'main.triage', ticket_type=PROBLEM_TICKET_TYPE), ), # User hasn’t answered the triage question ( False, None, - 302, partial(url_for, 'main.triage'), - 302, partial(url_for, 'main.triage'), + 302, partial(url_for, 'main.triage', ticket_type=PROBLEM_TICKET_TYPE), + 302, partial(url_for, 'main.triage', ticket_type=PROBLEM_TICKET_TYPE), ), # Escalation is needed for non-logged-in users @@ -548,6 +581,57 @@ def test_should_be_shown_the_bat_email( assert logged_in_response.location == expected_redirect_when_logged_in(_external=True) +@pytest.mark.parametrize( + ( + 'severe,' + 'expected_status_code, expected_redirect,' + 'expected_status_code_when_logged_in, expected_redirect_when_logged_in' + ), + [ + # User hasn’t answered the triage question + ( + None, + 302, partial(url_for, 'main.triage', ticket_type=GENERAL_TICKET_TYPE), + 302, partial(url_for, 'main.triage', ticket_type=GENERAL_TICKET_TYPE), + ), + + # Escalation is needed for non-logged-in users + ( + 'yes', + 302, partial(url_for, 'main.bat_phone'), + 200, no_redirect(), + ), + ] +) +def test_should_be_shown_the_bat_email_for_general_questions( + client, + active_user_with_permissions, + mocker, + service_one, + mock_get_services, + severe, + expected_status_code, + expected_redirect, + expected_status_code_when_logged_in, + expected_redirect_when_logged_in, +): + + mocker.patch('app.main.views.feedback.in_business_hours', return_value=False) + + feedback_page = url_for('main.feedback', ticket_type=GENERAL_TICKET_TYPE, severe=severe) + + response = client.get(feedback_page) + + assert response.status_code == expected_status_code + assert response.location == expected_redirect(_external=True) + + # logged in users should never be redirected to the bat email page + client.login(active_user_with_permissions, mocker, service_one) + logged_in_response = client.get(feedback_page) + assert logged_in_response.status_code == expected_status_code_when_logged_in + assert logged_in_response.location == expected_redirect_when_logged_in(_external=True) + + def test_bat_email_page( client, active_user_with_permissions,