From 6d5f542a88e3562fa409c41044309bbbc14729e8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 8 Jul 2019 12:00:40 +0100 Subject: [PATCH 1/3] Count of orgs and live services for platform admin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes it consistent that an option which contains more options has a hint about how many options it contains. Also adds a formatter to get us ready for 1,000 services 🎉 --- app/__init__.py | 5 +++++ app/main/views/choose_account.py | 10 ++++++++++ app/templates/views/choose-account.html | 3 +++ app/templates/views/signedout.html | 4 ++-- tests/app/main/views/accounts/test_choose_accounts.py | 7 ++++++- tests/app/main/views/test_index.py | 2 +- tests/app/main/views/test_service_settings.py | 2 ++ tests/conftest.py | 10 +++++++++- 8 files changed, 38 insertions(+), 5 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 527495702..43c93f90f 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -345,6 +345,10 @@ def format_delta(date): ) +def format_thousands(number): + return "{:,.0f}".format(number) + + def valid_phone_number(phone_number): try: validate_phone_number(phone_number) @@ -688,6 +692,7 @@ def add_template_filters(application): formatted_list, nl2br, format_phone_number_human_readable, + format_thousands, id_safe, ]: application.add_template_filter(fn) diff --git a/app/main/views/choose_account.py b/app/main/views/choose_account.py index febecaeea..3626a66e6 100644 --- a/app/main/views/choose_account.py +++ b/app/main/views/choose_account.py @@ -1,7 +1,9 @@ from flask import redirect, render_template, session, url_for from flask_login import current_user +from app import status_api_client from app.main import main +from app.models.organisation import Organisations from app.utils import PermanentRedirect, user_is_logged_in @@ -18,9 +20,17 @@ def services_or_dashboard(): @main.route("/accounts") @user_is_logged_in def choose_account(): + org_count, live_service_count = None, None + if current_user.platform_admin: + org_count, live_service_count = ( + len(Organisations()), + status_api_client.get_count_of_live_services_and_organisations()['services'], + ) return render_template( 'views/choose-account.html', can_add_service=current_user.is_gov_user, + org_count=org_count, + live_service_count=live_service_count, ) diff --git a/app/templates/views/choose-account.html b/app/templates/views/choose-account.html index 9457f520e..62924ed19 100644 --- a/app/templates/views/choose-account.html +++ b/app/templates/views/choose-account.html @@ -69,6 +69,9 @@ diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index 14b5d545e..a3de3624d 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -120,12 +120,12 @@
-
{{ counts.services }}
+
{{ counts.services|format_thousands }}
services
-
{{ counts.organisations }}
+
{{ counts.organisations|format_thousands }}
organisations
diff --git a/tests/app/main/views/accounts/test_choose_accounts.py b/tests/app/main/views/accounts/test_choose_accounts.py index 4ecd2d559..bc3c3b69b 100644 --- a/tests/app/main/views/accounts/test_choose_accounts.py +++ b/tests/app/main/views/accounts/test_choose_accounts.py @@ -157,16 +157,21 @@ def test_choose_account_should_show_choose_accounts_page_if_no_services( def test_choose_account_should_should_organisations_link_for_platform_admin( client_request, platform_admin_user, + mock_get_organisations, mock_get_orgs_and_services, mock_get_organisation_services, + mock_get_service_and_organisation_counts, ): client_request.login(platform_admin_user) page = client_request.get('main.choose_account') - first_link = page.select_one('.browse-list-item a') + first_item = page.select_one('.browse-list-item') + first_link = first_item.select_one('a') + first_hint = first_item.select_one('.browse-list-hint') assert first_link.text == 'All organisations' assert first_link['href'] == url_for('main.organisations') + assert normalize_spaces(first_hint.text) == '3 organisations, 9,999 live services' def test_choose_account_should_show_back_to_service_link( diff --git a/tests/app/main/views/test_index.py b/tests/app/main/views/test_index.py index e668f7068..b49204057 100644 --- a/tests/app/main/views/test_index.py +++ b/tests/app/main/views/test_index.py @@ -34,7 +34,7 @@ def test_non_logged_in_user_can_see_homepage( assert normalize_spaces(page.select_one('#whos-using-notify').text) == ( 'Who’s using GOV.UK Notify ' 'Services ' - '9999 services ' + '9,999 services ' 'Organisations ' '111 organisations ' 'See the list of services and organisations.' diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 917e4d447..87d56542a 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -3647,6 +3647,8 @@ def test_service_switch_can_upload_document_lets_contact_details_be_added_and_sh def test_archive_service_after_confirm( client_request, mocker, + mock_get_organisations, + mock_get_service_and_organisation_counts, mock_get_organisations_and_services_for_user, user, fake_uuid, diff --git a/tests/conftest.py b/tests/conftest.py index 39d56b010..15a92a572 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3121,7 +3121,15 @@ def mock_get_organisations(mocker): organisation_json('7aa5d4e9-4385-4488-a489-07812ba13385', 'Org 3'), ] - return mocker.patch('app.organisations_client.get_organisations', side_effect=_get_organisations) + mocker.patch( + 'app.models.organisation.Organisations.client', + side_effect=_get_organisations, + ) + + return mocker.patch( + 'app.notify_client.organisations_api_client.organisations_client.get_organisations', + side_effect=_get_organisations, + ) @pytest.fixture(scope='function') From 959dd6ac388f0d93a94f1b02877eac395b1ada46 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 8 Jul 2019 14:09:29 +0100 Subject: [PATCH 2/3] Make one method for comma-formatting numbers We were doing this a few different ways in different places. --- app/__init__.py | 9 +++++++-- app/main/forms.py | 7 ++++--- app/main/views/jobs.py | 5 +++-- app/main/views/service_settings.py | 11 ++++------- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 43c93f90f..2ff39d8e4 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -3,6 +3,7 @@ import os import urllib from datetime import datetime, timedelta, timezone from functools import partial +from numbers import Number from time import monotonic import ago @@ -345,8 +346,12 @@ def format_delta(date): ) -def format_thousands(number): - return "{:,.0f}".format(number) +def format_thousands(value): + if isinstance(value, Number): + return '{:,.0f}'.format(value) + if value is None: + return '' + return value def valid_phone_number(phone_number): diff --git a/app/main/forms.py b/app/main/forms.py index 6ab4e1035..2017b6433 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -34,6 +34,7 @@ from wtforms.fields.html5 import EmailField, SearchField, TelField from wtforms.validators import URL, DataRequired, Length, Optional, Regexp from wtforms.widgets import CheckboxInput, ListWidget +from app import format_thousands from app.main.validators import ( Blacklist, CsvFileValidator, @@ -209,9 +210,9 @@ class ForgivingIntegerField(StringField): error = None try: if int(self.data) > self.POSTGRES_MAX_INT: - error = 'Number of {} must be {:,.0f} or less'.format( + error = 'Number of {} must be {} or less'.format( self.things, - self.POSTGRES_MAX_INT, + format_thousands(self.POSTGRES_MAX_INT), ) except ValueError: error = 'Enter the number of {} {}'.format( @@ -234,7 +235,7 @@ class ForgivingIntegerField(StringField): try: value = int(self.data) - value = '{:,.0f}'.format(value) + value = format_thousands(value) except (ValueError, TypeError): value = self.data if self.data is not None else '' diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index acb1a4346..8d1bc5b48 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -25,6 +25,7 @@ from notifications_utils.template import Template, WithSubjectTemplate from app import ( current_service, format_datetime_short, + format_thousands, job_api_client, notification_api_client, service_api_client, @@ -186,8 +187,8 @@ def cancel_letter_job(service_id, job_id): except HTTPError as e: flash(e.message, 'dangerous') return redirect(url_for('main.view_job', service_id=service_id, job_id=job_id)) - flash("Cancelled {:,.0f} letters from {}".format( - number_of_letters, job['original_file_name'] + flash("Cancelled {} letters from {}".format( + format_thousands(number_of_letters), job['original_file_name'] ), 'default_with_tick') return redirect(url_for('main.service_dashboard', service_id=service_id)) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 9a50c48df..46db6b1c9 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -19,6 +19,7 @@ from app import ( billing_api_client, current_service, email_branding_client, + format_thousands, inbound_number_client, letter_branding_client, notification_api_client, @@ -212,9 +213,9 @@ def submit_request_to_go_live(service_id): service_dashboard=url_for('main.service_dashboard', service_id=current_service.id, _external=True), organisation_type=str(current_service.organisation_type).title(), agreement=current_service.organisation.as_human_readable(current_user.email_domain), - volume_email_formatted=format_if_number(current_service.volume_email), - volume_sms_formatted=format_if_number(current_service.volume_sms), - volume_letter_formatted=format_if_number(current_service.volume_letter), + volume_email_formatted=format_thousands(current_service.volume_email), + volume_sms_formatted=format_thousands(current_service.volume_sms), + volume_letter_formatted=format_thousands(current_service.volume_letter), research_consent='Yes' if current_service.consent_to_research else 'No', existing_live='Yes' if current_user.live_services else 'No', email_address=current_user.email_address, @@ -1121,7 +1122,3 @@ def check_contact_details_type(contact_details): return 'email_address' else: return 'phone_number' - - -def format_if_number(value): - return '{:,.0f}'.format(value) if isinstance(value, int) else '' From c11a43cbc4ff19507fe043b89cece19a88308d42 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 8 Jul 2019 14:32:18 +0100 Subject: [PATCH 3/3] Update live services count when service is counted If we change our mind and decide whether a service should/should not be counted in the list of live services then we should also drop the cache which stores the count of how many live services there are. --- app/main/views/service_settings.py | 2 +- app/models/service.py | 3 +++ app/notify_client/service_api_client.py | 7 +++++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 46db6b1c9..807b17147 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -263,7 +263,7 @@ def service_switch_count_as_live(service_id): ) if form.validate_on_submit(): - current_service.update(count_as_live=form.enabled.data) + current_service.update_count_as_live(form.enabled.data) return redirect(url_for('.service_settings', service_id=service_id)) return render_template( diff --git a/app/models/service.py b/app/models/service.py index 3a45f6a17..7b09ca5e5 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -65,6 +65,9 @@ class Service(JSONModel): def update(self, **kwargs): return service_api_client.update_service(self.id, **kwargs) + def update_count_as_live(self, count_as_live): + return service_api_client.update_count_as_live(self.id, count_as_live=count_as_live) + def update_status(self, live): return service_api_client.update_status(self.id, live=live) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index aa6086c6a..8adbee048 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -114,6 +114,13 @@ class ServiceAPIClient(NotifyAdminAPIClient): go_live_at=str(datetime.utcnow()) if live else None ) + @cache.delete('live-service-and-organisation-counts') + def update_count_as_live(self, service_id, count_as_live): + return self.update_service( + service_id, + count_as_live=count_as_live, + ) + # This method is not cached because it calls through to one which is def update_service_with_properties(self, service_id, properties): return self.update_service(service_id, **properties)