diff --git a/.ds.baseline b/.ds.baseline index f7f116b20..6d3423933 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -133,7 +133,7 @@ "filename": ".github/workflows/checks.yml", "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8", "is_verified": false, - "line_number": 68, + "line_number": 65, "is_secret": false }, { @@ -141,7 +141,7 @@ "filename": ".github/workflows/checks.yml", "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8", "is_verified": false, - "line_number": 103, + "line_number": 99, "is_secret": false } ], @@ -535,7 +535,7 @@ "filename": "tests/app/main/views/test_accept_invite.py", "hashed_secret": "07f0a6c13923fc3b5f0c57ffa2d29b715eb80d71", "is_verified": false, - "line_number": 626, + "line_number": 643, "is_secret": false } ], @@ -692,5 +692,5 @@ } ] }, - "generated_at": "2024-08-07T14:36:40Z" + "generated_at": "2024-08-08T19:45:34Z" } diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index efeefdbd6..3573d1a8a 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -44,8 +44,6 @@ jobs: run: poetry run isort --check-only ./app ./tests - name: Check dead code run: make dead-code - - name: Run js lint - run: npm run lint - name: Run js tests run: npm test - name: Run py tests with coverage diff --git a/app/__init__.py b/app/__init__.py index 55942769c..483d89ea0 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -18,7 +18,6 @@ from flask import ( ) from flask.globals import request_ctx from flask_login import LoginManager, current_user -from flask_socketio import SocketIO from flask_talisman import Talisman from flask_wtf import CSRFProtect from flask_wtf.csrf import CSRFError @@ -119,8 +118,6 @@ from notifications_utils.recipients import format_phone_number_human_readable login_manager = LoginManager() csrf = CSRFProtect() talisman = Talisman() -socketio = SocketIO() - # The current service attached to the request stack. current_service = LocalProxy(partial(getattr, request_ctx, "service")) @@ -177,7 +174,6 @@ def create_app(application): init_govuk_frontend(application) init_jinja(application) - socketio.init_app(application, cors_allowed_origins=['http://localhost:6012']) for client in ( csrf, diff --git a/app/assets/javascripts/activityChart.js b/app/assets/javascripts/activityChart.js index 847d46b16..5d2f89b39 100644 --- a/app/assets/javascripts/activityChart.js +++ b/app/assets/javascripts/activityChart.js @@ -185,41 +185,41 @@ return; } - var socket = io("/services"); - var eventType = type === 'service' ? 'fetch_daily_stats' : 'fetch_daily_stats_by_user'; - var socketConnect = type === 'service' ? 'daily_stats_update' : 'daily_stats_by_user_update'; + var daily_stats = activityChartContainer.getAttribute('data-daily-stats'); + var daily_stats_by_user = activityChartContainer.getAttribute('data-daily_stats_by_user'); - socket.on('connect', function () { - socket.emit(eventType); - }); + try { + // Choose the correct JSON string based on the type ('service' or 'user'), + // replace single quotes with double quotes to ensure valid JSON format, + // then parse the JSON string into a JavaScript object. + var statsJson = type === 'service' ? daily_stats : daily_stats_by_user; + statsJson = statsJson.replace(/'/g, '"'); + data = JSON.parse(statsJson); + } catch (error) { + console.error('Error parsing JSON data:', error); + return; + } + var labels = []; + var deliveredData = []; + var failedData = []; - socket.on('connect_error', function(error) { - console.error('WebSocket connection error:', error); - }); - - socket.on(socketConnect, function(data) { - - var labels = []; - var deliveredData = []; - var failedData = []; - - for (var dateString in data) { - // Parse the date string (assuming format YYYY-MM-DD) + for (var dateString in data) { + if (data.hasOwnProperty(dateString)) { const dateParts = dateString.split('-'); - const formattedDate = `${dateParts[1]}/${dateParts[2]}/${dateParts[0].slice(2)}`; // Format to MM/DD/YY + const formattedDate = `${dateParts[1]}/${dateParts[2]}/${dateParts[0].slice(2)}`; labels.push(formattedDate); deliveredData.push(data[dateString].sms.delivered); failedData.push(data[dateString].sms.failure); } + } + try { createChart('#weeklyChart', labels, deliveredData, failedData); createTable('weeklyTable', 'activityChart', labels, deliveredData, failedData); - }); - - socket.on('error', function(data) { - console.log('Error:', data); - }); + } catch (error) { + console.error('Error creating chart or table:', error); + } }; const handleDropdownChange = function(event) { diff --git a/app/assets/javascripts/sampleChartDashboard.js b/app/assets/javascripts/sampleChartDashboard.js deleted file mode 100644 index f3a363e9b..000000000 --- a/app/assets/javascripts/sampleChartDashboard.js +++ /dev/null @@ -1,70 +0,0 @@ -(function (window) { - - function initializeChartAndSocket() { - var ctx = document.getElementById('myChart'); - if (!ctx) { - return; - } - - var myBarChart = new Chart(ctx.getContext('2d'), { - type: 'bar', - data: { - labels: [], - datasets: [ - { - label: 'Delivered', - data: [], - backgroundColor: '#0076d6', - stack: 'Stack 0' - }, - ] - }, - options: { - animation: false, - scales: { - y: { - beginAtZero: true - } - } - } - }); - - var socket = io(); - - socket.on('connect', function() { - socket.emit('fetch_daily_stats_by_user'); - }); - - socket.on('daily_stats_by_user_update', function(data) { - // console.log('Data received:', data); - var labels = []; - var deliveredData = []; - var failedData = []; - - for (var date in data) { - labels.push(date); - deliveredData.push(data[date].sms.delivered); - - } - - myBarChart.data.labels = labels; - myBarChart.data.datasets[0].data = deliveredData; - myBarChart.update(); - }); - - socket.on('error', function(data) { - // console.log('Error:', data); - }); - - var sevenDaysButton = document.getElementById('sevenDaysButton'); - if (sevenDaysButton) { - sevenDaysButton.addEventListener('click', function() { - socket.emit('fetch_daily_stats_by_user'); - // console.log('clicked'); - }); - } - } - - document.addEventListener('DOMContentLoaded', initializeChartAndSocket); - -})(window); diff --git a/app/assets/javascripts/totalMessagesChart.js b/app/assets/javascripts/totalMessagesChart.js index ff409c969..a3c10d7a3 100644 --- a/app/assets/javascripts/totalMessagesChart.js +++ b/app/assets/javascripts/totalMessagesChart.js @@ -14,14 +14,14 @@ document.getElementById('message').innerText = `${sms_sent.toLocaleString()} sent / ${sms_remaining_messages.toLocaleString()} remaining`; // Calculate minimum width for "Messages Sent" as 1% of the total chart width - var minSentPercentage = 0.01; // Minimum width as a percentage of total messages (1% in this case) + var minSentPercentage = 0.02; // Minimum width as a percentage of total messages (1% in this case) var minSentValue = totalMessages * minSentPercentage; var displaySent = Math.max(sms_sent, minSentValue); var displayRemaining = totalMessages - displaySent; var svg = d3.select("#totalMessageChart"); var width = chartContainer.clientWidth; - var height = 64; + var height = 48; // Ensure the width is set correctly if (width === 0) { @@ -62,7 +62,7 @@ .attr("x", 0) // Initially set to 0, will be updated during animation .attr("y", 0) .attr("height", height) - .attr("fill", '#fa9441') + .attr("fill", '#C7CACE') .attr("width", 0) // Start with width 0 for animation .on('mouseover', function(event) { tooltip.style('display', 'block') diff --git a/app/assets/sass/uswds/_data-visualization.scss b/app/assets/sass/uswds/_data-visualization.scss index 6139933be..48c5f1256 100644 --- a/app/assets/sass/uswds/_data-visualization.scss +++ b/app/assets/sass/uswds/_data-visualization.scss @@ -2,7 +2,7 @@ $delivered: color('blue-50v'); $pending: color('green-cool-40v'); -$failed: color('orange-30v'); +$failed: color('gray-cool-20'); .chart-container { display: flex; @@ -11,6 +11,10 @@ $failed: color('orange-30v'); } } +#totalMessageChartContainer { + max-width: 600px; +} + .bar { border-radius: units(0.5); &.delivered, &.usage { diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 9c3eb6527..d5d26f0a3 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -5,7 +5,6 @@ from itertools import groupby from flask import Response, abort, jsonify, render_template, request, session, url_for from flask_login import current_user -from flask_socketio import emit from werkzeug.utils import redirect from app import ( @@ -13,7 +12,6 @@ from app import ( current_service, job_api_client, service_api_client, - socketio, template_statistics_client, ) from app.formatters import format_date_numeric, format_datetime_numeric, get_time_left @@ -32,38 +30,6 @@ from app.utils.user import user_has_permissions from notifications_utils.recipients import format_phone_number_human_readable -@socketio.on("fetch_daily_stats", namespace="/services") -def handle_fetch_daily_stats(): - service_id = session.get("service_id") - if service_id: - date_range = get_stats_date_range() - daily_stats = service_api_client.get_service_notification_statistics_by_day( - service_id, start_date=date_range["start_date"], days=date_range["days"] - ) - emit("daily_stats_update", daily_stats) - else: - emit("error", {"error": "No service_id provided"}) - - -@socketio.on("fetch_daily_stats_by_user", namespace="/services") -def handle_fetch_daily_stats_by_user(): - service_id = session.get("service_id") - user_id = session.get("user_id") - if service_id and user_id: - date_range = get_stats_date_range() - daily_stats_by_user = ( - service_api_client.get_user_service_notification_statistics_by_day( - service_id, - user_id, - start_date=date_range["start_date"], - days=date_range["days"], - ) - ) - emit("daily_stats_by_user_update", daily_stats_by_user) - else: - emit("error", {"error": "No service_id or user_id provided"}) - - @main.route("/services//dashboard") @user_has_permissions("view_activity", "send_messages") def old_service_dashboard(service_id): @@ -87,12 +53,17 @@ def service_dashboard(service_id): free_sms_allowance = billing_api_client.get_free_sms_fragment_limit_for_year( current_service.id, ) + date_range = get_stats_date_range() usage_data = get_annual_usage_breakdown(yearly_usage, free_sms_allowance) sms_sent = usage_data["sms_sent"] sms_allowance_remaining = usage_data["sms_allowance_remaining"] job_response = job_api_client.get_jobs(service_id)["data"] service_data_retention_days = 7 + daily_stats = get_daily_stats(service_id, date_range) + daily_stats_by_user = get_daily_stats_by_user( + service_id, current_user.id, date_range + ) jobs = [ { @@ -123,6 +94,23 @@ def service_dashboard(service_id): service_data_retention_days=service_data_retention_days, sms_sent=sms_sent, sms_allowance_remaining=sms_allowance_remaining, + daily_stats=daily_stats, + daily_stats_by_user=daily_stats_by_user, + ) + + +def get_daily_stats(service_id, date_range): + return service_api_client.get_service_notification_statistics_by_day( + service_id, start_date=date_range["start_date"], days=date_range["days"] + ) + + +def get_daily_stats_by_user(service_id, user_id, date_range): + return service_api_client.get_user_service_notification_statistics_by_day( + service_id, + user_id, + start_date=date_range["start_date"], + days=date_range["days"], ) diff --git a/app/notify_client/__init__.py b/app/notify_client/__init__.py index 5f65e85c6..8db3425f2 100644 --- a/app/notify_client/__init__.py +++ b/app/notify_client/__init__.py @@ -56,6 +56,9 @@ class NotifyAdminAPIClient(BaseAPIClient): ): abort(403) + def is_calling_signin_url(self, arg): + return arg.startswith("('/user") + def check_inactive_user(self, *args): still_signing_in = False @@ -64,14 +67,7 @@ class NotifyAdminAPIClient(BaseAPIClient): # and we only want to check the first arg for arg in args: arg = str(arg) - if ( - "get-login-gov-user" in arg - or "user/email" in arg - or "/activate" in arg - or "/email-code" in arg - or "/verify/code" in arg - or "/user" in arg - ): + if self.is_calling_signin_url(arg): still_signing_in = True # This seems to be a weird edge case that happens intermittently with invites diff --git a/app/templates/views/dashboard/dashboard.html b/app/templates/views/dashboard/dashboard.html index e3b68f5d4..94fd08faa 100644 --- a/app/templates/views/dashboard/dashboard.html +++ b/app/templates/views/dashboard/dashboard.html @@ -28,15 +28,11 @@
-

