CCM-11889: migrate updated created fields #786
Conversation
|
|
||
| return ( | ||
| <ErrorSummary ref={errorSummaryRef}> | ||
| <ErrorSummary ref={errorSummaryRef} tabIndex={-1}> |
There was a problem hiding this comment.
I think this is an AI-autocomplete typo, i.e. I didn't notice it had changed it. Reverted.
| <div | ||
| class="nhsuk-grid-row nhsuk-grid-column-two-thirds" | ||
| > | ||
| <div |
There was a problem hiding this comment.
this test is for the validation being triggered so I'm not convinced the error summary should be removed from the snapshot?
There was a problem hiding this comment.
The error summary should not show when validation passes (which is the bug that was being fixed). These tests were ambiguous so I've split the form-validation tests into success and fail scenarios.
| <div | ||
| class="nhsuk-grid-row nhsuk-grid-column-two-thirds" | ||
| > | ||
| <div |
There was a problem hiding this comment.
Updated as above.
| expect(scrollIntoViewMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('Renders NhsNotifyErrorSummary correctly with empty error state', async () => { |
There was a problem hiding this comment.
I think based on your changes, might also be worth adding tests for errorState object having formError/fieldError keys but with falsy values, since component also won't render in those scenarios either now?
| } from '@aws-sdk/client-cognito-identity-provider'; | ||
| import { logger } from '@/src/utils/logger'; | ||
|
|
||
| export const INTERNAL_ID_ATTRIBUTE = 'custom:nhs_notify_user_id'; |
There was a problem hiding this comment.
the ticket mentions username but I'm guessing this is what we're using instead?
There was a problem hiding this comment.
I've added a comment to the ticket with a diagram showing an example of the data model and how it will change in the migration.
There was a problem hiding this comment.
Essentially "username" is a reference to the Cognito username field, when the ticket was originally written we were going to use that as the primary identifier for the user but instead went with an internally defined UUID.
| @@ -1 +0,0 @@ | |||
| *.json | |||
There was a problem hiding this comment.
should we have something similar in the new dir?
| if (error instanceof ConditionalCheckFailedException) { | ||
| return 'lock-failure'; | ||
| } | ||
| logger.error( |
There was a problem hiding this comment.
do you not want one of these error logs before your lock-failure return?
| // act | ||
| let caughtError; | ||
| try { | ||
| await discoverUserPoolId('nonexistent'); |
There was a problem hiding this comment.
Not something to change but an FYI you canw write this as
await expect(discoverUserPoolId('nonexistent')).rejects.toThrowError('User pool nhs-notify-nonexistent-app not found')
| } while (paginationToken); | ||
|
|
||
| const userPoolId = userPools.find( | ||
| (pool) => pool.Name === TARGET_USER_POOL_NAME(env) |
There was a problem hiding this comment.
reduce unnecessary calls to AWS if you do this check before getting each next page
| )?.Value; | ||
| return [sub, internalId]; | ||
| }) | ||
| .filter(([sub, internalId]) => sub && internalId) |
There was a problem hiding this comment.
are we expecting to have any users that don't have sub and internalId defined?
| return [sub, internalId]; | ||
| }) | ||
| .filter(([sub, internalId]) => sub && internalId) | ||
| .reduce( |
There was a problem hiding this comment.
isn't this just Object.fromEntries?
| // Read parameters | ||
| const { dryRun, env, userDetailsFile } = readParams(); | ||
|
|
||
| if (!userDetailsFile) { |
There was a problem hiding this comment.
in what circumstances would you expect to start with a reference to a user details file that is only generated by this script?
| if (!dryRun) { | ||
| const updateResult = | ||
| entity === 'template' | ||
| ? await updateTemplateRecord( |
There was a problem hiding this comment.
it's unnecessary to split this out here - the only difference between what you're doing with templates and routing configs is the table name. just pass the table name into migrateRecord instead of entity and save a lot of duplicate code
| id: record.id, | ||
| }, | ||
| UpdateExpression: | ||
| 'SET createdBy = :createdBy, updatedBy = :updatedBy ADD #lockNumber :lockNumberIncrement', |
There was a problem hiding this comment.
do we really want to be incrementing lock number? elsewhere we've only been doing that for user-initiated changes.
| }, | ||
| ExpressionAttributeValues: { | ||
| ':expectedLockNumber': record.lockNumber ?? 0, | ||
| ':createdBy': newCreatedBy ?? record.createdBy, |
There was a problem hiding this comment.
why the fallback? if we don't have a value to set, why do the update in the first place? and also would we expect there to be any records we want to update that are missing these fields?
4 participants
Description
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.