From 2f79ef136d34b23443216dd9cf5542e1491b1310 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 15 May 2020 13:55:04 +0100 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20cache=20page=20count=20for=20on?= =?UTF-8?q?e=20off=20letters?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why we did this originally[1]: > Calculating the number of pages in a letter is quite slow. And the > send yourself a test pages need to load _fast_. Since filling in > placeholders is very unlikely to change the number of pages in the > resultant letter, it’s pretty safe to cache that count, and makes the > subsequent pages load a lot faster. However things have changed since then: - this journey is used for sending real letters, not just test ones - we’re doing enough letters that even an unlikely discrepancy will (and does) happen - we cache the generation of the PDF now[2], so at least it’s not generating the PDF twice, once for the preview and once for the page count - it’s no longer necessary to step through each address placeholder to populate a one-off letter, so a little bit slower isn’t so bad 1. https://github.com/alphagov/notifications-admin/pull/1277/commits/e7896f283a0d34d433d032987ec90ef430dd0ba9 2. https://github.com/alphagov/notifications-template-preview/blob/c9c6271aa09fc6b2d271604d66939f72e76250ad/app/preview.py#L140 --- app/main/views/send.py | 9 ++--- tests/app/main/views/test_send.py | 57 +------------------------------ 2 files changed, 3 insertions(+), 63 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index ee4e0e5c5..1f6cdcef0 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -298,7 +298,6 @@ def get_sender_details(service_id, template_type): def send_test(service_id, template_id): session['recipient'] = None session['placeholders'] = {} - session['send_test_letter_page_count'] = None db_template = current_service.get_template_with_user_permission_or_403(template_id, current_user) if db_template['template_type'] == 'letter': @@ -351,8 +350,6 @@ def send_one_off_letter_address(service_id, template_id): db_template = current_service.get_template_with_user_permission_or_403(template_id, current_user) - session['send_test_letter_page_count'] = get_page_count_for_letter(db_template) - template = get_template( db_template, current_service, @@ -363,7 +360,7 @@ def send_one_off_letter_address(service_id, template_id): template_id=template_id, filetype='png', ), - page_count=session['send_test_letter_page_count'], + page_count=get_page_count_for_letter(db_template), email_reply_to=None, sms_sender=None ) @@ -436,8 +433,6 @@ def send_test_step(service_id, template_id, step_index): db_template = current_service.get_template_with_user_permission_or_403(template_id, current_user) - if not session.get('send_test_letter_page_count'): - session['send_test_letter_page_count'] = get_page_count_for_letter(db_template) email_reply_to = None sms_sender = None if db_template['template_type'] == 'email': @@ -454,7 +449,7 @@ def send_test_step(service_id, template_id, step_index): template_id=template_id, filetype='png', ), - page_count=session['send_test_letter_page_count'], + page_count=get_page_count_for_letter(db_template), email_reply_to=email_reply_to, sms_sender=sms_sender ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 3e1ac453f..7e682d5b4 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1469,7 +1469,6 @@ def test_send_one_off_or_test_shows_placeholders_in_correct_order( with client_request.session_transaction() as session: session['recipient'] = None session['placeholders'] = prefilled - session['send_test_letter_page_count'] = None page = client_request.get( endpoint, @@ -1584,7 +1583,6 @@ def test_send_one_off_has_sticky_header_for_letter_on_non_address_placeholders( mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9) with client_request.session_transaction() as session: - session['send_test_letter_page_count'] = 1 session['recipient'] = '' session['placeholders'] = { 'address line 1': 'foo', @@ -1872,7 +1870,6 @@ def test_send_test_redirects_to_start_if_you_skip_steps( mocker.patch('app.user_api_client.get_user', return_value=user) with platform_admin_client.session_transaction() as session: - session['send_test_letter_page_count'] = 1 session['placeholders'] = {'address_line_1': 'foo'} response = platform_admin_client.get(url_for( @@ -2061,7 +2058,6 @@ def test_send_test_sms_message_back_link_with_multiple_placeholders( with client_request.session_transaction() as session: session['recipient'] = '07900900123' session['placeholders'] = {'phone number': '07900900123', 'one': 'bar'} - session['send_test_letter_page_count'] = None page = client_request.get( 'main.send_test_step', @@ -2106,7 +2102,6 @@ def test_send_test_sms_message_back_link_in_tour( with client_request.session_transaction() as session: session['recipient'] = '07900900123' session['placeholders'] = {'phone number': '07900900123', 'one': 'bar'} - session['send_test_letter_page_count'] = None page = client_request.get( 'main.send_test_step', @@ -2122,30 +2117,6 @@ def test_send_test_sms_message_back_link_in_tour( ) -def test_send_test_letter_clears_previous_page_cache( - platform_admin_client, - mocker, - service_one, - mock_login, - mock_get_service, - mock_get_service_letter_template, - fake_uuid, -): - - with platform_admin_client.session_transaction() as session: - session['send_test_letter_page_count'] = 'WRONG' - - response = platform_admin_client.get(url_for( - 'main.send_test', - service_id=service_one['id'], - template_id=fake_uuid, - )) - assert response.status_code == 302 - - with platform_admin_client.session_transaction() as session: - assert session['send_test_letter_page_count'] is None - - def test_send_test_letter_redirects_to_right_url( platform_admin_client, fake_uuid, @@ -2155,9 +2126,8 @@ def test_send_test_letter_redirects_to_right_url( mock_get_service_statistics, mocker, ): - + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9) with platform_admin_client.session_transaction() as session: - session['send_test_letter_page_count'] = 1 session['recipient'] = '' session['placeholders'] = { 'address line 1': 'foo', @@ -2210,30 +2180,6 @@ def test_send_test_populates_field_from_session( assert page.select('input')[0]['value'] == 'Jo' -def test_send_test_caches_page_count( - logged_in_client, - mocker, - service_one, - mock_login, - mock_get_service, - mock_get_service_letter_template, - fake_uuid, -): - - mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9) - - logged_in_client.get( - url_for( - 'main.send_test', - service_id=service_one['id'], - template_id=fake_uuid, - ), - follow_redirects=True, - ) - with logged_in_client.session_transaction() as session: - assert session['send_test_letter_page_count'] == 9 - - def test_send_one_off_back_link_populates_address_textarea( client_request, mocker, @@ -2296,7 +2242,6 @@ def test_send_one_off_letter_copes_with_placeholder_from_address_block( with client_request.session_transaction() as session: session['recipient'] = None session['placeholders'] = {} - session['send_test_letter_page_count'] = None page = client_request.post( 'main.send_one_off_letter_address',