Bug/58880 pupil add edit validation bug (#2505)

* lockfile update * Fix bug by transforming the upn earlier in the sequence * Bump minor and patch deps * Re-compute assets version * adding tests for upns with a space * Update assets version * Fix errors generated during update - so they at least show the error page to screen * Use the clean upn when checking for duplicates in the database --------- Co-authored-by: Guy Harwood <[email protected]> Co-authored-by: Mohsen Qureshi <[email protected]> Co-authored-by: Guy Harwood <[email protected]>
DFEAGILEDEVOPS · Jun 1, 2023 · 88affd4 · 88affd4
1 parent c6d3480
commit 88affd4
Show file tree

Hide file tree

Showing 10 changed files with 661 additions and 667 deletions.
diff --git a/admin/controllers/pupil.js b/admin/controllers/pupil.js
@@ -251,7 +251,7 @@ const controller = {
       await pupilEditService.update(pupil, req.body, req.user.schoolId, req.user.id)
       req.flash('info', 'Changes to pupil details have been saved')
     } catch (error) {
-      next(error)
+      return next(error)
     }
     const highlight = JSON.stringify([pupil.urlSlug.toString()])
     res.locals.isSubmitMetaRedirectUrl = true

diff --git a/admin/lib/validator/pupil-validator.js b/admin/lib/validator/pupil-validator.js
@@ -150,13 +150,13 @@ module.exports.validate = async (pupilData, schoolId, isMultiplePupilsSubmission
   if (isEmpty(upn)) {
     upnErrorArr.push(addPupilErrorMessages.upnRequired)
   }
-  // Check that the UPN is unique
+  // Check that the UPN is unique, if there are no other UPN errors (those would short circuit this check)
   if (upnErrorArr.length === 0) {
-    const existingPupil = await pupilDataService.sqlFindOneByUpnAndSchoolId(pupilData.upn, schoolId)
+    const existingPupil = await pupilDataService.sqlFindOneByUpnAndSchoolId(upn, schoolId)
     // if pupil is not stored already under the same id and UPN
     if (!isEmpty(upn) && existingPupil &&
       existingPupil.urlSlug.toString() !== pupilData.urlSlug &&
-      existingPupil.upn === pupilData.upn) {
+      existingPupil.upn === upn) {
       upnErrorArr.push(addPupilErrorMessages.upnDuplicate)
     }
   }

diff --git a/admin/package.json b/admin/package.json
@@ -199,4 +199,4 @@
       "axios": "axios/dist/node/axios.cjs"
     }
   }
-}
+}
diff --git a/admin/services/pupil-add-service.js b/admin/services/pupil-add-service.js
@@ -20,10 +20,11 @@ const pupilAddService = {
     if (pupilData === undefined || Object.keys(pupilData).length < 11) throw new Error('pupilData is required')
     if (isNaN(schoolId)) throw new Error('schoolId is required')
     if (isNaN(userId)) throw new Error('userId is required')
+    const cleanUPN = R.pathOr('', ['upn'], pupilData).trim().toUpperCase()
 
     const pupilDataRow = {
       school_id: schoolId,
-      upn: pupilData.upn,
+      upn: cleanUPN,
       foreName: pupilData.foreName,
       lastName: pupilData.lastName,
       middleNames: pupilData.middleNames,
@@ -45,8 +46,6 @@ const pupilAddService = {
     const saveData = R.omit(['dob-day', 'dob-month', 'dob-year', 'ageReason'], pupilDataRow)
     // @ts-ignore
     saveData.dateOfBirth = dateService.createUTCFromDayMonthYear(pupilDataRow['dob-day'], pupilDataRow['dob-month'], pupilDataRow['dob-year'])
-    saveData.upn = R.pathOr('', ['upn'], pupilDataRow).trim().toUpperCase()
-
     const res = await pupilDataService.sqlCreate(saveData)
     const pupilRecord = await pupilDataService.sqlFindOneById(res.insertId)
 

diff --git a/admin/spec/back-end/service/pupil-add-service.spec.js b/admin/spec/back-end/service/pupil-add-service.spec.js
@@ -128,5 +128,22 @@ describe('pupil-add-service', () => {
       expect(saveArg['dob-year']).toBeUndefined()
       expect(pupilAgeReasonDataService.sqlInsertPupilAgeReason).toHaveBeenCalled()
     })
+
+    test('validates the UPN with the trimmed and uppercased UPN', async () => {
+      jest.spyOn(pupilValidator, 'validate').mockImplementation(validationFunctionSucceeds)
+      jest.spyOn(pupilDataService, 'sqlCreate').mockResolvedValue({ insertId: 1 })
+      jest.spyOn(pupilDataService, 'sqlFindOneById').mockImplementation()
+      jest.spyOn(pupilAgeReasonDataService, 'sqlInsertPupilAgeReason').mockImplementation()
+      jest.spyOn(redisCacheService, 'drop').mockImplementation()
+      reqBody.ageReason = 'reason'
+      reqBody.upn = ' l860100210012 ' // what the user typed into the form
+      const transformedUPN = 'L860100210012' // what is expected to get passed to the validator and the sqlCreate functions
+      await service.addPupil(reqBody, schoolId, userId)
+      expect(pupilValidator.validate).toHaveBeenCalled()
+      const validationArgs = pupilValidator.validate.mock.calls[0]
+      expect(validationArgs[0].upn).toBe(transformedUPN)
+      const sqlCreateArgs = pupilDataService.sqlCreate.mock.calls[0]
+      expect(sqlCreateArgs[0].upn).toBe(transformedUPN) // the DB should get the same transformed upn
+    })
   })
 })
-Original file line number
+Diff line change
@@ Expand Up / @@ -199,4 +199,4 @@ @@
           "axios": "axios/dist/node/axios.cjs"
         }
       }
-    }
+    }