code review feedback

.ds.baseline

@@ -133,7 +133,7 @@
         "filename": ".github/workflows/checks.yml",
         "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8",
         "is_verified": false,
-        "line_number": 67,
+        "line_number": 68,
         "is_secret": false
       },
       {
@@ -141,7 +141,7 @@
         "filename": ".github/workflows/checks.yml",
         "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8",
         "is_verified": false,
-        "line_number": 101,
+        "line_number": 103,
         "is_secret": false
       }
     ],
@@ -692,5 +692,5 @@
       }
     ]
   },
-  "generated_at": "2024-07-24T14:13:02Z"
+  "generated_at": "2024-08-07T14:36:40Z"
 }

.github/workflows/checks.yml

@@ -54,6 +54,7 @@ jobs:
         run: poetry run coverage report --fail-under=90
 
   end-to-end-tests:
+    if: ${{ github.actor != 'dependabot[bot]' }}
     permissions:
       checks: write
       pull-requests: write
@@ -84,6 +85,7 @@ jobs:
         ports:
           # Maps tcp port 6379 on service container to the host
           - 6379:6379
+
     steps:
       - uses: actions/checkout@v4
       - uses: ./.github/actions/setup-project

Makefile

@@ -61,7 +61,7 @@ py-lint: ## Run python linting scanners and black
 	poetry self add poetry-dotenv-plugin
 	poetry run black .
 	poetry run flake8 .
-	poetry run isort --check-only ./app ./tests
+	poetry run isort ./app ./tests
 
 .PHONY: avg-complexity
 avg-complexity:

app/__init__.py

@@ -177,7 +177,7 @@ def create_app(application):
 
     init_govuk_frontend(application)
     init_jinja(application)
-    socketio.init_app(application)
+    socketio.init_app(application, cors_allowed_origins=["http://localhost:6012"])
 
     for client in (
         csrf,

app/assets/javascripts/activityChart.js

@@ -185,7 +185,7 @@
             return;
         }
 
-        var socket = io();
+        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';
 
@@ -193,6 +193,10 @@
             socket.emit(eventType);
         });
 
+        socket.on('connect_error', function(error) {
+            console.error('WebSocket connection error:', error);
+        });
+
         socket.on(socketConnect, function(data) {
 
             var labels = [];

app/main/views/dashboard.py

@@ -32,7 +32,7 @@ from app.utils.user import user_has_permissions
 from notifications_utils.recipients import format_phone_number_human_readable
 
 
-@socketio.on("fetch_daily_stats")
+@socketio.on("fetch_daily_stats", namespace="/services")
 def handle_fetch_daily_stats():
     service_id = session.get("service_id")
     if service_id:
@@ -45,7 +45,7 @@ def handle_fetch_daily_stats():
         emit("error", {"error": "No service_id provided"})
 
 
-@socketio.on("fetch_daily_stats_by_user")
+@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")

docs/debug-issues-with-staging-api.md

@@ -0,0 +1,26 @@
+### Setting Up Environment Variables for Local Development to the Staging API
+
+When you’re working locally, you can point your local admin repo to the staging API and use that to help debug
+issues with the staging data set.  To do this, you’ll need to modify your .env file for the admin project and
+include the following new environment variables:
+
+- `ADMIN_CLIENT_SECRET`
+- `ADMIN_CLIENT_USERNAME`
+- `DANGEROUS_SALT`
+- `SECRET_KEY`
+
+Additionally, update `API_HOST_NAME` and `NOTIFY_ENVIRONMENT`:
+
+1. Change `API_HOST_NAME` to `API_HOST_NAME=https://notify-api-staging.app.cloud.gov`
+2. Change `NOTIFY_ENVIRONMENT` to `NOTIFY_ENVIRONMENT=staging`
+
+### Retrieving Environment Variables for Staging
+
+You can retrieve the values needed for these by using the `cf` CLI (Cloud Foundry CLI tool) and making sure
+you’re targeting the `notify-staging` space.
+
+1. `cf login -a [api.fr.cloud.gov](http://api.fr.cloud.gov/) --sso`
+2. select `notify-staging`
+3. `cf env notify-admin-staging`
+
+By pointing your local environment to staging, it should mirror what's in staging.

tests/javascripts/activityChart.test.js

@@ -124,3 +124,43 @@ test('Check HTML content after chart creation', () => {
   expect(container.querySelector('svg')).not.toBeNull();
   expect(container.querySelectorAll('rect').length).toBeGreaterThan(0);
 });
+
+
+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);
+});

