Conversation
This reverts commit df98bc0.
| import { makeVerifiedContactDetail } from 'helpers/factories/contact-details-factory'; | ||
|
|
||
| const generateEmailAddress = () => | ||
| faker.internet.email({ provider: 'nhs.net' }).toLowerCase(); |
There was a problem hiding this comment.
why faker instead of having consistent test data?
There was a problem hiding this comment.
Faker was already installed and it was easier than thinking of a new email address for each test case. I didn't want to use the same email address between test cases because of the uniqueness constraint.
| }, | ||
| data: { | ||
| type: 'EMAIL', | ||
| value: ` ${email.toUpperCase()} `, |
There was a problem hiding this comment.
why the use of toUpperCase?
There was a problem hiding this comment.
Just to prove the normalisation is working. Input is an uppercase email address padded with whitespace, output is a lowercase email address with no padding.
| '', | ||
| 'email@subdomain.nhs.net', | ||
| `l${'o'.repeat(310)}ng@nhs.net`, | ||
| 'email@notnhsdotnet.com', |
There was a problem hiding this comment.
we need to support domains other than nhs.net (sorry! restricting emails to only nhs.net was a requirement until recently. i only learned today that we need to support any domain)
There was a problem hiding this comment.
Ah okay, no problem
There was a problem hiding this comment.
I've made this use exactly the same rules as Core.
| }); | ||
|
|
||
| test.describe('sms verification', () => { | ||
| for (const locale of ['GB', 'GG', 'IM', 'JE'] as Locale[]) { |
There was a problem hiding this comment.
this is very interesting. where did this list of supported locales come from?
There was a problem hiding this comment.
I think it was mentioned the other week that we only wanted to support UK mobile numbers. I adapted the phone number verification from core, which had tests relating to the channel islands, so I went with accepting those too - they all use the +44 prefix. Happy to be corrected if this is wrong.
| ttl: this.getUnverifiedTtl(now), | ||
| createdAt: now.toISOString(), | ||
| createdBy: `INTERNAL_USER#${user.internalUserId}`, | ||
| updatedAt: now.toISOString(), |
There was a problem hiding this comment.
is updatedAt/updatedBy meaningful here? like once it's been created the only change that can be made to it is to change the status to VERIFIED, and it'd be very difficult for that to be done by a different user to the one that created it
There was a problem hiding this comment.
I wasn't sure, I think I just saw it on one of the REFCOMs and thought there was no harm in having it.
| name, # To support backup and restore which will result in a new name otherwise | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
can we have an index to get by ID? will be needed for later parts of contact details verification, and it's simpler if it's just in there from the start (sorry! this was on a REFCOM that did not make it onto the ticket)
There was a problem hiding this comment.
I made this the primary index on the table because it enforces the uniqueness constraint. Otherwise you need to list all of the contact details for the client and check the uniqueness in memory.
For the other access patterns, I was going to add GSI's - primarily one to look up by ID. But I felt this was the best way to enforce the uniqueness.
| new PutCommand({ | ||
| TableName: this.tableName, | ||
| Item: { | ||
| PK: `CLIENT#${user.clientId}`, |
There was a problem hiding this comment.
this schema doesn't match at all what's on the ticket (or what's on the REFCOM that the ticket is based on) - if there are good reasons for doing it like this instead we should have a chat about it because changing it to work like this has huge implications for the rest of the contact details verification work
There was a problem hiding this comment.
I've renamed the keys to match whats on the REFCOM, PK/SK was just sort of habitual 😆
2 participants
Description
Adds infrastructure and API endpoint for storing contact detail verification requests
crypto.randomInt. Hashed OTP is stored in the database.Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.