65031-allow-schools-to-view-register-post-check (#2879)

* Remove route guard - teachers can now view after the check period in the admin and read only phases. * fix: upgrade lz-string from 1.4.4 to 1.5.0 Snyk has created this PR to upgrade lz-string from 1.4.4 to 1.5.0. See this package in yarn: lz-string See this project in Snyk: https://app.snyk.io/org/activemq/project/5124a9a7-a78c-4908-9a94-cfcecee0ec46?utm_source=github&utm_medium=referral&page=upgrade-pr * update assets version * dep update * Narrow the skipping to one failing test * Upgrade appinsights * Upgrade applicationinsights to v3.2.2 which required a new config env var APPLICATIONINSIGHTS_CONNECTION_STRING * test updates --------- Co-authored-by: snyk-bot <[email protected]> Co-authored-by: Mohsen Qureshi <[email protected]>
DFEAGILEDEVOPS · Sep 9, 2024 · 341be46 · 341be46
1 parent c28978d
commit 341be46
Show file tree

Hide file tree

Showing 14 changed files with 724 additions and 119 deletions.
diff --git a/admin/config.js b/admin/config.js
@@ -127,6 +127,7 @@ module.exports = {
       Key: process.env.APPINSIGHTS_INSTRUMENTATIONKEY,
       CollectDependencies: {}.hasOwnProperty.call(process.env, 'APPINSIGHTS_COLLECT_DEPS') ? toBool(process.env.APPINSIGHTS_COLLECT_DEPS) : true,
       CollectExceptions: {}.hasOwnProperty.call(process.env, 'APPINSIGHTS_COLLECT_EXCEPTIONS') ? toBool(process.env.APPINSIGHTS_COLLECT_EXCEPTIONS) : true,
+      ConnectionString: process.env.APPLICATIONINSIGHTS_CONNECTION_STRING,
       LiveMetrics: {}.hasOwnProperty.call(process.env, 'APPINSIGHTS_LIVE_METRICS') ? toBool(process.env.APPINSIGHTS_LIVE_METRICS) : true,
       InstanceId: `${os.hostname()}:${process.pid}`
     }

diff --git a/admin/helpers/app-insights.js b/admin/helpers/app-insights.js
@@ -2,35 +2,39 @@
 
 const appInsights = require('applicationinsights')
 const config = require('../config')
-const {
-  getBuildNumber
-} = require('./healthcheck')
+// const {
+//   getBuildNumber
+// } = require('./healthcheck')
 
 const startInsightsIfConfigured = async () => {
-  if (config.Monitoring.ApplicationInsights.Key) {
+  if (config.Monitoring.ApplicationInsights.ConnectionString) {
     console.log('initialising application insights module')
     appInsights.setup()
-      .setAutoDependencyCorrelation(true)
       .setAutoCollectRequests(true)
-      .setAutoCollectPerformance(true)
+      // setAutoCollectPerformance() - for some reason this next call causes a configuration warning 'Extended metrics are no longer supported. ...'
+      .setAutoCollectPerformance(true, true)
       .setAutoCollectExceptions(config.Monitoring.ApplicationInsights.CollectExceptions)
       .setAutoCollectDependencies(config.Monitoring.ApplicationInsights.CollectDependencies)
       .setAutoCollectConsole(false)
-      .setUseDiskRetryCaching(true)
+      .setAutoCollectPreAggregatedMetrics(true)
       .setSendLiveMetrics(config.Monitoring.ApplicationInsights.LiveMetrics)
+      .setInternalLogging(false, true)
+      .enableWebInstrumentation(false)
       .start()
 
-    let buildNumber
-    try {
-      buildNumber = await getBuildNumber()
-    } catch (error) {
-      buildNumber = 'NOT FOUND'
-    }
-    appInsights.defaultClient.commonProperties = {
-      buildNumber
-    }
-    appInsights.defaultClient.context.tags[appInsights.defaultClient.context.keys.cloudRole] = 'Admin-App'
-    appInsights.defaultClient.context.tags[appInsights.defaultClient.context.keys.cloudRoleInstance] = config.Monitoring.ApplicationInsights.InstanceId
+    // Commented out in ticket #65031 as part of maintenance upgrades
+    // V3 of the appinsights sdk does not support context/commonProperties
+    //   let buildNumber
+    //   try {
+    //     buildNumber = await getBuildNumber()
+    //   } catch (error) {
+    //     buildNumber = 'NOT FOUND'
+    //   }
+    //   appInsights.defaultClient.commonProperties = {
+    //     buildNumber
+    //   }
+    //   appInsights.defaultClient.context.tags[appInsights.defaultClient.context.keys.cloudRole] = 'Admin-App'
+    //   appInsights.defaultClient.context.tags[appInsights.defaultClient.context.keys.cloudRoleInstance] = config.Monitoring.ApplicationInsights.InstanceId
   }
 }
 

diff --git a/admin/package.json b/admin/package.json
@@ -45,7 +45,7 @@
     "@azure/storage-blob": "12.18.0",
     "@azure/storage-queue": "12.17.0",
     "adm-zip": "^0.5.10",
-    "applicationinsights": "^2.7.0",
+    "applicationinsights": "^3.2.2",
     "axios": "^1.7.4",
     "bcryptjs": "^2.4.3",
     "bluebird": "^3.5.1",

diff --git a/admin/routes/pupil-register.js b/admin/routes/pupil-register.js
@@ -12,7 +12,6 @@ const pupilController = require('../controllers/pupil')
 router.get(
   ['/', '/pupils-list'],
   isAuthenticated([roles.teacher, roles.helpdesk, roles.staAdmin]),
-  ifNotRole(roles.staAdmin, isPostLiveOrLaterCheckPhase),
   pupilRegister.listPupils
 )
 router.get(

diff --git a/admin/yarn.lock b/admin/yarn.lock
diff --git a/docs/environment-variables.md b/docs/environment-variables.md
@@ -23,7 +23,8 @@ Env Var | Type | Default value | Required | Components | Description
  APPINSIGHTS_COLLECT_EXCEPTIONS | Boolean | true | Optional | AA | Set to false if you do not want App Insights to collect exception information.
  APPINSIGHTS_INSTRUMENTATIONKEY | String | NULL | Optional | AA,PA | The Azure Application Insights account key, required for logging / monitoring.
  APPINSIGHTS_LIVE_METRICS | Boolean | true | Optional | AA | Set to false if you do not want App Insights to collection live metrics.
- APPINSIGHTS_WINSTON_LOGGER | Boolean | false | Optional | AA | Boolean flag.  Set to 1 to allow the winston logger to send to Application Insights, or 0 to disable. If enabled, you should also set `EXPRESS_LOGGING_WINSTON`to `1` and `APPINSIGHTS_INSTRUMENTATIONKEY`to  the Azure-specified value.
+ APPINSIGHTS_WINSTON_LOGGER | Boolean | false | Optional | AA | Boolean flag.  Set to 1 to allow the winston logger to send to Application Insights, or 0 to disable. If enabled, you should also set `EXPRESS_LOGGING_WINSTON`to `1` and `APPINSIGHTS_INSTRUMENTATIONKEY` to the Azure-specified value.
+ APPLICATIONINSIGHTS_CONNECTION_STRING | String | '' | Optional, but should be set for Prod | AA (Others to follow in v3 rollout) | Connection string for v3 of Application Insights node package.  This env var name is automatically supported by AI.
  ASSET_PATH | String | / | Required | AA | The URL to the admin container that serves static assets: css, js, images.
  AUTH_MODE | String | local | Optional | AA | Valid modes are `local` and `dfe`.
  AZURE_QUEUE_PREFIX | String | '' | Optional | AA | A prefix that can be automatically applied to the required queues.

diff --git a/example.env.txt b/example.env.txt
@@ -16,6 +16,15 @@ REMOTE_IP_CHECK_URL='https://ifconfig.me/ip'
 #####################
 # Azure Settings
 #####################
+# For Application Insights v2 - this will be deprecated from March 31st, 2025
+# https://github.com/microsoft/ApplicationInsights-node.js
+# APPINSIGHTS_INSTRUMENTATIONKEY='31b74aeb-7f5a-4cd5-bc7b-b038d2f64f73'
+#
+# For AppInsights v3 we need to use connection strings:
+APPLICATIONINSIGHTS_CONNECTION_STRING="InstrumentationKey=31b74aeb-7f5a-4cd5-bc7b-b038d2f64f73;IngestionEndpoint=https://westeurope-5.in.applicationinsights.azure.com/;LiveEndpoint=https://westeurope.livediagnostics.monitor.azure.com/;ApplicationId=de751a51-81c7-4898-8079-6a1377478c17"
+#
+# SERVICE BUS
+#
 AZURE_SERVICE_BUS_CONNECTION_STRING=foo
 AZURE_STORAGE_CONNECTION_STRING=bar
 # Local service bus settings don't need production scale.

diff --git a/load-test/check-submit-proxy/package.json b/load-test/check-submit-proxy/package.json
@@ -8,6 +8,6 @@
   },
   "author": "",
   "dependencies": {
-    "lz-string": "^1.4.4"
+    "lz-string": "^1.5.0"
   }
 }
diff --git a/load-test/check-submit-proxy/yarn.lock b/load-test/check-submit-proxy/yarn.lock
@@ -2,7 +2,7 @@
 # yarn lockfile v1
 
 
-lz-string@^1.4.4:
-  version "1.4.4"
-  resolved "https://registry.yarnpkg.com/lz-string/-/lz-string-1.4.4.tgz#c0d8eaf36059f705796e1e344811cf4c498d3a26"
-  integrity sha1-wNjq82BZ9wV5bh40SBHPTEmNOiY=
+lz-string@^1.5.0:
+  version "1.5.0"
+  resolved "https://registry.yarnpkg.com/lz-string/-/lz-string-1.5.0.tgz#c1ab50f77887b712621201ba9fd4e3a6ed099941"
+  integrity sha512-h5bgJWpxJNswbU7qCrV0tIKQCaS3blPDrqKWx+QxzuzL1zGUzij9XCWLrSLsJPu5t+eWA/ycetzYAO5IOMcWAQ==
diff --git a/test/admin-hpa/features/step_definitions/post_check_window_steps.rb b/test/admin-hpa/features/step_definitions/post_check_window_steps.rb
@@ -19,8 +19,8 @@
 Then(/^teachers can only have read only access$/) do
   step 'I am logged in'
   pupil_register_page.load
-  expect(pupil_register_page).to have_no_add_pupil
-  expect(pupil_register_page).to have_no_add_multiple_pupil
+  expect(pupil_register_page).to have_add_pupil_disabled
+  expect(pupil_register_page).to have_add_multiple_pupil_disabled
   pupils_not_taking_check_page.load
   find(".govuk-button--disabled", text: 'Select pupils and reason')
   access_arrangements_page.load

diff --git a/test/admin-hpa/features/step_definitions/sta_admin_add_pupils_steps.rb b/test/admin-hpa/features/step_definitions/sta_admin_add_pupils_steps.rb
@@ -12,5 +12,7 @@
 
 Then(/^I should not be able to add a pupil$/) do
   school_landing_page.pupil_register.click
-  expect(find('h1').text).to eql 'Section unavailable'
+  expect(find('h1').text).to eql "View, add or edit pupils on your school's register"
+  expect(pupil_register_page).to have_add_pupil_disabled
+  expect(pupil_register_page).to have_add_multiple_pupil_disabled
 end
diff --git a/test/admin-hpa/features/support/page_objects/pupil_register_page.rb b/test/admin-hpa/features/support/page_objects/pupil_register_page.rb
@@ -3,7 +3,9 @@ class PupilRegisterPage < SitePrism::Page
 
   element :heading, '.govuk-heading-xl', text: "View, add or edit pupils on your school's register"
   element :add_pupil, 'a', text: 'Add pupil'
+  element :add_pupil_disabled, 'a[aria-disabled="true"]', text: "Add pupil"
   element :add_multiple_pupil, 'a', text: 'Add multiple pupils'
+  element :add_multiple_pupil_disabled, 'a[aria-disabled="true"]', text: 'Add multiple pupils'
   element :info_message, '.govuk-info-message', text: 'Changes to pupil details have been saved'
   element :new_pupil_info_message, '.govuk-info-message', text: '1 new pupil has been added'
   element :add_multiple_pupil_info_message, '.govuk-info-message'

diff --git a/test/se_manager/.cache/selenium/se-metadata.json b/test/se_manager/.cache/selenium/se-metadata.json
@@ -4,7 +4,7 @@
       "browser_name": "chrome",
       "major_browser_version": "127",
       "browser_version": "127.0.6533.119",
-      "browser_ttl": 1725372632
+      "browser_ttl": 1725618358
     }
   ],
   "drivers": [],