tests/javascripts/totalMessagesChart.test.js

@@ -15,10 +15,11 @@ function loadScript(scriptContent) {
 Object.defineProperty(HTMLElement.prototype, 'clientWidth', {
   value: 600,
   writable: true,
+  configurable: true,
 });
 
 // beforeAll hook to set up the DOM and load D3.js script
-beforeAll(done => {
+beforeEach(() => {
   // Set up the DOM with the D3 script included
   document.body.innerHTML = `
     <div id="totalMessageChartContainer" data-sms-sent="100" data-sms-allowance-remaining="249900" style="width: 600px;">
@@ -33,15 +34,13 @@ beforeAll(done => {
   loadScript(d3ScriptContent);
 
   // Wait a bit to ensure the script is executed
-  setTimeout(() => {
-    // Require the actual JavaScript file you are testing
-    require('../../app/assets/javascripts/totalMessagesChart.js');
-
-    // Call the function to create the chart
-    window.createTotalMessagesChart();
-
-    done();
-  }, 100);
+  return new Promise(resolve => {
+    setTimeout(() => {
+      // Require the actual JavaScript file you are testing
+      require('../../app/assets/javascripts/totalMessagesChart.js');
+      resolve();
+    }, 100);
+  });
 });
 
 // Single test to check if D3 is loaded correctly
@@ -52,15 +51,20 @@ test('D3 is loaded correctly', () => {
 });
 
 // Test to check if the SVG element is correctly set up
-test('SVG element is correctly set up', () => {
-  const svg = document.getElementById('totalMessageChart');
-  expect(svg).not.toBeNull();
-  expect(svg.getAttribute('width')).toBe('600');
-  expect(svg.getAttribute('height')).toBe('64');
+test('SVG element is correctly set up', done => {
+  window.createTotalMessagesChart();
+
+  setTimeout(() => {
+    const svg = document.getElementById('totalMessageChart');
+    expect(svg.getAttribute('width')).toBe('600');
+    expect(svg.getAttribute('height')).toBe('64');
+    done();
+  }, 1000); // Ensure enough time for the DOM updates
 });
 
 // Test to check if the table is created and populated correctly
 test('Populates the accessible table correctly', () => {
+  window.createTotalMessagesChart();
   const table = document.getElementById('totalMessageTable').getElementsByTagName('table')[0];
   expect(table).toBeDefined();
 
@@ -84,6 +88,8 @@ test('Chart title is correctly set', () => {
 
 // Test to check if the chart resizes correctly on window resize
 test('Chart resizes correctly on window resize', done => {
+  window.createTotalMessagesChart();
+
   setTimeout(() => {
     const svg = document.getElementById('totalMessageChart');
     const chartContainer = document.getElementById('totalMessageChartContainer');
@@ -92,7 +98,7 @@ test('Chart resizes correctly on window resize', done => {
     expect(svg.getAttribute('width')).toBe('600');
 
     // Set new container width
-    Object.defineProperty(chartContainer, 'clientWidth', { value: 800 });
+    Object.defineProperty(chartContainer, 'clientWidth', { value: 800, configurable: true });
 
     // Trigger resize event
     window.dispatchEvent(new Event('resize'));
@@ -101,9 +107,9 @@ test('Chart resizes correctly on window resize', done => {
       // Check if SVG width is updated
       expect(svg.getAttribute('width')).toBe('800');
       done();
-    }, 500); // Adjust the timeout if necessary
+    }, 1000); // Adjust the timeout if necessary
   }, 1000); // Initial wait for the chart to render
-}, 10000); // Adjust the overall test timeout if necessary
+}, 15000); // Adjust the overall test timeout if necessary
 
 // Testing the tooltip
 test('Tooltip displays on hover', () => {
@@ -148,6 +154,7 @@ test('Tooltip displays on hover', () => {
 
 // Test to ensure SVG bars are created and animated correctly
 test('SVG bars are created and animated correctly', done => {
+  window.createTotalMessagesChart();
   const svg = document.getElementById('totalMessageChart');
 
   // Initial check