diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 639abbc1a..42b90f06d 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -20,12 +20,11 @@ from app import ( current_service, format_date_numeric, format_datetime_numeric, - job_api_client, service_api_client, template_statistics_client, ) from app.main import main -from app.statistics_utils import add_rate_to_job, get_formatted_percentage +from app.statistics_utils import get_formatted_percentage from app.utils import ( DELIVERED_STATUSES, FAILURE_STATUSES, @@ -281,14 +280,6 @@ def get_dashboard_partials(service_id): all_statistics = template_statistics_client.get_template_statistics_for_service(service_id, limit_days=7) template_statistics = aggregate_template_usage(all_statistics) - scheduled_jobs, immediate_jobs = [], [] - if job_api_client.has_jobs(service_id): - scheduled_jobs = job_api_client.get_scheduled_jobs(service_id) - immediate_jobs = [ - add_rate_to_job(job) - for job in job_api_client.get_immediate_jobs(service_id) - ] - stats = aggregate_notifications_stats(all_statistics) column_width, max_notifiction_count = get_column_properties(3) @@ -310,7 +301,6 @@ def get_dashboard_partials(service_id): return { 'upcoming': render_template( 'views/dashboard/_upcoming.html', - scheduled_jobs=scheduled_jobs ), 'inbox': render_template( 'views/dashboard/_inbox.html', @@ -337,9 +327,8 @@ def get_dashboard_partials(service_id): ), 'jobs': render_template( 'views/dashboard/_jobs.html', - jobs=immediate_jobs + jobs=current_service.immediate_jobs, ), - 'has_jobs': bool(immediate_jobs), 'usage': render_template( 'views/dashboard/_usage.html', column_width=column_width, diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index f1d3d8d0b..42f0d7818 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -19,14 +19,12 @@ from app import ( current_service, format_datetime_short, format_thousands, - job_api_client, notification_api_client, service_api_client, ) from app.main import main from app.main.forms import SearchNotificationsForm from app.models.job import Job -from app.statistics_utils import add_rate_to_job from app.utils import ( generate_next_dict, generate_notifications_csv, @@ -44,31 +42,25 @@ from app.utils import ( @main.route("/services//jobs") @user_has_permissions() def view_jobs(service_id): - page = int(request.args.get('page', 1)) - jobs_response = job_api_client.get_page_of_jobs(service_id, page=page) - jobs = [ - add_rate_to_job(job) for job in jobs_response['data'] - ] + jobs = current_service.get_page_of_jobs(page=request.args.get('page')) prev_page = None - if jobs_response['links'].get('prev', None): - prev_page = generate_previous_dict('main.view_jobs', service_id, page) + if jobs.prev_page: + prev_page = generate_previous_dict('main.view_jobs', service_id, jobs.current_page) next_page = None - if jobs_response['links'].get('next', None): - next_page = generate_next_dict('main.view_jobs', service_id, page) + if jobs.next_page: + next_page = generate_next_dict('main.view_jobs', service_id, jobs.current_page) scheduled_jobs = '' - if not current_user.has_permissions('view_activity') and page == 1: + if not current_user.has_permissions('view_activity') and jobs.current_page == 1: scheduled_jobs = render_template( 'views/dashboard/_upcoming.html', - scheduled_jobs=job_api_client.get_scheduled_jobs(service_id), hide_heading=True, ) return render_template( 'views/jobs/jobs.html', jobs=jobs, - page=page, prev_page=prev_page, next_page=next_page, scheduled_jobs=scheduled_jobs, diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 84cac95ad..109c26ad7 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -15,12 +15,7 @@ from notifications_utils.pdf import pdf_page_count from PyPDF2.utils import PdfReadError from requests import RequestException -from app import ( - current_service, - job_api_client, - notification_api_client, - service_api_client, -) +from app import current_service, notification_api_client, service_api_client from app.extensions import antivirus_client from app.main import main from app.main.forms import LetterUploadPostageForm, PDFUploadForm @@ -47,20 +42,18 @@ MAX_FILE_UPLOAD_SIZE = 2 * 1024 * 1024 # 2MB def uploads(service_id): # No tests have been written, this has been quickly prepared for user research. # It's also very like that a new view will be created to show uploads. - page = int(request.args.get('page', 1)) - uploads_response = job_api_client.get_uploads(service_id, page=page) + uploads = current_service.get_page_of_uploads(page=request.args.get('page')) prev_page = None - if uploads_response['links'].get('prev', None): - prev_page = generate_previous_dict('main.uploads', service_id, page) + if uploads.next_page: + prev_page = generate_previous_dict('main.uploads', service_id, uploads.current_page) next_page = None - if uploads_response['links'].get('next', None): - next_page = generate_next_dict('main.uploads', service_id, page) + if uploads.prev_page: + next_page = generate_next_dict('main.uploads', service_id, uploads.current_page) return render_template( 'views/jobs/jobs.html', - jobs=uploads_response['data'], - page=page, + jobs=uploads, prev_page=prev_page, next_page=next_page, scheduled_jobs='', diff --git a/app/models/job.py b/app/models/job.py index 3bee787e4..e81e31890 100644 --- a/app/models/job.py +++ b/app/models/job.py @@ -7,7 +7,7 @@ from notifications_utils.letter_timings import ( ) from werkzeug.utils import cached_property -from app.models import JSONModel +from app.models import JSONModel, ModelList from app.notify_client.job_api_client import job_api_client from app.notify_client.notification_api_client import notification_api_client from app.notify_client.service_api_client import service_api_client @@ -147,6 +147,20 @@ class Job(JSONModel): def letter_timings(self): return get_letter_timings(self.created_at, postage=self.postage) + @property + def failure_rate(self): + if not self.notifications_delivered: + return 100 if self.notifications_failed else 0 + return ( + self.notifications_failed / ( + self.notifications_failed + self.notifications_delivered + ) * 100 + ) + + @property + def high_failure_rate(self): + return self.failure_rate > 30 + def get_notifications(self, status): return notification_api_client.get_notifications_for_service( self.service, self.id, status=status, @@ -157,3 +171,31 @@ class Job(JSONModel): return job_api_client.cancel_letter_job(self.service, self.id) else: return job_api_client.cancel_job(self.service, self.id) + + +class ImmediateJobs(ModelList): + client = job_api_client.get_immediate_jobs + model = Job + + +class ScheduledJobs(ImmediateJobs): + client = job_api_client.get_scheduled_jobs + + +class PaginatedJobs(ImmediateJobs): + + client = job_api_client.get_page_of_jobs + + def __init__(self, service_id, page): + try: + self.current_page = int(page) + except TypeError: + self.current_page = 1 + response = self.client(service_id, page=self.current_page) + self.items = response['data'] + self.prev_page = response['links'].get('prev', None) + self.next_page = response['links'].get('next', None) + + +class PaginatedUploads(PaginatedJobs): + client = job_api_client.get_uploads diff --git a/app/models/service.py b/app/models/service.py index 5fdda9085..5e1901ff3 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -5,6 +5,12 @@ from notifications_utils.take import Take from werkzeug.utils import cached_property from app.models import JSONModel +from app.models.job import ( + ImmediateJobs, + PaginatedJobs, + PaginatedUploads, + ScheduledJobs, +) from app.models.organisation import Organisation from app.models.user import InvitedUsers, User, Users from app.notify_client.api_key_api_client import api_key_api_client @@ -103,10 +109,28 @@ class Service(JSONModel): def has_permission(self, permission): return permission in self.permissions + def get_page_of_jobs(self, page): + return PaginatedJobs(self.id, page=page) + + def get_page_of_uploads(self, page): + return PaginatedUploads(self.id, page=page) + @cached_property def has_jobs(self): return job_api_client.has_jobs(self.id) + @cached_property + def immediate_jobs(self): + if not self.has_jobs: + return [] + return ImmediateJobs(self.id) + + @cached_property + def scheduled_jobs(self): + if not self.has_jobs: + return [] + return ScheduledJobs(self.id) + @cached_property def invited_users(self): return InvitedUsers(self.id) diff --git a/app/statistics_utils.py b/app/statistics_utils.py index 55629d3dc..4fda30193 100644 --- a/app/statistics_utils.py +++ b/app/statistics_utils.py @@ -78,19 +78,3 @@ def statistics_by_state(statistics): 'failed': statistics['emails_failed'] } } - - -def get_failure_rate_for_job(job): - if not job.get('notifications_delivered'): - return 1 if job.get('notifications_failed') else 0 - return ( - job.get('notifications_failed', 0) / - (job.get('notifications_failed', 0) + job.get('notifications_delivered', 0)) - ) - - -def add_rate_to_job(job): - return dict( - failure_rate=(get_failure_rate_for_job(job)) * 100, - **job - ) diff --git a/app/templates/views/dashboard/_jobs.html b/app/templates/views/dashboard/_jobs.html index f959013fb..9020b926c 100644 --- a/app/templates/views/dashboard/_jobs.html +++ b/app/templates/views/dashboard/_jobs.html @@ -26,22 +26,22 @@ {% endif %} Sent {{ - (item.scheduled_for if item.scheduled_for else item.created_at)|format_datetime_relative + (item.scheduled_for or item.created_at)|format_datetime_relative }} {% endcall %} {% call field() %} {{ big_number( - item.get('notification_count', 0) - item.get('notifications_delivered', 0) - item.get('notifications_failed', 0), + item.notifications_sending, smallest=True ) }} {% endcall %} {% call field() %} - {{ big_number(item.get('notifications_delivered', 0), smallest=True) }} + {{ big_number(item.notifications_delivered, smallest=True) }} {% endcall %} - {% call field(status='error' if item.get('failure_rate', 0) > 3 else '') %} - {{ big_number(item.get('notifications_failed', 0), smallest=True) }} + {% call field(status='error' if item.high_failure_rate else '') %} + {{ big_number(item.notifications_failed, smallest=True) }} {% endcall %} {% endcall %} diff --git a/app/templates/views/dashboard/_upcoming.html b/app/templates/views/dashboard/_upcoming.html index 0a63907bb..95b0572f7 100644 --- a/app/templates/views/dashboard/_upcoming.html +++ b/app/templates/views/dashboard/_upcoming.html @@ -3,7 +3,7 @@ {% from "components/show-more.html" import show_more %}
- {% if scheduled_jobs %} + {% if current_service.scheduled_jobs %}
{% if not hide_heading %}

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

{% endif %} {% call(item, row_number) list_table( - scheduled_jobs, + current_service.scheduled_jobs, caption="In the next few days", caption_visible=False, empty_message='Nothing to see here', diff --git a/app/templates/views/dashboard/dashboard.html b/app/templates/views/dashboard/dashboard.html index 5f69b2c10..d914d0b91 100644 --- a/app/templates/views/dashboard/dashboard.html +++ b/app/templates/views/dashboard/dashboard.html @@ -35,7 +35,7 @@ {{ ajax_block(partials, updates_url, 'template-statistics', interval=5) }} - {% if partials['has_jobs'] %} + {% if current_service.immediate_jobs %} {{ ajax_block(partials, updates_url, 'jobs', interval=5) }} {{ show_more( url_for('.view_jobs', service_id=current_service.id), diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index a31f5bb89..4e76f56e6 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -1366,7 +1366,8 @@ def test_should_show_all_jobs_with_valid_statuses( mock_get_service_templates_when_no_templates_exist, mock_get_jobs, mock_get_usage, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_free_sms_fragment_limit, ): logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 796a3d8f0..f8723505a 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -518,7 +518,7 @@ def test_should_cancel_job( mock_get_service_template, mocker, ): - mock_cancel = mocker.patch('app.main.jobs.job_api_client.cancel_job') + mock_cancel = mocker.patch('app.job_api_client.cancel_job') client_request.post( 'main.cancel_job', service_id=SERVICE_ONE_ID, @@ -687,7 +687,7 @@ def test_dont_cancel_letter_job_when_to_early_to_cancel( 'app.notification_api_client.get_notification_count_for_job_id', return_value=number_of_processed_notifications ) - mock_cancel = mocker.patch('app.main.jobs.job_api_client.cancel_letter_job') + mock_cancel = mocker.patch('app.job_api_client.cancel_letter_job') page = client_request.post( 'main.cancel_letter_job', service_id=SERVICE_ONE_ID, diff --git a/tests/app/test_statistics_utils.py b/tests/app/test_statistics_utils.py index 2cd218f58..8abde1cf2 100644 --- a/tests/app/test_statistics_utils.py +++ b/tests/app/test_statistics_utils.py @@ -1,7 +1,7 @@ import pytest +from app.models.job import Job from app.statistics_utils import ( - add_rate_to_job, add_rates_to, statistics_by_state, sum_of_statistics, @@ -128,7 +128,7 @@ def test_service_statistics_by_state(): (1, 4, 20) ]) def test_add_rate_to_job_calculates_rate(failed, delivered, expected_failure_rate): - resp = add_rate_to_job( + resp = Job( { 'notifications_failed': failed, 'notifications_delivered': delivered, @@ -136,11 +136,11 @@ def test_add_rate_to_job_calculates_rate(failed, delivered, expected_failure_rat } ) - assert resp['failure_rate'] == expected_failure_rate + assert resp.failure_rate == expected_failure_rate def test_add_rate_to_job_preserves_initial_fields(): - resp = add_rate_to_job( + resp = Job( { 'notifications_failed': 0, 'notifications_delivered': 0, @@ -148,4 +148,5 @@ def test_add_rate_to_job_preserves_initial_fields(): } ) - assert set(resp.keys()) == {'notifications_failed', 'notifications_delivered', 'id', 'failure_rate'} + assert resp.notifications_failed == resp.notifications_delivered == resp.failure_rate == 0 + assert resp.id == 'foo' diff --git a/tests/conftest.py b/tests/conftest.py index 295ce6380..60fa823d2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1732,12 +1732,16 @@ def mock_get_uploads(mocker, api_user_active): 'notification_count': 10, 'created_at': '2016-01-01 11:09:00.061258', 'statistics': [{'count': 8, 'status': 'delivered'}, {'count': 2, 'status': 'temporary-failure'}], + 'scheduled_for': None, + 'job_status': 'finished', 'upload_type': 'job'}, {'id': 'job_id_1', 'original_file_name': 'some.csv', 'notification_count': 1, 'created_at': '2016-01-01 11:09:00.061258', 'statistics': [{'count': 1, 'status': 'delivered'}], + 'scheduled_for': None, + 'job_status': 'finished', 'upload_type': 'letter'} ] return { @@ -1747,8 +1751,8 @@ def mock_get_uploads(mocker, api_user_active): 'next': 'services/{}/uploads?page={}'.format(service_id, page + 1) } } - - return mocker.patch('app.job_api_client.get_uploads', side_effect=_get_uploads) + # Why is mocking on the model needed? + return mocker.patch('app.models.job.PaginatedUploads.client', side_effect=_get_uploads) @pytest.fixture(scope='function')