Feature/60290 pupil feedback persistence move to sql from table stora…

…ge (#3008) * build out skeleton controller * feedback controller implementation complete * controller, service and queue definitions complete * align tests with removal of pupil feedback storage queue * feedback function and tests * create migrations for pupil feedback table * finish off feedback service * fix issues with sql and types, update validation * implement pupil feedback submission * lint fix * updates to pupil feedback tests * lint fixes --------- Co-authored-by: Mohsen Qureshi <[email protected]>
DFEAGILEDEVOPS · Jan 23, 2025 · e9950f8 · e9950f8
1 parent ead6f1d
commit e9950f8
Show file tree

Hide file tree

Showing 44 changed files with 736 additions and 489 deletions.
diff --git a/admin/services/check-start/check-start.service.js b/admin/services/check-start/check-start.service.js
@@ -304,6 +304,10 @@ const checkStartService = {
         url: `${config.PupilApi.baseUrl}/submit`,
         token: jwtToken
       }
+      const pupilFeedbackData = {
+        url: `${config.PupilApi.baseUrl}/feedback`,
+        token: jwtToken
+      }
 
       const payload = {
         checkCode: o.check_checkCode,
@@ -325,9 +329,9 @@ const checkStartService = {
         tokens: {
           checkStarted: sasTokens[queueNameService.NAMES.CHECK_STARTED],
           pupilPreferences: sasTokens[queueNameService.NAMES.PUPIL_PREFS],
-          pupilFeedback: sasTokens[queueNameService.NAMES.PUPIL_FEEDBACK],
           checkComplete: checkCompleteData,
-          checkSubmission: checkSubmissionData
+          checkSubmission: checkSubmissionData,
+          pupilFeedback: pupilFeedbackData
         },
         questions: checkFormService.prepareQuestionData(
           JSON.parse(o.checkForm_formData)

diff --git a/admin/services/check-start/check-start.service.spec.js b/admin/services/check-start/check-start.service.spec.js
@@ -292,9 +292,6 @@ describe('check-start.service', () => {
         tokenData[queueNameService.NAMES.PUPIL_PREFS] = {
           queueName: queueNameService.NAMES.PUPIL_PREFS, token: 'aab', url: 'xyz'
         }
-        tokenData[queueNameService.NAMES.PUPIL_FEEDBACK] = {
-          queueName: queueNameService.NAMES.PUPIL_FEEDBACK, token: 'aab', url: 'xyz'
-        }
         jest.spyOn(sasTokenService, 'getTokens').mockResolvedValue(tokenData)
         const mockSignMethod = async (payload) => {
           return payload.checkCode
@@ -395,12 +392,21 @@ describe('check-start.service', () => {
         expect(payload[0].tokens.checkSubmission.url).toStrictEqual(expectedUrl)
       })
 
-      test('checkSubmission tokens data should be populated when submission feature active', async () => {
-        config.FeatureToggles.checkSubmissionApi = true
+      test('checkSubmission tokens data should be populated', async () => {
         const payload = await checkStartService.createPupilCheckPayloads([mockCheckFormAllocationLive], 1)
-        expect(payload[0].tokens.checkComplete).toBeUndefined()
         expect(payload[0].tokens.checkSubmission).toBeDefined()
       })
+
+      test('pupilFeedback tokens data should be populated', async () => {
+        const payload = await checkStartService.createPupilCheckPayloads([mockCheckFormAllocationLive], 1)
+        expect(payload[0].tokens.pupilFeedback).toBeDefined()
+      })
+
+      test('pupilFeedback JWT token should be the same as check submission', async () => {
+        const payload = await checkStartService.createPupilCheckPayloads([mockCheckFormAllocationLive], 1)
+        expect(payload[0].tokens.pupilFeedback.token)
+          .toStrictEqual(payload[0].tokens.checkSubmission.token)
+      })
     })
   })
 })
diff --git a/admin/services/sas-token.service.js b/admin/services/sas-token.service.js
@@ -69,8 +69,7 @@ const sasTokenService = {
   getTokens: async function (hasLiveChecks, expiryDate) {
     const queueNames = [
       queueNameService.NAMES.CHECK_STARTED,
-      queueNameService.NAMES.PUPIL_PREFS,
-      queueNameService.NAMES.PUPIL_FEEDBACK
+      queueNameService.NAMES.PUPIL_PREFS
     ]
 
     // Attempt to retrieve all tokens from redis

diff --git a/admin/services/storage-queue-name-service.js b/admin/services/storage-queue-name-service.js
@@ -2,7 +2,6 @@
 
 const NAMES = {
   CHECK_STARTED: 'check-started',
-  PUPIL_FEEDBACK: 'pupil-feedback',
   PUPIL_PREFS: 'pupil-prefs',
   TEST_PUPIL_CONNECTION: 'test-pupil-connection'
 }

diff --git a/admin/spec/back-end/service/sas-token.service.spec.js b/admin/spec/back-end/service/sas-token.service.spec.js
@@ -99,7 +99,6 @@ describe('sas-token.service', () => {
       const mockRedisResponse = [
         { queueName: queueNameService.NAMES.CHECK_STARTED, token: 'aaa' },
         { queueName: queueNameService.NAMES.PUPIL_PREFS, token: 'aab' },
-        { queueName: queueNameService.NAMES.PUPIL_FEEDBACK, token: 'aab' },
         { queueName: queueNameService.NAMES.CHECK_SUBMIT, token: 'aab' }
       ]
       jest.spyOn(redisCacheService, 'getMany').mockResolvedValue(mockRedisResponse)
@@ -129,15 +128,13 @@ describe('sas-token.service', () => {
       jest.spyOn(logger, 'debug').mockImplementation()
       await sut.getTokens(true, moment().add(4, 'hours'))
       expect(redisCacheService.getMany).toHaveBeenCalledTimes(1)
-      expect(sasTokenService.generateSasToken).toHaveBeenCalledTimes(3)
+      expect(sasTokenService.generateSasToken).toHaveBeenCalledTimes(2)
     })
 
     test('calls out to generate sas tokens if any are not found in redis', async () => {
       // mock a response where the values are partially found in the cache
       const mockRedisResponse = [
         { queueName: queueNameService.NAMES.CHECK_STARTED, token: 'aaa' },
-        { queueName: queueNameService.NAMES.PUPIL_PREFS, token: 'aab' },
-        undefined,
         undefined
       ]
       jest.spyOn(redisCacheService, 'getMany').mockReturnValue(mockRedisResponse)
@@ -153,7 +150,7 @@ describe('sas-token.service', () => {
       expect(redisCacheService.getMany).toHaveBeenCalledTimes(1)
       expect(sasTokenService.generateSasToken).toHaveBeenCalledTimes(1)
       // Expect `res` to have 4 properties, 2 from redis, and 2 generated
-      expect(Object.keys(res).length).toBe(3)
+      expect(Object.keys(res).length).toBe(2)
     })
   })
 })
diff --git a/admin/tests-integration/sas-token-expiry.spec.js b/admin/tests-integration/sas-token-expiry.spec.js
@@ -33,15 +33,15 @@ async function postMessageToQueue (payload, queueUrl, sasTokenUrl) {
 
 describe('sas-token-expiry', () => {
   beforeEach(async () => {
-    const queueKey = redisKeyService.getSasTokenKey(queueNameService.NAMES.PUPIL_FEEDBACK)
+    const queueKey = redisKeyService.getSasTokenKey(queueNameService.NAMES.CHECK_STARTED)
     await redisCacheService.drop(queueKey)
   })
   afterAll(async () => { await redisCacheService.disconnect() })
 
   test('should send a message successfully with valid token', async () => {
     const sasExpiryDate = moment().add(1, 'minute')
     const sasToken = await sut.generateSasToken(
-      queueNameService.NAMES.PUPIL_FEEDBACK,
+      queueNameService.NAMES.CHECK_STARTED,
       sasExpiryDate
     )
     const message = { message: 'integration test message' }
@@ -51,7 +51,7 @@ describe('sas-token-expiry', () => {
   test('should return specific properties and content when attempting to submit with expired sas tokens', async () => {
     const sasExpiryDate = moment().add(2, 'seconds')
     const sasToken = await sut.generateSasToken(
-      queueNameService.NAMES.PUPIL_FEEDBACK,
+      queueNameService.NAMES.CHECK_STARTED,
       sasExpiryDate
     )
     try {

diff --git a/db/migrations/schema-objects/20250117111900.do.pupil-feedback-table.sql b/db/migrations/schema-objects/20250117111900.do.pupil-feedback-table.sql
@@ -0,0 +1,14 @@
+CREATE TABLE [mtc_admin].[pupilFeedback] (
+    [id] INT IDENTITY (1, 1) NOT NULL,
+    [createdAt] DATETIMEOFFSET (3) CONSTRAINT [DEFAULT_pupilFeedback_createdAt] DEFAULT GETUTCDATE() NOT NULL,
+    [updatedAt] DATETIMEOFFSET (3) CONSTRAINT [DEFAULT_pupilFeedback_updatedAt] DEFAULT GETUTCDATE() NOT NULL,
+    [version] [timestamp] NOT NULL,
+    [check_id] INT NOT NULL,
+    [pupil_id] INT NOT NULL,
+    [school_id] INT NOT NULL,
+    [feedback] NVARCHAR (255) NOT NULL,
+    CONSTRAINT [PK_pupilFeedback] PRIMARY KEY CLUSTERED ([id] ASC),
+    CONSTRAINT [FK_pupilFeedback_check] FOREIGN KEY ([check_id]) REFERENCES [mtc_admin].[check] ([id]),
+    CONSTRAINT [FK_pupilFeedback_pupil] FOREIGN KEY ([pupil_id]) REFERENCES [mtc_admin].[pupil] ([id]),
+    CONSTRAINT [FK_pupilFeedback_school] FOREIGN KEY ([school_id]) REFERENCES [mtc_admin].[school] ([id])
+);
diff --git a/db/migrations/schema-objects/20250117111900.undo.pupil-feedback-table.sql b/db/migrations/schema-objects/20250117111900.undo.pupil-feedback-table.sql
@@ -0,0 +1 @@
+DROP TABLE [mtc_admin].[pupilFeedback];
diff --git a/deploy/service-bus/deploy.config.js b/deploy/service-bus/deploy.config.js
@@ -111,6 +111,9 @@ const config = {
       maxSizeInMegabytes: {}.hasOwnProperty.call(process.env, 'SERVICE_BUS_QUEUE_MAX_SIZE_MEGABYTES_PS_REPORT_EXEC') ? parseInt(process.env.SERVICE_BUS_QUEUE_MAX_SIZE_MEGABYTES_PS_REPORT_EXEC, 10) : oneGigabyte,
       defaultMessageTimeToLive: sixHours,
       maxDeliveryCount: 1
+    },
+    {
+      name: 'pupil-feedback'
     }
   ]
 }

diff --git a/deploy/storage/tables-queues.json b/deploy/storage/tables-queues.json
@@ -1,13 +1,11 @@
 {
   "queues": [
     "check-started",
-    "pupil-feedback",
     "pupil-prefs"
   ],
   "tables": [
     "checkResult",
     "pupilEvent",
-    "pupilFeedback",
     "receivedCheck"
   ],
   "blobcontainers": [

diff --git a/func-consumption/package.json b/func-consumption/package.json
@@ -45,7 +45,6 @@
     "@azure/functions": "^4.6.0",
     "@azure/service-bus": "^7.9.4",
     "@azure/storage-blob": "~12.26.0",
-    "@azure/storage-queue": "^12.22.0",
     "applicationinsights": "^2.9.6",
     "async": "^3.2.5",
     "body-parser": "^1.20.2",

diff --git a/func-consumption/yarn.lock b/func-consumption/yarn.lock
@@ -292,23 +292,6 @@
     events "^3.0.0"
     tslib "^2.2.0"
 
-"@azure/storage-queue@^12.22.0":
-  version "12.25.0"
-  resolved "https://registry.yarnpkg.com/@azure/storage-queue/-/storage-queue-12.25.0.tgz#69803070857d2341ab22941e2409ed5994e70f66"
-  integrity sha512-uoobHFbH/o7wIul/sCm32X2YFq6zb1XpNdpKIms9I60mwG3BBaOpEs5pgQV5a5ONG5WMSHlo8E1dNFB5ZZIa1g==
-  dependencies:
-    "@azure/abort-controller" "^2.1.2"
-    "@azure/core-auth" "^1.4.0"
-    "@azure/core-client" "^1.6.2"
-    "@azure/core-http-compat" "^2.0.0"
-    "@azure/core-paging" "^1.1.1"
-    "@azure/core-rest-pipeline" "^1.10.1"
-    "@azure/core-tracing" "^1.1.2"
-    "@azure/core-util" "^1.6.1"
-    "@azure/core-xml" "^1.4.3"
-    "@azure/logger" "^1.0.0"
-    tslib "^2.2.0"
-
 "@colors/[email protected]", "@colors/colors@^1.6.0":
   version "1.6.0"
   resolved "https://registry.yarnpkg.com/@colors/colors/-/colors-1.6.0.tgz#ec6cd237440700bc23ca23087f513c75508958b0"

diff --git a/func-ps-report/package.json b/func-ps-report/package.json
@@ -37,7 +37,6 @@
     "@azure/functions": "^4.6.0",
     "@azure/service-bus": "^7.9.4",
     "@azure/storage-blob": "~12.26.0",
-    "@azure/storage-queue": "^12.22.0",
     "applicationinsights": "^2.9.6",
     "async": "^3.2.5",
     "body-parser": "^1.20.2",

diff --git a/func-ps-report/yarn.lock b/func-ps-report/yarn.lock
@@ -292,23 +292,6 @@
     events "^3.0.0"
     tslib "^2.2.0"
 
-"@azure/storage-queue@^12.22.0":
-  version "12.25.0"
-  resolved "https://registry.yarnpkg.com/@azure/storage-queue/-/storage-queue-12.25.0.tgz#69803070857d2341ab22941e2409ed5994e70f66"
-  integrity sha512-uoobHFbH/o7wIul/sCm32X2YFq6zb1XpNdpKIms9I60mwG3BBaOpEs5pgQV5a5ONG5WMSHlo8E1dNFB5ZZIa1g==
-  dependencies:
-    "@azure/abort-controller" "^2.1.2"
-    "@azure/core-auth" "^1.4.0"
-    "@azure/core-client" "^1.6.2"
-    "@azure/core-http-compat" "^2.0.0"
-    "@azure/core-paging" "^1.1.1"
-    "@azure/core-rest-pipeline" "^1.10.1"
-    "@azure/core-tracing" "^1.1.2"
-    "@azure/core-util" "^1.6.1"
-    "@azure/core-xml" "^1.4.3"
-    "@azure/logger" "^1.0.0"
-    tslib "^2.2.0"
-
 "@colors/[email protected]", "@colors/colors@^1.6.0":
   version "1.6.0"
   resolved "https://registry.yarnpkg.com/@colors/colors/-/colors-1.6.0.tgz#ec6cd237440700bc23ca23087f513c75508958b0"

diff --git a/func-throttled/package.json b/func-throttled/package.json
@@ -36,7 +36,6 @@
     "@azure/functions": "^4.6.0",
     "@azure/service-bus": "^7.9.4",
     "@azure/storage-blob": "~12.26.0",
-    "@azure/storage-queue": "^12.22.0",
     "applicationinsights": "^2.9.6",
     "async": "^3.2.5",
     "body-parser": "^1.20.2",

diff --git a/func-throttled/yarn.lock b/func-throttled/yarn.lock
@@ -292,23 +292,6 @@
     events "^3.0.0"
     tslib "^2.2.0"
 
-"@azure/storage-queue@^12.22.0":
-  version "12.25.0"
-  resolved "https://registry.yarnpkg.com/@azure/storage-queue/-/storage-queue-12.25.0.tgz#69803070857d2341ab22941e2409ed5994e70f66"
-  integrity sha512-uoobHFbH/o7wIul/sCm32X2YFq6zb1XpNdpKIms9I60mwG3BBaOpEs5pgQV5a5ONG5WMSHlo8E1dNFB5ZZIa1g==
-  dependencies:
-    "@azure/abort-controller" "^2.1.2"
-    "@azure/core-auth" "^1.4.0"
-    "@azure/core-client" "^1.6.2"
-    "@azure/core-http-compat" "^2.0.0"
-    "@azure/core-paging" "^1.1.1"
-    "@azure/core-rest-pipeline" "^1.10.1"
-    "@azure/core-tracing" "^1.1.2"
-    "@azure/core-util" "^1.6.1"
-    "@azure/core-xml" "^1.4.3"
-    "@azure/logger" "^1.0.0"
-    tslib "^2.2.0"
-
 "@colors/[email protected]", "@colors/colors@^1.6.0":
   version "1.6.0"
   resolved "https://registry.yarnpkg.com/@colors/colors/-/colors-1.6.0.tgz#ec6cd237440700bc23ca23087f513c75508958b0"

diff --git a/pupil-api/package.json b/pupil-api/package.json
@@ -17,7 +17,6 @@
     "@azure/functions": "^4.6.0",
     "@azure/service-bus": "^7.9.4",
     "@azure/storage-blob": "~12.26.0",
-    "@azure/storage-queue": "^12.22.0",
     "applicationinsights": "^2.9.6",
     "async": "^3.2.5",
     "body-parser": "^1.20.2",

diff --git a/pupil-api/yarn.lock b/pupil-api/yarn.lock
@@ -299,23 +299,6 @@
     events "^3.0.0"
     tslib "^2.2.0"
 
-"@azure/storage-queue@^12.22.0":
-  version "12.25.0"
-  resolved "https://registry.yarnpkg.com/@azure/storage-queue/-/storage-queue-12.25.0.tgz#69803070857d2341ab22941e2409ed5994e70f66"
-  integrity sha512-uoobHFbH/o7wIul/sCm32X2YFq6zb1XpNdpKIms9I60mwG3BBaOpEs5pgQV5a5ONG5WMSHlo8E1dNFB5ZZIa1g==
-  dependencies:
-    "@azure/abort-controller" "^2.1.2"
-    "@azure/core-auth" "^1.4.0"
-    "@azure/core-client" "^1.6.2"
-    "@azure/core-http-compat" "^2.0.0"
-    "@azure/core-paging" "^1.1.1"
-    "@azure/core-rest-pipeline" "^1.10.1"
-    "@azure/core-tracing" "^1.1.2"
-    "@azure/core-util" "^1.6.1"
-    "@azure/core-xml" "^1.4.3"
-    "@azure/logger" "^1.0.0"
-    tslib "^2.2.0"
-
 "@colors/[email protected]", "@colors/colors@^1.6.0":
   version "1.6.0"
   resolved "https://registry.yarnpkg.com/@colors/colors/-/colors-1.6.0.tgz#ec6cd237440700bc23ca23087f513c75508958b0"

diff --git a/pupil-spa/src/app/services/feedback/feedback.service.mock.ts b/pupil-spa/src/app/services/feedback/feedback.service.mock.ts
@@ -2,5 +2,5 @@ import { IFeedbackService } from './feedback.service';
 
 export class FeedbackServiceMock implements IFeedbackService {
   async postFeedback(): Promise<boolean> { return true }
-  async queueSubmit(payload: Object): Promise<void> { return }
+  async submitFeedback(payload: Object): Promise<void> { return }
 }