@@ -16,7 +16,7 @@
       "arch": "x86_64",
       "lang": "ruby",
       "selenium_version": "4.23",
-      "stats_ttl": 1725372623
+      "stats_ttl": 1725618346
     }
   ]
 }
diff --git a/tslib/src/functions-ps-report/ps-report-2-pupil-data/ps-report.service.spec.ts b/tslib/src/functions-ps-report/ps-report-2-pupil-data/ps-report.service.spec.ts
@@ -4,7 +4,7 @@ import { type IPsReportDataService } from './ps-report.data.service'
 import { type IOutputBinding } from '.'
 import { type PsReportSchoolFanOutMessage } from '../common/ps-report-service-bus-messages'
 
-describe.skip('PsReportService', () => {
+describe('PsReportService', () => {
   let sut: PsReportService
   let logger: ILogger
   let psReportDataService: IPsReportDataService
@@ -62,7 +62,7 @@ describe.skip('PsReportService', () => {
     expect(psReportDataService.getPupilData).toHaveBeenCalledTimes(3)
   })
 
-  test('it outputs the results from getPupilData() once per pupil onto the outputBinding', async () => {
+  test.skip('it outputs the results from getPupilData() once per pupil onto the outputBinding', async () => {
     (psReportDataService.getPupils as jest.Mock).mockResolvedValueOnce([{ id: 1 }, { id: 2 }, { id: 3 }])
     ;(psReportDataService.getSchool as jest.Mock).mockResolvedValueOnce(mockSchool)
     ;(psReportDataService.getPupilData as jest.Mock)