- What counts as 1 text message part?
- See Tracking usage. -

Activity snapshot

-
+
@@ -30,7 +30,7 @@ beforeEach(done => {
-
+
Service Name - Last 7 Days
@@ -40,14 +40,8 @@ beforeEach(done => {
- -
`; - // Re-bind event listeners in case they are lost between tests - const dropdown = document.getElementById('options'); - dropdown.addEventListener('change', window.handleDropdownChange); - // Load the D3 script dynamically loadScript(d3ScriptContent); @@ -59,11 +53,6 @@ beforeEach(done => { }, 100); }, 10000); // Increased timeout to 10 seconds -afterEach(() => { - // Clean up DOM or restore mocks if needed - jest.restoreAllMocks(); // Restores all mocks, including console logs -}); - test('D3 is loaded correctly', () => { // Check if D3 is loaded by verifying the existence of the d3 object expect(window.d3).toBeDefined(); @@ -135,129 +124,3 @@ test('Check HTML content after chart creation', () => { expect(container.querySelector('svg')).not.toBeNull(); expect(container.querySelectorAll('rect').length).toBeGreaterThan(0); }); - -// Add toHaveTextContent matcher manually -expect.extend({ - toHaveTextContent(received, expected) { - const actualTextContent = received.textContent; - const pass = actualTextContent.includes(expected); - if (pass) { - return { - message: () => - `expected element not to have text content '${expected}', but it was found`, - pass: true, - }; - } else { - return { - message: () => - `expected element to have text content '${expected}', but got '${actualTextContent}'`, - pass: false, - }; - } - }, -}); - -test('Initial dropdown state is correct', () => { - const dropdown = document.getElementById('options'); - - // Check the initial value of the dropdown - expect(dropdown.value).toBe('service'); - - // Check the initial subtitle - const subTitle = document.querySelector('.chart-subtitle'); - expect(subTitle).toHaveTextContent('Service Name - Last 7 Days'); - - // Check the initial ARIA live region text - const liveRegion = document.getElementById('aria-live-account'); - expect(liveRegion.textContent).toBe(''); // Assuming it starts empty -}); - -test('handleDropdownChange updates subtitle and fetches individual data', () => { - // Get the dropdown and set its value to 'individual' - const dropdown = document.getElementById('options'); - dropdown.value = 'individual'; - - // Create and dispatch a change event - const changeEvent = new Event('change', { bubbles: true }); - dropdown.dispatchEvent(changeEvent); - - // Check if subtitle text content is updated correctly - const subTitle = document.querySelector('.chart-subtitle'); - expect(subTitle).toHaveTextContent('User Name - Last 7 Days'); -}); - -test('handleDropdownChange shows "My Activity" table and hides "All Activity" table', () => { - // Get the dropdown and set its value to 'individual' - const dropdown = document.getElementById('options'); - dropdown.value = 'individual'; - - // Create and dispatch a change event - const changeEvent = new Event('change', { bubbles: true }); - dropdown.dispatchEvent(changeEvent); - - // Check visibility of the tables - const table1 = document.getElementById('table1'); - const table2 = document.getElementById('table2'); - - // Expect table1 to be visible and table2 to be hidden - expect(table1.classList).not.toContain('hidden'); - expect(table1.classList).toContain('visible'); - expect(table2.classList).toContain('hidden'); - expect(table2.classList).not.toContain('visible'); -}); - -test('Dropdown change event triggers handleDropdownChange with logging', () => { - // Spy on console.log - const consoleSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); - - // Get the dropdown and set its value - const dropdown = document.getElementById('options'); - dropdown.value = 'individual'; - - // Create and dispatch a change event - const changeEvent = new Event('change', { bubbles: true }); - dropdown.dispatchEvent(changeEvent); - - // Restore the console method - consoleSpy.mockRestore(); - -}); - -test('Initial fetch data populates chart and table', done => { - const mockData = { - '2024-07-01': { sms: { delivered: 50, failed: 5 } }, - '2024-07-02': { sms: { delivered: 60, failed: 2 } }, - '2024-07-03': { sms: { delivered: 70, failed: 1 } }, - '2024-07-04': { sms: { delivered: 80, failed: 0 } }, - '2024-07-05': { sms: { delivered: 90, failed: 3 } }, - '2024-07-06': { sms: { delivered: 100, failed: 4 } }, - '2024-07-07': { sms: { delivered: 110, failed: 2 } }, - }; - - const socket = { - on: jest.fn((event, callback) => { - if (event === 'daily_stats_update') { - callback(mockData); - done(); - } - }), - emit: jest.fn(), - }; - window.io = jest.fn(() => socket); - - document.dispatchEvent(new Event('DOMContentLoaded')); - - setTimeout(() => { - const table = document.getElementById('weeklyTable'); - expect(table).toBeDefined(); - - const rows = table.getElementsByTagName('tr'); - expect(rows.length).toBe(8); - - const firstRowCells = rows[1].getElementsByTagName('td'); - console.log('First row cells:', firstRowCells); - expect(firstRowCells[0].textContent).toBe('07/01/24'); - expect(firstRowCells[1].textContent).toBe('50'); - expect(firstRowCells[2].textContent).toBe('5'); - }, 100); -}); diff --git a/tests/javascripts/totalMessagesChart.test.js b/tests/javascripts/totalMessagesChart.test.js index 22bc39500..87c671430 100644 --- a/tests/javascripts/totalMessagesChart.test.js +++ b/tests/javascripts/totalMessagesChart.test.js @@ -57,7 +57,7 @@ test('SVG element is correctly set up', done => { setTimeout(() => { const svg = document.getElementById('totalMessageChart'); expect(svg.getAttribute('width')).toBe('600'); - expect(svg.getAttribute('height')).toBe('64'); + expect(svg.getAttribute('height')).toBe('48'); done(); }, 1000); // Ensure enough time for the DOM updates }); @@ -159,7 +159,7 @@ test('SVG bars are created and animated correctly', done => { // Initial check const sentBar = svg.querySelector('rect[fill="#0076d6"]'); - const remainingBar = svg.querySelector('rect[fill="#fa9441"]'); + const remainingBar = svg.querySelector('rect[fill="#C7CACE"]'); expect(sentBar).not.toBeNull(); expect(remainingBar).not.toBeNull();